Remove NOTIFICATION_FAVICON_UPDATED

Change notification NOTIFICATION_FAVICON_UPDATED into an event sent to
FaviconDriverObserver and change client code to be observers instead.

Pass the FaviconDriver to OnFaviconUpdated() event so that listeners can
find the content::WebContents corresponding to the event (as some of the
client code did listen to multiple content::WebContents).

BUG=362072

Review URL: https://codereview.chromium.org/1059743003

Cr-Commit-Position: refs/heads/master@{#324201}

chrome/browser/android/tab_android.cc

@@ -395,6 +395,10 @@ void TabAndroid::OnFaviconAvailable(const gfx::Image& image) {
                               gfx::ConvertToJavaBitmap(&favicon).obj());
 }
 
+void TabAndroid::OnFaviconUpdated(favicon::FaviconDriver* favicon_driver,
+                                  bool icon_url_changed) {
+}
+
 void TabAndroid::Destroy(JNIEnv* env, jobject obj) {
   delete this;
 }

chrome/browser/android/tab_android.h

@@ -139,6 +139,8 @@ class TabAndroid : public CoreTabHelperDelegate,
 
   // favicon::FaviconDriverObserver -------------------------------------------
   void OnFaviconAvailable(const gfx::Image& image) override;
+  void OnFaviconUpdated(favicon::FaviconDriver* favicon_driver,
+                        bool icon_url_changed) override;
 
   // Methods called from Java via JNI -----------------------------------------
 

chrome/browser/chrome_notification_types.h

@@ -228,16 +228,6 @@ enum NotificationType {
   // the LoginHandler that should be cancelled.
   NOTIFICATION_AUTH_CANCELLED,
 
-  // Favicon ------------------------------------------------------------------
-
-  // Sent by FaviconTabHelper when a tab's favicon has been successfully
-  // updated. The details are a bool indicating whether the
-  // NavigationEntry's favicon URL has changed since the previous
-  // NOTIFICATION_FAVICON_UPDATED notification. The details are true if
-  // there was no previous NOTIFICATION_FAVICON_UPDATED notification for the
-  // current NavigationEntry.
-  NOTIFICATION_FAVICON_UPDATED,
-
   // Profiles -----------------------------------------------------------------
 
   // Sent after a Profile has been created. This notification is sent both for

chrome/browser/extensions/api/tabs/tabs_event_router.cc

@@ -11,6 +11,7 @@
 #include "chrome/browser/extensions/api/tabs/tabs_windows_api.h"
 #include "chrome/browser/extensions/api/tabs/windows_event_router.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
+#include "chrome/browser/favicon/favicon_tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_iterator.h"
@@ -94,7 +95,8 @@ base::DictionaryValue* TabsEventRouter::TabEntry::DidNavigate(
   return changed_properties;
 }
 
-TabsEventRouter::TabsEventRouter(Profile* profile) : profile_(profile) {
+TabsEventRouter::TabsEventRouter(Profile* profile)
+    : profile_(profile), favicon_scoped_observer_(this) {
   DCHECK(!profile->IsOffTheRecord());
 
   BrowserList::AddObserver(this);
@@ -148,8 +150,7 @@ void TabsEventRouter::RegisterForTabNotifications(WebContents* contents) {
   registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
                  content::Source<WebContents>(contents));
 
-  registrar_.Add(this, chrome::NOTIFICATION_FAVICON_UPDATED,
-                 content::Source<WebContents>(contents));
+  favicon_scoped_observer_.Add(FaviconTabHelper::FromWebContents(contents));
 
   ZoomController::FromWebContents(contents)->AddObserver(this);
 }
@@ -159,8 +160,7 @@ void TabsEventRouter::UnregisterForTabNotifications(WebContents* contents) {
       content::Source<NavigationController>(&contents->GetController()));
   registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
       content::Source<WebContents>(contents));
-  registrar_.Remove(this, chrome::NOTIFICATION_FAVICON_UPDATED,
-      content::Source<WebContents>(contents));
+  favicon_scoped_observer_.Remove(FaviconTabHelper::FromWebContents(contents));
 
   ZoomController::FromWebContents(contents)->RemoveObserver(this);
 }
