Avoid cyclic dependency FaviconHandler<-->FaviconDriverImpl

components/favicon/core/favicon_handler.h

 // Copyright (c) 2012 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 COMPONENTS_FAVICON_CORE_FAVICON_HANDLER_H_
 #define COMPONENTS_FAVICON_CORE_FAVICON_HANDLER_H_
 
 #include <stddef.h>
 
 #include <map>
 #include <vector>
 
 #include "base/callback_forward.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.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"
 #include "ui/gfx/image/image.h"
 #include "url/gurl.h"
 
 class SkBitmap;
 
 namespace favicon {
 
-class FaviconDriver;
 class FaviconService;
 class TestFaviconHandler;
 
 // FaviconHandler works with FaviconDriver to fetch the specific type of
 // favicon.
 //
 // FetchFavicon requests the favicon from the favicon service which in turn
 // requests the favicon from the history database. At this point
 // we only know the URL of the page, and not necessarily the url of the
 // favicon. To ensure we handle reloading stale favicons as well as
@@ skipping 31 matching lines @@
 //
 // When the renderer downloads favicons, it considers the entire list of
 // favicon candidates, if |download_largest_favicon_| is true, the largest
 // favicon will be used, otherwise the one that best matches the preferred size
 // is chosen (or the first one if there is no preferred  size). Once the
 // matching favicon has been determined, SetFavicon is called which updates
 // the page's favicon and notifies the database to save the favicon.
 
 class FaviconHandler {
  public:
+  class Delegate {
+   public:
+    // Mimics WebContents::ImageDownloadCallback.
+    typedef base::Callback<void(
+        int id,
+        int status_code,
+        const GURL& image_url,
+        const std::vector<SkBitmap>& bitmaps,
+        const std::vector<gfx::Size>& original_bitmap_sizes)>
+        ImageDownloadCallback;
+
+    // Starts the download for the given favicon. When finished, the callback
+    // is called with the results. Returns the unique id of the download
+    // request, which will also be passed to the callback. In case of error, 0
+    // is returned and no callback will be called.
+    // Bitmaps with pixel sizes larger than |max_bitmap_size| are filtered out
+    // from the bitmap results. If there are no bitmap results <=
+    // |max_bitmap_size|, the smallest bitmap is resized to |max_bitmap_size|
+    // and is the only result. A |max_bitmap_size| of 0 means unlimited.
+    virtual int DownloadImage(const GURL& url,
+                              int max_image_size,
+                              ImageDownloadCallback callback) = 0;

[pkotwicz, 2017/02/22 02:37:54]

Might as well use WebContents::ImageDownloadCallback

[mastiz, 2017/02/23 21:55:54]

I can't add a dependency to WebContents in shared code, can I?

[pkotwicz, 2017/02/24 16:04:46]

You're right. I forgot about iOS

+
+    // Returns whether the user is operating in an off-the-record context.
+    virtual bool IsOffTheRecord() = 0;
+
+    // Returns whether |url| is bookmarked.
+    virtual bool IsBookmarked(const GURL& url) = 0;
+
+    // Notifies that the favicon image has been updated, which the delegate
+    // would typically propagate to FaviconDriverObserver::OnFaviconUpdated(),
+    // see its documentation for details.

[pkotwicz, 2017/02/22 02:37:54]

Nit: "Replace the ',' with '.'

"Notifies that the favicon image has been updated. Most delegates propagate the
notification to FaviconDriverObserver::OnFaviconUpdated(). See its documentation
for details."

[mastiz, 2017/02/23 21:55:54]

Done.

+    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;
+  };
+
+  // |delegate| must not be nullptr and must outlive this class.
   FaviconHandler(FaviconService* service,
-                 FaviconDriver* driver,
+                 Delegate* delegate,
                  FaviconDriverObserver::NotificationIconType handler_type);
   virtual ~FaviconHandler();
 
-  // Returns the bit mask of favicon_base::IconType based on the handler's type.
-  static int GetIconTypesFromHandlerType(
-      FaviconDriverObserver::NotificationIconType handler_type);
-
   // Initiates loading the favicon for the specified url.
   void FetchFavicon(const GURL& url);
 
   // Message Handler.  Must be public, because also called from
   // PrerenderContents. Collects the |image_urls| list.
   void OnUpdateFaviconURL(const GURL& page_url,
                           const std::vector<favicon::FaviconURL>& candidates);
 
-  // Called when the history request for favicon data mapped to |url_| has
-  // completed and the renderer has told us the icon URLs used by |url_|
-  void OnGotInitialHistoryDataAndIconURLCandidates();
-
-  // Message handler for ImageHostMsg_DidDownloadImage. Called when the image
-  // at |image_url| has been downloaded.
-  // |bitmaps| is a list of all the frames of the image at |image_url|.
-  // |original_bitmap_sizes| are the sizes of |bitmaps| before they were resized
-  // to the maximum bitmap size passed to DownloadFavicon().
-  void OnDidDownloadFavicon(
-      int id,
-      const GURL& image_url,
-      const std::vector<SkBitmap>& bitmaps,
-      const std::vector<gfx::Size>& original_bitmap_sizes);
-
   // For testing.
   const std::vector<favicon::FaviconURL>& image_urls() const {
     return image_urls_;
   }
 
   // Returns whether the handler is waiting for a download to complete or for
   // data from the FaviconService. Reserved for testing.
   bool HasPendingTasksForTest();
 
  protected:
   // These virtual methods make FaviconHandler testable and are overridden by
   // TestFaviconHandler.
 
-  // Asks the render to download favicon, returns the request id.
-  virtual int DownloadFavicon(const GURL& image_url, int max_bitmap_size);
-
   // Ask the favicon from history
   virtual void UpdateFaviconMappingAndFetch(
       const GURL& page_url,
       const GURL& icon_url,
       favicon_base::IconType icon_type,
       const favicon_base::FaviconResultsCallback& callback,
       base::CancelableTaskTracker* tracker);
 
   virtual void GetFaviconFromFaviconService(
       const GURL& icon_url,
@@ skipping 39 matching lines @@
                      const gfx::Image& image,
                      float score,
                      favicon_base::IconType icon_type);
 
     GURL image_url;
     gfx::Image image;
     float score;
     favicon_base::IconType icon_type;
   };
 
+  // Returns the bit mask of favicon_base::IconType based on the handler's type.
+  static int GetIconTypesFromHandlerType(
+      FaviconDriverObserver::NotificationIconType handler_type);

[pkotwicz, 2017/02/22 02:37:54]

Nit: Reorder the functions in the .cc file to match order in .h file

[mastiz, 2017/02/23 21:55:54]

I started to do this when I realized that many of the already existing functions
are not sorted. I would rather not do it here, I prefer keeping the diff
smaller, unless you insist.

[pkotwicz, 2017/02/24 16:04:46]

Sorting the functions in a follow up CL is fine by me

+
   // Get the maximal icon size in pixels for a icon of type |icon_type| for the
   // current platform.
   static int GetMaximalIconSize(favicon_base::IconType icon_type);
 
+  // Called when the history request for favicon data mapped to |url_| has
+  // completed and the renderer has told us the icon URLs used by |url_|
+  void OnGotInitialHistoryDataAndIconURLCandidates();
+
   // See description above class for details.
   void OnFaviconDataForInitialURLFromFaviconService(const std::vector<
       favicon_base::FaviconRawBitmapResult>& favicon_bitmap_results);
 
   // If the favicon currently mapped to |url_| has expired, downloads the
   // current candidate favicon from the renderer. Otherwise requests data for
   // the current favicon from history. If data is requested from history,
   // OnFaviconData() is called with the history data once it has been retrieved.
   void DownloadCurrentCandidateOrAskFaviconService();
 
   // See description above class for details.
   void OnFaviconData(const std::vector<favicon_base::FaviconRawBitmapResult>&
                          favicon_bitmap_results);
 
   // Schedules a download for the specified entry. This adds the request to
   // download_requests_.
   void ScheduleDownload(const GURL& image_url,
                         favicon_base::IconType icon_type);
 
+  // Triggered when a download of an image has finished.
+  void DidDownloadFavicon(int id,

[pkotwicz, 2017/02/22 02:37:54]

Nit: Keep the function name as OnDidDownloadFavicon()

[mastiz, 2017/02/23 21:55:54]

Done.

+                          int http_status_code,
+                          const GURL& image_url,
+                          const std::vector<SkBitmap>& bitmaps,
+                          const std::vector<gfx::Size>& original_bitmap_sizes);
+
   // Updates |favicon_candidate_| and returns true if it is an exact match.
   bool UpdateFaviconCandidate(const GURL& image_url,
                               const gfx::Image& image,
                               float score,
                               favicon_base::IconType icon_type);
 
   // Sets the image data for the favicon.
   void SetFavicon(const GURL& icon_url,
                   const gfx::Image& image,
                   favicon_base::IconType icon_type);
@@ skipping 61 matching lines @@
 
   // The icon URL and the icon type of the favicon in the most recent
   // FaviconDriver::OnFaviconAvailable() notification.
   GURL notification_icon_url_;
   favicon_base::IconType notification_icon_type_;
 
   // The FaviconService which implements favicon operations. May be null during
   // testing.
   FaviconService* service_;
 
-  // This handler's driver, owns this object.
-  FaviconDriver* driver_;
+  // This handler's delegate.
+  Delegate* delegate_;
 
   // The index of the favicon URL in |image_urls_| which is currently being
   // requested from history or downloaded.
   size_t current_candidate_index_;
 
   // Best image we've seen so far.  As images are downloaded from the page they
   // are stored here. When there is an exact match, or no more images are
   // available the favicon service and the current page are updated (assuming
   // the image is for a favicon).
   FaviconCandidate best_favicon_candidate_;
 
+  base::WeakPtrFactory<FaviconHandler> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(FaviconHandler);
 };
 
 }  // namespace favicon
 
 #endif  // COMPONENTS_FAVICON_CORE_FAVICON_HANDLER_H_