Show confirmation step when turned on

Show a confirmation step when cookie blocking was enabled for a
specific site.

Bug: 967668
Change-Id: I09f2ebf71c196d6a7085a18405da9d59f581a183
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849853Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704225}

chrome/app/generated_resources.grd

@@ -2515,6 +2515,9 @@ are declared in tools/grit/grit_rule.gni.
       <message name="IDS_COOKIE_CONTROLS_NOT_WORKING_TITLE" desc="Label shown on a dialog that allows users to turn off third-party cookie blocking for a specific site.">
         Site not working?
       </message>
+      <message name="IDS_COOKIE_CONTROLS_TURNED_ON_TITLE" desc="Label shown to a user after cookie blocking has been turned on for a specific site.">
+        Third-party cookie blocking is on
+      </message>
      <message name="IDS_COOKIE_CONTROLS_NOT_WORKING_DESCRIPTION" desc="Label shown on a dialog that allows users to turn off third-party cookie blocking for a specific site.">
         Some sites use third-party cookies to load their pages. If a site isn't working, you can try turning off cookie blocking.
       </message>

chrome/app/generated_resources_grd/IDS_COOKIE_CONTROLS_TURNED_ON_TITLE.png.sha1

+f75771d80ea4534f8aee1f9e9a6786947ccdffbc
\ No newline at end of file

chrome/browser/ui/cookie_controls/cookie_controls_controller.cc

@@ -54,8 +54,7 @@ void CookieControlsController::Update(content::WebContents* web_contents) {
         this, TabSpecificContentSettings::FromWebContents(web_contents));
   }
   for (auto& observer : observers_)
