Add unit test for the Settings API Bubble.

Refactored the helper functions into views-specific code and rest (so that the non-views code can be used in more places).

Fixed a bug where if you have two extensions simultaneously overwriting settings, the bubble now knows which one is active.

Also prevent the bubble from writing the suppress flag when the user choses to disable the extension (that way they get the bubble again if the extension becomes enabled for some reason).

BUG=356204
R=sky@chromium.org, yoz@chromium.org
TBR=sky

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

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

chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc

@@ -111,16 +111,21 @@ SettingsOverridesAPI::GetFactoryInstance() {
 void SettingsOverridesAPI::SetPref(const std::string& extension_id,
                                    const std::string& pref_key,
                                    base::Value* value) {
-  PreferenceAPI::Get(profile_)->SetExtensionControlledPref(
-      extension_id,
-      pref_key,
-      kExtensionPrefsScopeRegular,
-      value);
+  PreferenceAPI* prefs = PreferenceAPI::Get(profile_);
+  if (!prefs)
+    return;  // Expected in unit tests.
+  prefs->SetExtensionControlledPref(extension_id,
+                                    pref_key,
+                                    kExtensionPrefsScopeRegular,
+                                    value);
 }
 
 void SettingsOverridesAPI::UnsetPref(const std::string& extension_id,
                                      const std::string& pref_key) {
-  PreferenceAPI::Get(profile_)->RemoveExtensionControlledPref(
+  PreferenceAPI* prefs = PreferenceAPI::Get(profile_);
+  if (!prefs)
+    return;  // Expected in unit tests.
+  prefs->RemoveExtensionControlledPref(
       extension_id,
       pref_key,
       kExtensionPrefsScopeRegular);

chrome/browser/extensions/settings_api_bubble_controller.cc

@@ -6,6 +6,7 @@
 
 #include "base/metrics/histogram.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/settings_api_helpers.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
 #include "chrome/common/extensions/manifest_handlers/settings_overrides_handler.h"
@@ -93,35 +94,34 @@ bool SettingsApiBubbleDelegate::ShouldIncludeExtension(
   if (prefs->HasSettingsApiBubbleBeenAcknowledged(extension_id))
     return false;
 
-  const SettingsOverrides* settings = SettingsOverrides::Get(extension);
-  if (!settings)
-    return false;
-
-  bool should_include = false;
+  const extensions::Extension* override = NULL;
   switch (type_) {
     case extensions::BUBBLE_TYPE_HOME_PAGE:
-      should_include = settings->homepage != NULL;
+      override = extensions::OverridesHomepage(profile_, NULL);
       break;
     case extensions::BUBBLE_TYPE_STARTUP_PAGES:
-      should_include = !settings->startup_pages.empty();
+      override = extensions::OverridesStartupPages(profile_, NULL);
       break;
     case extensions::BUBBLE_TYPE_SEARCH_ENGINE:
-      should_include = settings->search_engine != NULL;
+      override = extensions::OverridesSearchEngine(profile_, NULL);
       break;
   }
 
-  if (should_include && extension_id_ != extension_id) {
-    DCHECK(extension_id_.empty());
-    extension_id_ = extension_id;
-  }
-  return should_include;
+  if (!override || override->id() != extension->id())
+    return false;
+
+  extension_id_ = extension_id;
+  return true;
 }
 
 void SettingsApiBubbleDelegate::AcknowledgeExtension(
     const std::string& extension_id,
     ExtensionMessageBubbleController::BubbleAction user_action) {
-  extensions::ExtensionPrefs* prefs = extensions::ExtensionPrefs::Get(profile_);
-  prefs->SetSettingsApiBubbleBeenAcknowledged(extension_id, true);
+  if (user_action != ExtensionMessageBubbleController::ACTION_EXECUTE) {
+    extensions::ExtensionPrefs* prefs =
+        extensions::ExtensionPrefs::Get(profile_);
+    prefs->SetSettingsApiBubbleBeenAcknowledged(extension_id, true);
+  }
 }
 
 void SettingsApiBubbleDelegate::PerformAction(

chrome/browser/extensions/settings_api_helpers.cc

+// Copyright (c) 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/settings_api_helpers.h"
+
+#include "chrome/browser/extensions/api/preference/preference_api.h"
+#include "chrome/common/pref_names.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension_set.h"
+
+namespace extensions {
+
+const extensions::SettingsOverrides* FindOverridingExtension(
+    content::BrowserContext* browser_context,
+    SettingsApiOverrideType type,
+    const Extension** extension) {
+  const extensions::ExtensionSet& extensions =
+      extensions::ExtensionRegistry::Get(browser_context)->enabled_extensions();
+
+  for (extensions::ExtensionSet::const_iterator it = extensions.begin();
+       it != extensions.end();
+       ++it) {
+    const extensions::SettingsOverrides* settings =
+        extensions::SettingsOverrides::Get(*it);
+    if (settings) {
+      if (type == BUBBLE_TYPE_HOME_PAGE && !settings->homepage)
+        continue;
+      if (type == BUBBLE_TYPE_STARTUP_PAGES && settings->startup_pages.empty())
+        continue;
+      if (type == BUBBLE_TYPE_SEARCH_ENGINE && !settings->search_engine)
+        continue;
+
+      std::string key;
+      switch (type) {
+        case BUBBLE_TYPE_HOME_PAGE:
+          key = prefs::kHomePage;
+          break;
+        case BUBBLE_TYPE_STARTUP_PAGES:
+          key = prefs::kRestoreOnStartup;
+          break;
+        case BUBBLE_TYPE_SEARCH_ENGINE:
+          key = prefs::kDefaultSearchProviderEnabled;
+          break;
+      }
+
+      // Found an extension overriding the current type, check if primary.
+      PreferenceAPI* preference_api = PreferenceAPI::Get(browser_context);
+      if (preference_api &&  // Expected to be NULL in unit tests.
+          !preference_api->DoesExtensionControlPref((*it)->id(), key, NULL))
+        continue;  // Not primary.
+
+      // Found the primary extension, return its setting.
+      *extension = *it;
+      return settings;
+    }
+  }
+
+  return NULL;
+}
+
+const Extension* OverridesHomepage(content::BrowserContext* browser_context,
+                                   GURL* home_page_url) {
+  const extensions::Extension* extension = NULL;
+  const extensions::SettingsOverrides* settings =
+      FindOverridingExtension(
+          browser_context, BUBBLE_TYPE_HOME_PAGE, &extension);
+
+  if (settings && home_page_url)
+    *home_page_url = *settings->homepage;
+  return extension;
+}
+
+const Extension* OverridesStartupPages(content::BrowserContext* browser_context,
+                                       std::vector<GURL>* startup_pages) {
+  const extensions::Extension* extension = NULL;
+  const extensions::SettingsOverrides* settings =
+      FindOverridingExtension(
+          browser_context, BUBBLE_TYPE_STARTUP_PAGES, &extension);
+  if (settings && startup_pages) {
+    startup_pages->clear();
+    for (std::vector<GURL>::const_iterator it = settings->startup_pages.begin();
+         it != settings->startup_pages.end();
+         ++it)
+      startup_pages->push_back(GURL(*it));
+  }
+  return extension;
+}
+
+const Extension* OverridesSearchEngine(
+    content::BrowserContext* browser_context,
+    api::manifest_types::ChromeSettingsOverrides::Search_provider*
+        search_provider) {
+  const extensions::Extension* extension = NULL;
+  const extensions::SettingsOverrides* settings =
+      FindOverridingExtension(
+          browser_context, BUBBLE_TYPE_SEARCH_ENGINE, &extension);
+  if (settings && search_provider)
+    search_provider = settings->search_engine.get();
+  return extension;
+}
+
+}  // namespace extensions

chrome/browser/extensions/settings_api_helpers.h

+// Copyright (c) 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_SETTINGS_API_HELPERS_H_
+#define CHROME_BROWSER_EXTENSIONS_SETTINGS_API_HELPERS_H_
+
+#include "chrome/common/extensions/manifest_handlers/settings_overrides_handler.h"
+
+namespace content {
+class BrowserContext;
+}
+
+namespace extensions {
+
+struct SettingsOverrides;
+
+// Find which |extension| is overriding a particular |type| of setting. Returns
+// the SettingsOverride object, or NULL if no |extension| is overriding that
+// particular setting.
+const extensions::SettingsOverrides* FindOverridingExtension(
+    content::BrowserContext* browser_context,
+    SettingsApiOverrideType type,
+    const Extension** extension);
+
+// Returns which extension is overriding the homepage in a given
+// |browser_context|. |home_page_url|, if non-NULL, will contain the home_page
+// value the extension has set.
+const Extension* OverridesHomepage(content::BrowserContext* browser_context,
+                                   GURL* home_page_url);
+
+// Returns which extension is overriding the homepage in a given
+// |browser_context|. |startup_pages|, if non-NULL, will contain the vector of
+// startup page URLs the extension has set.
+const Extension* OverridesStartupPages(content::BrowserContext* browser_context,
+                                       std::vector<GURL>* startup_pages);
+
+// Returns which extension is overriding the search engine in a given
+// |browser_context|. |search_provider|, if non-NULL, will contain the search
+// provider the extension has set.
+const Extension* OverridesSearchEngine(
+    content::BrowserContext* browser_context,
+    api::manifest_types::ChromeSettingsOverrides::Search_provider*
+        search_provider);
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_SETTINGS_API_HELPERS_H_

chrome/browser/ui/views/extensions/extension_message_bubble_view.cc

@@ -12,10 +12,10 @@
 #include "chrome/browser/extensions/extension_message_bubble_controller.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/settings_api_bubble_controller.h"
+#include "chrome/browser/extensions/settings_api_helpers.h"
 #include "chrome/browser/extensions/suspicious_extension_bubble_controller.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
-#include "chrome/browser/ui/views/settings_api_bubble_helper_views.h"
 #include "chrome/browser/ui/views/toolbar/browser_actions_container.h"
 #include "chrome/browser/ui/views/toolbar/browser_actions_container_observer.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"

chrome/browser/ui/views/settings_api_bubble_helper_views.cc

@@ -5,8 +5,7 @@
 #include "chrome/browser/ui/views/settings_api_bubble_helper_views.h"
 
 #include "chrome/browser/extensions/settings_api_bubble_controller.h"
-#include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/extensions/settings_api_helpers.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/views/extensions/extension_message_bubble_view.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
@@ -14,8 +13,6 @@
 #include "chrome/browser/ui/views/toolbar/home_button.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
 #include "chrome/common/extensions/manifest_handlers/settings_overrides_handler.h"
-#include "content/public/browser/browser_context.h"
-#include "extensions/browser/extension_registry.h"
 
 namespace {
 
@@ -88,70 +85,4 @@ void MaybeShowExtensionControlledSearchNotification(
   }
 }
 
-const extensions::SettingsOverrides* FindOverridingExtension(
-    content::BrowserContext* browser_context,
-    SettingsApiOverrideType type,
-    const Extension** extension) {
-  const extensions::ExtensionSet& extensions =
-      extensions::ExtensionRegistry::Get(browser_context)->enabled_extensions();
-
-  for (extensions::ExtensionSet::const_iterator it = extensions.begin();
-       it != extensions.end();
-       ++it) {
-    const extensions::SettingsOverrides* settings =
-        extensions::SettingsOverrides::Get(*it);
-    if (settings) {
-      if ((type == BUBBLE_TYPE_HOME_PAGE && settings->homepage) ||
-          (type == BUBBLE_TYPE_STARTUP_PAGES &&
-              !settings->startup_pages.empty()) ||
-          (type == BUBBLE_TYPE_SEARCH_ENGINE && settings->search_engine)) {
-        *extension = *it;
-        return settings;
-      }
-    }
-  }
-
-  return NULL;
-}
-
-const Extension* OverridesHomepage(content::BrowserContext* browser_context,
-                                   GURL* home_page_url) {
-  const extensions::Extension* extension = NULL;
-  const extensions::SettingsOverrides* settings =
-      FindOverridingExtension(
-          browser_context, BUBBLE_TYPE_HOME_PAGE, &extension);
-  if (settings && home_page_url)
-    *home_page_url = *settings->homepage;
-  return extension;
-}
-
-const Extension* OverridesStartupPages(content::BrowserContext* browser_context,
-                                       std::vector<GURL>* startup_pages) {
-  const extensions::Extension* extension = NULL;
-  const extensions::SettingsOverrides* settings =
-      FindOverridingExtension(
-          browser_context, BUBBLE_TYPE_STARTUP_PAGES, &extension);
-  if (settings && startup_pages) {
-    startup_pages->clear();
-    for (std::vector<GURL>::const_iterator it = settings->startup_pages.begin();
-         it != settings->startup_pages.end();
-         ++it)
-      startup_pages->push_back(GURL(*it));
-  }
-  return extension;
-}
-
-const Extension* OverridesSearchEngine(
-    content::BrowserContext* browser_context,
-    api::manifest_types::ChromeSettingsOverrides::Search_provider*
-        search_provider) {
-  const extensions::Extension* extension = NULL;
-  const extensions::SettingsOverrides* settings =
-      FindOverridingExtension(
-          browser_context, BUBBLE_TYPE_SEARCH_ENGINE, &extension);
-  if (settings && search_provider)
-    search_provider = settings->search_engine.get();
-  return extension;
-}
-
 }  // namespace extensions

chrome/browser/ui/views/settings_api_bubble_helper_views.h

@@ -5,14 +5,11 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_SETTINGS_API_BUBBLE_HELPER_VIEWS_H_
 #define CHROME_BROWSER_UI_VIEWS_SETTINGS_API_BUBBLE_HELPER_VIEWS_H_
 
-#include "chrome/common/extensions/manifest_handlers/settings_overrides_handler.h"
-
 struct AutocompleteMatch;
 class Browser;
 class Profile;
 
 namespace content {
-class BrowserContext;
 class WebContents;
 }
 
@@ -31,34 +28,6 @@ void MaybeShowExtensionControlledSearchNotification(
     content::WebContents* web_contents,
     const AutocompleteMatch& match);
 
-// Find which |extension| is overriding a particular |type| of setting. Returns
-// the SettingsOverride object, or NULL if no |extension| is overriding that
-// particular setting.
-const extensions::SettingsOverrides* FindOverridingExtension(
-    content::BrowserContext* browser_context,
-    SettingsApiOverrideType type,
-    const Extension** extension);
-
-// Returns which extension is overriding the homepage in a given
-// |browser_context|. |home_page_url|, if non-NULL, will contain the home_page
-// value the extension has set.
-const Extension* OverridesHomepage(content::BrowserContext* browser_context,
-                                   GURL* home_page_url);
-
-// Returns which extension is overriding the homepage in a given
-// |browser_context|. |startup_pages|, if non-NULL, will contain the vector of
-// startup page URLs the extension has set.
-const Extension* OverridesStartupPages(content::BrowserContext* browser_context,
-                                       std::vector<GURL>* startup_pages);
-
-// Returns which extension is overriding the search engine in a given
-// |browser_context|. |search_provider|, if non-NULL, will contain the search
-// provider the extension has set.
-const Extension* OverridesSearchEngine(
-    content::BrowserContext* browser_context,
-    api::manifest_types::ChromeSettingsOverrides::Search_provider*
-        search_provider);
-
 }  // namespace extensions
 
 #endif  // CHROME_BROWSER_UI_VIEWS_SETTINGS_API_BUBBLE_HELPER_VIEWS_H_

chrome/chrome_browser_extensions.gypi

@@ -830,6 +830,8 @@
         'browser/extensions/sandboxed_unpacker.h',
         'browser/extensions/script_executor.cc',
         'browser/extensions/script_executor.h',
+        'browser/extensions/settings_api_helpers.cc',
+        'browser/extensions/settings_api_helpers.h',
         'browser/extensions/settings_api_bubble_controller.cc',
         'browser/extensions/settings_api_bubble_controller.h',
         'browser/extensions/standard_management_policy_provider.cc',

extensions/browser/extension_prefs.cc

@@ -1860,10 +1860,8 @@ void ExtensionPrefs::SetInstallSignature(
 std::string ExtensionPrefs::GetInstallParam(
     const std::string& extension_id) const {
   const base::DictionaryValue* extension = GetExtensionPref(extension_id);
-  if (!extension) {
-    NOTREACHED();
+  if (!extension)  // Expected during unit testing.
     return std::string();
-  }
   std::string install_parameter;
   if (!extension->GetString(kPrefInstallParam, &install_parameter))
     return std::string();