Introduce the UrlListManager

This CL introduces the skeleton of a class which can drive the UI
of "blocked URL" lists (i.e. the popup and redirect setting bubbles).

Currently, it only exposes an observer interface to unify how popup
and redirect UI surfaces receive updates about new URLs that were
blocked.

Follow-up CLs can add functionality including storing the actual
blocked URLs within the manager.

Bug: 895555
Change-Id: I6e95cd4ca3c8d6109cbfdb770516dc6d18c98554
Reviewed-on: https://chromium-review.googlesource.com/c/1334420
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610035}

chrome/browser/ui/BUILD.gn

@@ -91,6 +91,8 @@ jumbo_split_static_library("ui") {
     "blocked_content/scoped_visibility_tracker.h",
     "blocked_content/tab_under_navigation_throttle.cc",
     "blocked_content/tab_under_navigation_throttle.h",
+    "blocked_content/url_list_manager.cc",
+    "blocked_content/url_list_manager.h",
     "bookmarks/bookmark_bar.h",
     "bookmarks/bookmark_bubble_observer.h",
     "bookmarks/bookmark_editor.cc",

chrome/browser/ui/blocked_content/framebust_block_tab_helper.cc

@@ -28,9 +28,7 @@ void FramebustBlockTabHelper::AddBlockedUrl(const GURL& blocked_url,
   callbacks_.push_back(std::move(click_callback));
   DCHECK_EQ(blocked_urls_.size(), callbacks_.size());
 
-  for (Observer& observer : observers_) {
-    observer.OnBlockedUrlAdded(blocked_url);
-  }
+  manager_.NotifyObservers(0 /* id */, blocked_url);
   UpdateLocationBarUI(web_contents());
 }
 
@@ -49,14 +47,6 @@ void FramebustBlockTabHelper::OnBlockedUrlClicked(size_t index) {
       ui::PAGE_TRANSITION_LINK, false));
 }
 
-void FramebustBlockTabHelper::AddObserver(Observer* observer) {
-  observers_.AddObserver(observer);
-}
-
-void FramebustBlockTabHelper::RemoveObserver(const Observer* observer) {
-  observers_.RemoveObserver(observer);
-}
-
 FramebustBlockTabHelper::FramebustBlockTabHelper(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents) {}

chrome/browser/ui/blocked_content/framebust_block_tab_helper.h

@@ -9,7 +9,7 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
-#include "base/observer_list.h"
+#include "chrome/browser/ui/blocked_content/url_list_manager.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "url/gurl.h"
@@ -27,16 +27,6 @@ class FramebustBlockTabHelper
   using ClickCallback = base::OnceCallback<
       void(const GURL&, size_t /* index */, size_t /* total_size */)>;
 
-  // There is expected to be at most one observer at a time.
-  class Observer {
-   public:
-    // Called whenever a new URL was blocked.
-    virtual void OnBlockedUrlAdded(const GURL& blocked_url) = 0;
-
-   protected:
-    virtual ~Observer() = default;
-  };
-
   ~FramebustBlockTabHelper() override;
 
   // Shows the blocked Framebust icon in the Omnibox for the |blocked_url|.
@@ -52,12 +42,11 @@ class FramebustBlockTabHelper
   // vector of blocked URLs.
   void OnBlockedUrlClicked(size_t index);
 
-  void AddObserver(Observer* observer);
-  void RemoveObserver(const Observer* observer);
-
   // Returns all of the currently blocked URLs.
   const std::vector<GURL>& blocked_urls() const { return blocked_urls_; }
 
+  UrlListManager* manager() { return &manager_; }
+
  private:
   friend class content::WebContentsUserData<FramebustBlockTabHelper>;
 
@@ -67,6 +56,8 @@ class FramebustBlockTabHelper
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
 
+  UrlListManager manager_;
+
   // Remembers all the currently blocked URLs. This is cleared on each
   // navigation.
   std::vector<GURL> blocked_urls_;
@@ -75,8 +66,6 @@ class FramebustBlockTabHelper
   // distribution of the URLs in blocked_urls().
   std::vector<ClickCallback> callbacks_;
 
-  base::ObserverList<Observer>::Unchecked observers_;
-
   DISALLOW_COPY_AND_ASSIGN(FramebustBlockTabHelper);
 };
 

chrome/browser/ui/blocked_content/popup_blocker_tab_helper.cc

@@ -54,14 +54,6 @@ PopupBlockerTabHelper::PopupBlockerTabHelper(content::WebContents* web_contents)
 PopupBlockerTabHelper::~PopupBlockerTabHelper() {
 }
 
