Simplify ExtensionGlobalError ownership
This CL makes ExtensionService owner of ExtensionGlobalError. It also fixes one memory leak.
BUG=110222
TEST=
Review URL: http://codereview.chromium.org/9224004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@119032 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/extensions/extension_global_error.cc b/chrome/browser/extensions/extension_global_error.cc
index dfcb016..173964a 100644
--- a/chrome/browser/extensions/extension_global_error.cc
+++ b/chrome/browser/extensions/extension_global_error.cc
@@ -13,13 +13,12 @@
#include "grit/generated_resources.h"
#include "ui/base/l10n/l10n_util.h"
-ExtensionGlobalError::ExtensionGlobalError(
- base::WeakPtr<ExtensionService> extension_service)
- : should_delete_self_on_close_(true),
- extension_service_(extension_service),
+ExtensionGlobalError::ExtensionGlobalError(ExtensionService* extension_service)
+ : extension_service_(extension_service),
external_extension_ids_(new ExtensionIdSet),
blacklisted_extension_ids_(new ExtensionIdSet),
orphaned_extension_ids_(new ExtensionIdSet) {
+ DCHECK(extension_service_);
}
ExtensionGlobalError::~ExtensionGlobalError() {
@@ -37,21 +36,6 @@
orphaned_extension_ids_->insert(id);
}
-void ExtensionGlobalError::set_accept_callback(
- ExtensionGlobalErrorCallback callback) {
- accept_callback_ = callback;
-}
-
-void ExtensionGlobalError::set_cancel_callback(
- ExtensionGlobalErrorCallback callback) {
- cancel_callback_ = callback;
-}
-
-void ExtensionGlobalError::set_closed_callback(
- ExtensionGlobalErrorCallback callback) {
- cancel_callback_ = callback;
-}
-
bool ExtensionGlobalError::HasBadge() {
return false;
}
@@ -99,17 +83,12 @@
}
string16 ExtensionGlobalError::GenerateMessage() {
- if (extension_service_.get()) {
- return
- GenerateMessageSection(external_extension_ids_.get(),
- IDS_EXTENSION_ALERT_ITEM_EXTERNAL) +
- GenerateMessageSection(blacklisted_extension_ids_.get(),
- IDS_EXTENSION_ALERT_ITEM_EXTERNAL) +
- GenerateMessageSection(orphaned_extension_ids_.get(),
- IDS_EXTENSION_ALERT_ITEM_EXTERNAL);
- } else {
- return string16();
- }
+ return GenerateMessageSection(external_extension_ids_.get(),
+ IDS_EXTENSION_ALERT_ITEM_EXTERNAL) +
+ GenerateMessageSection(blacklisted_extension_ids_.get(),
+ IDS_EXTENSION_ALERT_ITEM_EXTERNAL) +
+ GenerateMessageSection(orphaned_extension_ids_.get(),
+ IDS_EXTENSION_ALERT_ITEM_EXTERNAL);
}
string16 ExtensionGlobalError::GetBubbleViewMessage() {
@@ -128,22 +107,13 @@
}
void ExtensionGlobalError::OnBubbleViewDidClose(Browser* browser) {
- if (!closed_callback_.is_null()) {
- closed_callback_.Run(*this, browser);
- }
- if (should_delete_self_on_close_) {
- delete this;
- }
+ extension_service_->HandleExtensionAlertClosed();
}
void ExtensionGlobalError::BubbleViewAcceptButtonPressed(Browser* browser) {
- if (!accept_callback_.is_null()) {
- accept_callback_.Run(*this, browser);
- }
+ extension_service_->HandleExtensionAlertAccept();
}
void ExtensionGlobalError::BubbleViewCancelButtonPressed(Browser* browser) {
- if (!cancel_callback_.is_null()) {
- cancel_callback_.Run(*this, browser);
- }
+ extension_service_->HandleExtensionAlertDetails(browser);
}
diff --git a/chrome/browser/extensions/extension_global_error.h b/chrome/browser/extensions/extension_global_error.h
index 6d8423ff..7a45468 100644
--- a/chrome/browser/extensions/extension_global_error.h
+++ b/chrome/browser/extensions/extension_global_error.h
@@ -7,9 +7,7 @@
#pragma once
#include "base/basictypes.h"
-#include "base/callback.h"
#include "base/compiler_specific.h"
-#include "base/memory/weak_ptr.h"
#include "chrome/browser/ui/global_error.h"
#include "chrome/common/extensions/extension.h"
@@ -20,27 +18,9 @@
// occur related to installed extensions.
class ExtensionGlobalError : public GlobalError {
public:
- explicit ExtensionGlobalError(
- base::WeakPtr<ExtensionService> extension_service);
+ explicit ExtensionGlobalError(ExtensionService* extension_service);
virtual ~ExtensionGlobalError();
- // Indicate whether this instance should manage its own lifetime. Because
- // the GlobalError class can be used in several different ways, it's
- // important to understand who has responsibility for memory management.
- //
- // Briefly: if the GlobalError has a menu item, or if it's added to the
- // GlobalErrorService queue, then it's externally managed. If your code
- // calls ShowBubbleView(), then you manage it.
- //
- // The default value is true; in case the default is wrong, we prefer to
- // crash during development than to leak in production (fail fast).
- //
- // TODO(sail): This could be handled automatically with a few changes to the
- // GlobalError interface.
- void set_should_delete_self_on_close(bool value) {
- should_delete_self_on_close_ = value;
- }
-
// Inform us that a given extension is of a certain type that the user
// hasn't yet acknowledged.
void AddExternalExtension(const std::string& id);
@@ -61,19 +41,6 @@
return orphaned_extension_ids_.get();
}
- typedef base::Callback<void(const ExtensionGlobalError&, Browser* browser)>
- ExtensionGlobalErrorCallback;
-
- // Called when the user presses the "Accept" button on the alert.
- void set_accept_callback(ExtensionGlobalErrorCallback callback);
-
- // Called when the user presses the "Cancel" button on the alert.
- void set_cancel_callback(ExtensionGlobalErrorCallback callback);
-
- // Called when the alert is dismissed with no direct user action
- // (e.g., the browser exits).
- void set_closed_callback(ExtensionGlobalErrorCallback callback);
-
// GlobalError methods.
virtual bool HasBadge() OVERRIDE;
virtual bool HasMenuItem() OVERRIDE;
@@ -91,13 +58,10 @@
private:
bool should_delete_self_on_close_;
- base::WeakPtr<ExtensionService> extension_service_;
+ ExtensionService* extension_service_;
scoped_ptr<ExtensionIdSet> external_extension_ids_;
scoped_ptr<ExtensionIdSet> blacklisted_extension_ids_;
scoped_ptr<ExtensionIdSet> orphaned_extension_ids_;
- ExtensionGlobalErrorCallback accept_callback_;
- ExtensionGlobalErrorCallback cancel_callback_;
- ExtensionGlobalErrorCallback closed_callback_;
string16 message_; // Displayed in the body of the alert.
// For a given set of extension IDs, generates appropriate text
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index 7891b83..a260db3d 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -1820,72 +1820,69 @@
// Build up the lists of extensions that require acknowledgment.
// If this is the first time, grandfather extensions that would have
// caused notification.
- scoped_ptr<ExtensionGlobalError> global_error(
- new ExtensionGlobalError(AsWeakPtr()));
+ extension_global_error_.reset(new ExtensionGlobalError(this));
bool needs_alert = false;
for (ExtensionSet::const_iterator iter = extensions_.begin();
iter != extensions_.end(); ++iter) {
const Extension* e = *iter;
if (Extension::IsExternalLocation(e->location())) {
if (!extension_prefs_->IsExternalExtensionAcknowledged(e->id())) {
- global_error->AddExternalExtension(e->id());
+ extension_global_error_->AddExternalExtension(e->id());
needs_alert = true;
}
}
if (extension_prefs_->IsExtensionBlacklisted(e->id())) {
if (!extension_prefs_->IsBlacklistedExtensionAcknowledged(e->id())) {
- global_error->AddBlacklistedExtension(e->id());
+ extension_global_error_->AddBlacklistedExtension(e->id());
needs_alert = true;
}
}
if (extension_prefs_->IsExtensionOrphaned(e->id())) {
if (!extension_prefs_->IsOrphanedExtensionAcknowledged(e->id())) {
- global_error->AddOrphanedExtension(e->id());
+ extension_global_error_->AddOrphanedExtension(e->id());
needs_alert = true;
}
}
}
+ bool did_show_alert = false;
if (needs_alert) {
if (extension_prefs_->SetAlertSystemFirstRun()) {
- global_error->set_accept_callback(
- base::Bind(&ExtensionService::HandleExtensionAlertAccept,
- base::Unretained(this)));
- global_error->set_cancel_callback(
- base::Bind(&ExtensionService::HandleExtensionAlertDetails,
- base::Unretained(this)));
- ShowExtensionAlert(global_error.release());
+ CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ Browser* browser = BrowserList::GetLastActiveWithProfile(profile_);
+ if (browser) {
+ extension_global_error_->ShowBubbleView(browser);
+ did_show_alert = true;
+ }
} else {
// First run. Just acknowledge all the extensions, silently, by
// shortcutting the display of the UI and going straight to the
// callback for pressing the Accept button.
- HandleExtensionAlertAccept(*global_error.get(), NULL);
+ HandleExtensionAlertAccept();
}
}
+
+ if (!did_show_alert)
+ extension_global_error_.reset();
}
-void ExtensionService::ShowExtensionAlert(ExtensionGlobalError* global_error) {
- CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
- Browser* browser = BrowserList::GetLastActiveWithProfile(profile_);
- if (browser) {
- global_error->ShowBubbleView(browser);
- }
+void ExtensionService::HandleExtensionAlertClosed() {
+ extension_global_error_.reset();
}
-void ExtensionService::HandleExtensionAlertAccept(
- const ExtensionGlobalError& global_error, Browser* browser) {
+void ExtensionService::HandleExtensionAlertAccept() {
const ExtensionIdSet *extension_ids =
- global_error.get_external_extension_ids();
+ extension_global_error_->get_external_extension_ids();
for (ExtensionIdSet::const_iterator iter = extension_ids->begin();
iter != extension_ids->end(); ++iter) {
AcknowledgeExternalExtension(*iter);
}
- extension_ids = global_error.get_blacklisted_extension_ids();
+ extension_ids = extension_global_error_->get_blacklisted_extension_ids();
for (ExtensionIdSet::const_iterator iter = extension_ids->begin();
iter != extension_ids->end(); ++iter) {
extension_prefs_->AcknowledgeBlacklistedExtension(*iter);
}
- extension_ids = global_error.get_orphaned_extension_ids();
+ extension_ids = extension_global_error_->get_orphaned_extension_ids();
for (ExtensionIdSet::const_iterator iter = extension_ids->begin();
iter != extension_ids->end(); ++iter) {
extension_prefs_->AcknowledgeOrphanedExtension(*iter);
@@ -1896,11 +1893,9 @@
extension_prefs_->AcknowledgeExternalExtension(id);
}
-void ExtensionService::HandleExtensionAlertDetails(
- const ExtensionGlobalError& global_error, Browser* browser) {
- if (browser) {
- browser->ShowExtensionsTab();
- }
+void ExtensionService::HandleExtensionAlertDetails(Browser* browser) {
+ DCHECK(browser);
+ browser->ShowExtensionsTab();
}
void ExtensionService::UnloadExtension(
diff --git a/chrome/browser/extensions/extension_service.h b/chrome/browser/extensions/extension_service.h
index 3467812b..3e105e6a 100644
--- a/chrome/browser/extensions/extension_service.h
+++ b/chrome/browser/extensions/extension_service.h
@@ -533,8 +533,7 @@
// Marks alertable extensions as acknowledged, after the user presses the
// accept button.
- void HandleExtensionAlertAccept(const ExtensionGlobalError& global_error,
- Browser* browser);
+ void HandleExtensionAlertAccept();
// Given a (presumably just-installed) extension id, mark that extension as
// acknowledged.
@@ -542,11 +541,10 @@
// Opens the Extensions page because the user wants to get more details
// about the alerts.
- void HandleExtensionAlertDetails(const ExtensionGlobalError& global_error,
- Browser* browser);
+ void HandleExtensionAlertDetails(Browser* browser);
- // Displays the extension alert in the last-active browser window.
- void ShowExtensionAlert(ExtensionGlobalError* global_error);
+ // Called when the extension alert is closed.
+ void HandleExtensionAlertClosed();
// content::NotificationObserver
virtual void Observe(int type,
@@ -847,6 +845,8 @@
ShellIntegration::ShortcutInfo shortcut_info_;
ImageLoadingTracker tracker_;
+ scoped_ptr<ExtensionGlobalError> extension_global_error_;
+
FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
InstallAppsWithUnlimtedStorage);
FRIEND_TEST_ALL_PREFIXES(ExtensionServiceTest,
diff --git a/chrome/browser/extensions/extension_warning_set_unittest.cc b/chrome/browser/extensions/extension_warning_set_unittest.cc
index 8d02d08..48620640 100644
--- a/chrome/browser/extensions/extension_warning_set_unittest.cc
+++ b/chrome/browser/extensions/extension_warning_set_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
@@ -7,8 +7,6 @@
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
-class ExtensionglobalError;
-
namespace {
class MockExtensionWarningSet : public ExtensionWarningSet {