-    observer.OnStatusChanged(GetStatus(web_contents));
-  PresentBlockedCookieCounter();
+    observer.OnStatusChanged(GetStatus(web_contents), GetBlockedCookieCount());
 }
 
 CookieControlsController::Status CookieControlsController::GetStatus(
@@ -87,13 +86,6 @@ void CookieControlsController::OnCookieBlockingEnabledForSite(
   Update(GetWebContents());
 }
 
-int CookieControlsController::GetBlockedDomainCount() {
-  const LocalSharedObjectsContainer& blocked_objects =
-      tab_observer_->tab_specific_content_settings()
-          ->blocked_local_shared_objects();
-  return blocked_objects.GetDomainCount();
-}
-
 int CookieControlsController::GetBlockedCookieCount() {
   const LocalSharedObjectsContainer& blocked_objects =
       tab_observer_->tab_specific_content_settings()
@@ -102,11 +94,9 @@ int CookieControlsController::GetBlockedCookieCount() {
 }
 
 void CookieControlsController::PresentBlockedCookieCounter() {
-  const LocalSharedObjectsContainer& blocked_objects =
-      tab_observer_->tab_specific_content_settings()
-          ->blocked_local_shared_objects();
+  int blocked_cookies = GetBlockedCookieCount();
   for (auto& observer : observers_)
-    observer.OnBlockedCookiesCountChanged(blocked_objects.GetObjectCount());
+    observer.OnBlockedCookiesCountChanged(blocked_cookies);
 }
 
 void CookieControlsController::OnPrefChanged() {

chrome/browser/ui/cookie_controls/cookie_controls_controller.h

@@ -47,11 +47,6 @@ class CookieControlsController {
   // blocking.
   void OnCookieBlockingEnabledForSite(bool block_third_party_cookies);
 
-  // Returns the number of registrable domains with blocked cookies.
-  int GetBlockedDomainCount();
-
-  // Returns the number of blocked cookies.
-  int GetBlockedCookieCount();
 
   void AddObserver(CookieControlsView* obs);
   void RemoveObserver(CookieControlsView* obs);
@@ -82,6 +77,9 @@ class CookieControlsController {
   // Updates the blocked cookie count of |icon_|.
   void PresentBlockedCookieCounter();
 
+  // Returns the number of blocked cookies.
+  int GetBlockedCookieCount();
+
   // Callback for when the cookie controls or third-party cookie blocking
   // preference changes.
   void OnPrefChanged();

chrome/browser/ui/cookie_controls/cookie_controls_controller_unittest.cc

@@ -20,7 +20,7 @@ namespace {
 
 class MockCookieControlsView : public CookieControlsView {
  public:
-  MOCK_METHOD1(OnStatusChanged, void(CookieControlsController::Status));
+  MOCK_METHOD2(OnStatusChanged, void(CookieControlsController::Status, int));
   MOCK_METHOD1(OnBlockedCookiesCountChanged, void(int));
 };
 
@@ -80,8 +80,7 @@ class CookieControlsTest : public ChromeRenderViewHostTestHarness {
 
 TEST_F(CookieControlsTest, NewTabPage) {
   EXPECT_CALL(*mock(),
-              OnStatusChanged(CookieControlsController::Status::kDisabled));
-  EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
+              OnStatusChanged(CookieControlsController::Status::kDisabled, 0));
   cookie_controls()->Update(web_contents());
 }
 
@@ -89,8 +88,7 @@ TEST_F(CookieControlsTest, SomeWebSite) {
   // Visiting a website should enable the UI.
   NavigateAndCommit(GURL("https://example.com"));
   EXPECT_CALL(*mock(),
-              OnStatusChanged(CookieControlsController::Status::kEnabled));
-  EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
+              OnStatusChanged(CookieControlsController::Status::kEnabled, 0));
   cookie_controls()->Update(web_contents());
   testing::Mock::VerifyAndClearExpectations(mock());
 
@@ -98,36 +96,31 @@ TEST_F(CookieControlsTest, SomeWebSite) {
   EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
   tab_specific_content_settings()->OnWebDatabaseAccessed(
       GURL("https://example.com"), /*blocked=*/false);
-  EXPECT_EQ(0, cookie_controls()->GetBlockedDomainCount());
   testing::Mock::VerifyAndClearExpectations(mock());
 
   // Blocking cookies should update the blocked cookie count.
   EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(1));
   tab_specific_content_settings()->OnWebDatabaseAccessed(
       GURL("https://thirdparty.com"), /*blocked=*/true);
-  EXPECT_EQ(1, cookie_controls()->GetBlockedDomainCount());
   testing::Mock::VerifyAndClearExpectations(mock());
 
   // Navigating somewhere else should reset the cookie count.
   NavigateAndCommit(GURL("https://somethingelse.com"));
   EXPECT_CALL(*mock(),
-              OnStatusChanged(CookieControlsController::Status::kEnabled));
-  EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
+              OnStatusChanged(CookieControlsController::Status::kEnabled, 0));
   cookie_controls()->Update(web_contents());
 }
 
 TEST_F(CookieControlsTest, PreferenceDisabled) {
   NavigateAndCommit(GURL("https://example.com"));
   EXPECT_CALL(*mock(),
-              OnStatusChanged(CookieControlsController::Status::kEnabled));
-  EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
+              OnStatusChanged(CookieControlsController::Status::kEnabled, 0));
   cookie_controls()->Update(web_contents());
   testing::Mock::VerifyAndClearExpectations(mock());
 
   // Disabling the feature should disable the UI.
   EXPECT_CALL(*mock(),
-              OnStatusChanged(CookieControlsController::Status::kDisabled));
-  EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
+              OnStatusChanged(CookieControlsController::Status::kDisabled, 0));
   profile()->GetPrefs()->SetInteger(
       prefs::kCookieControlsMode,
       static_cast<int>(content_settings::CookieControlsMode::kOff));
@@ -137,38 +130,35 @@ TEST_F(CookieControlsTest, PreferenceDisabled) {
 TEST_F(CookieControlsTest, DisableForSite) {
   NavigateAndCommit(GURL("https://example.com"));
   EXPECT_CALL(*mock(),
-              OnStatusChanged(CookieControlsController::Status::kEnabled));
-  EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
+              OnStatusChanged(CookieControlsController::Status::kEnabled, 0));
   cookie_controls()->Update(web_contents());
   testing::Mock::VerifyAndClearExpectations(mock());
 
   // Disabling cookie blocking for example.com should update the ui.
-  EXPECT_CALL(*mock(), OnStatusChanged(
-                           CookieControlsController::Status::kDisabledForSite));
-  EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
+  EXPECT_CALL(
+      *mock(),
+      OnStatusChanged(CookieControlsController::Status::kDisabledForSite, 0));
   cookie_controls()->OnCookieBlockingEnabledForSite(false);
   testing::Mock::VerifyAndClearExpectations(mock());
 
   // Visiting some other site, should switch back to kEnabled.
   NavigateAndCommit(GURL("https://somethingelse.com"));
   EXPECT_CALL(*mock(),
-              OnStatusChanged(CookieControlsController::Status::kEnabled));
-  EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
+              OnStatusChanged(CookieControlsController::Status::kEnabled, 0));
   cookie_controls()->Update(web_contents());
   testing::Mock::VerifyAndClearExpectations(mock());
 
   // Visiting example.com should set status to kDisabledForSite.
   NavigateAndCommit(GURL("https://example.com"));
-  EXPECT_CALL(*mock(), OnStatusChanged(
-                           CookieControlsController::Status::kDisabledForSite));
-  EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
+  EXPECT_CALL(
+      *mock(),
+      OnStatusChanged(CookieControlsController::Status::kDisabledForSite, 0));
   cookie_controls()->Update(web_contents());
   testing::Mock::VerifyAndClearExpectations(mock());
 
   // Enabling example.com again should change status to kEnabled.
   EXPECT_CALL(*mock(),
-              OnStatusChanged(CookieControlsController::Status::kEnabled));
-  EXPECT_CALL(*mock(), OnBlockedCookiesCountChanged(0));
+              OnStatusChanged(CookieControlsController::Status::kEnabled, 0));
   cookie_controls()->OnCookieBlockingEnabledForSite(true);
   testing::Mock::VerifyAndClearExpectations(mock());
 }

chrome/browser/ui/cookie_controls/cookie_controls_view.h

@@ -11,7 +11,8 @@
 // Interface for the CookieControls UI.
 class CookieControlsView : public base::CheckedObserver {
  public:
-  virtual void OnStatusChanged(CookieControlsController::Status status) = 0;
+  virtual void OnStatusChanged(CookieControlsController::Status status,
+                               int blocked_cookies) = 0;
   virtual void OnBlockedCookiesCountChanged(int blocked_cookies) = 0;
 };
 

chrome/browser/ui/views/location_bar/cookie_controls_bubble_view.cc

@@ -55,19 +55,22 @@ CookieControlsBubbleView* CookieControlsBubbleView::GetCookieBubble() {
 }
 
 void CookieControlsBubbleView::OnStatusChanged(
-    CookieControlsController::Status new_status) {
-  if (status_ == new_status)
+    CookieControlsController::Status new_status,
+    int blocked_cookies) {
+  if (status_ == new_status) {
+    OnBlockedCookiesCountChanged(blocked_cookies);
     return;
-
-  show_disable_cookie_blocking_ui_ = false;
+  }
+  intermediate_step_ = IntermediateStep::kNone;
   status_ = new_status;
+  blocked_cookies_ = blocked_cookies;
   UpdateUi();
 }
 
 void CookieControlsBubbleView::OnBlockedCookiesCountChanged(
     int blocked_cookies) {
   // The blocked cookie count changes quite frequently, so avoid unnecessary
-  // UI updates and unnecessarily calling GetBlockedDomainCount() if possible.
+  // UI updates if possible.
   if (blocked_cookies_ == blocked_cookies)
     return;
 
@@ -109,10 +112,18 @@ void CookieControlsBubbleView::UpdateUi() {
   text_->SetVisible(false);
   header_view_->SetVisible(false);
 
-  if (show_disable_cookie_blocking_ui_) {
+  if (intermediate_step_ == IntermediateStep::kTurnOffButton) {
     text_->SetVisible(true);
     text_->SetText(
         l10n_util::GetStringUTF16(IDS_COOKIE_CONTROLS_NOT_WORKING_DESCRIPTION));
+  } else if (intermediate_step_ == IntermediateStep::kBlockingIsOn) {
+    header_view_->SetVisible(true);
+    header_view_->SetImage(
+        ui::ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
+            IDR_COOKIE_BLOCKING_ON_HEADER));
+    text_->SetVisible(true);
+    text_->SetText(
+        l10n_util::GetStringUTF16(IDS_COOKIE_CONTROLS_BLOCKED_MESSAGE));
   } else if (status_ == CookieControlsController::Status::kEnabled) {
     header_view_->SetVisible(true);
     header_view_->SetImage(
@@ -137,7 +148,7 @@ void CookieControlsBubbleView::UpdateUi() {
   // The show_disable_cookie_blocking_ui_ state has a different title
   // configuration. To avoid jumping UI, don't resize the bubble. This should be
   // safe as the bubble in this state has less content than in Enabled state.
-  if (!show_disable_cookie_blocking_ui_)
+  if (intermediate_step_ != IntermediateStep::kTurnOffButton)
     SizeToContents();
 }
 
@@ -149,7 +160,7 @@ void CookieControlsBubbleView::CloseBubble() {
 }
 
 int CookieControlsBubbleView::GetDialogButtons() const {
-  if (show_disable_cookie_blocking_ui_ ||
+  if (intermediate_step_ == IntermediateStep::kTurnOffButton ||
       status_ == CookieControlsController::Status::kDisabledForSite) {
     return ui::DIALOG_BUTTON_OK;
   }
@@ -158,9 +169,10 @@ int CookieControlsBubbleView::GetDialogButtons() const {
 
 base::string16 CookieControlsBubbleView::GetDialogButtonLabel(
     ui::DialogButton button) const {
-  if (show_disable_cookie_blocking_ui_)
+  if (intermediate_step_ == IntermediateStep::kTurnOffButton)
     return l10n_util::GetStringUTF16(IDS_COOKIE_CONTROLS_TURN_OFF_BUTTON);
   DCHECK_EQ(status_, CookieControlsController::Status::kDisabledForSite);
+  DCHECK_EQ(intermediate_step_, IntermediateStep::kNone);
   return l10n_util::GetStringUTF16(IDS_COOKIE_CONTROLS_TURN_ON_BUTTON);
 }
 
@@ -201,13 +213,19 @@ gfx::Size CookieControlsBubbleView::CalculatePreferredSize() const {
 }
 
 base::string16 CookieControlsBubbleView::GetWindowTitle() const {
-  if (show_disable_cookie_blocking_ui_)
-    return l10n_util::GetStringUTF16(IDS_COOKIE_CONTROLS_NOT_WORKING_TITLE);
+  switch (intermediate_step_) {
+    case IntermediateStep::kTurnOffButton:
+      return l10n_util::GetStringUTF16(IDS_COOKIE_CONTROLS_NOT_WORKING_TITLE);
+    case IntermediateStep::kBlockingIsOn:
+      return l10n_util::GetStringUTF16(IDS_COOKIE_CONTROLS_TURNED_ON_TITLE);
+    case IntermediateStep::kNone: {
+      // Determine title based on status_ instead.
+    }
+  }
   switch (status_) {
     case CookieControlsController::Status::kEnabled:
-      return l10n_util::GetPluralStringFUTF16(
-          IDS_COOKIE_CONTROLS_DIALOG_TITLE,
-          controller_->GetBlockedCookieCount());
+      return l10n_util::GetPluralStringFUTF16(IDS_COOKIE_CONTROLS_DIALOG_TITLE,
+                                              blocked_cookies_.value_or(0));
     case CookieControlsController::Status::kDisabledForSite:
       return l10n_util::GetStringUTF16(IDS_COOKIE_CONTROLS_DIALOG_TITLE_OFF);
     case CookieControlsController::Status::kUninitialized:
@@ -237,11 +255,14 @@ void CookieControlsBubbleView::WindowClosing() {
 }
 
 bool CookieControlsBubbleView::Accept() {
-  if (show_disable_cookie_blocking_ui_) {
+  if (intermediate_step_ == IntermediateStep::kTurnOffButton) {
     controller_->OnCookieBlockingEnabledForSite(false);
   } else {
     DCHECK_EQ(status_, CookieControlsController::Status::kDisabledForSite);
+    DCHECK_EQ(intermediate_step_, IntermediateStep::kNone);
     controller_->OnCookieBlockingEnabledForSite(true);
+    intermediate_step_ = IntermediateStep::kBlockingIsOn;
+    UpdateUi();
   }
   return false;
 }
@@ -256,6 +277,6 @@ void CookieControlsBubbleView::LinkClicked(views::Link* source,
   DCHECK_EQ(status_, CookieControlsController::Status::kEnabled);
   // Don't go through the controller as this is an intermediary state that
   // is only relevant for the bubble UI.
-  show_disable_cookie_blocking_ui_ = true;
+  intermediate_step_ = IntermediateStep::kTurnOffButton;
   UpdateUi();
 }

chrome/browser/ui/views/location_bar/cookie_controls_bubble_view.h

@@ -38,10 +38,19 @@ class CookieControlsBubbleView : public LocationBarBubbleDelegateView,
   static CookieControlsBubbleView* GetCookieBubble();
 
   // CookieControlsView:
-  void OnStatusChanged(CookieControlsController::Status status) override;
+  void OnStatusChanged(CookieControlsController::Status status,
+                       int blocked_cookies) override;
   void OnBlockedCookiesCountChanged(int blocked_cookies) override;
 
  private:
+  enum class IntermediateStep {
+    kNone,
+    // Show a button to disable cookie blocking on the current site.
+    kTurnOffButton,
+    // Show a confirmation that cookie blocking was turned on.
+    kBlockingIsOn,
+  };
+
   CookieControlsBubbleView(views::View* anchor_view,
                            content::WebContents* web_contents,
                            CookieControlsController* cookie_contols);
@@ -72,9 +81,7 @@ class CookieControlsBubbleView : public LocationBarBubbleDelegateView,
   CookieControlsController::Status status_ =
       CookieControlsController::Status::kUninitialized;
 
-  // If true, display an intermediate step with a button to disable cookie
-  // blocking on the current site.
-  bool show_disable_cookie_blocking_ui_ = false;
+  IntermediateStep intermediate_step_ = IntermediateStep::kNone;
 
   base::Optional<int> blocked_cookies_;
 

chrome/browser/ui/views/location_bar/cookie_controls_icon_view.cc

@@ -40,12 +40,14 @@ bool CookieControlsIconView::Update() {
 }
 
 void CookieControlsIconView::OnStatusChanged(
-    CookieControlsController::Status status) {
+    CookieControlsController::Status status,
+    int blocked_cookies) {
   if (status_ != status) {
     status_ = status;
     SetVisible(ShouldBeVisible());
     UpdateIconImage();
   }
+  OnBlockedCookiesCountChanged(blocked_cookies);
 }
 
 void CookieControlsIconView::OnBlockedCookiesCountChanged(int blocked_cookies) {

chrome/browser/ui/views/location_bar/cookie_controls_icon_view.h

@@ -21,7 +21,8 @@ class CookieControlsIconView : public PageActionIconView,
   ~CookieControlsIconView() override;
 
   // CookieControlsUI:
-  void OnStatusChanged(CookieControlsController::Status status) override;
+  void OnStatusChanged(CookieControlsController::Status status,
+                       int blocked_cookies) override;
   void OnBlockedCookiesCountChanged(int blocked_cookies) override;
 
   // PageActionIconView: