Do not attempt to download favicons with 404 status in WebView

android_webview/browser/icon_helper.cc

Index: android_webview/browser/icon_helper.cc
diff --git a/android_webview/browser/icon_helper.cc b/android_webview/browser/icon_helper.cc
index e227532626ee1ecb3fbf9f69ce35ebff3a3fb898..df195db1ee37c2431f578459a8100bd383d81939 100644
--- a/android_webview/browser/icon_helper.cc
+++ b/android_webview/browser/icon_helper.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/hash.h"
 #include "base/logging.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
@@ -20,7 +21,8 @@ namespace android_webview {
 
 IconHelper::IconHelper(WebContents* web_contents)
     : WebContentsObserver(web_contents),
-      listener_(NULL) {
+      listener_(NULL),
+      missing_favicon_urls_() {
 }
 
 IconHelper::~IconHelper() {
@@ -37,6 +39,11 @@ void IconHelper::DownloadFaviconCallback(
     const std::vector<SkBitmap>& bitmaps,
     const std::vector<gfx::Size>& original_bitmap_sizes) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (http_status_code == 404) {
+    MarkUnableToDownloadFavicon(image_url);
+    return;
+  }
+
   if (bitmaps.size() == 0) {
     return;
   }
@@ -60,7 +67,10 @@ void IconHelper::DidUpdateFaviconURL(int32 page_id,
 
     switch(i->icon_type) {
       case content::FaviconURL::FAVICON:
-        if (listener_ && !listener_->ShouldDownloadFavicon(i->icon_url)) break;
+        if ((listener_ && !listener_->ShouldDownloadFavicon(i->icon_url)) ||
+            WasUnableToDownloadFavicon(i->icon_url)) {
+          break;
+        }
         web_contents()->DownloadImage(i->icon_url,
             true,  // Is a favicon
             0,  // No maximum size
@@ -85,4 +95,26 @@ void IconHelper::DidUpdateFaviconURL(int32 page_id,
   }
 }
 
+void IconHelper::DidStartNavigationToPendingEntry(
+    const GURL& url,
+    content::NavigationController::ReloadType reload_type) {
+  if (reload_type == content::NavigationController::RELOAD_IGNORING_CACHE)
+    ClearUnableToDownloadFavicons();

[mkosiba (inactive), 2014/04/24 15:42:44]

just curious - why don't we want to clear when we navigate to different urls
too?

[cimamoglu (inactive), 2014/04/28 10:41:04]

The original downstream bug description specifically defines the problem as
"WebView tries to fetch favicon.ico when WebView.goBack()" We would like to keep
track of past favicon 404s, so navigating to different URLs should not cause us
to clear. Also, this is essentially how it's done in the original CL.

+}
+
+void IconHelper::MarkUnableToDownloadFavicon(const GURL& icon_url) {
+  MissingFaviconURLHash url_hash = base::Hash(icon_url.spec());
+  missing_favicon_urls_.insert(url_hash);
+}
+
+bool IconHelper::WasUnableToDownloadFavicon(const GURL& icon_url) const {
+  MissingFaviconURLHash url_hash = base::Hash(icon_url.spec());
+  return missing_favicon_urls_.find(url_hash) != missing_favicon_urls_.end();
+}
+
+void IconHelper::ClearUnableToDownloadFavicons() {
+  missing_favicon_urls_.clear();
+}
+
+

[benm (inactive), 2014/04/24 19:03:21]

nit: double new line

[cimamoglu (inactive), 2014/04/28 10:41:04]

Done.

 }  // namespace android_webview