-void PopupBlockerTabHelper::AddObserver(Observer* observer) {
-  observers_.AddObserver(observer);
-}
-
-void PopupBlockerTabHelper::RemoveObserver(Observer* observer) {
-  observers_.RemoveObserver(observer);
-}
-
 void PopupBlockerTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   // Clear all page actions, blocked content notifications and browser actions
@@ -101,8 +93,7 @@ void PopupBlockerTabHelper::AddBlockedPopup(
       std::move(*params), window_features, block_type);
   TabSpecificContentSettings::FromWebContents(web_contents())->
       OnContentBlocked(CONTENT_SETTINGS_TYPE_POPUPS);
-  for (auto& observer : observers_)
-    observer.BlockedPopupAdded(id, blocked_popups_[id]->params.url);
+  manager_.NotifyObservers(id, blocked_popups_[id]->params.url);
 
 #if defined(OS_ANDROID)
   // Should replace existing popup infobars, with an updated count of how many

chrome/browser/ui/blocked_content/popup_blocker_tab_helper.h

@@ -12,9 +12,9 @@
 #include <memory>
 
 #include "base/macros.h"
-#include "base/observer_list.h"
 #include "chrome/browser/ui/blocked_content/blocked_window_params.h"
 #include "chrome/browser/ui/blocked_content/popup_blocker.h"
+#include "chrome/browser/ui/blocked_content/url_list_manager.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 #include "ui/base/window_open_disposition.h"
@@ -52,19 +52,8 @@ class PopupBlockerTabHelper
     kMaxValue = kClickedThroughAbusive
   };
 
-  class Observer {
-   public:
-    virtual void BlockedPopupAdded(int32_t id, const GURL& url) {}
-
-   protected:
-    virtual ~Observer() = default;
-  };
-
   ~PopupBlockerTabHelper() override;
 
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
-
   // Returns the number of blocked popups.
   size_t GetBlockedPopupsCount() const;
 
@@ -87,6 +76,8 @@ class PopupBlockerTabHelper
   // Logs a histogram measuring popup blocker actions.
   static void LogAction(Action action);
 
+  UrlListManager* manager() { return &manager_; }
+
  private:
   struct BlockedRequest;
   friend class content::WebContentsUserData<PopupBlockerTabHelper>;
@@ -96,12 +87,12 @@ class PopupBlockerTabHelper
   // Called when the blocked popup notification is hidden.
   void HidePopupNotification();
 
+  UrlListManager manager_;
+
   // 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.
   std::map<int32_t, std::unique_ptr<BlockedRequest>> blocked_popups_;
 
-  base::ObserverList<Observer>::Unchecked observers_;
-
   int32_t next_id_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(PopupBlockerTabHelper);

chrome/browser/ui/blocked_content/url_list_manager.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/blocked_content/url_list_manager.h"
+
+UrlListManager::UrlListManager() = default;
+
+UrlListManager::~UrlListManager() = default;
+
+void UrlListManager::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void UrlListManager::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void UrlListManager::NotifyObservers(int32_t id, const GURL& url) {
+  for (auto& observer : observers_) {
+    observer.BlockedUrlAdded(id, url);
+  }
+}

chrome/browser/ui/blocked_content/url_list_manager.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_BLOCKED_CONTENT_URL_LIST_MANAGER_H_
+#define CHROME_BROWSER_UI_BLOCKED_CONTENT_URL_LIST_MANAGER_H_
+
+#include <stdint.h>
+
+#include "base/macros.h"
+#include "base/observer_list.h"
+
+class GURL;
+
+// This class manages lists of blocked URLs in order to drive UI surfaces.
+// Currently it is used by the redirect / popup blocked UIs.
+//
+// TODO(csharrison): Currently this object is composed within the framebust /
+// popup tab helpers. Eventually those objects could be replaced almost entirely
+// by shared logic here.
+class UrlListManager {
+ public:
+  class Observer {
+   public:
+    virtual ~Observer() {}
+    virtual void BlockedUrlAdded(int32_t id, const GURL& url) = 0;
+  };
+
+  UrlListManager();
+  ~UrlListManager();
+
+  void AddObserver(Observer* observer);
+  void RemoveObserver(Observer* observer);
+
+  void NotifyObservers(int32_t id, const GURL& url);
+
+ private:
+  base::ObserverList<Observer>::Unchecked observers_;
+
+  DISALLOW_COPY_AND_ASSIGN(UrlListManager);
+};
+
+#endif  // CHROME_BROWSER_UI_BLOCKED_CONTENT_URL_LIST_MANAGER_H_

