Refactor FaviconDriver::OnFaviconAvailable()

components/favicon/core/favicon_driver_observer.h

Index: components/favicon/core/favicon_driver_observer.h
diff --git a/components/favicon/core/favicon_driver_observer.h b/components/favicon/core/favicon_driver_observer.h
index eb3fd48f61ef712aa9fe1510eb5087019d504552..d34ef0d13bc9d5e79657dac9bda47b5742dd2fe0 100644
--- a/components/favicon/core/favicon_driver_observer.h
+++ b/components/favicon/core/favicon_driver_observer.h
@@ -6,6 +6,7 @@
 #define COMPONENTS_FAVICON_CORE_FAVICON_DRIVER_OBSERVER_H_
 
 #include "base/macros.h"
+#include "url/gurl.h"

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

You should be able to forward declare GURL.

 
 namespace gfx {
 class Image;
@@ -19,17 +20,47 @@ class FaviconDriver;
 // FaviconDriver.
 class FaviconDriverObserver {
  public:
+  // The type of the icon in the OnFaviconUpdated() notification.
+  enum NotificationIconType {
+    // Multi-resolution 16x16 (gfx::kFaviconSize) device independant pixel
+    // favicon of type favicon_base::FAVICON. If the page does not provide a
+    // 16x16 DIP icon, the icon is generated by resizing another icon.
+    NON_TOUCH_16_DIP,

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

I'm not a fan of NON_TOUCH_ here. I prefer DEFAULT, LARGEST and TOUCH_LARGEST.

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

I think that the names that I have are more descriptive of what the constants
are for.
In particular, I can foresee people thinking that LARGEST refers to the largest
icon available. LARGEST refers to the largest favicon excluding touch icons.

It would be nice if we could eliminate NON_TOUCH_LARGEST and TOUCH_LARGEST and
just have LARGEST. However, we can't do that because of super bad Android code

+    // Largest icon specified by the page which is of type
+    // favicon_base::FAVICON.
+    NON_TOUCH_LARGEST,
+    // Largest icon specified by the page which is of type
+    // favicon_base::TOUCH_ICON or of type favicon_base::TOUCH_PRECOMPOSED_ICON.
+    TOUCH_LARGEST
+  };
+
   FaviconDriverObserver() {}
   virtual ~FaviconDriverObserver() {}
 
-  // Called when favicon |image| is retrieved from either web site or cached
-  // 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.
+  // Called when either:
+  // 1) Chrome determines the best icon for the page for
+  //    |notification_icon_type|.
+  //    Not called if the site does not provide a custom icon and the best icon
+  //    for the page is the default one provided by Chrome.
+  // 2) The site changes its icon via Javascript.
+  // |icon_url_changed| is false if OnFaviconAvailable() was already called for
+  // |notification_icon_type| for the current page URL and |icon_url| is the
+  // same as for the previous notification for |notification_icon_type|.
+  // Example:
+  // Page: www.google.com
+  // Icon: www.google.com/favicon.ico
+  // Data for www.google.com/favicon.ico in the database has expired.
+  // i) OnFaviconUpdated() is called with |icon_url_changed| == true to notify
+  //    that a favicon was found in the history database.
+  // ii) As the history data has expired, the icon at www.google.com/favicon.ico
+  //     is redownloaded and stored into the database. OnFaviconUpdated() is
+  //     called with |icon_url_changed| == false to notify that the icon in the
+  //     history database MAY have changed visually.
   virtual void OnFaviconUpdated(FaviconDriver* favicon_driver,
-                                bool icon_url_changed) = 0;
+                                NotificationIconType notification_icon_type,
+                                const GURL& icon_url,
+                                bool icon_url_changed,
+                                const gfx::Image& image) = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(FaviconDriverObserver);