Refactor FaviconDriver::OnFaviconAvailable()

components/favicon/core/favicon_driver.h

Index: components/favicon/core/favicon_driver.h
diff --git a/components/favicon/core/favicon_driver.h b/components/favicon/core/favicon_driver.h
index 0c2747ebc646174db59a4cc7d95a0222f86a9943..3c15d327d460f12e66e9dcad2bbaf8cdff94997f 100644
--- a/components/favicon/core/favicon_driver.h
+++ b/components/favicon/core/favicon_driver.h
@@ -8,6 +8,7 @@
 #include "base/macros.h"
 #include "base/observer_list.h"
 #include "base/strings/string16.h"
+#include "components/favicon/core/favicon_driver_observer.h"
 
 class GURL;
 
@@ -17,8 +18,6 @@ class Image;
 
 namespace favicon {
 
-class FaviconDriverObserver;
-
 // Interface that allows favicon core code to obtain information about the
 // current page. This is partially implemented by FaviconDriverImpl, and
 // concrete implementation should be based on that class instead of directly
@@ -38,6 +37,9 @@ class FaviconDriver {
   // should rarely happen.
   virtual gfx::Image GetFavicon() const = 0;
 
+  // Returns the URL of the favicon for the page.
+  virtual GURL GetFaviconURL() const = 0;

[sky, 2015/11/20 00:44:26]

I'm worried about the timing of this. Why is it needed?

[pkotwicz, 2015/11/20 03:13:29]

I think that it is reasonable for a caller to be able to both get the current
favicon gfx::Image and the current favicon GURL.

This is used in tab_contents_tag_browsertest.cc

[sky, 2015/11/20 16:03:59]

I don't like adding code just for a test, especially tricky code like this.

[pkotwicz, 2015/11/21 23:04:08]

Done.

+
   // Returns true if we have the favicon for the page.
   virtual bool FaviconIsValid() const = 0;
 
@@ -61,27 +63,14 @@ class FaviconDriver {
   // otherwise.
   virtual GURL GetActiveURL() = 0;
 
-  // Sets whether the page's favicon is valid (if false, the default favicon is
-  // being used).
-  virtual void SetActiveFaviconValidity(bool valid) = 0;
-
-  // Returns the URL of the current page's favicon.
-  virtual GURL GetActiveFaviconURL() = 0;
-
-  // Sets the URL of the favicon's bitmap.
-  virtual void SetActiveFaviconURL(const GURL& url) = 0;
-
-  // Sets the bitmap of the current page's favicon.
-  virtual void SetActiveFaviconImage(const gfx::Image& image) = 0;
-
-  // Notifies the driver a favicon image is available. |image| is not
-  // necessarily 16x16. |icon_url| is the url the image is from. If
-  // |is_active_favicon| is true the image corresponds to the favicon
-  // (possibly empty) of the page.
-  virtual void OnFaviconAvailable(const GURL& page_url,
-                                  const GURL& icon_url,
-                                  const gfx::Image& image,
-                                  bool is_active_favicon) = 0;
+  // Notifies the driver that the favicon image has been updated.
+  // See comment for FaviconDriverObserver::OnFaviconUpdated() for more details.
+  virtual void OnFaviconUpdated(
+      const GURL& page_url,
+      FaviconDriverObserver::NotificationIconType notification_icon_type,
+      const GURL& icon_url,
+      bool icon_url_changed,
+      const gfx::Image& image) = 0;
 
   // Returns whether the driver is waiting for a download to complete or for
   // data from the FaviconService. Reserved for testing.
@@ -91,14 +80,13 @@ class FaviconDriver {
   FaviconDriver();
   virtual ~FaviconDriver();
 
-  // Informs FaviconDriverObservers that favicon |image| has been retrieved from
-  // either website or cached storage.
-  void NotifyFaviconAvailable(const gfx::Image& image);
+  // Notifies FaviconDriverObservers that the favicon image has been updated.
+  void NotifyFaviconUpdatedObservers(
+    FaviconDriverObserver::NotificationIconType notification_icon_type,
+    const GURL& icon_url,
+    bool icon_url_changed,
+    const gfx::Image& image);
 
-  // Informs FaviconDriverObservers that favicon has changed for the current
-  // page. |icon_url_changed| is true if the favicon URL has also changed since
-  // the last call.
-  virtual void NotifyFaviconUpdated(bool icon_url_changed);
 
  private:
   base::ObserverList<FaviconDriverObserver> observer_list_;