Convert FaviconDownloader to use the new navigation callbacks.

chrome/browser/extensions/favicon_downloader.cc

Index: chrome/browser/extensions/favicon_downloader.cc
diff --git a/chrome/browser/extensions/favicon_downloader.cc b/chrome/browser/extensions/favicon_downloader.cc
index 39ac3dd6f2b898456181510138f2a95d67e0addc..334fb78df93c94dab5a88103326374f50b5677db 100644
--- a/chrome/browser/extensions/favicon_downloader.cc
+++ b/chrome/browser/extensions/favicon_downloader.cc
@@ -6,6 +6,7 @@
 
 #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"
@@ -100,7 +101,7 @@ void FaviconDownloader::DidDownloadFavicon(
     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;
 
@@ -112,9 +113,11 @@ void FaviconDownloader::DidDownloadFavicon(
 }
 
 // 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();