Remove "BlockageIndicated" from TabSpecificContentSettings

Image models are not 1:1 with content settings. This CL replaces
per-tab storage from TSCS to a new per-WebContents container,
ContentSettingImageModelState, which contains information that
is 1:1 with image models.

This allows us to remove custom logic for image models that aren't
backed by a single content setting.

The new behavior works like this:
 1. Per-tab state is stored in ContentSettingImageModelState
 2. ShouldRunAnimation / SetAnimationHasRun updates the state
 3. State is additionally updated when an image is hidden

(3) entails a bunch of changes to UpdateFromWebContents, namely,
making it return a boolean for whether the update triggered visibility.

The benefit of this CL is that all of the logic to control animation of
image models come from ContentSettingImageModel.

This CL has no intended user-facing behavior change.

Bug: 900645
Change-Id: I99c36a8184a0e04e6356e58fbc93a5aed3f03127
Reviewed-on: https://chromium-review.googlesource.com/c/1310674Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607331}

chrome/browser/content_settings/tab_specific_content_settings.cc

@@ -282,19 +282,6 @@ bool TabSpecificContentSettings::IsContentBlocked(
   return false;
 }
 
-bool TabSpecificContentSettings::IsBlockageIndicated(
-    ContentSettingsType content_type) const {
-  const auto& it = content_settings_status_.find(content_type);
-  if (it != content_settings_status_.end())
-    return it->second.blockage_indicated_to_user;
-  return false;
-}
-
-void TabSpecificContentSettings::SetBlockageHasBeenIndicated(
-    ContentSettingsType content_type) {
-  content_settings_status_[content_type].blockage_indicated_to_user = true;
-}
-
 bool TabSpecificContentSettings::IsContentAllowed(
     ContentSettingsType content_type) const {
   DCHECK_NE(CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS, content_type)
@@ -666,7 +653,6 @@ ClearContentSettingsExceptForNavigationRelatedSettings() {
         status.first == CONTENT_SETTINGS_TYPE_JAVASCRIPT)
       continue;
     status.second.blocked = false;
-    status.second.blockage_indicated_to_user = false;
     status.second.allowed = false;
   }
   microphone_camera_state_ = MICROPHONE_CAMERA_NOT_ACCESSED;
@@ -685,7 +671,6 @@ void TabSpecificContentSettings::ClearNavigationRelatedContentSettings() {
     ContentSettingsStatus& status =
         content_settings_status_[type];
     status.blocked = false;
-    status.blockage_indicated_to_user = false;
     status.allowed = false;
   }
   content::NotificationService::current()->Notify(
@@ -699,11 +684,10 @@ void TabSpecificContentSettings::FlashDownloadBlocked() {
                              base::UTF8ToUTF16(content::kFlashPluginName));
 }
 
