diff options
author | ivankr@chromium.org <ivankr@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-13 12:45:00 +0000 |
---|---|---|
committer | ivankr@chromium.org <ivankr@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-13 12:45:00 +0000 |
commit | 0cf77af7a54e7a817171c249a808b404b36ba4a4 (patch) | |
tree | 9eb43a09bc7d3dabdf46cecda716fc56f830b62f /chrome/browser/protector | |
parent | daa3b8227052fe93d204f5c46cf9dcd66b62b4a8 (diff) | |
download | chromium_src-0cf77af7a54e7a817171c249a808b404b36ba4a4.zip chromium_src-0cf77af7a54e7a817171c249a808b404b36ba4a4.tar.gz chromium_src-0cf77af7a54e7a817171c249a808b404b36ba4a4.tar.bz2 |
[protector] Added changes do not hide the currently shown change, if any.
BUG=114288
TEST=ProtectorServiceTest.*
Review URL: http://codereview.chromium.org/9664063
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@126381 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser/protector')
5 files changed, 151 insertions, 88 deletions
diff --git a/chrome/browser/protector/protector_service.cc b/chrome/browser/protector/protector_service.cc index 488fc6a..93e353d 100644 --- a/chrome/browser/protector/protector_service.cc +++ b/chrome/browser/protector/protector_service.cc @@ -20,7 +20,8 @@ namespace protector { ProtectorService::ProtectorService(Profile* profile) - : profile_(profile) { + : profile_(profile), + has_active_change_(false) { } ProtectorService::~ProtectorService() { @@ -40,7 +41,9 @@ void ProtectorService::ShowChange(BaseSettingChange* change) { new SettingsChangeGlobalError(change, this); new_item.error.reset(error); items_.push_back(new_item); - error->ShowForProfile(profile_); + // 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 { @@ -80,12 +83,14 @@ void ProtectorService::OnApplyChange(SettingsChangeGlobalError* error, Browser* browser) { DVLOG(1) << "Apply change"; error->change()->Apply(browser); + has_active_change_ = false; } void ProtectorService::OnDiscardChange(SettingsChangeGlobalError* error, Browser* browser) { DVLOG(1) << "Discard change"; error->change()->Discard(browser); + has_active_change_ = false; } void ProtectorService::OnDecisionTimeout(SettingsChangeGlobalError* error) { @@ -98,6 +103,17 @@ void ProtectorService::OnRemovedFromProfile(SettingsChangeGlobalError* error) { std::find_if(items_.begin(), items_.end(), MatchItemByError(error)); DCHECK(item != items_.end()); items_.erase(item); + + 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(); + has_active_change_ = true; + return; + } + } + } } BaseSettingChange* ProtectorService::GetLastChange() { diff --git a/chrome/browser/protector/protector_service.h b/chrome/browser/protector/protector_service.h index efb7843..942e567 100644 --- a/chrome/browser/protector/protector_service.h +++ b/chrome/browser/protector/protector_service.h @@ -113,6 +113,10 @@ class ProtectorService : public ProfileKeyedService, // Profile which settings we are protecting. Profile* profile_; + // True if there is a change that has been shown and not yet accepted or + // discarded by user. + bool has_active_change_; + DISALLOW_COPY_AND_ASSIGN(ProtectorService); }; diff --git a/chrome/browser/protector/protector_service_browsertest.cc b/chrome/browser/protector/protector_service_browsertest.cc index 5e41dbf..17912ff 100644 --- a/chrome/browser/protector/protector_service_browsertest.cc +++ b/chrome/browser/protector/protector_service_browsertest.cc @@ -154,7 +154,6 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, BubbleClosedInsideApply) { ASSERT_TRUE(bubble_view); EXPECT_CALL(*mock_change_, Apply(browser())).WillOnce(InvokeWithoutArgs( bubble_view, &GlobalErrorBubbleViewBase::CloseBubbleView)); - // Pressing Cancel applies the change. error->BubbleViewCancelButtonPressed(browser()); ui_test_utils::RunAllPendingInMessageLoop(); @@ -236,6 +235,11 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + // The first bubble view has been displayed. + GlobalError* error = GetGlobalError(mock_change_); + ASSERT_TRUE(error); + ASSERT_TRUE(error->HasShownBubbleView()); + // ProtectService will own this change instance as well. MockSettingChange* mock_change2 = new NiceMock<MockSettingChange>(); // Show the second change. @@ -246,27 +250,73 @@ IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); EXPECT_TRUE(IsGlobalErrorActive(mock_change2)); + // The second bubble view hasn't been displayed because the first is still + // shown. + GlobalError* error2 = GetGlobalError(mock_change2); + ASSERT_TRUE(error2); + EXPECT_FALSE(error2->HasShownBubbleView()); + // Apply the first change, mimicking a button click; the second should still // be active. EXPECT_CALL(*mock_change_, Apply(browser())); + error->BubbleViewCancelButtonPressed(browser()); + error->GetBubbleView()->CloseBubbleView(); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); + EXPECT_TRUE(IsGlobalErrorActive(mock_change2)); + + // Now the second bubble view should be shown. + ASSERT_TRUE(error2->HasShownBubbleView()); + + // Finally apply the second change. + EXPECT_CALL(*mock_change2, Apply(browser())); + error2->BubbleViewCancelButtonPressed(browser()); + error2->GetBubbleView()->CloseBubbleView(); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); + EXPECT_FALSE(IsGlobalErrorActive(mock_change2)); +} + +IN_PROC_BROWSER_TEST_F(ProtectorServiceTest, + ShowMultipleChangesAndApplyManuallyBeforeOther) { + // Show the first change. + EXPECT_CALL(*mock_change_, MockInit(browser()->profile())). + WillOnce(Return(true)); + protector_service_->ShowChange(mock_change_); + ui_test_utils::RunAllPendingInMessageLoop(); + EXPECT_TRUE(IsGlobalErrorActive(mock_change_)); + + // The first bubble view has been displayed. GlobalError* error = GetGlobalError(mock_change_); ASSERT_TRUE(error); - error->ShowBubbleView(browser()); + ASSERT_TRUE(error->HasShownBubbleView()); + + // Apply the first change, mimicking a button click. + EXPECT_CALL(*mock_change_, Apply(browser())); error->BubbleViewCancelButtonPressed(browser()); error->GetBubbleView()->CloseBubbleView(); ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); + + // 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)); + protector_service_->ShowChange(mock_change2); + ui_test_utils::RunAllPendingInMessageLoop(); EXPECT_TRUE(IsGlobalErrorActive(mock_change2)); + // The second bubble view has been displayed. + GlobalError* error2 = GetGlobalError(mock_change2); + ASSERT_TRUE(error2); + ASSERT_TRUE(error2->HasShownBubbleView()); + // Finally apply the second change. EXPECT_CALL(*mock_change2, Apply(browser())); - GlobalError* error2 = GetGlobalError(mock_change2); - ASSERT_TRUE(error); - error2->ShowBubbleView(browser()); error2->BubbleViewCancelButtonPressed(browser()); error2->GetBubbleView()->CloseBubbleView(); ui_test_utils::RunAllPendingInMessageLoop(); - EXPECT_FALSE(IsGlobalErrorActive(mock_change_)); EXPECT_FALSE(IsGlobalErrorActive(mock_change2)); } diff --git a/chrome/browser/protector/settings_change_global_error.cc b/chrome/browser/protector/settings_change_global_error.cc index cfb1d69..a51103b 100644 --- a/chrome/browser/protector/settings_change_global_error.cc +++ b/chrome/browser/protector/settings_change_global_error.cc @@ -67,6 +67,46 @@ SettingsChangeGlobalError::~SettingsChangeGlobalError() { menu_ids.Get().reset(menu_id_ - IDC_SHOW_SETTINGS_CHANGE_FIRST); } +void SettingsChangeGlobalError::AddToProfile(Profile* profile, + bool show_bubble) { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + profile_ = profile; + GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(this); + BrowserList::AddObserver(this); + if (show_bubble) { + ShowBubble(); + } else { + // Start inactivity timer. + BrowserThread::PostDelayedTask( + BrowserThread::UI, FROM_HERE, + base::Bind(&SettingsChangeGlobalError::OnInactiveTimeout, + weak_factory_.GetWeakPtr()), + base::TimeDelta::FromMilliseconds(kMenuItemDisplayPeriodMs)); + } +} + +void SettingsChangeGlobalError::RemoveFromProfile() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + if (profile_) { + GlobalErrorServiceFactory::GetForProfile(profile_)->RemoveGlobalError(this); + profile_ = NULL; + } + BrowserList::RemoveObserver(this); + // This will delete |this|. + delegate_->OnRemovedFromProfile(this); +} + +void SettingsChangeGlobalError::ShowBubble() { + DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); + DCHECK(profile_); + Browser* browser = BrowserList::FindTabbedBrowser( + profile_, + // match incognito + true); + if (browser) + ShowBubbleInBrowser(browser); +} + bool SettingsChangeGlobalError::HasBadge() { return true; } @@ -92,7 +132,7 @@ int SettingsChangeGlobalError::MenuItemIconResourceID() { } void SettingsChangeGlobalError::ExecuteMenuItem(Browser* browser) { - ShowInBrowser(browser); + ShowBubbleInBrowser(browser); } bool SettingsChangeGlobalError::HasBubbleView() { @@ -124,45 +164,6 @@ string16 SettingsChangeGlobalError::GetBubbleViewCancelButtonLabel() { return change_->GetApplyButtonText(); } -void SettingsChangeGlobalError::BubbleViewAcceptButtonPressed( - Browser* browser) { - closed_by_button_ = true; - delegate_->OnDiscardChange(this, browser); -} - -void SettingsChangeGlobalError::BubbleViewCancelButtonPressed( - Browser* browser) { - closed_by_button_ = true; - delegate_->OnApplyChange(this, browser); -} - -void SettingsChangeGlobalError::OnBrowserSetLastActive( - const Browser* browser) { - if (show_on_browser_activation_ && browser && browser->is_type_tabbed()) { - // A tabbed browser window got activated, show the error bubble again. - // Calling Show() immediately from here does not always work because the - // old browser window may still have focus. - // Multiple posted Show() calls are fine since the first successful one - // will invalidate all the weak pointers. - // Note that Show() will display the bubble in the last active browser - // (which may not be |browser| at the moment Show() is executed). - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(&SettingsChangeGlobalError::Show, - weak_factory_.GetWeakPtr())); - } -} - -void SettingsChangeGlobalError::RemoveFromProfile() { - if (profile_) { - GlobalErrorServiceFactory::GetForProfile(profile_)->RemoveGlobalError(this); - profile_ = NULL; - } - BrowserList::RemoveObserver(this); - // This will delete |this|. - delegate_->OnRemovedFromProfile(this); -} - void SettingsChangeGlobalError::OnBubbleViewDidClose(Browser* browser) { if (!closed_by_button_) { BrowserThread::PostDelayedTask( @@ -185,38 +186,36 @@ void SettingsChangeGlobalError::OnBubbleViewDidClose(Browser* browser) { } } -void SettingsChangeGlobalError::ShowForProfile(Profile* profile) { - if (BrowserThread::CurrentlyOn(BrowserThread::UI)) { - AddToProfile(profile); - } else { - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(&SettingsChangeGlobalError::AddToProfile, - base::Unretained(this), - profile)); - } +void SettingsChangeGlobalError::BubbleViewAcceptButtonPressed( + Browser* browser) { + closed_by_button_ = true; + delegate_->OnDiscardChange(this, browser); } -void SettingsChangeGlobalError::AddToProfile(Profile* profile) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - profile_ = profile; - GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(this); - BrowserList::AddObserver(this); - Show(); +void SettingsChangeGlobalError::BubbleViewCancelButtonPressed( + Browser* browser) { + closed_by_button_ = true; + delegate_->OnApplyChange(this, browser); } -void SettingsChangeGlobalError::Show() { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); - DCHECK(profile_); - Browser* browser = BrowserList::FindTabbedBrowser( - profile_, - // match incognito - true); - if (browser) - ShowInBrowser(browser); +void SettingsChangeGlobalError::OnBrowserSetLastActive( + const Browser* browser) { + if (show_on_browser_activation_ && browser && browser->is_type_tabbed()) { + // A tabbed browser window got activated, show the error bubble again. + // Calling ShowBubble() immediately from here does not always work because + // the old browser window may still have focus. + // Multiple posted ShowBubble() calls are fine since the first successful + // one will invalidate all the weak pointers. + // Note that ShowBubble() will display the bubble in the last active browser + // (which may not be |browser| at the moment ShowBubble() is executed). + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, + base::Bind(&SettingsChangeGlobalError::ShowBubble, + weak_factory_.GetWeakPtr())); + } } -void SettingsChangeGlobalError::ShowInBrowser(Browser* browser) { +void SettingsChangeGlobalError::ShowBubbleInBrowser(Browser* browser) { show_on_browser_activation_ = false; // Cancel any previously posted tasks so that the global error // does not get removed on timeout while still showing the bubble. diff --git a/chrome/browser/protector/settings_change_global_error.h b/chrome/browser/protector/settings_change_global_error.h index 1c89356..a396df9 100644 --- a/chrome/browser/protector/settings_change_global_error.h +++ b/chrome/browser/protector/settings_change_global_error.h @@ -34,13 +34,16 @@ class SettingsChangeGlobalError : public GlobalError, SettingsChangeGlobalErrorDelegate* delegate); virtual ~SettingsChangeGlobalError(); - // Displays a global error bubble for the given browser profile. - // Can be called from any thread. - void ShowForProfile(Profile* profile); + // Adds a global error to the given browser profile and shows a bubble + // immediately if |show_bubble| is |true|. + void AddToProfile(Profile* profile, bool show_bubble); // Removes global error from its profile. void RemoveFromProfile(); + // Displays the bubble in the last active tabbed browser. + void ShowBubble(); + // Returns the change instance to which this error refers. BaseSettingChange* change() { return change_; } @@ -68,17 +71,8 @@ class SettingsChangeGlobalError : public GlobalError, virtual void OnBrowserRemoved(const Browser* browser) OVERRIDE {} virtual void OnBrowserSetLastActive(const Browser* browser) OVERRIDE; - // Helper called on the UI thread to add this global error to the default - // profile (stored in |profile_|). - void AddToProfile(Profile* profile); - - // Displays the bubble in the last active tabbed browser. Must be called - // on the UI thread. - void Show(); - - // Displays the bubble in |browser|'s window. Must be called - // on the UI thread. - void ShowInBrowser(Browser* browser); + // Displays the bubble in |browser|'s window. + void ShowBubbleInBrowser(Browser* browser); // Called when the wrench menu item has been displayed for enough time // without user interaction. |