Refactor ExtensionErrorUI to remove ExtensionService's work

Create an ExtensionErrorController to be in charge of ExtensionErrorUI so that
ExtensionService doens't have to know as much about it.
And, of course, add a few tests for otherwise untested behavior.

BUG=351891

Review URL: https://codereview.chromium.org/227373007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@262893 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/extensions/extension_error_controller.cc

+// Copyright 2014 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.
+
+#include "chrome/browser/extensions/extension_error_controller.h"
+
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/pending_extension_manager.h"
+#include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_system.h"
+#include "extensions/common/extension_set.h"
+
+namespace extensions {
+
+namespace {
+
+ExtensionErrorController::UICreateMethod g_create_ui =
+    ExtensionErrorUI::Create;
+
+}
+
+ExtensionErrorController::ExtensionErrorController(
+    content::BrowserContext* context,
+    bool is_first_run)
+    : browser_context_(context),
+      is_first_run_(is_first_run) {}
+
+ExtensionErrorController::~ExtensionErrorController() {}
+
+void ExtensionErrorController::ShowErrorIfNeeded() {
+  IdentifyAlertableExtensions();
+
+  if (!blacklisted_extensions_.is_empty()) {
+    if (!is_first_run_) {
+      error_ui_.reset(g_create_ui(this));
+      if (!error_ui_->ShowErrorInBubbleView())  // Couldn't find a browser.
+        error_ui_.reset();
+    } 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.
+      OnAlertClosed();
+    }
+  }
+}
+
+// static
+void ExtensionErrorController::SetUICreateMethodForTesting(
+    UICreateMethod method) {
+  g_create_ui = method;
+}
+
+content::BrowserContext* ExtensionErrorController::GetContext() {
+  return browser_context_;
+}
+
+const ExtensionSet& ExtensionErrorController::GetExternalExtensions() {
+  return external_extensions_;
+}
+
+const ExtensionSet& ExtensionErrorController::GetBlacklistedExtensions() {
+  return blacklisted_extensions_;
+}
+
+void ExtensionErrorController::OnAlertAccept() {
+  error_ui_->Close();
+}
+
+void ExtensionErrorController::OnAlertDetails() {
+  error_ui_->ShowExtensions();
+
+  // ShowExtensions() may cause the error UI to close synchronously, e.g. if it
+  // causes a navigation.
+  if (error_ui_)
+    error_ui_->Close();
+}
+
+void ExtensionErrorController::OnAlertClosed() {
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(browser_context_);
+  for (ExtensionSet::const_iterator iter = blacklisted_extensions_.begin();
+       iter != blacklisted_extensions_.end();
+       ++iter) {
+    prefs->AcknowledgeBlacklistedExtension((*iter)->id());
+  }
+
+  error_ui_.reset();
+}
+
+void ExtensionErrorController::IdentifyAlertableExtensions() {
+  ExtensionRegistry* registry = ExtensionRegistry::Get(browser_context_);
+  ExtensionPrefs* prefs = ExtensionPrefs::Get(browser_context_);
+
+  // Build up the lists of extensions that require acknowledgment. If this is
+  // the first time, grandfather extensions that would have caused
+  // notification.
+
+  const ExtensionSet& blacklisted_set = registry->blacklisted_extensions();
+  for (ExtensionSet::const_iterator iter = blacklisted_set.begin();
+       iter != blacklisted_set.end();
+       ++iter) {
+    if (!prefs->IsBlacklistedExtensionAcknowledged((*iter)->id()))
+      blacklisted_extensions_.Insert(*iter);
+  }
+
+  ExtensionSystem* system = ExtensionSystem::Get(browser_context_);
+  ManagementPolicy* management_policy = system->management_policy();
+  PendingExtensionManager* pending_extension_manager =
+      system->extension_service()->pending_extension_manager();
+  const ExtensionSet& enabled_set = registry->enabled_extensions();
+
+  for (ExtensionSet::const_iterator iter = enabled_set.begin();
+       iter != enabled_set.end();
+       ++iter) {
+    const Extension* extension = *iter;
+
+    // Skip for extensions that have pending updates. They will be checked again
+    // once the pending update is finished.
+    if (pending_extension_manager->IsIdPending(extension->id()))
+      continue;
+
+    // Extensions disabled by policy. Note: this no longer includes blacklisted
+    // extensions, though we still show the same UI.
+    if (!management_policy->UserMayLoad(extension, NULL /* ignore error */)) {
+      if (!prefs->IsBlacklistedExtensionAcknowledged(extension->id()))
+        blacklisted_extensions_.Insert(extension);
+    }
+  }
+}
+
+}  // namespace extensions

chrome/browser/extensions/extension_error_controller.h