-void TabSpecificContentSettings::SetPopupsBlocked(bool blocked) {
+void TabSpecificContentSettings::ClearPopupsBlocked() {
   ContentSettingsStatus& status =
       content_settings_status_[CONTENT_SETTINGS_TYPE_POPUPS];
-  status.blocked = blocked;
-  status.blockage_indicated_to_user = false;
+  status.blocked = false;
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED,
       content::Source<WebContents>(web_contents()),

chrome/browser/content_settings/tab_specific_content_settings.h

@@ -192,7 +192,7 @@ class TabSpecificContentSettings
   void FlashDownloadBlocked();
 
   // Changes the |content_blocked_| entry for popups.
-  void SetPopupsBlocked(bool blocked);
+  void ClearPopupsBlocked();
 
   // Called when audio has been blocked on the page.
   void OnAudioBlocked();
@@ -201,11 +201,6 @@ class TabSpecificContentSettings
   // page.
   bool IsContentBlocked(ContentSettingsType content_type) const;
 
-  // Returns true if content blockage was indicated to the user.
-  bool IsBlockageIndicated(ContentSettingsType content_type) const;
-
-  void SetBlockageHasBeenIndicated(ContentSettingsType content_type);
-
   // Returns whether a particular kind of content has been allowed. Currently
   // only tracks cookies.
   bool IsContentAllowed(ContentSettingsType content_type) const;
@@ -429,7 +424,6 @@ class TabSpecificContentSettings
 
   struct ContentSettingsStatus {
     bool blocked;
-    bool blockage_indicated_to_user;
     bool allowed;
   };
   // Stores which content setting types actually have blocked content.

chrome/browser/content_settings/tab_specific_content_settings_unittest.cc

@@ -80,7 +80,7 @@ TEST_F(TabSpecificContentSettingsTest, BlockedContent) {
 #if !defined(OS_ANDROID)
   content_settings->OnContentBlocked(CONTENT_SETTINGS_TYPE_IMAGES);
 #endif
-  content_settings->SetPopupsBlocked(true);
+  content_settings->OnContentBlocked(CONTENT_SETTINGS_TYPE_POPUPS);
   TabSpecificContentSettings::MicrophoneCameraState
       blocked_microphone_camera_state =
           TabSpecificContentSettings::MICROPHONE_ACCESSED |

chrome/browser/ui/BUILD.gn

@@ -824,6 +824,8 @@ jumbo_split_static_library("ui") {
       "content_settings/content_setting_bubble_model_delegate.h",
       "content_settings/content_setting_image_model.cc",
       "content_settings/content_setting_image_model.h",
+      "content_settings/content_setting_image_model_states.cc",
+      "content_settings/content_setting_image_model_states.h",
       "exclusive_access/exclusive_access_bubble.cc",
       "exclusive_access/exclusive_access_bubble.h",
       "exclusive_access/exclusive_access_bubble_hide_callback.h",

chrome/browser/ui/blocked_content/framebust_block_tab_helper.cc

@@ -70,7 +70,6 @@ void FramebustBlockTabHelper::DidFinishNavigation(
   }
   blocked_urls_.clear();
   callbacks_.clear();
-  animation_has_run_ = false;
 
   UpdateLocationBarUI(web_contents());
 }

chrome/browser/ui/blocked_content/framebust_block_tab_helper.h

@@ -58,10 +58,6 @@ class FramebustBlockTabHelper
   // Returns all of the currently blocked URLs.
   const std::vector<GURL>& blocked_urls() const { return blocked_urls_; }
 
-  // Remembers if the animation has run.
-  void set_animation_has_run() { animation_has_run_ = true; }
-  bool animation_has_run() const { return animation_has_run_; }
-
  private:
   friend class content::WebContentsUserData<FramebustBlockTabHelper>;
 
@@ -79,9 +75,6 @@ class FramebustBlockTabHelper
   // distribution of the URLs in blocked_urls().
   std::vector<ClickCallback> callbacks_;
 
-  // Remembers if the animation has run.
-  bool animation_has_run_ = false;
-
   base::ObserverList<Observer>::Unchecked observers_;
 
   DISALLOW_COPY_AND_ASSIGN(FramebustBlockTabHelper);

chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc

@@ -76,15 +76,14 @@ void PopupBlockerTabHelper::DidFinishNavigation(
   // Close blocked popups.
   if (!blocked_popups_.empty()) {
     blocked_popups_.clear();
-    PopupNotificationVisibilityChanged(false);
+    HidePopupNotification();
   }
 }
 
-void PopupBlockerTabHelper::PopupNotificationVisibilityChanged(
-    bool visible) {
+void PopupBlockerTabHelper::HidePopupNotification() {
   if (!web_contents()->IsBeingDestroyed()) {
-    TabSpecificContentSettings::FromWebContents(web_contents())->
-        SetPopupsBlocked(visible);
+    TabSpecificContentSettings::FromWebContents(web_contents())
+        ->ClearPopupsBlocked();
   }
 }
 
@@ -165,7 +164,7 @@ void PopupBlockerTabHelper::ShowBlockedPopup(
 
   blocked_popups_.erase(id);
   if (blocked_popups_.empty())
-    PopupNotificationVisibilityChanged(false);
+    HidePopupNotification();
 }
 
 size_t PopupBlockerTabHelper::GetBlockedPopupsCount() const {

chrome/browser/ui/blocked_content/popup_blocker_tab_helper.h

@@ -93,8 +93,8 @@ class PopupBlockerTabHelper
 
   explicit PopupBlockerTabHelper(content::WebContents* web_contents);
 
-  // Called when the blocked popup notification is shown or hidden.
-  void PopupNotificationVisibilityChanged(bool visible);
+  // Called when the blocked popup notification is hidden.
+  void HidePopupNotification();
 
   // Note, this container should be sorted based on the position in the popup
   // list, so it is keyed by an id which is continually increased.

chrome/browser/ui/content_settings/content_setting_image_model.h

@@ -67,9 +67,7 @@ class ContentSettingImageModel {
   static std::unique_ptr<ContentSettingImageModel> CreateForContentType(
       ImageType image_type);
 
-  // Notifies this model that its setting might have changed and it may need to
-  // update its visibility, icon and tooltip.
-  virtual void UpdateFromWebContents(content::WebContents* web_contents) = 0;
+  void Update(content::WebContents* contents);
 
   // Creates the model for the bubble that will be attached to this image.
   // The bubble model is owned by the caller.
@@ -79,20 +77,14 @@ class ContentSettingImageModel {
       Profile* profile);
 
   // Whether the animation should be run for the given |web_contents|.
-  virtual bool ShouldRunAnimation(content::WebContents* web_contents) = 0;
+  bool ShouldRunAnimation(content::WebContents* web_contents);
 
   // Remembers that the animation has already run for the given |web_contents|,
   // so that we do not restart it when the parent view is updated.
-  virtual void SetAnimationHasRun(content::WebContents* web_contents) = 0;
+  void SetAnimationHasRun(content::WebContents* web_contents);
 
   bool is_visible() const { return is_visible_; }
 
-#if defined(OS_MACOSX)
-  // Calls UpdateFromWebContents() and returns true if the icon has changed.
-  bool UpdateFromWebContentsAndCheckIfIconChanged(
-      content::WebContents* web_contents);
-#endif
-
   // Retrieve the icon that represents this content setting. Blocked content
   // settings icons will have a blocked badge.
   gfx::Image GetIcon(SkColor icon_color) const;
@@ -107,6 +99,11 @@ class ContentSettingImageModel {
  protected:
   explicit ContentSettingImageModel(ImageType type);
 
+  // Notifies this model that its setting might have changed and it may need to
+  // update its visibility, icon and tooltip. This method returns whether the
+  // model should be visible.
+  virtual bool UpdateAndGetVisibility(content::WebContents* web_contents) = 0;
+
   // Internal implementation by subclasses of bubble model creation.
   virtual ContentSettingBubbleModel* CreateBubbleModelImpl(
       ContentSettingBubbleModel::Delegate* delegate,
@@ -118,7 +115,6 @@ class ContentSettingImageModel {
     icon_badge_ = &badge;
   }
 
-  void set_visible(bool visible) { is_visible_ = visible; }
   void set_explanatory_string_id(int text_id) {
     explanatory_string_id_ = text_id;
   }
@@ -147,8 +143,6 @@ class ContentSettingSimpleImageModel : public ContentSettingImageModel {
       ContentSettingBubbleModel::Delegate* delegate,
       content::WebContents* web_contents,
       Profile* profile) override;
-  bool ShouldRunAnimation(content::WebContents* web_contents) override;
-  void SetAnimationHasRun(content::WebContents* web_contents) override;
 
   ContentSettingsType content_type() { return content_type_; }
 
@@ -162,16 +156,13 @@ class ContentSettingFramebustBlockImageModel : public ContentSettingImageModel {
  public:
   ContentSettingFramebustBlockImageModel();
 
-  void UpdateFromWebContents(content::WebContents* web_contents) override;
+  bool UpdateAndGetVisibility(content::WebContents* web_contents) override;
 
   ContentSettingBubbleModel* CreateBubbleModelImpl(
       ContentSettingBubbleModel::Delegate* delegate,
       content::WebContents* web_contents,
       Profile* profile) override;
 
-  bool ShouldRunAnimation(content::WebContents* web_contents) override;
-  void SetAnimationHasRun(content::WebContents* web_contents) override;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(ContentSettingFramebustBlockImageModel);
 };

chrome/browser/ui/content_settings/content_setting_image_model_states.cc

+// Copyright 2018 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/ui/content_settings/content_setting_image_model_states.h"
+
+#include "base/logging.h"
+
+ContentSettingImageModelStates::~ContentSettingImageModelStates() = default;
+
+// static
+ContentSettingImageModelStates* ContentSettingImageModelStates::Get(
+    content::WebContents* contents) {
+  DCHECK(contents);
+  if (auto* state = FromWebContents(contents))
+    return state;
+  CreateForWebContents(contents);
+  return FromWebContents(contents);
+}
+
+void ContentSettingImageModelStates::SetAnimationHasRun(
+    ImageType type,
+    bool animation_has_run) {
+  VerifyType(type);
+  animations_[static_cast<int>(type)] = animation_has_run;
+}
+
+bool ContentSettingImageModelStates::AnimationHasRun(ImageType type) const {
+  VerifyType(type);
+  return animations_[static_cast<int>(type)];
+}
+
+ContentSettingImageModelStates::ContentSettingImageModelStates(
+    content::WebContents* contents) {}
+
+void ContentSettingImageModelStates::VerifyType(ImageType type) const {
+  CHECK_GE(type, static_cast<ImageType>(0));
+  CHECK_LT(type, ImageType::NUM_IMAGE_TYPES);
+}

chrome/browser/ui/content_settings/content_setting_image_model_states.h

+// Copyright 2018 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_UI_CONTENT_SETTINGS_CONTENT_SETTING_IMAGE_MODEL_STATES_H_
+#define CHROME_BROWSER_UI_CONTENT_SETTINGS_CONTENT_SETTING_IMAGE_MODEL_STATES_H_
+
+#include "base/macros.h"
+#include "chrome/browser/ui/content_settings/content_setting_image_model.h"
+#include "content/public/browser/web_contents_user_data.h"
+
+using ImageType = ContentSettingImageModel::ImageType;
+
+// Class that keeps track of the tab-specific state associated with each
+// ContentSettingImageModel. Each tab will have one instance of this class,
+// which keeps track of states for each ImageType.
+class ContentSettingImageModelStates
+    : public content::WebContentsUserData<ContentSettingImageModelStates> {
+ public:
+  ~ContentSettingImageModelStates() override;
+
+  static ContentSettingImageModelStates* Get(content::WebContents* contents);
+
+  void SetAnimationHasRun(ImageType type, bool animation_has_run);
+  bool AnimationHasRun(ImageType type) const;
+
+ private:
+  friend class content::WebContentsUserData<ContentSettingImageModelStates>;
+  explicit ContentSettingImageModelStates(content::WebContents* contents);
+
+  // ImageTypes are used for direct access into a raw array, use this method to
+  // CHECK that everything is in-bounds.
+  void VerifyType(ImageType type) const;
+
+  // Array of bool for whether an animation has run for a given image model.
+  // This bit is reset to false when the image is hidden.
+  bool animations_[static_cast<int>(ImageType::NUM_IMAGE_TYPES)] = {};
+
+  DISALLOW_COPY_AND_ASSIGN(ContentSettingImageModelStates);
+};
+
+#endif  // CHROME_BROWSER_UI_CONTENT_SETTINGS_CONTENT_SETTING_IMAGE_MODEL_STATES_H_

chrome/browser/ui/content_settings/content_setting_image_model_unittest.cc

@@ -47,8 +47,7 @@ class NotificationForwarder : public content::NotificationObserver {
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override {
     if (type == chrome::NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED) {
-      model_->UpdateFromWebContents(
-          content::Source<content::WebContents>(source).ptr());
+      model_->Update(content::Source<content::WebContents>(source).ptr());
     }
   }
 
@@ -62,7 +61,7 @@ class NotificationForwarder : public content::NotificationObserver {
 class ContentSettingImageModelTest : public ChromeRenderViewHostTestHarness {
 };
 
-TEST_F(ContentSettingImageModelTest, UpdateFromWebContents) {
+TEST_F(ContentSettingImageModelTest, Update) {
   TabSpecificContentSettings::CreateForWebContents(web_contents());
   TabSpecificContentSettings* content_settings =
       TabSpecificContentSettings::FromWebContents(web_contents());
@@ -73,19 +72,19 @@ TEST_F(ContentSettingImageModelTest, UpdateFromWebContents) {
   EXPECT_TRUE(content_setting_image_model->get_tooltip().empty());
 
   content_settings->OnContentBlocked(CONTENT_SETTINGS_TYPE_IMAGES);
-  content_setting_image_model->UpdateFromWebContents(web_contents());
+  content_setting_image_model->Update(web_contents());
 
   EXPECT_TRUE(content_setting_image_model->is_visible());
   EXPECT_TRUE(HasIcon(*content_setting_image_model));
   EXPECT_FALSE(content_setting_image_model->get_tooltip().empty());
 }
 
-TEST_F(ContentSettingImageModelTest, RPHUpdateFromWebContents) {
+TEST_F(ContentSettingImageModelTest, RPHUpdate) {
   TabSpecificContentSettings::CreateForWebContents(web_contents());
   auto content_setting_image_model =
       ContentSettingImageModel::CreateForContentType(
           ContentSettingImageModel::ImageType::PROTOCOL_HANDLERS);
-  content_setting_image_model->UpdateFromWebContents(web_contents());
+  content_setting_image_model->Update(web_contents());
   EXPECT_FALSE(content_setting_image_model->is_visible());
 
   TabSpecificContentSettings* content_settings =
@@ -93,7 +92,7 @@ TEST_F(ContentSettingImageModelTest, RPHUpdateFromWebContents) {
   content_settings->set_pending_protocol_handler(
       ProtocolHandler::CreateProtocolHandler(
           "mailto", GURL("http://www.google.com/")));
-  content_setting_image_model->UpdateFromWebContents(web_contents());
+  content_setting_image_model->Update(web_contents());
   EXPECT_TRUE(content_setting_image_model->is_visible());
 }
 
@@ -116,7 +115,7 @@ TEST_F(ContentSettingImageModelTest, CookieAccessed) {
       net::CanonicalCookie::Create(origin, "A=B", base::Time::Now(), options));
   ASSERT_TRUE(cookie);
   content_settings->OnCookieChange(origin, origin, *cookie, false);
-  content_setting_image_model->UpdateFromWebContents(web_contents());
+  content_setting_image_model->Update(web_contents());
   EXPECT_TRUE(content_setting_image_model->is_visible());
   EXPECT_TRUE(HasIcon(*content_setting_image_model));
   EXPECT_FALSE(content_setting_image_model->get_tooltip().empty());
@@ -144,7 +143,7 @@ TEST_F(ContentSettingImageModelTest, SubresourceFilter) {
   EXPECT_TRUE(content_setting_image_model->get_tooltip().empty());
 
   content_settings->OnContentBlocked(CONTENT_SETTINGS_TYPE_ADS);
-  content_setting_image_model->UpdateFromWebContents(web_contents());
+  content_setting_image_model->Update(web_contents());
 
   EXPECT_TRUE(content_setting_image_model->is_visible());
   EXPECT_TRUE(HasIcon(*content_setting_image_model));

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

@@ -46,13 +46,13 @@ void ContentSettingImageView::Update() {
       delegate_->GetContentSettingWebContents();
   // Note: We explicitly want to call this even if |web_contents| is NULL, so we
   // get hidden properly while the user is editing the omnibox.
-  content_setting_image_model_->UpdateFromWebContents(web_contents);
+  content_setting_image_model_->Update(web_contents);
 
   if (!content_setting_image_model_->is_visible()) {
     SetVisible(false);
     return;
   }
-
+  DCHECK(web_contents);
   UpdateImage();
   SetVisible(true);