@@ -501,12 +501,8 @@ void TabsEventRouter::Observe(int type,
         content::Source<NavigationController>(&contents->GetController()));
     registrar_.Remove(this, content::NOTIFICATION_WEB_CONTENTS_DESTROYED,
         content::Source<WebContents>(contents));
-    registrar_.Remove(this, chrome::NOTIFICATION_FAVICON_UPDATED,
-        content::Source<WebContents>(contents));
-  } else if (type == chrome::NOTIFICATION_FAVICON_UPDATED) {
-    bool icon_url_changed = *content::Details<bool>(details).ptr();
-    if (icon_url_changed)
-      FaviconUrlUpdated(content::Source<WebContents>(source).ptr());
+    favicon_scoped_observer_.Remove(
+        FaviconTabHelper::FromWebContents(contents));
   } else {
     NOTREACHED();
   }
@@ -585,4 +581,16 @@ void TabsEventRouter::OnZoomChanged(
                 EventRouter::USER_GESTURE_UNKNOWN);
 }
 
+void TabsEventRouter::OnFaviconAvailable(const gfx::Image& image) {
+}
+
+void TabsEventRouter::OnFaviconUpdated(favicon::FaviconDriver* favicon_driver,
+                                       bool icon_url_changed) {
+  if (icon_url_changed) {
+    FaviconTabHelper* favicon_tab_helper =
+        static_cast<FaviconTabHelper*>(favicon_driver);
+    FaviconUrlUpdated(favicon_tab_helper->web_contents());
+  }
+}
+
 }  // namespace extensions

chrome/browser/extensions/api/tabs/tabs_event_router.h

@@ -10,13 +10,17 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/scoped_observer.h"
 #include "chrome/browser/extensions/api/tabs/tabs_api.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
+#include "components/favicon/core/favicon_driver_observer.h"
 #include "components/ui/zoom/zoom_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "extensions/browser/event_router.h"
 
+class FaviconTabHelper;
+
 namespace content {
 class WebContents;
 }
