Refactor FaviconDriver::OnFaviconAvailable()

components/favicon/core/favicon_handler.h

Index: components/favicon/core/favicon_handler.h
diff --git a/components/favicon/core/favicon_handler.h b/components/favicon/core/favicon_handler.h
index a65d19b3d678c30180b78b82149c42e2e6a534ae..b423919b13012bf1b8be97836092b274daab477f 100644
--- a/components/favicon/core/favicon_handler.h
+++ b/components/favicon/core/favicon_handler.h
@@ -12,6 +12,7 @@
 #include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
 #include "base/task/cancelable_task_tracker.h"
+#include "components/favicon/core/favicon_driver_observer.h"
 #include "components/favicon/core/favicon_url.h"
 #include "components/favicon_base/favicon_callback.h"
 #include "ui/gfx/favicon_size.h"
@@ -78,29 +79,14 @@ class TestFaviconHandler;
 
 class FaviconHandler {
  public:
-  enum Type {
-    // Selects the icon URL of type favicon_base::FAVICON with bitmaps whose
-    // edge sizes most closely match "16 * favicon_base::GetFaviconScales()".
-    // When favicon_base::GetFaviconScales() returns multiple scales, the ideal
-    // match is a .ico file.
-    FAVICON,
-
-    // Selects the icon URL of type favicon_base::FAVICON with the largest
-    // bitmap.
-    LARGEST_FAVICON,
-
-    // Selects the icon URL of type favicon_base::TOUCH_ICON or
-    // favicon_base::TOUCH_PRECOMPOSED_ICON with the largest bitmap.
-    LARGEST_TOUCH
-  };
-
   FaviconHandler(FaviconService* service,
                  FaviconDriver* driver,
-                 Type handler_type);
+                 FaviconDriverObserver::NotificationIconType handler_type);

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

I find it weird that this takes a NotificationIconType. I think the type should
be called FaviconType and declared in its own header that both places consume.

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

I wanted to avoid confusion with favicon_base::IconType in
components/favicon_base/favicon_types.h
What do you suggest?

[pkotwicz, 2015/11/20 16:06:11]

I wanted to avoid confusion with favicon_base::IconType in
components/favicon_base/favicon_types.h
What do you suggest?

[sky, 2015/11/20 16:34:40]

Ok, fair enough.

   virtual ~FaviconHandler();
 
   // Returns the bit mask of favicon_base::IconType based on the handler's type.
-  static int GetIconTypesFromHandlerType(Type icon_type);
+  static int GetIconTypesFromHandlerType(
+      FaviconDriverObserver::NotificationIconType handler_type);
 
   // Initiates loading the favicon for the specified url.
   void FetchFavicon(const GURL& url);
@@ -234,14 +220,17 @@ class FaviconHandler {
                   const gfx::Image& image,
                   favicon_base::IconType icon_type);
 
-  // Notifies |driver_| favicon available. See
-  // FaviconDriver::NotifyFaviconAvailable() for |is_active_favicon| in detail.
-  void NotifyFaviconAvailable(
+  // Notifies |driver_| that FaviconHandler found an icon which matches the
+  // |handler_type_| criteria. NotifyFaviconUpdated() can be called multiple
+  // times for the same page if:
+  // - a better match is found for |handler_type_| (e.g. newer bitmap data)
+  // - Javascript changes the page's icon URLs.
+  void NotifyFaviconUpdated(
       const std::vector<favicon_base::FaviconRawBitmapResult>&
           favicon_bitmap_results);
-  void NotifyFaviconAvailable(const GURL& icon_url,
-                              favicon_base::IconType icon_type,
-                              const gfx::Image& image);
+  void NotifyFaviconUpdated(const GURL& icon_url,
+                            favicon_base::IconType icon_type,
+                            const gfx::Image& image);
 
   // Return the current candidate if any.
   favicon::FaviconURL* current_candidate() {
@@ -259,6 +248,8 @@ class FaviconHandler {
   // Used for FaviconService requests.
   base::CancelableTaskTracker cancelable_task_tracker_;
 
+  FaviconDriverObserver::NotificationIconType handler_type_;
+
   // URL of the page we're requesting the favicon for.
   GURL url_;