Update favicon URLs on FinishLoad for main frame

Previously it was done on didStopLoading for _any_ frame.
That causes a stream of spurious favicon URL updates for
every <iframe>.
This is now moved to on didFinishLoad and get triggered only
for the main frame.  This ensures that icon loads always get
initiated after all of the other page resources have been
fetched.  Note that DidFinishDocumentLoad runs potentially
before all subresources have been fetched, which means that
the icon requests may compete with subresources for network
bandwidth.  didFinishLoad is the closest place to where
IconController does it, and we already have icons at this point.

BUG=131567

Review URL: https://chromiumcodereview.appspot.com/10831163

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@150232 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 30 insertions, 20 deletions

diff --git a/chrome/renderer/chrome_render_view_observer.cc b/chrome/renderer/chrome_render_view_observer.cc
index 1aca18d..9b7e3f5 100644
--- a/chrome/renderer/chrome_render_view_observer.cc
+++ b/chrome/renderer/chrome_render_view_observer.cc
@@ -698,23 +698,21 @@ void ChromeRenderViewObserver::DidStopLoading() {
         routing_id(), render_view()->GetPageId(), osd_url,
         search_provider::AUTODETECTED_PROVIDER));
   }
+}
+
+void ChromeRenderViewObserver::DidFinishLoad(WebKit::WebFrame* frame) {
+  if (frame->parent())
+    return;
 
+  // Please note that we are updating favicons only for the _main_ frame.
+  // Updating Favicon URLs at DidFinishLoad ensures that icon loads always get
+  // initiated after all of the other page resources have been fetched, so icon
+  // loads should not compete with page resources for network bandwidth.
   int icon_types = WebIconURL::TypeFavicon;
   if (chrome::kEnableTouchIcon)
     icon_types |= WebIconURL::TypeTouchPrecomposed | WebIconURL::TypeTouch;
 
-  WebVector<WebIconURL> icon_urls =
-      render_view()->GetWebView()->mainFrame()->iconURLs(icon_types);
-  std::vector<FaviconURL> urls;
-  for (size_t i = 0; i < icon_urls.size(); i++) {
-    WebURL url = icon_urls[i].iconURL();
-    if (!url.isEmpty())
-      urls.push_back(FaviconURL(url, ToFaviconType(icon_urls[i].iconType())));
-  }
-  if (!urls.empty()) {
-    Send(new IconHostMsg_UpdateFaviconURL(
-        routing_id(), render_view()->GetPageId(), urls));
-  }
+  CollectAndUpdateFaviconURLs(frame, icon_types);
 }
 
 void ChromeRenderViewObserver::DidChangeIcon(WebFrame* frame,
@@ -726,14 +724,7 @@ void ChromeRenderViewObserver::DidChangeIcon(WebFrame* frame,
       icon_type != WebIconURL::TypeFavicon)
     return;
 
-  WebVector<WebIconURL> icon_urls = frame->iconURLs(icon_type);
-  std::vector<FaviconURL> urls;
-  for (size_t i = 0; i < icon_urls.size(); i++) {
-    urls.push_back(FaviconURL(icon_urls[i].iconURL(),
-                              ToFaviconType(icon_urls[i].iconType())));
-  }
-  Send(new IconHostMsg_UpdateFaviconURL(
-      routing_id(), render_view()->GetPageId(), urls));
+  CollectAndUpdateFaviconURLs(frame, icon_type);
 }
 
 void ChromeRenderViewObserver::DidCommitProvisionalLoad(
@@ -1098,6 +1089,21 @@ SkBitmap ChromeRenderViewObserver::ImageFromDataUrl(const GURL& url) const {
   return SkBitmap();
 }
 
+void ChromeRenderViewObserver::CollectAndUpdateFaviconURLs(
+    WebKit::WebFrame* frame, int icon_types) {
+  WebVector<WebIconURL> icon_urls = frame->iconURLs(icon_types);
+  std::vector<FaviconURL> urls;
+  for (size_t i = 0; i < icon_urls.size(); i++) {
+    WebURL url = icon_urls[i].iconURL();
+    if (!url.isEmpty())
+      urls.push_back(FaviconURL(url, ToFaviconType(icon_urls[i].iconType())));
+  }
+  if (!urls.empty()) {
+    Send(new IconHostMsg_UpdateFaviconURL(
+        routing_id(), render_view()->GetPageId(), urls));
+  }
+}
+
 bool ChromeRenderViewObserver::IsStrictSecurityHost(const std::string& host) {
   return (strict_security_hosts_.find(host) != strict_security_hosts_.end());
 }
diff --git a/chrome/renderer/chrome_render_view_observer.h b/chrome/renderer/chrome_render_view_observer.h
index 7d86ea6..d35c6ec 100644
--- a/chrome/renderer/chrome_render_view_observer.h
+++ b/chrome/renderer/chrome_render_view_observer.h
@@ -66,6 +66,7 @@ class ChromeRenderViewObserver : public content::RenderViewObserver,
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
   virtual void DidStartLoading() OVERRIDE;
   virtual void DidStopLoading() OVERRIDE;
+  virtual void DidFinishLoad(WebKit::WebFrame* frame) OVERRIDE;
   virtual void DidChangeIcon(WebKit::WebFrame* frame,
                              WebKit::WebIconURL::Type icon_type) OVERRIDE;
   virtual void DidCommitProvisionalLoad(WebKit::WebFrame* frame,
@@ -184,6 +185,9 @@ class ChromeRenderViewObserver : public content::RenderViewObserver,
   // Decodes a data: URL image or returns an empty image in case of failure.
   SkBitmap ImageFromDataUrl(const GURL&) const;
 
+  // Collects favicons of given types from the frame and sends UpdateFaviconURL.
+  void CollectAndUpdateFaviconURLs(WebKit::WebFrame* frame, int icon_types);
+
   // Determines if a host is in the strict security host set.
   bool IsStrictSecurityHost(const std::string& host);