@@ -30,6 +34,7 @@ namespace extensions {
 class TabsEventRouter : public TabStripModelObserver,
                         public chrome::BrowserListObserver,
                         public content::NotificationObserver,
+                        public favicon::FaviconDriverObserver,
                         public ui_zoom::ZoomObserver {
  public:
   explicit TabsEventRouter(Profile* profile);
@@ -76,6 +81,11 @@ class TabsEventRouter : public TabStripModelObserver,
   void OnZoomChanged(
       const ui_zoom::ZoomController::ZoomChangedEventData& data) override;
 
+  // favicon::FaviconDriverObserver.
+  void OnFaviconAvailable(const gfx::Image& image) override;
+  void OnFaviconUpdated(favicon::FaviconDriver* favicon_driver,
+                        bool icon_url_changed) override;
+
  private:
   // "Synthetic" event. Called from TabInsertedAt if new tab is detected.
   void TabCreatedAt(content::WebContents* contents, int index, bool active);
@@ -169,6 +179,8 @@ class TabsEventRouter : public TabStripModelObserver,
   // The main profile that owns this event router.
   Profile* profile_;
 
+  ScopedObserver<FaviconTabHelper, TabsEventRouter> favicon_scoped_observer_;
+
   DISALLOW_COPY_AND_ASSIGN(TabsEventRouter);
 };
 

chrome/browser/favicon/favicon_handler_unittest.cc

@@ -231,6 +231,8 @@ class TestFaviconDriver : public favicon::FaviconDriver {
     SetActiveFaviconImage(image);
   }
 
+  void NotifyFaviconUpdated(bool icon_url_changed) override {}
+
   size_t num_active_favicon() const { return num_active_favicon_; }
   size_t num_favicon_available() const { return num_favicon_available_; }
   void ResetNumActiveFavicon() { num_active_favicon_ = 0; }

chrome/browser/favicon/favicon_tab_helper.cc

@@ -268,20 +268,16 @@ void FaviconTabHelper::OnFaviconAvailable(const gfx::Image& image,
                                           const GURL& icon_url,
                                           bool is_active_favicon) {
   if (is_active_favicon) {
+    bool icon_url_changed = GetActiveFaviconURL() != icon_url;
     // No matter what happens, we need to mark the favicon as being set.
     SetActiveFaviconValidity(true);
-    bool icon_url_changed = GetActiveFaviconURL() != icon_url;
     SetActiveFaviconURL(icon_url);
 
     if (image.IsEmpty())
       return;
 
     SetActiveFaviconImage(image);
-    content::NotificationService::current()->Notify(
-        chrome::NOTIFICATION_FAVICON_UPDATED,
-        content::Source<WebContents>(web_contents()),
-        content::Details<bool>(&icon_url_changed));
-    web_contents()->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB);
+    NotifyFaviconUpdated(icon_url_changed);
   }
   if (!image.IsEmpty()) {
     FOR_EACH_OBSERVER(favicon::FaviconDriverObserver, observer_list_,
@@ -289,6 +285,12 @@ void FaviconTabHelper::OnFaviconAvailable(const gfx::Image& image,
   }
 }
 
+void FaviconTabHelper::NotifyFaviconUpdated(bool icon_url_changed) {
+  FOR_EACH_OBSERVER(favicon::FaviconDriverObserver, observer_list_,
+                    OnFaviconUpdated(this, icon_url_changed));
+  web_contents()->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB);
+}
+
 void FaviconTabHelper::DidDownloadFavicon(
     int id,
     int http_status_code,

chrome/browser/favicon/favicon_tab_helper.h

@@ -102,6 +102,7 @@ class FaviconTabHelper : public content::WebContentsObserver,
   void OnFaviconAvailable(const gfx::Image& image,
                           const GURL& url,
                           bool is_active_favicon) override;
+  void NotifyFaviconUpdated(bool icon_url_changed) override;
 
   // Favicon download callback.
   void DidDownloadFavicon(

chrome/browser/favicon/favicon_tab_helper_browsertest.cc

@@ -6,6 +6,7 @@
 
 #include "base/memory/weak_ptr.h"
 #include "base/run_loop.h"
+#include "base/scoped_observer.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/ui/browser.h"
@@ -13,6 +14,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "components/favicon/core/favicon_driver_observer.h"
 #include "components/favicon/core/favicon_handler.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -99,16 +101,19 @@ class FaviconTabHelperPendingTaskChecker {
 // - The pending navigation.
 // - FaviconHandler's pending favicon database requests.
 // - FaviconHandler's pending downloads.
-class PendingTaskWaiter : public content::NotificationObserver {
+class PendingTaskWaiter : public content::NotificationObserver,
+                          public favicon::FaviconDriverObserver {
  public:
   PendingTaskWaiter(content::WebContents* web_contents,
                     FaviconTabHelperPendingTaskChecker* checker)
-      : checker_(checker), load_stopped_(false), weak_factory_(this) {
-    registrar_.Add(this, chrome::NOTIFICATION_FAVICON_UPDATED,
-                   content::Source<content::WebContents>(web_contents));
+      : checker_(checker),
+        load_stopped_(false),
+        scoped_observer_(this),
+        weak_factory_(this) {
     registrar_.Add(this, content::NOTIFICATION_LOAD_STOP,
                    content::Source<content::NavigationController>(
                        &web_contents->GetController()));
+    scoped_observer_.Add(FaviconTabHelper::FromWebContents(web_contents));
   }
   ~PendingTaskWaiter() override {}
 
@@ -129,10 +134,21 @@ class PendingTaskWaiter : public content::NotificationObserver {
     if (type == content::NOTIFICATION_LOAD_STOP)
       load_stopped_ = true;
 
+    OnNotification();
+  }
+
+  // favicon::Favicon
+  void OnFaviconAvailable(const gfx::Image& image) override {}
+  void OnFaviconUpdated(favicon::FaviconDriver* favicon_driver,
+                        bool icon_url_changed) override {
+    OnNotification();
+  }
+
+  void OnNotification() {
     if (!quit_closure_.is_null()) {
       // We stop waiting based on changes in state to FaviconHandler which occur
-      // immediately after NOTIFICATION_FAVICON_UPDATED is sent. Post a task to
-      // check if we can stop waiting.
+      // immediately after OnFaviconUpdated() is called. Post a task to check if
+      // we can stop waiting.
       base::MessageLoopForUI::current()->PostTask(
           FROM_HERE, base::Bind(&PendingTaskWaiter::EndLoopIfCanStopWaiting,
                                 weak_factory_.GetWeakPtr()));
@@ -151,6 +167,7 @@ class PendingTaskWaiter : public content::NotificationObserver {
   bool load_stopped_;
   base::Closure quit_closure_;
   content::NotificationRegistrar registrar_;
+  ScopedObserver<FaviconTabHelper, PendingTaskWaiter> scoped_observer_;
   base::WeakPtrFactory<PendingTaskWaiter> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PendingTaskWaiter);

chrome/browser/prerender/prerender_browsertest.cc

@@ -8,12 +8,14 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
+#include "base/macros.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
 #include "base/path_service.h"
 #include "base/prefs/pref_service.h"
 #include "base/run_loop.h"
+#include "base/scoped_observer.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -66,6 +68,7 @@
 #include "chrome/test/base/test_switches.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/favicon/core/favicon_driver_observer.h"
 #include "components/variations/entropy_provider.h"
 #include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_message_filter.h"
@@ -131,6 +134,42 @@ namespace prerender {
 
 namespace {
 
+class FaviconUpdateWatcher : public favicon::FaviconDriverObserver {
+ public:
+  explicit FaviconUpdateWatcher(content::WebContents* web_contents)
+      : seen_(false), running_(false), scoped_observer_(this) {
+    scoped_observer_.Add(FaviconTabHelper::FromWebContents(web_contents));
+  }
+
+  void Wait() {
+    if (seen_)
+      return;
+
+    running_ = true;
+    message_loop_runner_ = new content::MessageLoopRunner;
+    message_loop_runner_->Run();
+  }
+
+ private:
+  void OnFaviconAvailable(const gfx::Image& image) override {}
+  void OnFaviconUpdated(favicon::FaviconDriver* favicon_driver,
+                        bool icon_url_changed) override {
+    seen_ = true;
+    if (!running_)
+      return;
+
+    message_loop_runner_->Quit();
+    running_ = false;
+  }
+
+  bool seen_;
+  bool running_;
+  ScopedObserver<FaviconTabHelper, FaviconUpdateWatcher> scoped_observer_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(FaviconUpdateWatcher);
+};
+
 class MockNetworkChangeNotifierWIFI : public NetworkChangeNotifier {
  public:
   ConnectionType GetCurrentConnectionType() const override {
@@ -3146,9 +3185,7 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderFavicon) {
   if (!FaviconTabHelper::FromWebContents(
           GetActiveWebContents())->FaviconIsValid()) {
     // If the favicon has not been set yet, wait for it to be.
-    content::WindowedNotificationObserver favicon_update_watcher(
-        chrome::NOTIFICATION_FAVICON_UPDATED,
-        content::Source<WebContents>(GetActiveWebContents()));
+    FaviconUpdateWatcher favicon_update_watcher(GetActiveWebContents());
     favicon_update_watcher.Wait();
   }
   EXPECT_TRUE(FaviconTabHelper::FromWebContents(

components/favicon/core/favicon_driver.h

@@ -74,6 +74,11 @@ class FaviconDriver {
                                   const GURL& icon_url,
                                   bool is_active_favicon) = 0;
 
+  // Sends notification that the current page favicon has changed.
+  // |icon_url_changed| is true if the URL of the favicon changed in addition to
+  // the favicon image.
+  virtual void NotifyFaviconUpdated(bool icon_url_changed) = 0;
+
  protected:
   FaviconDriver() {}
   virtual ~FaviconDriver() {}

components/favicon/core/favicon_driver_observer.h

@@ -13,6 +13,8 @@ class Image;
 
 namespace favicon {
 
+class FaviconDriver;
+
 // An observer implemented by classes which are interested in event from
 // FaviconDriver.
 class FaviconDriverObserver {
@@ -24,6 +26,11 @@ class FaviconDriverObserver {
   // storage.
   virtual void OnFaviconAvailable(const gfx::Image& image) = 0;
 
+  // Called when favicon has changed for the current page. |icon_url_changed| is
+  // true if the favicon URL has also changed.
+  virtual void OnFaviconUpdated(FaviconDriver* favicon_driver,
+                                bool icon_url_changed) = 0;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(FaviconDriverObserver);
 };