chrome/browser/ui/content_settings/content_setting_bubble_model.cc

@@ -955,7 +955,7 @@ void ContentSettingCookiesBubbleModel::OnCustomLinkClicked() {
 // ContentSettingPopupBubbleModel ----------------------------------------------
 
 class ContentSettingPopupBubbleModel : public ContentSettingSingleRadioGroup,
-                                       public PopupBlockerTabHelper::Observer {
+                                       public UrlListManager::Observer {
  public:
   ContentSettingPopupBubbleModel(Delegate* delegate,
                                  WebContents* web_contents,
@@ -966,7 +966,7 @@ class ContentSettingPopupBubbleModel : public ContentSettingSingleRadioGroup,
   void CommitChanges() override;
 
   // PopupBlockerTabHelper::Observer:
-  void BlockedPopupAdded(int32_t id, const GURL& url) override;
+  void BlockedUrlAdded(int32_t id, const GURL& url) override;
 
   // content::NotificationObserver:
   void Observe(int type,
@@ -980,8 +980,7 @@ class ContentSettingPopupBubbleModel : public ContentSettingSingleRadioGroup,
     return bubble_content().list_items[index].item_id;
   }
 
-  ScopedObserver<PopupBlockerTabHelper, PopupBlockerTabHelper::Observer>
-      popup_blocker_observer_;
+  ScopedObserver<UrlListManager, UrlListManager::Observer> url_list_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(ContentSettingPopupBubbleModel);
 };
@@ -994,7 +993,7 @@ ContentSettingPopupBubbleModel::ContentSettingPopupBubbleModel(
                                      web_contents,
                                      profile,
                                      CONTENT_SETTINGS_TYPE_POPUPS),
-      popup_blocker_observer_(this) {
+      url_list_observer_(this) {
   if (!web_contents)
     return;
 
@@ -1006,13 +1005,13 @@ ContentSettingPopupBubbleModel::ContentSettingPopupBubbleModel(
   for (const auto& blocked_popup : blocked_popups)
     AddListItem(CreateUrlListItem(blocked_popup.first, blocked_popup.second));
 
-  popup_blocker_observer_.Add(helper);
+  url_list_observer_.Add(helper->manager());
   content_settings::RecordPopupsAction(
       content_settings::POPUPS_ACTION_DISPLAYED_BUBBLE);
 }
 
-void ContentSettingPopupBubbleModel::BlockedPopupAdded(int32_t id,
-                                                       const GURL& url) {
+void ContentSettingPopupBubbleModel::BlockedUrlAdded(int32_t id,
+                                                     const GURL& url) {
   AddListItem(CreateUrlListItem(id, url));
 }
 
@@ -1022,7 +1021,7 @@ void ContentSettingPopupBubbleModel::Observe(
     const content::NotificationDetails& details) {
   ContentSettingSingleRadioGroup::Observe(type, source, details);
   if (type == content::NOTIFICATION_WEB_CONTENTS_DESTROYED)
-    popup_blocker_observer_.RemoveAll();
+    url_list_observer_.RemoveAll();
 }
 
 void ContentSettingPopupBubbleModel::OnListItemClicked(int index,
@@ -1047,12 +1046,7 @@ void ContentSettingPopupBubbleModel::CommitChanges() {
   ContentSettingSingleRadioGroup::CommitChanges();
 }
 
-ContentSettingPopupBubbleModel::~ContentSettingPopupBubbleModel() {
-  if (web_contents()) {
-    auto* helper = PopupBlockerTabHelper::FromWebContents(web_contents());
-    helper->RemoveObserver(this);
-  }
-}
+ContentSettingPopupBubbleModel::~ContentSettingPopupBubbleModel() = default;
 
 // ContentSettingMediaStreamBubbleModel ----------------------------------------
 
@@ -1565,7 +1559,8 @@ ContentSettingFramebustBlockBubbleModel::
     : ContentSettingSingleRadioGroup(delegate,
                                      web_contents,
                                      profile,
-                                     CONTENT_SETTINGS_TYPE_POPUPS) {
+                                     CONTENT_SETTINGS_TYPE_POPUPS),
+      url_list_observer_(this) {
   if (!web_contents)
     return;
 
@@ -1576,27 +1571,18 @@ ContentSettingFramebustBlockBubbleModel::
   for (const auto& blocked_url : helper->blocked_urls())
     AddListItem(CreateUrlListItem(0 /* id */, blocked_url));
 
-  helper->AddObserver(this);
+  url_list_observer_.Add(helper->manager());
 }
 
 ContentSettingFramebustBlockBubbleModel::
-    ~ContentSettingFramebustBlockBubbleModel() {
-  if (web_contents()) {
-    FramebustBlockTabHelper::FromWebContents(web_contents())
-        ->RemoveObserver(this);
-  }
-}
+    ~ContentSettingFramebustBlockBubbleModel() = default;
 
 void ContentSettingFramebustBlockBubbleModel::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
-  // The order is important because ContentSettingBubbleModel::Observer() clears
-  // the value of |web_contents()|.
-  if (type == content::NOTIFICATION_WEB_CONTENTS_DESTROYED) {
-    FramebustBlockTabHelper::FromWebContents(web_contents())
-        ->RemoveObserver(this);
-  }
+  if (type == content::NOTIFICATION_WEB_CONTENTS_DESTROYED)
+    url_list_observer_.RemoveAll();
   ContentSettingSingleRadioGroup::Observe(type, source, details);
 }
 
@@ -1615,7 +1601,8 @@ ContentSettingFramebustBlockBubbleModel::AsFramebustBlockBubbleModel() {
   return this;
 }
 
-void ContentSettingFramebustBlockBubbleModel::OnBlockedUrlAdded(
+void ContentSettingFramebustBlockBubbleModel::BlockedUrlAdded(
+    int32_t id,
     const GURL& blocked_url) {
   AddListItem(CreateUrlListItem(0 /* id */, blocked_url));
 }

chrome/browser/ui/content_settings/content_setting_bubble_model.h

@@ -14,10 +14,12 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/scoped_observer.h"
 #include "base/strings/string16.h"
 #include "build/build_config.h"
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
 #include "chrome/browser/ui/blocked_content/framebust_block_tab_helper.h"
+#include "chrome/browser/ui/blocked_content/url_list_manager.h"
 #include "chrome/common/custom_handlers/protocol_handler.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/content_settings/core/common/content_settings_types.h"
@@ -477,7 +479,7 @@ class ContentSettingSingleRadioGroup : public ContentSettingSimpleBubbleModel {
 // The model for the blocked Framebust bubble.
 class ContentSettingFramebustBlockBubbleModel
     : public ContentSettingSingleRadioGroup,
-      public FramebustBlockTabHelper::Observer {
+      public UrlListManager::Observer {
  public:
   ContentSettingFramebustBlockBubbleModel(Delegate* delegate,
                                           content::WebContents* web_contents,
@@ -495,12 +497,14 @@ class ContentSettingFramebustBlockBubbleModel
   ContentSettingFramebustBlockBubbleModel* AsFramebustBlockBubbleModel()
       override;
 
-  // FramebustBlockTabHelper::Observer:
-  void OnBlockedUrlAdded(const GURL& blocked_url) override;
+  // UrlListManager::Observer:
+  void BlockedUrlAdded(int32_t id, const GURL& blocked_url) override;
 
  private:
   ListItem CreateListItem(const GURL& url);
 
+  ScopedObserver<UrlListManager, UrlListManager::Observer> url_list_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(ContentSettingFramebustBlockBubbleModel);
 };
 #endif  // !defined(OS_ANDROID)

chrome/browser/ui/content_settings/framebust_block_browsertest.cc

@@ -12,6 +12,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/ui/blocked_content/framebust_block_tab_helper.h"
+#include "chrome/browser/ui/blocked_content/url_list_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_content_setting_bubble_model_delegate.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -42,7 +43,7 @@ const int kDisallowRadioButtonIndex = 1;
 }  // namespace
 
 class FramebustBlockBrowserTest : public InProcessBrowserTest,
-                                  public FramebustBlockTabHelper::Observer {
+                                  public UrlListManager::Observer {
  public:
   FramebustBlockBrowserTest() {
     scoped_feature_list_.InitAndEnableFeature(
@@ -55,11 +56,12 @@ class FramebustBlockBrowserTest : public InProcessBrowserTest,
     ASSERT_TRUE(embedded_test_server()->Start());
     current_browser_ = InProcessBrowserTest::browser();
     FramebustBlockTabHelper::FromWebContents(GetWebContents())
+        ->manager()
         ->AddObserver(this);
   }
 
-  // FramebustBlockTabHelper::Observer:
-  void OnBlockedUrlAdded(const GURL& blocked_url) override {
+  // UrlListManager::Observer:
+  void BlockedUrlAdded(int32_t id, const GURL& blocked_url) override {
     if (!blocked_url_added_closure_.is_null())
       std::move(blocked_url_added_closure_).Run();
   }