Introduce timeout for downloading a favicon for adding to Home screen

chrome/browser/android/shortcut_data_fetcher.cc

 // Copyright 2015 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/android/shortcut_data_fetcher.h"
 
 #include "base/basictypes.h"
 #include "base/location.h"
 #include "base/strings/string16.h"
 #include "base/task/cancelable_task_tracker.h"
@@ skipping 22 matching lines @@
 // Android's preferred icon size in DP is 48, as defined in
 // http://developer.android.com/design/style/iconography.html
 const int ShortcutDataFetcher::kPreferredIconSizeInDp = 48;
 
 ShortcutDataFetcher::ShortcutDataFetcher(
     content::WebContents* web_contents,
     Observer* observer)
     : 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 *
           gfx::Screen::GetScreenFor(web_contents->GetNativeView())->
               GetPrimaryDisplay().device_scale_factor()) {
   // Send a message to the renderer to retrieve information about the page.
   is_waiting_for_web_application_info_ = true;
   Send(new ChromeViewMsg_GetWebApplicationInfo(routing_id()));
 }
 
@@ skipping 59 matching lines @@
         preferred_icon_size_in_px_,
         false,
         base::Bind(&ShortcutDataFetcher::OnManifestIconFetched,
                    this));
   } else {
     // Grab the best favicon for the page.
     FetchFavicon();
   }
 
   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) {
   if (!is_waiting_for_web_application_info_) return false;
 
   bool handled = true;
 
   IPC_BEGIN_MESSAGE_MAP(ShortcutDataFetcher, message)
     IPC_MESSAGE_HANDLER(ChromeViewHostMsg_DidGetWebApplicationInfo,
                         OnDidGetWebApplicationInfo)
@@ skipping 25 matching lines @@
                                            ServiceAccessType::EXPLICIT_ACCESS);
 
   // Using favicon if its size is not smaller than platform required size,
   // otherwise using the largest icon among all avaliable icons.
   int threshold_to_get_any_largest_icon = preferred_icon_size_in_px_ - 1;
   favicon_service->GetLargestRawFaviconForPageURL(
       shortcut_info_.url,
       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,
       FROM_HERE,
       base::Bind(&ShortcutDataFetcher::CreateLauncherIcon,
                  this,
                  bitmap_result));
 }
 
 void ShortcutDataFetcher::CreateLauncherIcon(
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
   if (!web_contents() || !weak_observer_) return;
 
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   SkBitmap icon_bitmap;
   if (bitmap_result.is_valid()) {
     gfx::PNGCodec::Decode(bitmap_result.bitmap_data->front(),
                           bitmap_result.bitmap_data->size(),
                           &icon_bitmap);
   }
 
   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(
     int id,
     int http_status_code,
     const GURL& url,
     const std::vector<SkBitmap>& bitmaps,
     const std::vector<gfx::Size>& sizes) {
   if (!web_contents() || !weak_observer_) return;

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

Is there a reason why this path doesn't do the FinalizeLauncherIcon?

[gone, 2015/05/08 00:37:59]

Short answer: Pretty sure it didn't do it before.

Long answer: Making a launcher icon goes through steps to coerce favicons into a
format that the launcher would prefer, which includes discarding said icon if
it's not big enough to show to the user and replacing it with a color blob with
a single letter from the URL.  Based on the feedback I've been hearing about how
site-specified manifest icons should be accepted as is, I didn't pipe the icons
through that same pathway.

 
   // If getting the candidate manifest icon failed, the ShortcutHelper should
   // fallback to the favicon.
   // Otherwise, it sets the state as if there was no manifest icon pending.
   if (bitmaps.empty()) {
     FetchFavicon();
     return;
   }
 
   // There might be multiple bitmaps returned. The one to pick is bigger or
   // equal to the preferred size. |bitmaps| is ordered from bigger to smaller.
   int preferred_bitmap_index = 0;
   for (size_t i = 0; i < bitmaps.size(); ++i) {
     if (bitmaps[i].height() < preferred_icon_size_in_px_)
       break;
     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_);
 }