Introduce timeout for downloading a favicon for adding to Home screen

chrome/browser/android/shortcut_data_fetcher.cc

Index: chrome/browser/android/shortcut_data_fetcher.cc
diff --git a/chrome/browser/android/shortcut_data_fetcher.cc b/chrome/browser/android/shortcut_data_fetcher.cc
index e8868bc9e6d24e56483cf2fad2137337e45d14fe..0d8f5365a12e5eaa73e6e78cce04ce0276dd0a46 100644
--- a/chrome/browser/android/shortcut_data_fetcher.cc
+++ b/chrome/browser/android/shortcut_data_fetcher.cc
@@ -40,6 +40,7 @@ ShortcutDataFetcher::ShortcutDataFetcher(
     : WebContentsObserver(web_contents),
       weak_observer_(observer),
       is_ready_(false),
+      icon_timeout_timer_(false, false),
       shortcut_info_(dom_distiller::url_utils::GetOriginalUrlFromDistillerUrl(
                      web_contents->GetURL())),
       preferred_icon_size_in_px_(kPreferredIconSizeInDp *
@@ -119,6 +120,14 @@ void ShortcutDataFetcher::OnDidGetManifest(const content::Manifest& manifest) {
   }
 
   weak_observer_->OnTitleAvailable(shortcut_info_.title);
+
+  // Kick off a timeout for downloading the icon.  If an icon isn't set within
+  // the timeout, fall back to using a dynamically-generated launcher icon.
+  icon_timeout_timer_.Start(FROM_HERE,

[benwells, 2015/05/07 04:26:12]

Is there any reason why not to put this in FetchFavicon, just after calling
GetLargestRawFaviconForPageURL? It would be clearer there that this is the
fallback for that.

[gone, 2015/05/07 18:35:29]

AIUI we're also trying to catch slow manifest icon retrievals.

+                            base::TimeDelta::FromMilliseconds(3000),
+                            base::Bind(&ShortcutDataFetcher::OnFaviconFetched,
+                                       this,
+                                       favicon_base::FaviconRawBitmapResult()));
 }
 
 bool ShortcutDataFetcher::OnMessageReceived(const IPC::Message& message) {
@@ -164,12 +173,14 @@ void ShortcutDataFetcher::FetchFavicon() {
       icon_types,
       threshold_to_get_any_largest_icon,
       base::Bind(&ShortcutDataFetcher::OnFaviconFetched, this),
-      &cancelable_task_tracker_);
+      &favicon_task_tracker_);
 }
 
 void ShortcutDataFetcher::OnFaviconFetched(

[benwells, 2015/05/07 04:26:12]

What happens if a second callback happens? E.g. the timer happens, then the
callback for GetLargestRawFaviconForPageURL fires.

[gone, 2015/05/07 18:35:29]

Should be caught by the !shortcut_icon_.drawsNothing() checks.  It's one of the
reasons I moved setting the shortcut_icon_ field onto the UI thread in
NotifyObserver.

[benwells, 2015/05/08 00:34:16]

Ah, OK. That and the other comment makes sense, but this logic feels pretty hard
to follow. Could you make it more explicit?

E.g. instead of checking that the shortcut icon isn't empty any more, can we
just have a boolean 'got shortcut'?

[gone, 2015/05/08 00:44:43]

Used 'is_icon_saved_'; seems more explicit that we've got just the icon.

     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
-  if (!web_contents() || !weak_observer_) return;
+  if (!web_contents() || !weak_observer_ || !shortcut_icon_.drawsNothing()) {
+    return;
+  }
 
   content::BrowserThread::PostTask(
       content::BrowserThread::IO,
@@ -192,14 +203,14 @@ void ShortcutDataFetcher::CreateLauncherIcon(
   }
 
   if (weak_observer_) {
-    shortcut_icon_ = weak_observer_->FinalizeLauncherIcon(icon_bitmap,
-                                                          shortcut_info_.url);
+    icon_bitmap = weak_observer_->FinalizeLauncherIcon(icon_bitmap,
+                                                       shortcut_info_.url);
   }
 
   content::BrowserThread::PostTask(
       content::BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&ShortcutDataFetcher::NotifyObserver, this));
+      base::Bind(&ShortcutDataFetcher::NotifyObserver, this, icon_bitmap));
 }
 
 void ShortcutDataFetcher::OnManifestIconFetched(
@@ -227,14 +238,15 @@ void ShortcutDataFetcher::OnManifestIconFetched(
     preferred_bitmap_index = i;
   }
 
-  shortcut_icon_ = bitmaps[preferred_bitmap_index];
-  NotifyObserver();
+  NotifyObserver(bitmaps[preferred_bitmap_index]);
 }
 
-void ShortcutDataFetcher::NotifyObserver() {
-  if (!web_contents() || !weak_observer_) return;
+void ShortcutDataFetcher::NotifyObserver(const SkBitmap& bitmap) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  if (!web_contents() || !weak_observer_ || !shortcut_icon_.drawsNothing())
+    return;
 
+  shortcut_icon_ = bitmap;
   is_ready_ = true;
   weak_observer_->OnDataAvailable(shortcut_info_, shortcut_icon_);
 }