From 2397ce85ad0afff2ef85b01c5c55cc32902bbf87 Mon Sep 17 00:00:00 2001 From: "finnur@chromium.org" Date: Thu, 3 Apr 2014 16:20:23 +0000 Subject: [PATCH] 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 --- .../settings_overrides/settings_overrides_api.cc | 17 +- ...extension_message_bubble_controller_unittest.cc | 337 ++++++++++++++++----- .../extensions/settings_api_bubble_controller.cc | 30 +- chrome/browser/extensions/settings_api_helpers.cc | 103 +++++++ .../settings_api_helpers.h} | 24 +- .../extensions/extension_message_bubble_view.cc | 2 +- .../ui/views/settings_api_bubble_helper_views.cc | 71 +---- .../ui/views/settings_api_bubble_helper_views.h | 31 -- chrome/chrome_browser_extensions.gypi | 2 + extensions/browser/extension_prefs.cc | 4 +- 10 files changed, 399 insertions(+), 222 deletions(-) create mode 100644 chrome/browser/extensions/settings_api_helpers.cc copy chrome/browser/{ui/views/settings_api_bubble_helper_views.h => extensions/settings_api_helpers.h} (65%) diff --git a/chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc b/chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc index 73a3d60a2757..fe310902c66b 100644 --- a/chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc +++ b/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); diff --git a/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc b/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc index 656ae7788f24..f84e31a74cf4 100644 --- a/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc +++ b/chrome/browser/extensions/extension_message_bubble_controller_unittest.cc @@ -9,13 +9,23 @@ #include "chrome/browser/extensions/extension_function_test_utils.h" #include "chrome/browser/extensions/extension_message_bubble.h" #include "chrome/browser/extensions/extension_service.h" +#include "chrome/browser/extensions/settings_api_bubble_controller.h" #include "chrome/browser/extensions/suspicious_extension_bubble_controller.h" #include "chrome/browser/extensions/test_extension_system.h" #include "chrome/test/base/testing_profile.h" #include "content/public/test/test_browser_thread_bundle.h" #include "extensions/common/extension.h" +#include "extensions/common/extension_builder.h" #include "extensions/common/feature_switch.h" +namespace { + +const char kId1[] = "iccfkkhkfiphcjdakkmcjmkfboccmndk"; +const char kId2[] = "ajjhifimiemdpmophmkkkcijegphclbl"; +const char kId3[] = "ioibbbfddncmmabjmpokikkeiofalaek"; + +} // namespace + namespace extensions { class TestDelegate { @@ -97,6 +107,30 @@ class TestDevModeBubbleController } }; +// A test class for the SettingsApiBubbleController. +class TestSettingsApiBubbleController : public SettingsApiBubbleController, + public TestDelegate { + public: + TestSettingsApiBubbleController(Profile* profile, + SettingsApiOverrideType type) + : SettingsApiBubbleController(profile, type) {} + + virtual void OnBubbleAction() OVERRIDE { + ++action_button_callback_count_; + SettingsApiBubbleController::OnBubbleAction(); + } + + virtual void OnBubbleDismiss() OVERRIDE { + ++dismiss_button_callback_count_; + SettingsApiBubbleController::OnBubbleDismiss(); + } + + virtual void OnLinkClicked() OVERRIDE { + ++link_click_callback_count_; + SettingsApiBubbleController::OnLinkClicked(); + } +}; + // A fake bubble used for testing the controller. Takes an action that specifies // what should happen when the bubble is "shown" (the bubble is actually not // shown, the corresponding action is taken immediately). @@ -145,12 +179,76 @@ class FakeExtensionMessageBubble : public ExtensionMessageBubble { class ExtensionMessageBubbleTest : public testing::Test { public: - ExtensionMessageBubbleTest() { + ExtensionMessageBubbleTest() {} + + void LoadGenericExtension(const std::string& index, + const std::string& id, + Manifest::Location location) { + extensions::ExtensionBuilder builder; + builder.SetManifest(extensions::DictionaryBuilder() + .Set("name", std::string("Extension " + index)) + .Set("version", "1.0") + .Set("manifest_version", 2)); + builder.SetLocation(location); + builder.SetID(id); + service_->AddExtension(builder.Build().get()); + } + + void LoadExtensionWithAction(const std::string& index, + const std::string& id, + Manifest::Location location) { + extensions::ExtensionBuilder builder; + builder.SetManifest(extensions::DictionaryBuilder() + .Set("name", std::string("Extension " + index)) + .Set("version", "1.0") + .Set("manifest_version", 2) + .Set("browser_action", + extensions::DictionaryBuilder().Set( + "default_title", "Default title"))); + builder.SetLocation(location); + builder.SetID(id); + service_->AddExtension(builder.Build().get()); + } + + void LoadExtensionOverridingHome(const std::string& index, + const std::string& id, + Manifest::Location location) { + extensions::ExtensionBuilder builder; + builder.SetManifest(extensions::DictionaryBuilder() + .Set("name", std::string("Extension " + index)) + .Set("version", "1.0") + .Set("manifest_version", 2) + .Set("chrome_settings_overrides", + extensions::DictionaryBuilder().Set( + "homepage", "http://www.google.com"))); + builder.SetLocation(location); + builder.SetID(id); + service_->AddExtension(builder.Build().get()); + } + + void LoadExtensionOverridingStart(const std::string& index, + const std::string& id, + Manifest::Location location) { + extensions::ExtensionBuilder builder; + builder.SetManifest(extensions::DictionaryBuilder() + .Set("name", std::string("Extension " + index)) + .Set("version", "1.0") + .Set("manifest_version", 2) + .Set("chrome_settings_overrides", + extensions::DictionaryBuilder().Set( + "startup_pages", + extensions::ListBuilder().Append( + "http://www.google.com")))); + builder.SetLocation(location); + builder.SetID(id); + service_->AddExtension(builder.Build().get()); + } + + void Init() { // The two lines of magical incantation required to get the extension // service to work inside a unit test and access the extension prefs. thread_bundle_.reset(new content::TestBrowserThreadBundle); profile_.reset(new TestingProfile); - static_cast( ExtensionSystem::Get(profile()))->CreateExtensionService( CommandLine::ForCurrentProcess(), @@ -158,49 +256,8 @@ class ExtensionMessageBubbleTest : public testing::Test { false); service_ = profile_->GetExtensionService(); service_->Init(); - - std::string basic_extension = - "{\"name\": \"Extension #\"," - "\"version\": \"1.0\"," - "\"manifest_version\": 2}"; - std::string basic_extension_with_action = - "{\"name\": \"Extension #\"," - "\"version\": \"1.0\"," - "\"browser_action\": {" - " \"default_title\": \"Default title\"" - "}," - "\"manifest_version\": 2}"; - - std::string extension_data; - base::ReplaceChars(basic_extension_with_action, "#", "1", &extension_data); - scoped_refptr my_test_extension1( - CreateExtension( - Manifest::COMMAND_LINE, - extension_data, - "Autogenerated 1")); - - base::ReplaceChars(basic_extension, "#", "2", &extension_data); - scoped_refptr my_test_extension2( - CreateExtension( - Manifest::UNPACKED, - extension_data, - "Autogenerated 2")); - - base::ReplaceChars(basic_extension, "#", "3", &extension_data); - scoped_refptr regular_extension( - CreateExtension( - Manifest::EXTERNAL_POLICY, - extension_data, - "Autogenerated 3")); - - extension_id1_ = my_test_extension1->id(); - extension_id2_ = my_test_extension2->id(); - extension_id3_ = regular_extension->id(); - - service_->AddExtension(regular_extension); - service_->AddExtension(my_test_extension1); - service_->AddExtension(my_test_extension2); } + virtual ~ExtensionMessageBubbleTest() { // Make sure the profile is destroyed before the thread bundle. profile_.reset(NULL); @@ -226,9 +283,6 @@ class ExtensionMessageBubbleTest : public testing::Test { } ExtensionService* service_; - std::string extension_id1_; - std::string extension_id2_; - std::string extension_id3_; private: scoped_ptr command_line_; @@ -246,8 +300,13 @@ class ExtensionMessageBubbleTest : public testing::Test { #endif TEST_F(ExtensionMessageBubbleTest, MAYBE_WipeoutControllerTest) { - // The test base class adds three extensions, and we control two of them in - // this test (ids are: extension_id1_ and extension_id2_). + Init(); + // Add three extensions, and control two of them in this test (extension 1 + // and 2). + LoadExtensionWithAction("1", kId1, Manifest::COMMAND_LINE); + LoadGenericExtension("2", kId2, Manifest::UNPACKED); + LoadGenericExtension("3", kId3, Manifest::EXTERNAL_POLICY); + scoped_ptr controller( new TestSuspiciousExtensionBubbleController(profile())); FakeExtensionMessageBubble bubble; @@ -256,8 +315,8 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_WipeoutControllerTest) { // Validate that we don't have a suppress value for the extensions. ExtensionPrefs* prefs = ExtensionPrefs::Get(profile()); - EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(extension_id1_)); - EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(extension_id2_)); + EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(kId1)); + EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(kId2)); EXPECT_FALSE(controller->ShouldShow()); std::vector suspicious_extensions = @@ -267,11 +326,10 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_WipeoutControllerTest) { EXPECT_EQ(0U, controller->dismiss_click_count()); // Now disable an extension, specifying the wipeout flag. - service_->DisableExtension(extension_id1_, - Extension::DISABLE_NOT_VERIFIED); + service_->DisableExtension(kId1, Extension::DISABLE_NOT_VERIFIED); - EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(extension_id1_)); - EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(extension_id2_)); + EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(kId1)); + EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(kId2)); controller.reset(new TestSuspiciousExtensionBubbleController( profile())); SuspiciousExtensionBubbleController::ClearProfileListForTesting(); @@ -283,15 +341,14 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_WipeoutControllerTest) { EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(1U, controller->dismiss_click_count()); // Now the acknowledge flag should be set only for the first extension. - EXPECT_TRUE(prefs->HasWipeoutBeenAcknowledged(extension_id1_)); - EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(extension_id2_)); + EXPECT_TRUE(prefs->HasWipeoutBeenAcknowledged(kId1)); + EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(kId2)); // Clear the flag. - prefs->SetWipeoutAcknowledged(extension_id1_, false); - EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(extension_id1_)); + prefs->SetWipeoutAcknowledged(kId1, false); + EXPECT_FALSE(prefs->HasWipeoutBeenAcknowledged(kId1)); // Now disable the other extension and exercise the link click code path. - service_->DisableExtension(extension_id2_, - Extension::DISABLE_NOT_VERIFIED); + service_->DisableExtension(kId2, Extension::DISABLE_NOT_VERIFIED); bubble.set_action_on_show( FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_LINK); @@ -306,7 +363,7 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_WipeoutControllerTest) { controller->Show(&bubble); // Simulate showing the bubble. EXPECT_EQ(1U, controller->link_click_count()); EXPECT_EQ(0U, controller->dismiss_click_count()); - EXPECT_TRUE(prefs->HasWipeoutBeenAcknowledged(extension_id1_)); + EXPECT_TRUE(prefs->HasWipeoutBeenAcknowledged(kId1)); } // The feature this is meant to test is only implemented on Windows. @@ -319,10 +376,14 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_WipeoutControllerTest) { TEST_F(ExtensionMessageBubbleTest, MAYBE_DevModeControllerTest) { FeatureSwitch::ScopedOverride force_dev_mode_highlighting( FeatureSwitch::force_dev_mode_highlighting(), true); - // The test base class adds three extensions, and we control two of them in - // this test (ids are: extension_id1_ and extension_id2_). Extension 1 is a - // regular extension, Extension 2 is UNPACKED so it counts as a DevMode - // extension. + Init(); + // Add three extensions, and control two of them in this test (extension 1 + // and 2). Extension 1 is a regular extension, Extension 2 is UNPACKED so it + // counts as a DevMode extension. + LoadExtensionWithAction("1", kId1, Manifest::COMMAND_LINE); + LoadGenericExtension("2", kId2, Manifest::UNPACKED); + LoadGenericExtension("3", kId3, Manifest::EXTERNAL_POLICY); + scoped_ptr controller( new TestDevModeBubbleController(profile())); @@ -345,8 +406,8 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_DevModeControllerTest) { EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(0U, controller->action_click_count()); EXPECT_EQ(1U, controller->dismiss_click_count()); - EXPECT_TRUE(service_->GetExtensionById(extension_id1_, false) != NULL); - EXPECT_TRUE(service_->GetExtensionById(extension_id2_, false) != NULL); + EXPECT_TRUE(service_->GetExtensionById(kId1, false) != NULL); + EXPECT_TRUE(service_->GetExtensionById(kId2, false) != NULL); // Do it again, but now press different button (Disable). bubble.set_action_on_show( @@ -361,12 +422,12 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_DevModeControllerTest) { EXPECT_EQ(0U, controller->link_click_count()); EXPECT_EQ(1U, controller->action_click_count()); EXPECT_EQ(0U, controller->dismiss_click_count()); - EXPECT_TRUE(service_->GetExtensionById(extension_id1_, false) == NULL); - EXPECT_TRUE(service_->GetExtensionById(extension_id2_, false) == NULL); + EXPECT_TRUE(service_->GetExtensionById(kId1, false) == NULL); + EXPECT_TRUE(service_->GetExtensionById(kId2, false) == NULL); // Re-enable the extensions (disabled by the action button above). - service_->EnableExtension(extension_id1_); - service_->EnableExtension(extension_id2_); + service_->EnableExtension(kId1); + service_->EnableExtension(kId2); // Show the dialog a third time, but now press the learn more link. bubble.set_action_on_show( @@ -381,12 +442,12 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_DevModeControllerTest) { EXPECT_EQ(1U, controller->link_click_count()); EXPECT_EQ(0U, controller->action_click_count()); EXPECT_EQ(0U, controller->dismiss_click_count()); - EXPECT_TRUE(service_->GetExtensionById(extension_id1_, false) != NULL); - EXPECT_TRUE(service_->GetExtensionById(extension_id2_, false) != NULL); + EXPECT_TRUE(service_->GetExtensionById(kId1, false) != NULL); + EXPECT_TRUE(service_->GetExtensionById(kId2, false) != NULL); // Now disable the unpacked extension. - service_->DisableExtension(extension_id1_, Extension::DISABLE_USER_ACTION); - service_->DisableExtension(extension_id2_, Extension::DISABLE_USER_ACTION); + service_->DisableExtension(kId1, Extension::DISABLE_USER_ACTION); + service_->DisableExtension(kId2, Extension::DISABLE_USER_ACTION); controller.reset(new TestDevModeBubbleController( profile())); @@ -396,4 +457,128 @@ TEST_F(ExtensionMessageBubbleTest, MAYBE_DevModeControllerTest) { EXPECT_EQ(0U, dev_mode_extensions.size()); } +// The feature this is meant to test is only implemented on Windows. +#if defined(OS_WIN) +#define MAYBE_SettingsApiControllerTest SettingsApiControllerTest +#else +#define MAYBE_SettingsApiControllerTest DISABLED_SettingsApiControllerTest +#endif + +TEST_F(ExtensionMessageBubbleTest, MAYBE_SettingsApiControllerTest) { + Init(); + extensions::ExtensionPrefs* prefs = + extensions::ExtensionPrefs::Get(profile()); + + for (int i = 0; i < 3; ++i) { + switch (static_cast(i)) { + case BUBBLE_TYPE_HOME_PAGE: + // Load two extensions overriding home page and one overriding something + // unrelated (to check for interference). Extension 2 should still win + // on the home page setting. + LoadExtensionOverridingHome("1", kId1, Manifest::UNPACKED); + LoadExtensionOverridingHome("2", kId2, Manifest::UNPACKED); + LoadExtensionOverridingStart("3", kId3, Manifest::UNPACKED); + break; + case BUBBLE_TYPE_SEARCH_ENGINE: + // We deliberately skip testing the search engine since it relies on + // TemplateURLServiceFactory that isn't available while unit testing. + // This test is only simulating the bubble interaction with the user and + // that is more or less the same for the search engine as it is for the + // others. + continue; + case BUBBLE_TYPE_STARTUP_PAGES: + // Load two extensions overriding start page and one overriding + // something unrelated (to check for interference). Extension 2 should + // still win on the startup page setting. + LoadExtensionOverridingStart("1", kId1, Manifest::UNPACKED); + LoadExtensionOverridingStart("2", kId2, Manifest::UNPACKED); + LoadExtensionOverridingHome("3", kId3, Manifest::UNPACKED); + break; + default: + NOTREACHED(); + break; + } + + scoped_ptr controller( + new TestSettingsApiBubbleController( + profile(), static_cast(i))); + + // The list will contain one enabled unpacked extension (ext 2). + EXPECT_TRUE(controller->ShouldShow(kId2)); + std::vector override_extensions = + controller->GetExtensionList(); + ASSERT_EQ(1U, override_extensions.size()); + EXPECT_TRUE(base::ASCIIToUTF16("Extension 2") == + override_extensions[0].c_str()); + EXPECT_EQ(0U, controller->link_click_count()); + EXPECT_EQ(0U, controller->dismiss_click_count()); + EXPECT_EQ(0U, controller->action_click_count()); + + // Simulate showing the bubble and dismissing it. + FakeExtensionMessageBubble bubble; + bubble.set_action_on_show( + FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_DISMISS_BUTTON); + controller->Show(&bubble); + EXPECT_EQ(0U, controller->link_click_count()); + EXPECT_EQ(0U, controller->action_click_count()); + EXPECT_EQ(1U, controller->dismiss_click_count()); + // No extension should have become disabled. + EXPECT_TRUE(service_->GetExtensionById(kId1, false) != NULL); + EXPECT_TRUE(service_->GetExtensionById(kId2, false) != NULL); + EXPECT_TRUE(service_->GetExtensionById(kId3, false) != NULL); + // Only extension 2 should have been acknowledged. + EXPECT_FALSE(prefs->HasSettingsApiBubbleBeenAcknowledged(kId1)); + EXPECT_TRUE(prefs->HasSettingsApiBubbleBeenAcknowledged(kId2)); + EXPECT_FALSE(prefs->HasSettingsApiBubbleBeenAcknowledged(kId3)); + // Clean up after ourselves. + prefs->SetSettingsApiBubbleBeenAcknowledged(kId2, false); + + // Simulate clicking the learn more link to dismiss it. + bubble.set_action_on_show( + FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_LINK); + controller.reset(new TestSettingsApiBubbleController( + profile(), static_cast(i))); + controller->Show(&bubble); + EXPECT_EQ(1U, controller->link_click_count()); + EXPECT_EQ(0U, controller->action_click_count()); + EXPECT_EQ(0U, controller->dismiss_click_count()); + // No extension should have become disabled. + EXPECT_TRUE(service_->GetExtensionById(kId1, false) != NULL); + EXPECT_TRUE(service_->GetExtensionById(kId2, false) != NULL); + EXPECT_TRUE(service_->GetExtensionById(kId3, false) != NULL); + // Only extension 2 should have been acknowledged. + EXPECT_FALSE(prefs->HasSettingsApiBubbleBeenAcknowledged(kId1)); + EXPECT_TRUE(prefs->HasSettingsApiBubbleBeenAcknowledged(kId2)); + EXPECT_FALSE(prefs->HasSettingsApiBubbleBeenAcknowledged(kId3)); + // Clean up after ourselves. + prefs->SetSettingsApiBubbleBeenAcknowledged(kId2, false); + + // Do it again, but now opt to disable the extension. + bubble.set_action_on_show( + FakeExtensionMessageBubble::BUBBLE_ACTION_CLICK_ACTION_BUTTON); + controller.reset(new TestSettingsApiBubbleController( + profile(), static_cast(i))); + EXPECT_TRUE(controller->ShouldShow(kId2)); + override_extensions = controller->GetExtensionList(); + EXPECT_EQ(1U, override_extensions.size()); + controller->Show(&bubble); // Simulate showing the bubble. + EXPECT_EQ(0U, controller->link_click_count()); + EXPECT_EQ(1U, controller->action_click_count()); + EXPECT_EQ(0U, controller->dismiss_click_count()); + // Only extension 2 should have become disabled. + EXPECT_TRUE(service_->GetExtensionById(kId1, false) != NULL); + EXPECT_TRUE(service_->GetExtensionById(kId2, false) == NULL); + EXPECT_TRUE(service_->GetExtensionById(kId3, false) != NULL); + // No extension should have been acknowledged (it got disabled). + EXPECT_FALSE(prefs->HasSettingsApiBubbleBeenAcknowledged(kId1)); + EXPECT_FALSE(prefs->HasSettingsApiBubbleBeenAcknowledged(kId2)); + EXPECT_FALSE(prefs->HasSettingsApiBubbleBeenAcknowledged(kId3)); + + // Clean up after ourselves. + service_->UninstallExtension(kId1, false, NULL); + service_->UninstallExtension(kId2, false, NULL); + service_->UninstallExtension(kId3, false, NULL); + } +} + } // namespace extensions diff --git a/chrome/browser/extensions/settings_api_bubble_controller.cc b/chrome/browser/extensions/settings_api_bubble_controller.cc index a5819bc0f812..f10ccc92ad9e 100644 --- a/chrome/browser/extensions/settings_api_bubble_controller.cc +++ b/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( diff --git a/chrome/browser/extensions/settings_api_helpers.cc b/chrome/browser/extensions/settings_api_helpers.cc new file mode 100644 index 000000000000..839098cbd4b0 --- /dev/null +++ b/chrome/browser/extensions/settings_api_helpers.cc @@ -0,0 +1,103 @@ +// 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* 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::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 diff --git a/chrome/browser/ui/views/settings_api_bubble_helper_views.h b/chrome/browser/extensions/settings_api_helpers.h similarity index 65% copy from chrome/browser/ui/views/settings_api_bubble_helper_views.h copy to chrome/browser/extensions/settings_api_helpers.h index a36a8064a6bc..db2ba36022a0 100644 --- a/chrome/browser/ui/views/settings_api_bubble_helper_views.h +++ b/chrome/browser/extensions/settings_api_helpers.h @@ -2,34 +2,18 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#ifndef CHROME_BROWSER_UI_VIEWS_SETTINGS_API_BUBBLE_HELPER_VIEWS_H_ -#define CHROME_BROWSER_UI_VIEWS_SETTINGS_API_BUBBLE_HELPER_VIEWS_H_ +#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" -struct AutocompleteMatch; -class Browser; -class Profile; - namespace content { class BrowserContext; -class WebContents; } namespace extensions { -// Shows a bubble notifying the user that the homepage is controlled by an -// extension. This bubble is shown only on the first use of the Home button -// after the controlling extension takes effect. -void MaybeShowExtensionControlledHomeNotification(Browser* browser); - -// Shows a bubble notifying the user that the search engine is controlled by an -// extension. This bubble is shown only on the first search after the -// controlling extension takes effect. -void MaybeShowExtensionControlledSearchNotification( - Profile* profile, - content::WebContents* web_contents, - const AutocompleteMatch& match); +struct SettingsOverrides; // Find which |extension| is overriding a particular |type| of setting. Returns // the SettingsOverride object, or NULL if no |extension| is overriding that @@ -61,4 +45,4 @@ const Extension* OverridesSearchEngine( } // namespace extensions -#endif // CHROME_BROWSER_UI_VIEWS_SETTINGS_API_BUBBLE_HELPER_VIEWS_H_ +#endif // CHROME_BROWSER_EXTENSIONS_SETTINGS_API_HELPERS_H_ diff --git a/chrome/browser/ui/views/extensions/extension_message_bubble_view.cc b/chrome/browser/ui/views/extensions/extension_message_bubble_view.cc index 05aaaf92ae70..2e58aae416ed 100644 --- a/chrome/browser/ui/views/extensions/extension_message_bubble_view.cc +++ b/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" diff --git a/chrome/browser/ui/views/settings_api_bubble_helper_views.cc b/chrome/browser/ui/views/settings_api_bubble_helper_views.cc index 5a91d26aac44..892734285782 100644 --- a/chrome/browser/ui/views/settings_api_bubble_helper_views.cc +++ b/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* 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::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 diff --git a/chrome/browser/ui/views/settings_api_bubble_helper_views.h b/chrome/browser/ui/views/settings_api_bubble_helper_views.h index a36a8064a6bc..a475ed71c3e4 100644 --- a/chrome/browser/ui/views/settings_api_bubble_helper_views.h +++ b/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* 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_ diff --git a/chrome/chrome_browser_extensions.gypi b/chrome/chrome_browser_extensions.gypi index 9c27f3064d27..f6b499cc4ce7 100644 --- a/chrome/chrome_browser_extensions.gypi +++ b/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', diff --git a/extensions/browser/extension_prefs.cc b/extensions/browser/extension_prefs.cc index 2850d418831f..7cbf38cdce1f 100644 --- a/extensions/browser/extension_prefs.cc +++ b/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(); -- 2.11.4.GIT