+// Copyright 2014 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_CONTROLLER_H_
+#define CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_CONTROLLER_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/extensions/extension_error_ui.h"
+#include "extensions/common/extension_set.h"
+
+namespace content {
+class BrowserContext;
+}
+
+namespace extensions {
+
+// The controller for the ExtensionErrorUI. This examines extensions for any
+// blacklisted or external extensions in order to notify the user with an error.
+// On acceptance, this will acknowledge the extensions.
+class ExtensionErrorController : public ExtensionErrorUI::Delegate {
+ public:
+  typedef ExtensionErrorUI* (*UICreateMethod)(ExtensionErrorUI::Delegate*);
+
+  ExtensionErrorController(content::BrowserContext* context, bool is_first_run);
+  virtual ~ExtensionErrorController();
+
+  void ShowErrorIfNeeded();
+
+  // Set the factory method for creating a new ExtensionErrorUI.
+  static void SetUICreateMethodForTesting(UICreateMethod method);
+
+ private:
+  // ExtensionErrorUI::Delegate implementation:
+  virtual content::BrowserContext* GetContext() OVERRIDE;
+  virtual const ExtensionSet& GetExternalExtensions() OVERRIDE;
+  virtual const ExtensionSet& GetBlacklistedExtensions() OVERRIDE;
+  virtual void OnAlertDetails() OVERRIDE;
+  virtual void OnAlertAccept() OVERRIDE;
+  virtual void OnAlertClosed() OVERRIDE;
+
+  // Find any extensions that the user should be alerted about (like blacklisted
+  // extensions).
+  void IdentifyAlertableExtensions();
+
+  // TODO(rdevlin.cronin): We never seem to use |external_extensions_| here,
+  // but we do warn about them. Investigate more.
+  ExtensionSet external_extensions_;
+
+  // The extensions that are blacklisted and need user approval.
+  ExtensionSet blacklisted_extensions_;
+
+  // The UI component of this controller.
+  scoped_ptr<ExtensionErrorUI> error_ui_;
+
+  // The BrowserContext with which we are associated.
+  content::BrowserContext* browser_context_;
+
+  // Whether or not this is the first run. If it is, we avoid noisy errors, and
+  // silently acknowledge blacklisted extensions.
+  bool is_first_run_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionErrorController);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_CONTROLLER_H_

chrome/browser/extensions/extension_error_controller_unittest.cc

+// Copyright 2014 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.
+
+#include "base/memory/ref_counted.h"
+#include "chrome/browser/extensions/extension_error_controller.h"
+#include "chrome/browser/extensions/extension_error_ui.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_service_unittest.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/test/base/testing_profile.h"
+#include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_builder.h"
+#include "extensions/common/value_builder.h"
+
+namespace extensions {
+
+namespace {
+
+// Create a mock for the UI component of the error alert that is shown for
+// blacklisted extensions. This allows us to test which extensions the alert
+// is showing, and also eliminates the UI component (since this is a unit
+// test).
+class MockExtensionErrorUI : public ExtensionErrorUI {
+ public:
+  explicit MockExtensionErrorUI(ExtensionErrorUI::Delegate* delegate);
+  virtual ~MockExtensionErrorUI();
+
+  // Wrappers around the similar methods in ExtensionErrorUI.
+  void CloseUI();
+  void Accept();
+  void Details();
+
+  ExtensionErrorUI::Delegate* delegate() { return delegate_; }
+
+ private:
+  // ExtensionErrorUI implementation.
+  virtual bool ShowErrorInBubbleView() OVERRIDE;
+  virtual void ShowExtensions() OVERRIDE;
+  virtual void Close() OVERRIDE;
+
+  // Keep a copy of the delegate around for ourselves.
+  ExtensionErrorUI::Delegate* delegate_;
+};
+
+// We use this as a slight hack to get the created Error UI, if any. We should
+// only ever have one (since this is a single-profile test), and this avoids
+// the need for any kind of accessor to the ErrorController from
+// ExtensionService.
+MockExtensionErrorUI* g_error_ui = NULL;
+
+MockExtensionErrorUI::MockExtensionErrorUI(
+    ExtensionErrorUI::Delegate* delegate)
+    : ExtensionErrorUI(delegate),
+      delegate_(delegate) {
+  // We should never make more than one of these in a test.
+  DCHECK(!g_error_ui);
+  g_error_ui = this;
+}
+
+MockExtensionErrorUI::~MockExtensionErrorUI() {
+  g_error_ui = NULL;
+}
+
+void MockExtensionErrorUI::CloseUI() {
+  BubbleViewDidClose();
+}
+
+void MockExtensionErrorUI::Accept() {
+  BubbleViewAcceptButtonPressed();
+}
+
+void MockExtensionErrorUI::Details() {
+  BubbleViewCancelButtonPressed();
+}
+
+bool MockExtensionErrorUI::ShowErrorInBubbleView() {
+  return true;
+}
+
+void MockExtensionErrorUI::ShowExtensions() {}
+
+void MockExtensionErrorUI::Close() {
+  CloseUI();
+}
+
+ExtensionErrorUI* CreateMockUI(ExtensionErrorUI::Delegate* delegate) {
+  return new MockExtensionErrorUI(delegate);
+}
+
+// Builds and returns a simple extension.
+scoped_refptr<const Extension> BuildExtension() {
+  return ExtensionBuilder()
+      .SetManifest(DictionaryBuilder().Set("name", "My Wonderful Extension")
+                                      .Set("version", "0.1.1.0")
+                                      .Set("manifest_version", 2)
+                                      .Build())
+      .Build();
+}
+
+}  // namespace
+
+class ExtensionErrorControllerUnitTest : public ExtensionServiceTestBase {
+ protected:
+  virtual void SetUp() OVERRIDE;
+
+  // Add an extension to chrome, and mark it as blacklisted in the prefs.
+  testing::AssertionResult AddBlacklistedExtension(const Extension* extension);
+
+  // Return the ExtensionPrefs associated with the test.
+  ExtensionPrefs* GetPrefs();
+
+  Profile* profile() { return profile_.get(); }
+};
+
+void ExtensionErrorControllerUnitTest::SetUp() {
+  ExtensionServiceTestBase::SetUp();
+  // Make sure we use the mock UI instead of the real UI.
+  ExtensionErrorController::SetUICreateMethodForTesting(CreateMockUI);
+
+  // We don't want a first-run ExtensionService, since we ignore warnings
+  // for new profiles.
+  ExtensionServiceInitParams params = CreateDefaultInitParams();
+  params.is_first_run = false;
+  InitializeExtensionService(params);
+}
+
+testing::AssertionResult
+ExtensionErrorControllerUnitTest::AddBlacklistedExtension(
+    const Extension* extension) {
+  GetPrefs()->SetExtensionBlacklisted(extension->id(), true);
+  service_->AddExtension(extension);
+
+  // Make sure the extension is added to the blacklisted set.
+  if (!ExtensionRegistry::Get(profile())->blacklisted_extensions()
+          .Contains(extension->id())) {
+    return testing::AssertionFailure()
+        << "Failed to add blacklisted extension.";
+  }
+
+  return testing::AssertionSuccess();
+}
+
+ExtensionPrefs* ExtensionErrorControllerUnitTest::GetPrefs() {
+  return ExtensionPrefs::Get(profile());
+}
+
+// Test that closing the extension alert for blacklisted extensions counts
+// as acknowledging them in the prefs.
+TEST_F(ExtensionErrorControllerUnitTest, ClosingAcknowledgesBlacklisted) {
+  // Add a blacklisted extension.
+  scoped_refptr<const Extension> extension = BuildExtension();
+  ASSERT_TRUE(AddBlacklistedExtension(extension));
+
+  service_->Init();
+
+  // Make sure that we created an error "ui" to warn about the blacklisted
+  // extension.
+  ASSERT_TRUE(g_error_ui);
+  ExtensionErrorUI::Delegate* delegate = g_error_ui->delegate();
+  ASSERT_TRUE(delegate);
+
+  // Make sure that the blacklisted extension is reported (and that no other
+  // extensions are).
+  const ExtensionSet& delegate_blacklisted_extensions =
+      delegate->GetBlacklistedExtensions();
+  EXPECT_EQ(1u, delegate_blacklisted_extensions.size());
+  EXPECT_TRUE(delegate_blacklisted_extensions.Contains(extension->id()));
+
+  // Close, and verify that the extension ids now acknowledged.
+  g_error_ui->CloseUI();
+  EXPECT_TRUE(GetPrefs()->IsBlacklistedExtensionAcknowledged(extension->id()));
+  // Verify we cleaned up after ourselves.
+  EXPECT_FALSE(g_error_ui);
+}
+
+// Test that clicking "accept" on the extension alert counts as acknowledging
+// blacklisted extensions.
+TEST_F(ExtensionErrorControllerUnitTest, AcceptingAcknowledgesBlacklisted) {
+  // Add a blacklisted extension.
+  scoped_refptr<const Extension> extension = BuildExtension();
+  ASSERT_TRUE(AddBlacklistedExtension(extension));
+
+  service_->Init();
+
+  // Make sure that we created an error "ui" to warn about the blacklisted
+  // extension.
+  ASSERT_TRUE(g_error_ui);
+
+  // Accept, and verify that the extension ids now acknowledged.
+  g_error_ui->Accept();
+  EXPECT_TRUE(GetPrefs()->IsBlacklistedExtensionAcknowledged(extension->id()));
+  // Verify we cleaned up after ourselves.
+  EXPECT_FALSE(g_error_ui);
+}
+
+// Test that we don't warn for extensions which are blacklisted, but have
+// already been acknowledged.
+TEST_F(ExtensionErrorControllerUnitTest, DontWarnForAcknowledgedBlacklisted) {
+  scoped_refptr<const Extension> extension = BuildExtension();
+  ASSERT_TRUE(AddBlacklistedExtension(extension));
+
+  GetPrefs()->AcknowledgeBlacklistedExtension(extension->id());
+
+  service_->Init();
+
+  // We should never have made an alert, because the extension should already
+  // be acknowledged.
+  ASSERT_FALSE(g_error_ui);
+}
+
+}  // namespace extensions

chrome/browser/extensions/extension_error_ui.cc

@@ -7,60 +7,40 @@
 #include "base/logging.h"
 #include "base/strings/string16.h"
 #include "base/strings/utf_string_conversions.h"
-#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/ui/global_error/global_error.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/extension_set.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
-using extensions::ExtensionIdSet;
+namespace extensions {
 
-ExtensionErrorUI::ExtensionErrorUI(ExtensionService* extension_service)
-    : extension_service_(extension_service),
-      external_extension_ids_(new ExtensionIdSet),
-      blacklisted_extension_ids_(new ExtensionIdSet) {
-  DCHECK(extension_service_);
-}
-
-ExtensionErrorUI::~ExtensionErrorUI() {
-}
-
-void ExtensionErrorUI::AddExternalExtension(const std::string& id) {
-  external_extension_ids_->insert(id);
-}
-
-void ExtensionErrorUI::AddBlacklistedExtension(const std::string& id) {
-  blacklisted_extension_ids_->insert(id);
-}
+namespace {
 
-base::string16 ExtensionErrorUI::GenerateMessageSection(
-    const ExtensionIdSet* extensions,
-    int extension_template_message_id,
-    int app_template_message_id) {
-  CHECK(extensions);
+base::string16 GenerateMessageSection(const ExtensionSet& extensions,
+                                      int extension_template_message_id,
+                                      int app_template_message_id) {
   CHECK(extension_template_message_id);
   CHECK(app_template_message_id);
   base::string16 message;
 
-  for (ExtensionIdSet::const_iterator iter = extensions->begin();
-       iter != extensions->end(); ++iter) {
-    const extensions::Extension* e =
-        extension_service_->GetInstalledExtension(*iter);
+  for (ExtensionSet::const_iterator iter = extensions.begin();
+       iter != extensions.end();
+       ++iter) {
     message += l10n_util::GetStringFUTF16(
-        e->is_app() ? app_template_message_id : extension_template_message_id,
-        base::UTF8ToUTF16(e->name())) + base::char16('\n');
+        (*iter)->is_app() ? app_template_message_id
+                          : extension_template_message_id,
+        base::UTF8ToUTF16((*iter)->name())) + base::char16('\n');
   }
   return message;
 }
 
-base::string16 ExtensionErrorUI::GenerateMessage() {
-  return GenerateMessageSection(external_extension_ids_.get(),
-                                IDS_EXTENSION_ALERT_ITEM_EXTERNAL,
-                                IDS_APP_ALERT_ITEM_EXTERNAL) +
-         GenerateMessageSection(blacklisted_extension_ids_.get(),
-                                IDS_EXTENSION_ALERT_ITEM_BLACKLISTED,
-                                IDS_APP_ALERT_ITEM_BLACKLISTED);
-}
+}  // namespace
+
+ExtensionErrorUI::ExtensionErrorUI(Delegate* delegate) : delegate_(delegate) {}
+
+ExtensionErrorUI::~ExtensionErrorUI() {}
 
 std::vector<base::string16> ExtensionErrorUI::GetBubbleViewMessages() {
   if (message_.empty()) {
@@ -84,14 +64,24 @@ base::string16 ExtensionErrorUI::GetBubbleViewCancelButtonLabel() {
 }
 
 void ExtensionErrorUI::BubbleViewDidClose() {
-  // This call deletes ExtensionErrorUI object referenced by this.
-  extension_service_->HandleExtensionAlertClosed();
+  delegate_->OnAlertClosed();
 }
 
 void ExtensionErrorUI::BubbleViewAcceptButtonPressed() {
-  extension_service_->HandleExtensionAlertAccept();
+  delegate_->OnAlertAccept();
 }
 
 void ExtensionErrorUI::BubbleViewCancelButtonPressed() {
-  extension_service_->HandleExtensionAlertDetails();
+  delegate_->OnAlertDetails();
 }
+
+base::string16 ExtensionErrorUI::GenerateMessage() {
+  return GenerateMessageSection(delegate_->GetExternalExtensions(),
+                                IDS_EXTENSION_ALERT_ITEM_EXTERNAL,
+                                IDS_APP_ALERT_ITEM_EXTERNAL) +
+         GenerateMessageSection(delegate_->GetBlacklistedExtensions(),
+                                IDS_EXTENSION_ALERT_ITEM_BLACKLISTED,
+                                IDS_APP_ALERT_ITEM_BLACKLISTED);
+}
+
+}  // namespace extensions

chrome/browser/extensions/extension_error_ui.h

@@ -5,36 +5,47 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_UI_H_
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_UI_H_
 
-#include "base/basictypes.h"
-#include "base/compiler_specific.h"
-#include "chrome/browser/ui/global_error/global_error.h"
-#include "extensions/common/extension.h"
+#include <vector>
 
-class Browser;
-class ExtensionService;
+#include "base/macros.h"
+#include "base/strings/string16.h"
+
+namespace content {
+class BrowserContext;
+}
+
+namespace extensions {
+
+class ExtensionSet;
 
 // This class encapsulates the UI we want to show users when certain events
 // occur related to installed extensions.
 class ExtensionErrorUI {
  public:
-  static ExtensionErrorUI* Create(ExtensionService* extension_service);
+  class Delegate {
+   public:
+    // Get the BrowserContext associated with this UI.
+    virtual content::BrowserContext* GetContext() = 0;
 
-  virtual ~ExtensionErrorUI();
+    // Get the set of external extensions to warn the user about.
+    virtual const ExtensionSet& GetExternalExtensions() = 0;
+
+    // Get the set of blacklisted extensions to warn the user about.
+    virtual const ExtensionSet& GetBlacklistedExtensions() = 0;
+
+    // Handle the user clicking to get more details on the extension alert.
+    virtual void OnAlertDetails() = 0;
+
+    // Handle the user clicking "accept" on the extension alert.
+    virtual void OnAlertAccept() = 0;
 
-  // Inform us that a given extension is of a certain type that the user
-  // hasn't yet acknowledged.
-  void AddExternalExtension(const std::string& id);
-  void AddBlacklistedExtension(const std::string& id);
+    // Handle the alert closing.
+    virtual void OnAlertClosed() = 0;
+  };
 
-  // Returns sets replaying the IDs that have been added with the
-  // Add[...]Extension methods.
-  const extensions::ExtensionIdSet* get_external_extension_ids() const {
-    return external_extension_ids_.get();
-  }
+  static ExtensionErrorUI* Create(Delegate* delegate);
 
-  const extensions::ExtensionIdSet* get_blacklisted_extension_ids() const {
-    return blacklisted_extension_ids_.get();
-  }
+  virtual ~ExtensionErrorUI();
 
   // Shows the installation error in a bubble view. Should return true if a
   // bubble is shown, false if one could not be shown.
@@ -51,9 +62,7 @@ class ExtensionErrorUI {
   virtual void Close() = 0;
 
  protected:
-  explicit ExtensionErrorUI(ExtensionService* extension_service);
-
-  ExtensionService* extension_service() const { return extension_service_; }
+  explicit ExtensionErrorUI(Delegate* delegate);
 
   // Model methods for the bubble view.
   base::string16 GetBubbleViewTitle();
@@ -68,23 +77,15 @@ class ExtensionErrorUI {
   void BubbleViewCancelButtonPressed();
 
  private:
-  bool should_delete_self_on_close_;
-  ExtensionService* extension_service_;
-  scoped_ptr<extensions::ExtensionIdSet> external_extension_ids_;
-  scoped_ptr<extensions::ExtensionIdSet> blacklisted_extension_ids_;
-  base::string16 message_;  // Displayed in the body of the alert.
+  base::string16 GenerateMessage();
 
-  // For a given set of extension IDs, generates appropriate text
-  // describing what the user needs to know about them.
-  base::string16 GenerateMessageSection(
-      const extensions::ExtensionIdSet* extensions,
-      int extension_template_message_id,
-      int app_template_message_id);
+  Delegate* delegate_;
 
-  // Generates the message displayed in the body of the alert.
-  base::string16 GenerateMessage();
+  base::string16 message_;  // Displayed in the body of the alert.
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionErrorUI);
 };
 
+}  // namespace extensions
+
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_UI_H_

chrome/browser/extensions/extension_error_ui_android.cc

@@ -4,13 +4,14 @@
 
 #include "chrome/browser/extensions/extension_error_ui_android.h"
 
+#include "base/logging.h"
+
+namespace extensions {
+
 ExtensionErrorUIAndroid::ExtensionErrorUIAndroid(
-    ExtensionService* extension_service)
-    : ExtensionErrorUI(extension_service) {
-}
+    ExtensionErrorUI::Delegate* delegate) : ExtensionErrorUI(delegate) {}
 
-ExtensionErrorUIAndroid::~ExtensionErrorUIAndroid() {
-}
+ExtensionErrorUIAndroid::~ExtensionErrorUIAndroid() {}
 
 // ExtensionErrorUI implementation:
 bool ExtensionErrorUIAndroid::ShowErrorInBubbleView() {
@@ -28,6 +29,8 @@ void ExtensionErrorUIAndroid::Close() {
 
 // static
 ExtensionErrorUI* ExtensionErrorUI::Create(
-    ExtensionService* extension_service) {
-  return new ExtensionErrorUIAndroid(extension_service);
+    ExtensionErrorUI::Delegate* delegate) {
+  return new ExtensionErrorUIAndroid(delegate);
 }
+
+}  // namespace extensions

chrome/browser/extensions/extension_error_ui_android.h

@@ -5,15 +5,14 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_UI_ANDROID_H_
 #define CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_UI_ANDROID_H_
 
-#include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "chrome/browser/extensions/extension_error_ui.h"
 
-class ExtensionService;
+namespace extensions {
 
 class ExtensionErrorUIAndroid : public ExtensionErrorUI {
  public:
-  explicit ExtensionErrorUIAndroid(ExtensionService* extension_service);
+  explicit ExtensionErrorUIAndroid(ExtensionErrorUI::Delegate* delegate);
   virtual ~ExtensionErrorUIAndroid();
 
   // ExtensionErrorUI implementation:
@@ -25,4 +24,6 @@ class ExtensionErrorUIAndroid : public ExtensionErrorUI {
   DISALLOW_COPY_AND_ASSIGN(ExtensionErrorUIAndroid);
 };
 
+}  // namespace extensions
+
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_UI_ANDROID_H_

chrome/browser/extensions/extension_error_ui_default.cc

@@ -5,15 +5,20 @@
 #include "chrome/browser/extensions/extension_error_ui_default.h"
 
 #include "base/logging.h"
+#include "base/strings/string_util.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/chrome_pages.h"
 #include "chrome/browser/ui/global_error/global_error_bubble_view_base.h"
 
+namespace extensions {
+
 ExtensionErrorUIDefault::ExtensionErrorUIDefault(
-    ExtensionService* extension_service)
-    : ExtensionErrorUI(extension_service),
+    ExtensionErrorUI::Delegate* delegate)
+    : ExtensionErrorUI(delegate),
+      profile_(Profile::FromBrowserContext(delegate->GetContext())),
       browser_(NULL),
       global_error_(new ExtensionGlobalError(this)) {
 }
@@ -23,8 +28,7 @@ ExtensionErrorUIDefault::~ExtensionErrorUIDefault() {
 
 bool ExtensionErrorUIDefault::ShowErrorInBubbleView() {
   Browser* browser =
-      chrome::FindLastActiveWithProfile(extension_service()->profile(),
-                                        chrome::GetActiveDesktop());
+      chrome::FindLastActiveWithProfile(profile_, chrome::GetActiveDesktop());
   if (!browser)
     return false;
 
@@ -35,7 +39,7 @@ bool ExtensionErrorUIDefault::ShowErrorInBubbleView() {
 
 void ExtensionErrorUIDefault::ShowExtensions() {
   DCHECK(browser_);
-  chrome::ShowExtensions(browser_, std::string());
+  chrome::ShowExtensions(browser_, base::EmptyString());
 }
 
 void ExtensionErrorUIDefault::Close() {
@@ -90,15 +94,13 @@ base::string16 ExtensionErrorUIDefault::ExtensionGlobalError::
   return error_ui_->GetBubbleViewCancelButtonLabel();
 }
 
-void ExtensionErrorUIDefault::ExtensionGlobalError::
-    OnBubbleViewDidClose(Browser* browser) {
-  // Calling BubbleViewDidClose on |error_ui_| will delete it. It owns us, so
-  // |this| will be deleted too.
+void ExtensionErrorUIDefault::ExtensionGlobalError::OnBubbleViewDidClose(
+    Browser* browser) {
   error_ui_->BubbleViewDidClose();
 }
 
 void ExtensionErrorUIDefault::ExtensionGlobalError::
-    BubbleViewAcceptButtonPressed(Browser* browser) {
+      BubbleViewAcceptButtonPressed(Browser* browser) {
   error_ui_->BubbleViewAcceptButtonPressed();
 }
 
@@ -109,6 +111,8 @@ void ExtensionErrorUIDefault::ExtensionGlobalError::
 
 // static
 ExtensionErrorUI* ExtensionErrorUI::Create(
-    ExtensionService* extension_service) {
-  return new ExtensionErrorUIDefault(extension_service);
+    ExtensionErrorUI::Delegate* delegate) {
+  return new ExtensionErrorUIDefault(delegate);
 }
+
+}  // namespace extensions

chrome/browser/extensions/extension_error_ui_default.h

@@ -13,10 +13,13 @@
 
 class Browser;
 class ExtensionService;
+class Profile;
+
+namespace extensions {
 
 class ExtensionErrorUIDefault : public ExtensionErrorUI {
  public:
-  explicit ExtensionErrorUIDefault(ExtensionService* extension_service);
+  explicit ExtensionErrorUIDefault(ExtensionErrorUI::Delegate* delegate);
   virtual ~ExtensionErrorUIDefault();
 
   // ExtensionErrorUI implementation:
@@ -49,6 +52,9 @@ class ExtensionErrorUIDefault : public ExtensionErrorUI {
     DISALLOW_COPY_AND_ASSIGN(ExtensionGlobalError);
   };
 
+  // The profile associated with this error.
+  Profile* profile_;
+
   // The browser the bubble view was shown into.
   Browser* browser_;
 
@@ -57,4 +63,6 @@ class ExtensionErrorUIDefault : public ExtensionErrorUI {
   DISALLOW_COPY_AND_ASSIGN(ExtensionErrorUIDefault);
 };
 
+}  // namespace extensions
+
 #endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_UI_DEFAULT_H_

chrome/browser/extensions/extension_service.cc

@@ -32,8 +32,8 @@
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/data_deleter.h"
 #include "chrome/browser/extensions/extension_disabled_ui.h"
+#include "chrome/browser/extensions/extension_error_controller.h"
 #include "chrome/browser/extensions/extension_error_reporter.h"
-#include "chrome/browser/extensions/extension_error_ui.h"
 #include "chrome/browser/extensions/extension_garbage_collector.h"
 #include "chrome/browser/extensions/extension_install_ui.h"
 #include "chrome/browser/extensions/extension_special_storage_policy.h"
@@ -388,6 +388,9 @@ ExtensionService::ExtensionService(Profile* profile,
   // if required.
   is_first_run_ = !extension_prefs_->SetAlertSystemFirstRun();
 
+  error_controller_.reset(
+      new extensions::ExtensionErrorController(profile_, is_first_run_));
+
 #if defined(ENABLE_EXTENSIONS)
   extension_action_storage_manager_.reset(
       new extensions::ExtensionActionStorageManager(profile_));
@@ -1226,9 +1229,8 @@ void ExtensionService::CheckForExternalUpdates() {
 
   // Do any required work that we would have done after completion of all
   // providers.
-  if (external_extension_providers_.empty()) {
+  if (external_extension_providers_.empty())
     OnAllExternalProvidersReady();
-  }
 }
 
 void ExtensionService::OnExternalProviderReady(
@@ -1273,86 +1275,10 @@ void ExtensionService::OnAllExternalProvidersReady() {
     if (Manifest::IsExternalLocation(info->extension_location))
       CheckExternalUninstall(info->extension_id);
   }
-  IdentifyAlertableExtensions();
-}
-
-void ExtensionService::IdentifyAlertableExtensions() {
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  // Build up the lists of extensions that require acknowledgment. If this is
-  // the first time, grandfather extensions that would have caused
-  // notification.
-  extension_error_ui_.reset(ExtensionErrorUI::Create(this));
 
-  bool did_show_alert = false;
-  if (PopulateExtensionErrorUI(extension_error_ui_.get())) {
-    if (!is_first_run_) {
-      CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-      did_show_alert = extension_error_ui_->ShowErrorInBubbleView();
-    } 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();
-    }
-  }
+  error_controller_->ShowErrorIfNeeded();
 
   UpdateExternalExtensionAlert();
-
-  if (!did_show_alert)
-    extension_error_ui_.reset();
-}
-
-bool ExtensionService::PopulateExtensionErrorUI(
-    ExtensionErrorUI* extension_error_ui) {
-  bool needs_alert = false;
-
-  // Extensions that are blacklisted.
-  const ExtensionSet& blacklisted_set = registry_->blacklisted_extensions();
-  for (ExtensionSet::const_iterator it = blacklisted_set.begin();
-       it != blacklisted_set.end(); ++it) {
-    std::string id = (*it)->id();
-    if (!extension_prefs_->IsBlacklistedExtensionAcknowledged(id)) {
-      extension_error_ui->AddBlacklistedExtension(id);
-      needs_alert = true;
-    }
-  }
-
-  const ExtensionSet& enabled_set = registry_->enabled_extensions();
-  for (ExtensionSet::const_iterator iter = enabled_set.begin();
-       iter != enabled_set.end(); ++iter) {
-    const Extension* e = iter->get();
-
-    // Skip for extensions that have pending updates. They will be checked again
-    // once the pending update is finished.
-    if (pending_extension_manager()->IsIdPending(e->id()))
-      continue;
-
-    // Extensions disabled by policy. Note: this no longer includes blacklisted
-    // extensions, though we still show the same UI.
-    if (!system_->management_policy()->UserMayLoad(e, NULL)) {
-      if (!extension_prefs_->IsBlacklistedExtensionAcknowledged(e->id())) {
-        extension_error_ui->AddBlacklistedExtension(e->id());
-        needs_alert = true;
-      }
-    }
-  }
-
-  return needs_alert;
-}
-
-void ExtensionService::HandleExtensionAlertClosed() {
-  const ExtensionIdSet* extension_ids =
-      extension_error_ui_->get_blacklisted_extension_ids();
-  for (ExtensionIdSet::const_iterator iter = extension_ids->begin();
-       iter != extension_ids->end(); ++iter) {
-    extension_prefs_->AcknowledgeBlacklistedExtension(*iter);
-  }
-  extension_error_ui_.reset();
-}
-
-void ExtensionService::HandleExtensionAlertAccept() {
-  extension_error_ui_->Close();
 }
 
 void ExtensionService::AcknowledgeExternalExtension(const std::string& id) {
@@ -1415,14 +1341,6 @@ void ExtensionService::ReconcileKnownDisabled() {
                  base::Unretained(extension_prefs_)));
 }
 
-void ExtensionService::HandleExtensionAlertDetails() {
-  extension_error_ui_->ShowExtensions();
-  // ShowExtensions may cause the error UI to close synchronously, e.g. if it
-  // causes a navigation.
-  if (extension_error_ui_)
-    extension_error_ui_->Close();
-}
-
 void ExtensionService::UpdateExternalExtensionAlert() {
   if (!FeatureSwitch::prompt_for_external_extensions()->IsEnabled())
     return;
@@ -2315,7 +2233,7 @@ void ExtensionService::Observe(int type,
 }
 
 void ExtensionService::OnExtensionInstallPrefChanged() {
-  IdentifyAlertableExtensions();
+  error_controller_->ShowErrorIfNeeded();
   CheckManagementPolicy();
 }
 
@@ -2451,7 +2369,7 @@ void ExtensionService::ManageBlacklist(
   UpdateBlockedExtensions(blocked, unchanged);
   UpdateGreylistedExtensions(greylist, unchanged, state_map);
 
-  IdentifyAlertableExtensions();
+  error_controller_->ShowErrorIfNeeded();
 }
 
 namespace {

chrome/browser/extensions/extension_service.h

@@ -34,7 +34,6 @@
 #include "extensions/common/manifest.h"
 #include "extensions/common/one_shot_event.h"
 
-class ExtensionErrorUI;
 class GURL;
 class Profile;
 
@@ -49,6 +48,7 @@ class BrowserEventRouter;
 class ComponentLoader;
 class CrxInstaller;
 class ExtensionActionStorageManager;
+class ExtensionErrorController;
 class ExtensionGarbageCollector;
 class ExtensionRegistry;
 class ExtensionSystem;
@@ -408,17 +408,6 @@ class ExtensionService
 
   void OnAllExternalProvidersReady();
 
-  // Once all external providers are done, generates any needed alerts about
-  // extensions.
-  void IdentifyAlertableExtensions();
-
-  // Given an ExtensionErrorUI alert, populates it with any extensions that
-  // need alerting. Returns true if the alert should be displayed at all.
-  //
-  // This method takes the extension_error_ui argument rather than using
-  // the member variable to make it easier to test the method in isolation.
-  bool PopulateExtensionErrorUI(ExtensionErrorUI* extension_error_ui);
-
   // Checks if there are any new external extensions to notify the user about.
   void UpdateExternalExtensionAlert();
 
@@ -434,18 +423,6 @@ class ExtensionService
   // Disable extensions that are known to be disabled yet are currently enabled.
   void ReconcileKnownDisabled();
 
-  // Opens the Extensions page because the user wants to get more details
-  // about the alerts.
-  void HandleExtensionAlertDetails();
-
-  // Called when the extension alert is closed. Updates prefs and deletes
-  // the active |extension_error_ui_|.
-  void HandleExtensionAlertClosed();
-
-  // Marks alertable extensions as acknowledged, after the user presses the
-  // accept button.
-  void HandleExtensionAlertAccept();
-
   // content::NotificationObserver
   virtual void Observe(int type,
                        const content::NotificationSource& source,
@@ -708,7 +685,10 @@ class ExtensionService
   // avoid trying to unload the same extension twice.
   std::set<std::string> extensions_being_terminated_;
 
-  scoped_ptr<ExtensionErrorUI> extension_error_ui_;
+  // The controller for the UI that alerts the user about any blacklisted
+  // extensions.
+  scoped_ptr<extensions::ExtensionErrorController> error_controller_;
+
   // Sequenced task runner for extension related file operations.
   scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
 

chrome/browser/extensions/extension_sync_service.cc

@@ -10,7 +10,6 @@
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/extensions/app_sync_data.h"
-#include "chrome/browser/extensions/extension_error_ui.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_sync_data.h"
 #include "chrome/browser/extensions/extension_sync_service_factory.h"

chrome/browser/extensions/extension_sync_service.h

@@ -19,7 +19,6 @@
 #include "sync/api/sync_change.h"
 #include "sync/api/syncable_service.h"
 
-class ExtensionErrorUI;
 class ExtensionSyncData;
 class Profile;
 
@@ -142,7 +141,6 @@ class ExtensionSyncService : public syncer::SyncableService,
   extensions::PendingEnables pending_app_enables_;
   extensions::PendingEnables pending_extension_enables_;
 
-  scoped_ptr<ExtensionErrorUI> extension_error_ui_;
   // Sequenced task runner for extension related file operations.
   scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
 

chrome/chrome_browser_extensions.gypi

@@ -683,6 +683,8 @@
         'browser/extensions/extension_creator_filter.h',
         'browser/extensions/extension_disabled_ui.cc',
         'browser/extensions/extension_disabled_ui.h',
+        'browser/extensions/extension_error_controller.cc',
+        'browser/extensions/extension_error_controller.h',
         'browser/extensions/extension_error_reporter.cc',
         'browser/extensions/extension_error_reporter.h',
         'browser/extensions/extension_error_ui.cc',

chrome/chrome_tests_unit.gypi

@@ -936,6 +936,7 @@
         'browser/extensions/extension_action_unittest.cc',
         'browser/extensions/extension_context_menu_model_unittest.cc',
         'browser/extensions/extension_creator_filter_unittest.cc',
+        'browser/extensions/extension_error_controller_unittest.cc',
         'browser/extensions/extension_function_test_utils.cc',
         'browser/extensions/extension_function_test_utils.h',
         'browser/extensions/extension_garbage_collector_unittest.cc',