Introduce timeout for downloading a favicon for adding to Home screen

Introduce timeout for downloading a favicon for adding to Home screen

If an icon can't be downloaded fast enough, generate a
default icon instead.

BUG=457424
Committed: https://crrev.com/9988636c1b615e83af9e72daf28cfd0cd1690870
Cr-Commit-Position: refs/heads/master@{#328970}

[gone, 2015-05-01 22:33:38]

dfalcantara@chromium.org changed reviewers:
+ benwells@chromium.org

[gone, 2015-05-06 17:49:07]

Hey Ben, this one's ready to go now that the other one's landed.

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

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
File chrome/browser/android/shortcut_data_fetcher.cc (right):

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
chrome/browser/android/shortcut_data_fetcher.cc:126:
icon_timeout_timer_.Start(FROM_HERE,
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.

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
chrome/browser/android/shortcut_data_fetcher.cc:179: void
ShortcutDataFetcher::OnFaviconFetched(
What happens if a second callback happens? E.g. the timer happens, then the
callback for GetLargestRawFaviconForPageURL fires.

[gone, 2015-05-07 18:35:30]

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
File chrome/browser/android/shortcut_data_fetcher.cc (right):

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
chrome/browser/android/shortcut_data_fetcher.cc:126:
icon_timeout_timer_.Start(FROM_HERE,
On 2015/05/07 04:26:12, benwells wrote:
> 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.

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

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
chrome/browser/android/shortcut_data_fetcher.cc:179: void
ShortcutDataFetcher::OnFaviconFetched(
On 2015/05/07 04:26:12, benwells wrote:
> What happens if a second callback happens? E.g. the timer happens, then the
> callback for GetLargestRawFaviconForPageURL fires.

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:17]

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
File chrome/browser/android/shortcut_data_fetcher.cc (right):

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
chrome/browser/android/shortcut_data_fetcher.cc:179: void
ShortcutDataFetcher::OnFaviconFetched(
On 2015/05/07 18:35:29, dfalcantara wrote:
> On 2015/05/07 04:26:12, benwells wrote:
> > What happens if a second callback happens? E.g. the timer happens, then the
> > callback for GetLargestRawFaviconForPageURL fires.
> 
> 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.

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'?

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
chrome/browser/android/shortcut_data_fetcher.cc:222: if (!web_contents() ||
!weak_observer_) return;
Is there a reason why this path doesn't do the FinalizeLauncherIcon?

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

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
File chrome/browser/android/shortcut_data_fetcher.cc (right):

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
chrome/browser/android/shortcut_data_fetcher.cc:222: if (!web_contents() ||
!weak_observer_) return;
On 2015/05/08 00:34:16, benwells wrote:
> Is there a reason why this path doesn't do the FinalizeLauncherIcon?

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.

[gone, 2015-05-08 00:44:44]

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
File chrome/browser/android/shortcut_data_fetcher.cc (right):

https://chromiumcodereview.appspot.com/1124433005/diff/20001/chrome/browser/a...
chrome/browser/android/shortcut_data_fetcher.cc:179: void
ShortcutDataFetcher::OnFaviconFetched(
On 2015/05/08 00:34:16, benwells wrote:
> On 2015/05/07 18:35:29, dfalcantara wrote:
> > On 2015/05/07 04:26:12, benwells wrote:
> > > What happens if a second callback happens? E.g. the timer happens, then
the
> > > callback for GetLargestRawFaviconForPageURL fires.
> > 
> > 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.
> 
> 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'?

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

[benwells, 2015-05-08 03:11:16]

lgtm

[dfalcantara1, 2015-05-08 04:13:51]

The CQ bit was checked by dfalcantara@google.com

[commit-bot: I haz the power, 2015-05-08 04:14:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124433005/40001

[commit-bot: I haz the power, 2015-05-08 04:20:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-08 04:20:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[gone, 2015-05-08 04:26:14]

dfalcantara@chromium.org changed reviewers:
+ tedchoc@chromium.org
- dfalcantara@google.com

[gone, 2015-05-08 04:26:14]

Ted for OWNERS, I guess?  Thought I was an OWNER.

[Ted C, 2015-05-08 16:57:29]

lgtm

[dfalcantara1, 2015-05-08 17:00:44]

The CQ bit was checked by dfalcantara@google.com

[commit-bot: I haz the power, 2015-05-08 17:02:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1124433005/40001

[commit-bot: I haz the power, 2015-05-08 17:24:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-08 17:25:38]

Patchset 3 (id:??) landed as
https://crrev.com/9988636c1b615e83af9e72daf28cfd0cd1690870
Cr-Commit-Position: refs/heads/master@{#328970}