Convert FaviconDownloader to use the new navigation callbacks.

chrome/browser/extensions/favicon_downloader.cc

 // Copyright 2013 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.
 
 #include "chrome/browser/extensions/favicon_downloader.h"
 
 #include "base/bind.h"
 #include "components/favicon/content/content_favicon_driver.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/favicon_url.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/geometry/size.h"
 
 FaviconDownloader::FaviconDownloader(
   content::WebContents* web_contents,
   const std::vector<GURL>& extra_favicon_urls,
   FaviconDownloaderCallback callback)
   : content::WebContentsObserver(web_contents),
@@ skipping 74 matching lines @@
   if (in_progress_requests_.empty() && !need_favicon_urls_)
     callback_.Run(true, favicon_map_);
 }
 
 void FaviconDownloader::DidDownloadFavicon(
     int id,
     int http_status_code,
     const GURL& image_url,
     const std::vector<SkBitmap>& bitmaps,
     const std::vector<gfx::Size>& original_bitmap_sizes) {
-  // Request may have been canceled by DidNavigateMainFrame().
+  // Request may have been canceled by DidFinishNavigation().
   if (in_progress_requests_.erase(id) == 0)
     return;
 
   favicon_map_[image_url] = bitmaps;
 
   // Once all requests have been resolved, perform post-download tasks.
   if (in_progress_requests_.empty() && !need_favicon_urls_)
     callback_.Run(true, favicon_map_);
 }
 
 // content::WebContentsObserver overrides:
-void FaviconDownloader::DidNavigateMainFrame(
-    const content::LoadCommittedDetails& details,
-    const content::FrameNavigateParams& params) {
+void FaviconDownloader::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame() || !navigation_handle->HasCommitted())

[Devlin, 2017/01/27 21:51:51]

I realize that this is the conversion that would leave the same behavior, but I
wonder if it's the most correct.  It seems like what we're trying to do here is
purge pending requests when the web contents navigates, because we know that we
don't need to finish them anymore.  I wonder if it would make sense for this to
be DidStartNavigation() instead of waiting for the navigation to finish?  WDYT?

[jam, 2017/01/27 23:04:25]

DidStartNavigation might not commit though, i.e. because the user starts
navigating to a url that turns out to be a download. 

[Devlin, 2017/01/28 02:24:23]

Interesting, okay.  And there's no way to know that during DidStartNavigation?

[jam, 2017/01/28 04:35:00]

Right, at the start is before the network request is made, so we don't know the
response data.

+    return;
+
   // Clear all pending requests.
   in_progress_requests_.clear();
   favicon_map_.clear();
   callback_.Run(false, favicon_map_);
 }
 
 void FaviconDownloader::DidUpdateFaviconURL(
     const std::vector<content::FaviconURL>& candidates) {
   // Only consider the first candidates we are given. This prevents pages that
   // change their favicon from spamming us.
   if (!need_favicon_urls_)
     return;
 
   need_favicon_urls_ = false;
   FetchIcons(candidates);
 }