diff options
author | ivankr@chromium.org <ivankr@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-04-11 15:44:49 +0000 |
---|---|---|
committer | ivankr@chromium.org <ivankr@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-04-11 15:44:49 +0000 |
commit | 444449a7be317f2c580c03d47d908d5de94c03e1 (patch) | |
tree | 13e40b29c9be7c621a6855ec57c95246907903b4 /chrome | |
parent | 377ec417301e7cefcc60682661999d050d94270a (diff) | |
download | chromium_src-444449a7be317f2c580c03d47d908d5de94c03e1.zip chromium_src-444449a7be317f2c580c03d47d908d5de94c03e1.tar.gz chromium_src-444449a7be317f2c580c03d47d908d5de94c03e1.tar.bz2 |
[protector] Support for collapsing multiple changes into a single one.
Each collapsible change has a key (domain). Subsequent changes with the same key are collapsed into CompositeSettingsChange.
BUG=114298
TEST=functional:protector.py; browser_tests:ProtectorServiceTest.*; unit_tests:CompositeSettingsChangeTest.*
Review URL: http://codereview.chromium.org/9968007
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@131773 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome')
19 files changed, 924 insertions, 50 deletions
diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd index 3204eb6..d6b9d60 100644 --- a/chrome/app/generated_resources.grd +++ b/chrome/app/generated_resources.grd @@ -1320,10 +1320,10 @@ Psst! Incognito mode <ph name="SHORTCUT_KEY">$1<ex>(Ctrl+Shift+N)</ex></ph> may Change to <ph name="URL">$1<ex>google.com</ex></ph> </message> - <message name="IDS_SETTING_CHANGE_NO_BACKUP_TITLE" desc="The title of the bubble and the wrench menu item with a generic settings change."> + <message name="IDS_SETTING_CHANGE_TITLE" desc="The title of the bubble and the wrench menu item with a generic settings change."> Attempted settings change </message> - <message name="IDS_SETTING_CHANGE_NO_BACKUP_BUBBLE_MESSAGE" desc="The text of the generic settings change notification."> + <message name="IDS_SETTING_CHANGE_BUBBLE_MESSAGE" desc="The text of the generic settings change notification."> Arrr! Something tried to commandeer your settings! </message> <message name="IDS_SETTING_CHANGE_NO_BACKUP_STARTUP_RESET_BUBBLE_MESSAGE" desc="The text of the generic settings change notification, additionally informing user that the startup settings have been reset to defaults."> @@ -1336,6 +1336,12 @@ Psst! Incognito mode <ph name="SHORTCUT_KEY">$1<ex>(Ctrl+Shift+N)</ex></ph> may <message name="IDS_KEEP_SETTING" desc="The title of the button that confirms staying with the previous settings."> Keep my settings </message> + <message name="IDS_CHANGE_SETTING" desc="The title of the button that confirms changing to the new settings, usually described by a domain name or search engine name."> + Change to <ph name="NEW_SETTING_NAME">$1<ex>Awesome Site</ex></ph> + </message> + <message name="IDS_CHANGE_SETTING_NO_NAME" desc="The title of the button that confirms changing to the new settings."> + Change settings + </message> <!-- Keywords --> <message name="IDS_AUTOCOMPLETE_MATCH_DESCRIPTION_SEPARATOR" desc="The separator between a result in the autocomplete popup and its description."> diff --git a/chrome/browser/protector/base_setting_change.cc b/chrome/browser/protector/base_setting_change.cc index 805d48b..4c5ba03 100644 --- a/chrome/browser/protector/base_setting_change.cc +++ b/chrome/browser/protector/base_setting_change.cc @@ -3,11 +3,20 @@ // found in the LICENSE file. #include "chrome/browser/protector/base_setting_change.h" +#include "chrome/browser/protector/composite_settings_change.h" #include "base/logging.h" namespace protector { +// static +const size_t kDefaultSearchProviderChangeNamePriority = 100U; +// static +const size_t kSessionStartupChangeNamePriority = 50U; + +// static +const size_t BaseSettingChange::kDefaultNamePriority = 0U; + BaseSettingChange::BaseSettingChange() : profile_(NULL) { } @@ -15,6 +24,20 @@ BaseSettingChange::BaseSettingChange() BaseSettingChange::~BaseSettingChange() { } +CompositeSettingsChange* BaseSettingChange::MergeWith( + BaseSettingChange* other) { + CompositeSettingsChange* composite_change = new CompositeSettingsChange(); + CHECK(composite_change->Init(profile_)); + composite_change->MergeWith(this); + composite_change->MergeWith(other); + return composite_change; +} + +bool BaseSettingChange::Contains(const BaseSettingChange* other) const { + // BaseSettingChange can only contain itself. + return this == other; +} + bool BaseSettingChange::Init(Profile* profile) { DCHECK(profile); profile_ = profile; @@ -30,4 +53,17 @@ void BaseSettingChange::Discard(Browser* browser) { void BaseSettingChange::Timeout() { } +BaseSettingChange::DisplayName BaseSettingChange::GetApplyDisplayName() const { + return DisplayName(kDefaultNamePriority, string16()); +} + +GURL BaseSettingChange::GetNewSettingURL() const { + return GURL(); +} + +bool BaseSettingChange::CanBeMerged() const { + // By default change can be collapsed if it has a non-empty keyword. + return !GetNewSettingURL().is_empty(); +} + } // namespace protector diff --git a/chrome/browser/protector/base_setting_change.h b/chrome/browser/protector/base_setting_change.h index 42c52ad..05e680b 100644 --- a/chrome/browser/protector/base_setting_change.h +++ b/chrome/browser/protector/base_setting_change.h @@ -6,12 +6,12 @@ #define CHROME_BROWSER_PROTECTOR_BASE_SETTING_CHANGE_H_ #pragma once -#include <string> -#include <vector> +#include <utility> #include "base/basictypes.h" #include "base/string16.h" #include "chrome/browser/tabs/pinned_tab_codec.h" +#include "googleurl/src/gurl.h" class Browser; class Profile; @@ -20,12 +20,34 @@ struct SessionStartupPref; namespace protector { +class CompositeSettingsChange; + // Base class for setting change tracked by Protector. class BaseSettingChange { public: + // Pair consisting of a display name representing either new or backup setting + // and priority for it. Priority is used for composite changes, consisting + // of multiple BaseSettingChange instances - the display name with the highest + // priority is used. + typedef std::pair<size_t, string16> DisplayName; + + // Default priority value. + static const size_t kDefaultNamePriority; + BaseSettingChange(); virtual ~BaseSettingChange(); + // Merges |this| with |other| and returns a CompositeSettingsChange instance. + // Returned instance takes ownership of |other|. + // Init should not be called for the returned instance. + // |this| may be another CompositeSettingsChange, in which case |other| is + // simply added to it and |this| is returned. |other| cannot be + // CompositeSettingsChange. + virtual CompositeSettingsChange* MergeWith(BaseSettingChange* other); + + // Returns true if |this| is a result of merging some changes with |other|. + virtual bool Contains(const BaseSettingChange* other) const; + // Applies initial actions to the setting if needed. Must be called before // any other calls are made, including text getters. // Returns true if initialization was successful. @@ -66,6 +88,22 @@ class BaseSettingChange { // Returns text for the button to discard the change with |Discard|. virtual string16 GetDiscardButtonText() const = 0; + // Returns the display name and priority for the new setting. If multiple + // BaseSettingChange instances are merged into CompositeSettingsChange + // instance, the display name with the highest priority will be used for the + // Apply button (Discard button will have a generic caption in that case). + // Returns an empty string in |second| if there is no specific representation + // for new setting value and a generic string should be used. + virtual DisplayName GetApplyDisplayName() const; + + // Returns a URL uniquely identifying new (to be applied) settings. + // ProtectorService uses this URLs to decide whether to merge a change + // with already existing active changes. The URL may be empty. + virtual GURL GetNewSettingURL() const; + + // Returns true if this change can be merged with other changes. + virtual bool CanBeMerged() const; + protected: // Profile instance we've been associated with by an |Init| call. Profile* profile() { return profile_; } @@ -76,6 +114,10 @@ class BaseSettingChange { DISALLOW_COPY_AND_ASSIGN(BaseSettingChange); }; +// Display name priorities of various change types: +extern const size_t kDefaultSearchProviderChangeNamePriority; +extern const size_t kSessionStartupChangeNamePriority; + // TODO(ivankr): CompositeSettingChange that incapsulates multiple // BaseSettingChange instances. diff --git a/chrome/browser/protector/composite_settings_change.cc b/chrome/browser/protector/composite_settings_change.cc new file mode 100644 index 0000000..a907047 --- /dev/null +++ b/chrome/browser/protector/composite_settings_change.cc @@ -0,0 +1,111 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/protector/composite_settings_change.h" + +#include "grit/generated_resources.h" +#include "ui/base/l10n/l10n_util.h" + +namespace protector { + + +CompositeSettingsChange::CompositeSettingsChange() { +} + +CompositeSettingsChange::~CompositeSettingsChange() { +} + +CompositeSettingsChange* CompositeSettingsChange::MergeWith( + BaseSettingChange* other) { + DCHECK(profile()); + DCHECK(other); + changes_.push_back(other); + apply_names_.push(other->GetApplyDisplayName()); + return this; +} + +bool CompositeSettingsChange::Contains(const BaseSettingChange* other) const { + return this == other || + std::find(changes_.begin(), changes_.end(), other) != changes_.end(); +} + +// Apart from PrefsBackupInvalidChange, which should never appear with other +// Preferences-related changes together, the Apply/Discard logic of change +// classes does not overlap, so it's safe to call them in any order. + +void CompositeSettingsChange::Apply(Browser* browser) { + DVLOG(1) << "Apply all changes"; + for (size_t i = 0; i < changes_.size(); ++i) + changes_[i]->Apply(browser); +} + +void CompositeSettingsChange::Discard(Browser* browser) { + DVLOG(1) << "Discard all changes"; + for (size_t i = 0; i < changes_.size(); ++i) + changes_[i]->Discard(browser); +} + +void CompositeSettingsChange::Timeout() { + DVLOG(1) << "Timeout all changes"; + for (size_t i = 0; i < changes_.size(); ++i) + changes_[i]->Timeout(); +} + +int CompositeSettingsChange::GetBadgeIconID() const { + DCHECK(changes_.size()); + // Use icons from the first change. + // TODO(ivankr): need something better, maybe a special icon. + return changes_[0]->GetBadgeIconID(); +} + +int CompositeSettingsChange::GetMenuItemIconID() const { + DCHECK(changes_.size()); + return changes_[0]->GetMenuItemIconID(); +} + +int CompositeSettingsChange::GetBubbleIconID() const { + DCHECK(changes_.size()); + return changes_[0]->GetBubbleIconID(); +} + +string16 CompositeSettingsChange::GetBubbleTitle() const { + return l10n_util::GetStringUTF16(IDS_SETTING_CHANGE_TITLE); +} + +string16 CompositeSettingsChange::GetBubbleMessage() const { + // TODO(ivankr): indicate what kind of changes happened (i.e., "something + // tried to change your search engine, startup pages, etc."). + return l10n_util::GetStringUTF16(IDS_SETTING_CHANGE_BUBBLE_MESSAGE); +} + +string16 CompositeSettingsChange::GetApplyButtonText() const { + DCHECK(apply_names_.size()); + string16 apply_text = apply_names_.top().second; + return apply_text.empty() ? + l10n_util::GetStringUTF16(IDS_CHANGE_SETTING_NO_NAME) : + l10n_util::GetStringFUTF16(IDS_CHANGE_SETTING, apply_text); +} + +string16 CompositeSettingsChange::GetDiscardButtonText() const { + return l10n_util::GetStringUTF16(IDS_KEEP_SETTING); +} + +BaseSettingChange::DisplayName +CompositeSettingsChange::GetApplyDisplayName() const { + // CompositeSettingsChange should never be put inside another one. + NOTREACHED(); + return BaseSettingChange::GetApplyDisplayName(); +} + +GURL CompositeSettingsChange::GetNewSettingURL() const { + DCHECK(changes_.size()); + return changes_[0]->GetNewSettingURL(); +} + +bool CompositeSettingsChange::CanBeMerged() const { + // We did that already, why not do it again. + return true; +} + +} // namespace protector diff --git a/chrome/browser/protector/composite_settings_change.h b/chrome/browser/protector/composite_settings_change.h new file mode 100644 index 0000000..727dd93 --- /dev/null +++ b/chrome/browser/protector/composite_settings_change.h @@ -0,0 +1,52 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_PROTECTOR_COMPOSITE_SETTINGS_CHANGE_H_ +#define CHROME_BROWSER_PROTECTOR_COMPOSITE_SETTINGS_CHANGE_H_ +#pragma once + +#include <queue> + +#include "base/memory/scoped_vector.h" +#include "chrome/browser/protector/base_setting_change.h" + +namespace protector { + +// Class for tracking multiple settings changes as a single change. +class CompositeSettingsChange : public BaseSettingChange { + public: + CompositeSettingsChange(); + virtual ~CompositeSettingsChange(); + + // BaseSettingChange overrides: + virtual CompositeSettingsChange* MergeWith(BaseSettingChange* other) OVERRIDE; + virtual bool Contains(const BaseSettingChange* other) const OVERRIDE; + virtual void Apply(Browser* browser) OVERRIDE; + virtual void Discard(Browser* browser) OVERRIDE; + virtual void Timeout() OVERRIDE; + virtual int GetBadgeIconID() const OVERRIDE; + virtual int GetMenuItemIconID() const OVERRIDE; + virtual int GetBubbleIconID() const OVERRIDE; + virtual string16 GetBubbleTitle() const OVERRIDE; + virtual string16 GetBubbleMessage() const OVERRIDE; + virtual string16 GetApplyButtonText() const OVERRIDE; + virtual string16 GetDiscardButtonText() const OVERRIDE; + virtual DisplayName GetApplyDisplayName() const OVERRIDE; + virtual GURL GetNewSettingURL() const OVERRIDE; + virtual bool CanBeMerged() const OVERRIDE; + + private: + // Vector with all merged changes. + ScopedVector<BaseSettingChange> changes_; + + // Display names of merged changes, sorted by their priority. The name with + // the highest priority is picked. + std::priority_queue<DisplayName> apply_names_; + + DISALLOW_COPY_AND_ASSIGN(CompositeSettingsChange); +}; + +} // namespace protector + +#endif // CHROME_BROWSER_PROTECTOR_COMPOSITE_SETTINGS_CHANGE_H_ diff --git a/chrome/browser/protector/composite_settings_change_unittest.cc b/chrome/browser/protector/composite_settings_change_unittest.cc new file mode 100644 index 0000000..56ac4f7 --- /dev/null +++ b/chrome/browser/protector/composite_settings_change_unittest.cc @@ -0,0 +1,122 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/utf_string_conversions.h" +#include "chrome/browser/protector/composite_settings_change.h" +#include "chrome/browser/protector/mock_setting_change.h" +#include "chrome/test/base/testing_profile.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "grit/generated_resources.h" +#include "ui/base/l10n/l10n_util.h" + +using ::testing::NiceMock; +using ::testing::Return; + +namespace protector { + +namespace { + +string16 kTitle1 = ASCIIToUTF16("A"); +string16 kTitle2 = ASCIIToUTF16("B"); +string16 kTitle3 = ASCIIToUTF16("C"); + +}; + +class CompositeSettingsChangeTest : public testing::Test { + protected: + string16 GetApplyButtonText(const string16& apply_text) { + return l10n_util::GetStringFUTF16(IDS_CHANGE_SETTING, apply_text); + } + + TestingProfile profile_; +}; + +// Verify that MergeWith works and Apply/Discard/Timeout call same functions +// of all merged changes. +TEST_F(CompositeSettingsChangeTest, MergeWith) { + // These instances will be owned by CompositeSettingsChange. + MockSettingChange* change1 = new NiceMock<MockSettingChange>; + MockSettingChange* change2 = new NiceMock<MockSettingChange>; + MockSettingChange* change3 = new NiceMock<MockSettingChange>; + + EXPECT_CALL(*change1, MockInit(&profile_)).WillOnce(Return(true)); + ASSERT_TRUE(change1->Init(&profile_)); + EXPECT_CALL(*change2, MockInit(&profile_)).WillOnce(Return(true)); + ASSERT_TRUE(change2->Init(&profile_)); + EXPECT_CALL(*change3, MockInit(&profile_)).WillOnce(Return(true)); + ASSERT_TRUE(change3->Init(&profile_)); + + // Compose 1st and 2nd changes together. + scoped_ptr<CompositeSettingsChange> composite_change( + change1->MergeWith(change2)); + + EXPECT_TRUE(composite_change->Contains(change1)); + EXPECT_TRUE(composite_change->Contains(change2)); + EXPECT_FALSE(composite_change->Contains(change3)); + EXPECT_TRUE(composite_change->Contains(composite_change.get())); + + // Add third change. + EXPECT_EQ(composite_change.get(), composite_change->MergeWith(change3)); + + EXPECT_TRUE(composite_change->Contains(change1)); + EXPECT_TRUE(composite_change->Contains(change2)); + EXPECT_TRUE(composite_change->Contains(change3)); + EXPECT_TRUE(composite_change->Contains(composite_change.get())); + + // Apply/Discard/Timeout calls should be proxied to inner change instances: + EXPECT_CALL(*change1, Apply(NULL)); + EXPECT_CALL(*change2, Apply(NULL)); + EXPECT_CALL(*change3, Apply(NULL)); + composite_change->Apply(NULL); + + EXPECT_CALL(*change1, Discard(NULL)); + EXPECT_CALL(*change2, Discard(NULL)); + EXPECT_CALL(*change3, Discard(NULL)); + composite_change->Discard(NULL); + + EXPECT_CALL(*change1, Timeout()); + EXPECT_CALL(*change2, Timeout()); + EXPECT_CALL(*change3, Timeout()); + composite_change->Timeout(); +} + +TEST_F(CompositeSettingsChangeTest, ApplyButtonText) { + // These instances will be owned by CompositeSettingsChange. + MockSettingChange* change1 = new NiceMock<MockSettingChange>; + MockSettingChange* change2 = new NiceMock<MockSettingChange>; + MockSettingChange* change3 = new NiceMock<MockSettingChange>; + + EXPECT_CALL(*change1, MockInit(&profile_)).WillOnce(Return(true)); + ASSERT_TRUE(change1->Init(&profile_)); + EXPECT_CALL(*change2, MockInit(&profile_)).WillOnce(Return(true)); + ASSERT_TRUE(change2->Init(&profile_)); + EXPECT_CALL(*change3, MockInit(&profile_)).WillOnce(Return(true)); + ASSERT_TRUE(change3->Init(&profile_)); + + // |change1| has higher priority. + EXPECT_CALL(*change1, GetApplyDisplayName()).WillOnce( + Return(BaseSettingChange::DisplayName(10, kTitle1))); + EXPECT_CALL(*change2, GetApplyDisplayName()).WillOnce( + Return(BaseSettingChange::DisplayName(5, kTitle2))); + + scoped_ptr<CompositeSettingsChange> composite_change( + change1->MergeWith(change2)); + + EXPECT_EQ(GetApplyButtonText(kTitle1), + composite_change->GetApplyButtonText()); + + // |change3| has the highest priority now. + EXPECT_CALL(*change3, GetApplyDisplayName()).WillOnce( + Return(BaseSettingChange::DisplayName(15, kTitle3))); + EXPECT_EQ(composite_change.get(), composite_change->MergeWith(change3)); + + EXPECT_EQ(GetApplyButtonText(kTitle3), + composite_change->GetApplyButtonText()); + + GURL url1("example.com"); + EXPECT_CALL(*change1, GetNewSettingURL()).WillOnce(Return(url1)); + EXPECT_EQ(url1, composite_change->GetNewSettingURL()); +} + +} // namespace protector diff --git a/chrome/browser/protector/default_search_provider_change.cc b/chrome/browser/protector/default_search_provider_change.cc index 28df6ed..774655a 100644 --- a/chrome/browser/protector/default_search_provider_change.cc +++ b/chrome/browser/protector/default_search_provider_change.cc @@ -97,6 +97,8 @@ class DefaultSearchProviderChange : public BaseSettingChange, virtual string16 GetBubbleMessage() const OVERRIDE; virtual string16 GetApplyButtonText() const OVERRIDE; virtual string16 GetDiscardButtonText() const OVERRIDE; + virtual DisplayName GetApplyDisplayName() const OVERRIDE; + virtual GURL GetNewSettingURL() const OVERRIDE; private: virtual ~DefaultSearchProviderChange(); @@ -322,6 +324,28 @@ string16 DefaultSearchProviderChange::GetDiscardButtonText() const { return l10n_util::GetStringUTF16(IDS_SELECT_SEARCH_ENGINE); } +BaseSettingChange::DisplayName +DefaultSearchProviderChange::GetApplyDisplayName() const { + if (new_search_provider_ && + new_search_provider_->short_name().length() <= kMaxDisplayedNameLength) { + return DisplayName(kDefaultSearchProviderChangeNamePriority, + new_search_provider_->short_name()); + } + // Return the default empty name. + return BaseSettingChange::GetApplyDisplayName(); +} + +GURL DefaultSearchProviderChange::GetNewSettingURL() const { + if (is_fallback_) { + // Do not collide this change when fallback was made, since the message + // and Discard behaviour is pretty different. + return GURL(); + } + if (!new_search_provider_) + return GURL(); + return TemplateURLService::GenerateSearchURL(new_search_provider_); +} + void DefaultSearchProviderChange::OnTemplateURLServiceChanged() { TemplateURLService* url_service = GetTemplateURLService(); if (url_service->GetDefaultSearchProvider() != default_search_provider_) { diff --git a/chrome/browser/protector/default_search_provider_change_browsertest.cc b/chrome/browser/protector/default_search_provider_change_browsertest.cc index 9e4fae8..e5a63e7 100644 --- a/chrome/browser/protector/default_search_provider_change_browsertest.cc +++ b/chrome/browser/protector/default_search_provider_change_browsertest.cc @@ -6,6 +6,7 @@ #include "base/metrics/histogram.h" #include "base/message_loop.h" #include "base/utf_string_conversions.h" +#include "chrome/browser/protector/base_setting_change.h" #include "chrome/browser/protector/histograms.h" #include "chrome/browser/protector/mock_protector_service.h" #include "chrome/browser/protector/protector_service_factory.h" @@ -25,17 +26,25 @@ using ::testing::Invoke; using ::testing::NiceMock; using ::testing::Return; +namespace protector { + namespace { // Keyword names and URLs used for testing. const string16 example_info = ASCIIToUTF16("Example.info"); const string16 example_info_long = ASCIIToUTF16("ExampleSearchEngine.info"); -const std::string http_example_info = "http://example.info/%s"; +const std::string http_example_info = "http://example.info/?q={searchTerms}"; +const std::string example_info_domain = "example.info"; const string16 example_com = ASCIIToUTF16("Example.com"); const string16 example_com_long = ASCIIToUTF16("ExampleSearchEngine.com"); -const std::string http_example_com = "http://example.com/%s"; +const std::string http_example_com = "http://example.com/?q={searchTerms}"; +const std::string example_com_domain = "example.com"; const string16 example_net = ASCIIToUTF16("Example.net"); -const std::string http_example_net = "http://example.net/%s"; +const std::string http_example_net = "http://example.net/?q={searchTerms}"; +const std::string example_domain = "example.net"; + +const BaseSettingChange::DisplayName kNoDisplayName( + BaseSettingChange::kDefaultNamePriority, string16()); // Convenience function. TemplateURL* MakeTemplateURL(const string16& short_name, @@ -51,9 +60,7 @@ TemplateURL* MakeTemplateURL(const string16& short_name, return new TemplateURL(data); } -}; - -namespace protector { +} // namespace class DefaultSearchProviderChangeTest : public InProcessBrowserTest { public: @@ -176,6 +183,8 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, BackupValid) { change->GetApplyButtonText()); EXPECT_EQ(GetKeepSearchButtonText(example_info), change->GetDiscardButtonText()); + EXPECT_EQ(example_com_domain, change->GetNewSettingURL().host()); + EXPECT_EQ(example_com, change->GetApplyDisplayName().second); // Discard does nothing - backup was already active. change->Discard(browser()); @@ -218,6 +227,8 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, BackupValidLongNames) { EXPECT_EQ(GetChangeSearchButtonText(example_com), change->GetApplyButtonText()); EXPECT_EQ(GetKeepSearchButtonText(), change->GetDiscardButtonText()); + EXPECT_EQ(example_com_domain, change->GetNewSettingURL().host()); + EXPECT_EQ(example_com, change->GetApplyDisplayName().second); } { @@ -235,6 +246,8 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, BackupValidLongNames) { EXPECT_EQ(GetChangeSearchButtonText(), change->GetApplyButtonText()); EXPECT_EQ(GetKeepSearchButtonText(example_info), change->GetDiscardButtonText()); + EXPECT_EQ(example_com_domain, change->GetNewSettingURL().host()); + EXPECT_EQ(kNoDisplayName, change->GetApplyDisplayName()); } } @@ -334,6 +347,8 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, EXPECT_EQ(GetChangeSearchButtonText(example_com), change->GetApplyButtonText()); EXPECT_EQ(GetOpenSettingsButtonText(), change->GetDiscardButtonText()); + EXPECT_EQ(GURL(), change->GetNewSettingURL()); + EXPECT_EQ(example_com, change->GetApplyDisplayName().second); // Verify that search engine settings are opened by Discard. ExpectSettingsOpened(chrome::kSearchEnginesSubPage); @@ -377,6 +392,8 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, EXPECT_EQ(GetOpenSettingsButtonText(), change->GetApplyButtonText()); EXPECT_EQ(GetKeepSearchButtonText(example_info), change->GetDiscardButtonText()); + EXPECT_EQ(GURL(), change->GetNewSettingURL()); + EXPECT_EQ(kNoDisplayName, change->GetApplyDisplayName()); // Discard does nothing - backup was already active. change->Discard(browser()); @@ -420,6 +437,8 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, change->GetBubbleMessage()); EXPECT_EQ(string16(), change->GetApplyButtonText()); EXPECT_EQ(GetOpenSettingsButtonText(), change->GetDiscardButtonText()); + EXPECT_EQ(GURL(), change->GetNewSettingURL()); + EXPECT_EQ(kNoDisplayName, change->GetApplyDisplayName()); // Verify that search engine settings are opened by Discard. ExpectSettingsOpened(chrome::kSearchEnginesSubPage); @@ -461,6 +480,8 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, change->GetBubbleMessage()); EXPECT_EQ(string16(), change->GetApplyButtonText()); EXPECT_EQ(GetOpenSettingsButtonText(), change->GetDiscardButtonText()); + EXPECT_EQ(GURL(), change->GetNewSettingURL()); + EXPECT_EQ(current_url->short_name(), change->GetApplyDisplayName().second); // Verify that search engine settings are opened by Discard. ExpectSettingsOpened(chrome::kSearchEnginesSubPage); @@ -525,6 +546,8 @@ IN_PROC_BROWSER_TEST_F(DefaultSearchProviderChangeTest, EXPECT_EQ(GetOpenSettingsButtonText(), change->GetApplyButtonText()); EXPECT_EQ(GetKeepSearchButtonText(example_info), change->GetDiscardButtonText()); + EXPECT_EQ(GURL(), change->GetNewSettingURL()); + EXPECT_EQ(kNoDisplayName, change->GetApplyDisplayName()); // Verify that search engine settings are opened by Apply. ExpectSettingsOpened(chrome::kSearchEnginesSubPage); diff --git a/chrome/browser/protector/mock_setting_change.cc b/chrome/browser/protector/mock_setting_change.cc index 53bc9f3..34581e0 100644 --- a/chrome/browser/protector/mock_setting_change.cc +++ b/chrome/browser/protector/mock_setting_change.cc @@ -25,6 +25,13 @@ MockSettingChange::MockSettingChange() { WillByDefault(Return(UTF8ToUTF16("Apply"))); ON_CALL(*this, GetDiscardButtonText()). WillByDefault(Return(UTF8ToUTF16("Discard"))); + + ON_CALL(*this, GetApplyDisplayName()). + WillByDefault(Return( + DisplayName(kDefaultNamePriority, UTF8ToUTF16("new settings")))); + + ON_CALL(*this, GetNewSettingURL()).WillByDefault(Return(GURL())); + ON_CALL(*this, CanBeMerged()).WillByDefault(Return(false)); } MockSettingChange::~MockSettingChange() { diff --git a/chrome/browser/protector/mock_setting_change.h b/chrome/browser/protector/mock_setting_change.h index c665ced..dd66626 100644 --- a/chrome/browser/protector/mock_setting_change.h +++ b/chrome/browser/protector/mock_setting_change.h @@ -32,6 +32,11 @@ class MockSettingChange : public BaseSettingChange { MOCK_CONST_METHOD0(GetBubbleMessage, string16()); MOCK_CONST_METHOD0(GetApplyButtonText, string16()); MOCK_CONST_METHOD0(GetDiscardButtonText, string16()); + + MOCK_CONST_METHOD0(GetApplyDisplayName, DisplayName()); + + MOCK_CONST_METHOD0(GetNewSettingURL, GURL()); + MOCK_CONST_METHOD0(CanBeMerged, bool()); }; } // namespace protector diff --git a/chrome/browser/protector/prefs_backup_invalid_change.cc b/chrome/browser/protector/prefs_backup_invalid_change.cc index a9cb839..2d86584 100644 --- a/chrome/browser/protector/prefs_backup_invalid_change.cc +++ b/chrome/browser/protector/prefs_backup_invalid_change.cc @@ -40,6 +40,7 @@ class PrefsBackupInvalidChange : public BasePrefsChange { virtual string16 GetBubbleMessage() const OVERRIDE; virtual string16 GetApplyButtonText() const OVERRIDE; virtual string16 GetDiscardButtonText() const OVERRIDE; + virtual bool CanBeMerged() const OVERRIDE; private: virtual ~PrefsBackupInvalidChange(); @@ -100,14 +101,14 @@ int PrefsBackupInvalidChange::GetBubbleIconID() const { } string16 PrefsBackupInvalidChange::GetBubbleTitle() const { - return l10n_util::GetStringUTF16(IDS_SETTING_CHANGE_NO_BACKUP_TITLE); + return l10n_util::GetStringUTF16(IDS_SETTING_CHANGE_TITLE); } string16 PrefsBackupInvalidChange::GetBubbleMessage() const { return startup_pref_reset_ ? l10n_util::GetStringUTF16( IDS_SETTING_CHANGE_NO_BACKUP_STARTUP_RESET_BUBBLE_MESSAGE) : - l10n_util::GetStringUTF16(IDS_SETTING_CHANGE_NO_BACKUP_BUBBLE_MESSAGE); + l10n_util::GetStringUTF16(IDS_SETTING_CHANGE_BUBBLE_MESSAGE); } string16 PrefsBackupInvalidChange::GetApplyButtonText() const { @@ -131,6 +132,10 @@ void PrefsBackupInvalidChange::ApplyDefaults(Profile* profile) { } } +bool PrefsBackupInvalidChange::CanBeMerged() const { + return false; +} + BaseSettingChange* CreatePrefsBackupInvalidChange() { return new PrefsBackupInvalidChange(); } diff --git a/chrome/browser/protector/protector_service.cc b/chrome/browser/protector/protector_service.cc index ce8b20a..4b790a7 100644 --- a/chrome/browser/protector/protector_service.cc +++ b/chrome/browser/protector/protector_service.cc @@ -5,16 +5,34 @@ #include "chrome/browser/protector/protector_service.h" #include "base/logging.h" +#include "chrome/browser/google/google_util.h" #include "chrome/browser/prefs/pref_service.h" -#include "chrome/browser/protector/settings_change_global_error.h" +#include "chrome/browser/protector/composite_settings_change.h" +#include "chrome/browser/protector/keys.h" #include "chrome/browser/protector/protected_prefs_watcher.h" +#include "chrome/browser/protector/settings_change_global_error.h" #include "chrome/browser/ui/browser.h" #include "chrome/common/chrome_notification_types.h" #include "chrome/common/pref_names.h" #include "content/public/browser/notification_source.h" +#include "net/base/registry_controlled_domain.h" namespace protector { +namespace { + +// Returns true if changes with URLs |url1| and |url2| can be merged. +bool CanMerge(const GURL& url1, const GURL& url2) { + VLOG(1) << "Checking if can merge " << url1.spec() << " with " << url2.spec(); + // All Google URLs are considered the same one. + if (google_util::IsGoogleHostname(url1.host())) + return google_util::IsGoogleHostname(url2.host()); + // Otherwise URLs must have the same domain. + return net::RegistryControlledDomainService::SameDomainOrHost(url1, url2); +} + +} // namespace + ProtectorService::ProtectorService(Profile* profile) : profile_(profile), has_active_change_(false) { @@ -28,20 +46,37 @@ ProtectorService::~ProtectorService() { void ProtectorService::ShowChange(BaseSettingChange* change) { DCHECK(change); - Item new_item; - new_item.change.reset(change); + DVLOG(1) << "Init change"; if (!change->Init(profile_)) { LOG(WARNING) << "Error while initializing, dismissing change"; + delete change; return; } - SettingsChangeGlobalError* error = - new SettingsChangeGlobalError(change, this); - new_item.error.reset(error); - items_.push_back(new_item); - // Do not show the bubble immediately if another one is active. - error->AddToProfile(profile_, !has_active_change_); - has_active_change_ = true; + + Item* item_to_merge_with = FindItemToMergeWith(change); + if (item_to_merge_with) { + // CompositeSettingsChange takes ownership of merged changes. + BaseSettingChange* existing_change = item_to_merge_with->change.release(); + CompositeSettingsChange* merged_change = existing_change->MergeWith(change); + item_to_merge_with->change.reset(merged_change); + item_to_merge_with->was_merged = true; + if (item_to_merge_with->error->GetBubbleView()) + item_to_merge_with->show_when_merged = true; + // Remove old GlobalError instance. Later in OnRemovedFromProfile() a new + // GlobalError instance will be created for the composite change. + item_to_merge_with->error->RemoveFromProfile(); + } else { + Item new_item; + SettingsChangeGlobalError* error = + new SettingsChangeGlobalError(change, this); + new_item.error.reset(error); + new_item.change.reset(change); + items_.push_back(new_item); + // Do not show the bubble immediately if another one is active. + error->AddToProfile(profile_, !has_active_change_); + has_active_change_ = true; + } } bool ProtectorService::IsShowingChange() const { @@ -61,8 +96,8 @@ void ProtectorService::DiscardChange(BaseSettingChange* change, } void ProtectorService::DismissChange(BaseSettingChange* change) { - std::vector<Item>::iterator item = - std::find_if(items_.begin(), items_.end(), MatchItemByChange(change)); + Items::iterator item = std::find_if(items_.begin(), items_.end(), + MatchItemByChange(change)); DCHECK(item != items_.end()); item->error->RemoveFromProfile(); } @@ -76,6 +111,19 @@ ProtectedPrefsWatcher* ProtectorService::GetPrefsWatcher() { return prefs_watcher_.get(); } +ProtectorService::Item* ProtectorService::FindItemToMergeWith( + const BaseSettingChange* change) { + if (!change->CanBeMerged()) + return NULL; + GURL url = change->GetNewSettingURL(); + for (Items::iterator item = items_.begin(); item != items_.end(); item++) { + if (item->change->CanBeMerged() && + CanMerge(url, item->change->GetNewSettingURL())) + return &*item; + } + return NULL; +} + void ProtectorService::Shutdown() { while (IsShowingChange()) items_[0].error->RemoveFromProfile(); @@ -101,13 +149,27 @@ void ProtectorService::OnDecisionTimeout(SettingsChangeGlobalError* error) { } void ProtectorService::OnRemovedFromProfile(SettingsChangeGlobalError* error) { - std::vector<Item>::iterator item = - std::find_if(items_.begin(), items_.end(), MatchItemByError(error)); + Items::iterator item = std::find_if(items_.begin(), items_.end(), + MatchItemByError(error)); DCHECK(item != items_.end()); + + if (item->was_merged) { + bool show_new_error = !has_active_change_ || item->show_when_merged; + item->was_merged = false; + item->show_when_merged = false; + // Item was merged with another change instance and error has been removed, + // create a new one for the composite change. + item->error.reset(new SettingsChangeGlobalError(item->change.get(), this)); + item->error->AddToProfile(profile_, show_new_error); + has_active_change_ = true; + return; + } + items_.erase(item); + // If no other change is shown and there are changes that haven't been shown + // yet, show the first one. if (!has_active_change_) { - // If there are changes that haven't been shown yet, show the first one. for (item = items_.begin(); item != items_.end(); ++item) { if (!item->error->HasShownBubbleView()) { item->error->ShowBubble(); @@ -122,7 +184,9 @@ BaseSettingChange* ProtectorService::GetLastChange() { return items_.empty() ? NULL : items_.back().change.get(); } -ProtectorService::Item::Item() { +ProtectorService::Item::Item() + : was_merged(false), + show_when_merged(false) { } ProtectorService::Item::~Item() { @@ -134,7 +198,7 @@ ProtectorService::MatchItemByChange::MatchItemByChange( bool ProtectorService::MatchItemByChange::operator()( const ProtectorService::Item& item) { - return other_ == item.change.get(); + return item.change->Contains(other_); } ProtectorService::MatchItemByError::MatchItemByError( diff --git a/chrome/browser/protector/protector_service.h b/chrome/browser/protector/protector_service.h index 7d0637d..ba56448 100644 --- a/chrome/browser/protector/protector_service.h +++ b/chrome/browser/protector/protector_service.h @@ -71,15 +71,24 @@ class ProtectorService : public ProfileKeyedService, private: friend class ProtectorServiceTest; - // Pair of error and corresponding change instance. linked_ptr is used because - // Item instances are stored in a std::vector and must be copyable. + // Each item consists of an error and corresponding change instance. + // linked_ptr is used because Item instances are stored in a std::vector and + // must be copyable. struct Item { Item(); ~Item(); linked_ptr<BaseSettingChange> change; linked_ptr<SettingsChangeGlobalError> error; + // When true, this means |change| was merged with another instance and + // |error| is in process of being removed from GlobalErrorService. + bool was_merged; + // Meaningful only when |was_merged| is true. In that case, true means that + // the new merged GlobalError instance will be immediately shown. + bool show_when_merged; }; + typedef std::vector<Item> Items; + // Matches Item by |change| field. class MatchItemByChange { public: @@ -102,6 +111,11 @@ class ProtectorService : public ProfileKeyedService, const SettingsChangeGlobalError* other_; }; + // Returns an Item instance whose change can be merged with |change|, if any. + // Otherwise returns |NULL|. Provided that the merge strategy is transitive, + // there can be only one such instance. + Item* FindItemToMergeWith(const BaseSettingChange* change); + // ProfileKeyedService implementation. virtual void Shutdown() OVERRIDE; @@ -115,7 +129,7 @@ class ProtectorService : public ProfileKeyedService, // Pointers to error bubble controllers and corresponding changes in the order // added. - std::vector<Item> items_; + Items items_; // Profile which settings we are protecting. Profile* profile_; diff --git a/chrome/browser/protector/protector_service_browsertest.cc b/chrome/browser/protector/protector_service_browsertest.cc index 17912ff..936359f 100644 --- a/chrome/browser/protector/protector_service_browsertest.cc +++ b/chrome/browser/protector/protector_service_browsertest.cc @@ -34,12 +34,13 @@ class ProtectorServiceTest : public InProcessBrowserTest { protected: GlobalError* GetGlobalError(BaseSettingChange* change) { - std::vector<ProtectorService::Item>::iterator item = - std::find_if(protector_service_->items_.begin(), - protector_service_->items_.end(), - ProtectorService::MatchItemByChange(change)); - return item == protector_service_->items_.end() ? - NULL : item->error.get(); + for (ProtectorService::Items::iterator item = + protector_service_->items_.begin(); + item != protector_service_->items_.end(); item++) { + if (item->change.get() == change) + return item->error.get(); + } + return NULL; } // Checks that |protector_service_| has an error instance corresponding to @@ -67,6 +68,7 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ChangeInitError) { EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); + EXPECT_FALSE(protector_service_->GetLastChange()); } IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ShowAndDismiss) { @@ -76,9 +78,11 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ShowAndDismiss) { protector_service_->ShowChange(mock_change_); ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + EXPECT_EQ(mock_change_, protector_service_->GetLastChange()); protector_service_->DismissChange(mock_change_); ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); + EXPECT_FALSE(protector_service_->GetLastChange()); } IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ShowAndApply) { @@ -167,16 +171,19 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ShowMultipleChangesAndApply) { protector_service_->ShowChange(mock_change_); ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + EXPECT_EQ(mock_change_, protector_service_->GetLastChange()); // ProtectService will own this change instance as well. MockSettingChange* mock_change2 = new NiceMock<MockSettingChange>(); // Show the second change. EXPECT_CALL(*mock_change2, MockInit(browser()->profile())). WillOnce(Return(true)); + EXPECT_CALL(*mock_change2, CanBeMerged()).WillRepeatedly(Return(false)); protector_service_->ShowChange(mock_change2); ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); EXPECT_TRUE(IsGlobalErrorActive(mock_change2)); + EXPECT_EQ(mock_change2, protector_service_->GetLastChange()); // Apply the first change, the second should still be active. EXPECT_CALL(*mock_change_, Apply(browser())); @@ -184,6 +191,7 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ShowMultipleChangesAndApply) { ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); EXPECT_TRUE(IsGlobalErrorActive(mock_change2)); + EXPECT_EQ(mock_change2, protector_service_->GetLastChange()); // Finally apply the second change. EXPECT_CALL(*mock_change2, Apply(browser())); @@ -191,6 +199,7 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ShowMultipleChangesAndApply) { ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); EXPECT_FALSE(IsGlobalErrorActive(mock_change2)); + EXPECT_FALSE(protector_service_->GetLastChange()); } IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, @@ -207,6 +216,7 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, // Show the second change. EXPECT_CALL(*mock_change2, MockInit(browser()->profile())). WillOnce(Return(true)); + EXPECT_CALL(*mock_change2, CanBeMerged()).WillRepeatedly(Return(false)); protector_service_->ShowChange(mock_change2); ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); @@ -245,6 +255,7 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, // Show the second change. EXPECT_CALL(*mock_change2, MockInit(browser()->profile())). WillOnce(Return(true)); + EXPECT_CALL(*mock_change2, CanBeMerged()).WillRepeatedly(Return(false)); protector_service_->ShowChange(mock_change2); ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); @@ -303,6 +314,7 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, // Show the second change. EXPECT_CALL(*mock_change2, MockInit(browser()->profile())). WillOnce(Return(true)); + EXPECT_CALL(*mock_change2, CanBeMerged()).WillRepeatedly(Return(false)); protector_service_->ShowChange(mock_change2); ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_TRUE(IsGlobalErrorActive(mock_change2)); @@ -320,6 +332,310 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, EXPECT_FALSE(IsGlobalErrorActive(mock_change2)); } +IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ShowMultipleDifferentURLs) { + GURL url1("http://example.com/"); + GURL url2("http://example.net/"); + + // Show the first change with some non-empty URL. + EXPECT_CALL(*mock_change_, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change_, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change_, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change_); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + EXPECT_EQ(mock_change_, protector_service_->GetLastChange()); + + // ProtectService will own this change instance as well. + MockSettingChange* mock_change2 = new NiceMock<MockSettingChange>(); + // Show the second change with another non-empty URL. + EXPECT_CALL(*mock_change2, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change2, GetNewSettingURL()).WillRepeatedly(Return(url2)); + EXPECT_CALL(*mock_change2, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change2); + ui_test_utils::RunAllPendingInMessageLoop(); + + // Both changes are shown separately, not composited. + EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + EXPECT_TRUE(IsGlobalErrorActive(mock_change2)); + EXPECT_EQ(mock_change2, protector_service_->GetLastChange()); + + protector_service_->DismissChange(mock_change_); + protector_service_->DismissChange(mock_change2); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_FALSE(protector_service_->GetLastChange()); +} + +IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ShowCompositeAndDismiss) { + GURL url1("http://example.com/"); + + // Show the first change. + EXPECT_CALL(*mock_change_, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change_, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change_, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change_); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + EXPECT_EQ(mock_change_, protector_service_->GetLastChange()); + + // The first bubble view has been displayed. + GlobalError* error = GetGlobalError(mock_change_); + ASSERT_TRUE(error); + EXPECT_TRUE(error->HasShownBubbleView()); + + // ProtectService will own this change instance as well. + MockSettingChange* mock_change2 = new NiceMock<MockSettingChange>(); + // Show the second change. + EXPECT_CALL(*mock_change2, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change2, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change2, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change2); + ui_test_utils::RunAllPendingInMessageLoop(); + + // Now ProtectorService should be showing a single composite change. + EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); + EXPECT_FALSE(IsGlobalErrorActive(mock_change2)); + + BaseSettingChange* composite_change = protector_service_->GetLastChange(); + ASSERT_TRUE(composite_change); + EXPECT_TRUE(IsGlobalErrorActive(composite_change)); + + // The second (composite) bubble view has been displayed. + GlobalError* error2 = GetGlobalError(composite_change); + ASSERT_TRUE(error2); + EXPECT_TRUE(error2->HasShownBubbleView()); + + protector_service_->DismissChange(composite_change); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_FALSE(IsGlobalErrorActive(composite_change)); + EXPECT_FALSE(protector_service_->GetLastChange()); + + // Show the third change. + MockSettingChange* mock_change3 = new NiceMock<MockSettingChange>(); + EXPECT_CALL(*mock_change3, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change3, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change3, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change3); + ui_test_utils::RunAllPendingInMessageLoop(); + + // The third change should not be composed with the previous. + EXPECT_TRUE(IsGlobalErrorActive(mock_change3)); + EXPECT_EQ(mock_change3, protector_service_->GetLastChange()); + + protector_service_->DismissChange(mock_change3); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_FALSE(IsGlobalErrorActive(mock_change3)); + EXPECT_FALSE(protector_service_->GetLastChange()); +} + +IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ShowCompositeAndOther) { + GURL url1("http://example.com/"); + GURL url2("http://example.net/"); + + // Show the first change. + EXPECT_CALL(*mock_change_, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change_, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change_, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change_); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + EXPECT_EQ(mock_change_, protector_service_->GetLastChange()); + + // ProtectService will own this change instance as well. + MockSettingChange* mock_change2 = new NiceMock<MockSettingChange>(); + // Show the second change. + EXPECT_CALL(*mock_change2, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change2, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change2, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change2); + ui_test_utils::RunAllPendingInMessageLoop(); + + // Now ProtectorService should be showing a single composite change. + BaseSettingChange* composite_change = protector_service_->GetLastChange(); + ASSERT_TRUE(composite_change); + EXPECT_TRUE(IsGlobalErrorActive(composite_change)); + + // Show the third change, with the same URL as 1st and 2nd. + MockSettingChange* mock_change3 = new NiceMock<MockSettingChange>(); + EXPECT_CALL(*mock_change3, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change3, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change3, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change3); + ui_test_utils::RunAllPendingInMessageLoop(); + + // The third change should be composed with the previous. + EXPECT_FALSE(IsGlobalErrorActive(mock_change3)); + EXPECT_EQ(composite_change, protector_service_->GetLastChange()); + EXPECT_TRUE(IsGlobalErrorActive(composite_change)); + + // Show the 4th change, now with a different URL. + MockSettingChange* mock_change4 = new NiceMock<MockSettingChange>(); + EXPECT_CALL(*mock_change4, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change4, GetNewSettingURL()).WillRepeatedly(Return(url2)); + EXPECT_CALL(*mock_change4, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change4); + ui_test_utils::RunAllPendingInMessageLoop(); + + // The 4th change is shown independently. + EXPECT_TRUE(IsGlobalErrorActive(composite_change)); + EXPECT_TRUE(IsGlobalErrorActive(mock_change4)); + EXPECT_EQ(mock_change4, protector_service_->GetLastChange()); + + protector_service_->DismissChange(composite_change); + protector_service_->DismissChange(mock_change4); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_FALSE(IsGlobalErrorActive(composite_change)); + EXPECT_FALSE(IsGlobalErrorActive(mock_change4)); + EXPECT_FALSE(protector_service_->GetLastChange()); +} + +IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ShowCompositeAndDismissSingle) { + GURL url1("http://example.com/"); + GURL url2("http://example.net/"); + + // Show the first change. + EXPECT_CALL(*mock_change_, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change_, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change_, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change_); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + EXPECT_EQ(mock_change_, protector_service_->GetLastChange()); + + // ProtectService will own this change instance as well. + MockSettingChange* mock_change2 = new NiceMock<MockSettingChange>(); + // Show the second change. + EXPECT_CALL(*mock_change2, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change2, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change2, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change2); + ui_test_utils::RunAllPendingInMessageLoop(); + + // Now ProtectorService should be showing a single composite change. + EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); + EXPECT_FALSE(IsGlobalErrorActive(mock_change2)); + + BaseSettingChange* composite_change = protector_service_->GetLastChange(); + ASSERT_TRUE(composite_change); + EXPECT_TRUE(IsGlobalErrorActive(composite_change)); + + // Show the third change with a different URL. + MockSettingChange* mock_change3 = new NiceMock<MockSettingChange>(); + EXPECT_CALL(*mock_change3, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change3, GetNewSettingURL()).WillRepeatedly(Return(url2)); + EXPECT_CALL(*mock_change3, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change3); + ui_test_utils::RunAllPendingInMessageLoop(); + + // The third change should not be composed with the previous. + EXPECT_TRUE(IsGlobalErrorActive(mock_change3)); + EXPECT_TRUE(IsGlobalErrorActive(composite_change)); + EXPECT_EQ(mock_change3, protector_service_->GetLastChange()); + + // Now dismiss the first change. + protector_service_->DismissChange(mock_change_); + ui_test_utils::RunAllPendingInMessageLoop(); + + // This should effectively dismiss the whole composite change. + EXPECT_FALSE(IsGlobalErrorActive(composite_change)); + EXPECT_TRUE(IsGlobalErrorActive(mock_change3)); + EXPECT_EQ(mock_change3, protector_service_->GetLastChange()); + + protector_service_->DismissChange(mock_change3); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_FALSE(IsGlobalErrorActive(mock_change3)); + EXPECT_FALSE(protector_service_->GetLastChange()); +} + +// Verifies that changes with different URLs but same domain are merged. +IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, SameDomainDifferentURLs) { + GURL url1("http://www.example.com/abc"); + GURL url2("http://example.com/def"); + + // Show the first change with some non-empty URL. + EXPECT_CALL(*mock_change_, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change_, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change_, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change_); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + EXPECT_EQ(mock_change_, protector_service_->GetLastChange()); + + // ProtectService will own this change instance as well. + MockSettingChange* mock_change2 = new NiceMock<MockSettingChange>(); + // Show the second change with another non-empty URL having same domain. + EXPECT_CALL(*mock_change2, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change2, GetNewSettingURL()).WillRepeatedly(Return(url2)); + EXPECT_CALL(*mock_change2, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change2); + ui_test_utils::RunAllPendingInMessageLoop(); + + // Changes should be merged. + EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); + EXPECT_FALSE(IsGlobalErrorActive(mock_change2)); + + BaseSettingChange* composite_change = protector_service_->GetLastChange(); + ASSERT_TRUE(composite_change); + EXPECT_TRUE(IsGlobalErrorActive(composite_change)); + + protector_service_->DismissChange(composite_change); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_FALSE(IsGlobalErrorActive(composite_change)); + EXPECT_FALSE(protector_service_->GetLastChange()); +} + +// Verifies that changes with different Google URLs are merged. +IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, DifferentGoogleDomains) { + GURL url1("http://www.google.com/search?q="); + GURL url2("http://google.ru/search?q="); + + // Show the first change with some non-empty URL. + EXPECT_CALL(*mock_change_, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change_, GetNewSettingURL()).WillRepeatedly(Return(url1)); + EXPECT_CALL(*mock_change_, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change_); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + EXPECT_EQ(mock_change_, protector_service_->GetLastChange()); + + // ProtectService will own this change instance as well. + MockSettingChange* mock_change2 = new NiceMock<MockSettingChange>(); + // Show the second change with another non-empty URL having same domain. + EXPECT_CALL(*mock_change2, MockInit(browser()->profile())). + WillOnce(Return(true)); + EXPECT_CALL(*mock_change2, GetNewSettingURL()).WillRepeatedly(Return(url2)); + EXPECT_CALL(*mock_change2, CanBeMerged()).WillRepeatedly(Return(true)); + protector_service_->ShowChange(mock_change2); + ui_test_utils::RunAllPendingInMessageLoop(); + + // Changes should be merged. + EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); + EXPECT_FALSE(IsGlobalErrorActive(mock_change2)); + + BaseSettingChange* composite_change = protector_service_->GetLastChange(); + ASSERT_TRUE(composite_change); + EXPECT_TRUE(IsGlobalErrorActive(composite_change)); + + protector_service_->DismissChange(composite_change); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_FALSE(IsGlobalErrorActive(composite_change)); + EXPECT_FALSE(protector_service_->GetLastChange()); +} + // TODO(ivankr): Timeout test. } // namespace protector diff --git a/chrome/browser/protector/session_startup_change.cc b/chrome/browser/protector/session_startup_change.cc index c1d6b19..2c76770 100644 --- a/chrome/browser/protector/session_startup_change.cc +++ b/chrome/browser/protector/session_startup_change.cc @@ -43,10 +43,16 @@ class SessionStartupChange : public BasePrefsChange { virtual string16 GetBubbleMessage() const OVERRIDE; virtual string16 GetApplyButtonText() const OVERRIDE; virtual string16 GetDiscardButtonText() const OVERRIDE; + virtual DisplayName GetApplyDisplayName() const OVERRIDE; + virtual GURL GetNewSettingURL() const OVERRIDE; private: virtual ~SessionStartupChange(); + // Returns the first URL among new startup pages. Returns an empty URL if + // there are no new startup pages. + GURL GetFirstNewURL() const; + // Opens all tabs in |tabs| and makes them pinned. void OpenPinnedTabs(Browser* browser, const PinnedTabCodec::Tabs& tabs); @@ -139,26 +145,43 @@ string16 SessionStartupChange::GetApplyButtonText() const { if (new_startup_pref_.type == SessionStartupPref::LAST) return l10n_util::GetStringUTF16(IDS_CHANGE_STARTUP_SETTINGS_RESTORE); - string16 first_url; - if (new_startup_pref_.type == SessionStartupPref::URLS && - !new_startup_pref_.urls.empty()) { - // Display the domain name of the first statrup URL. - first_url = UTF8ToUTF16(new_startup_pref_.urls[0].host()); - } else if (!new_pinned_tabs_.empty()) { - // Start with NTP or no URLs (basically the same): display the domain name - // of the first pinned tab, if any. - first_url = UTF8ToUTF16(new_pinned_tabs_[0].url.host()); - } - return first_url.empty() ? + // Display the domain name of the first startup/pinned URL. + string16 first_domain = UTF8ToUTF16(GetFirstNewURL().host()); + return first_domain.empty() ? l10n_util::GetStringUTF16(IDS_CHANGE_STARTUP_SETTINGS_NTP) : l10n_util::GetStringFUTF16(IDS_CHANGE_STARTUP_SETTINGS_URLS, - first_url); + first_domain); } string16 SessionStartupChange::GetDiscardButtonText() const { return l10n_util::GetStringUTF16(IDS_KEEP_SETTING); } +BaseSettingChange::DisplayName +SessionStartupChange::GetApplyDisplayName() const { + string16 first_domain = UTF8ToUTF16(GetFirstNewURL().host()); + return first_domain.empty() ? + BaseSettingChange::GetApplyDisplayName() : + DisplayName(kSessionStartupChangeNamePriority, first_domain); +} + +GURL SessionStartupChange::GetNewSettingURL() const { + VLOG(1) << "HP URL: " << GetFirstNewURL().spec(); + return GetFirstNewURL(); +} + +GURL SessionStartupChange::GetFirstNewURL() const { + // TODO(ivankr): this actually should return the first URL present in new + // startup prefs AND not in old. + if (new_startup_pref_.type == SessionStartupPref::URLS && + !new_startup_pref_.urls.empty()) { + return new_startup_pref_.urls[0]; + } + if (!new_pinned_tabs_.empty()) + return new_pinned_tabs_[0].url; + return GURL(); +} + void SessionStartupChange::OpenPinnedTabs( Browser* browser, const PinnedTabCodec::Tabs& tabs) { diff --git a/chrome/browser/protector/session_startup_change_unittest.cc b/chrome/browser/protector/session_startup_change_unittest.cc index 8809853..daac359 100644 --- a/chrome/browser/protector/session_startup_change_unittest.cc +++ b/chrome/browser/protector/session_startup_change_unittest.cc @@ -18,6 +18,11 @@ namespace { const char kStartupUrl1[] = "http://google.com"; const char kStartupUrl2[] = "http://example.com"; +const char kStartupHost1[] = "google.com"; +const char kStartupHost2[] = "example.com"; + +const BaseSettingChange::DisplayName kNoDisplayName( + BaseSettingChange::kDefaultNamePriority, string16()); } // namespace @@ -89,6 +94,8 @@ TEST_F(SessionStartupChangeTest, ApplyButtonCaptions) { backup_startup_pref, empty_pinned_tabs_)); ASSERT_TRUE(change->Init(&profile_)); EXPECT_EQ(open_ntp_caption, change->GetApplyButtonText()); + EXPECT_EQ(GURL(), change->GetNewSettingURL()); + EXPECT_EQ(kNoDisplayName, change->GetApplyDisplayName()); // Pinned tabs count as startup URLs as well. PinnedTabCodec::Tabs new_pinned_tabs; @@ -100,6 +107,8 @@ TEST_F(SessionStartupChangeTest, ApplyButtonCaptions) { backup_startup_pref, empty_pinned_tabs_)); ASSERT_TRUE(change->Init(&profile_)); EXPECT_EQ(open_url2_etc_caption, change->GetApplyButtonText()); + EXPECT_EQ(GURL(kStartupUrl2), change->GetNewSettingURL()); + EXPECT_EQ(UTF8ToUTF16(kStartupHost2), change->GetApplyDisplayName().second); // "Open URLs" with no URLs is the same as "Open NTP". initial_startup_pref_.type = SessionStartupPref::URLS; @@ -108,6 +117,8 @@ TEST_F(SessionStartupChangeTest, ApplyButtonCaptions) { backup_startup_pref, empty_pinned_tabs_)); ASSERT_TRUE(change->Init(&profile_)); EXPECT_EQ(open_ntp_caption, change->GetApplyButtonText()); + EXPECT_EQ(GURL(), change->GetNewSettingURL()); + EXPECT_EQ(kNoDisplayName, change->GetApplyDisplayName()); // Single URL. initial_startup_pref_.urls.push_back(GURL(kStartupUrl1)); @@ -116,6 +127,8 @@ TEST_F(SessionStartupChangeTest, ApplyButtonCaptions) { backup_startup_pref, empty_pinned_tabs_)); ASSERT_TRUE(change->Init(&profile_)); EXPECT_EQ(open_url1_etc_caption, change->GetApplyButtonText()); + EXPECT_EQ(GURL(kStartupUrl1), change->GetNewSettingURL()); + EXPECT_EQ(UTF8ToUTF16(kStartupHost1), change->GetApplyDisplayName().second); // Multiple URLs: name of the first one used. initial_startup_pref_.urls.push_back(GURL(kStartupUrl2)); @@ -124,6 +137,8 @@ TEST_F(SessionStartupChangeTest, ApplyButtonCaptions) { backup_startup_pref, empty_pinned_tabs_)); ASSERT_TRUE(change->Init(&profile_)); EXPECT_EQ(open_url1_etc_caption, change->GetApplyButtonText()); + EXPECT_EQ(GURL(kStartupUrl1), change->GetNewSettingURL()); + EXPECT_EQ(UTF8ToUTF16(kStartupHost1), change->GetApplyDisplayName().second); // Pinned tabs go after the startup URLs. change.reset( @@ -131,6 +146,8 @@ TEST_F(SessionStartupChangeTest, ApplyButtonCaptions) { backup_startup_pref, empty_pinned_tabs_)); ASSERT_TRUE(change->Init(&profile_)); EXPECT_EQ(open_url1_etc_caption, change->GetApplyButtonText()); + EXPECT_EQ(GURL(kStartupUrl1), change->GetNewSettingURL()); + EXPECT_EQ(UTF8ToUTF16(kStartupHost1), change->GetApplyDisplayName().second); } } // namespace protector diff --git a/chrome/browser/protector/settings_change_global_error.cc b/chrome/browser/protector/settings_change_global_error.cc index a51103b..9a18a04 100644 --- a/chrome/browser/protector/settings_change_global_error.cc +++ b/chrome/browser/protector/settings_change_global_error.cc @@ -165,6 +165,10 @@ string16 SettingsChangeGlobalError::GetBubbleViewCancelButtonLabel() { } void SettingsChangeGlobalError::OnBubbleViewDidClose(Browser* browser) { + // The bubble may be closed as the result of RemoveFromProfile call when + // merging this error with another one. + if (!profile_) + return; if (!closed_by_button_) { BrowserThread::PostDelayedTask( BrowserThread::UI, FROM_HERE, diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index be4472b..39761c0 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1801,6 +1801,8 @@ 'browser/protector/base_prefs_change.h', 'browser/protector/base_setting_change.cc', 'browser/protector/base_setting_change.h', + 'browser/protector/composite_settings_change.cc', + 'browser/protector/composite_settings_change.h', 'browser/protector/default_search_provider_change.cc', 'browser/protector/histograms.cc', 'browser/protector/histograms.h', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 21040cc..789456f 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -1616,6 +1616,7 @@ 'browser/profiles/profile_info_cache_unittest.h', 'browser/profiles/profile_info_util_unittest.cc', 'browser/profiles/profile_manager_unittest.cc', + 'browser/protector/composite_settings_change_unittest.cc', 'browser/protector/prefs_backup_invalid_change_unittest.cc', 'browser/protector/protected_prefs_watcher_unittest.cc', 'browser/protector/session_startup_change_unittest.cc', |