Fix FaviconHandler when it has multiple icon URLs and one of them is invalid

Fix FaviconHandler when it has multiple icon URLs and one of them is invalid

This CL prevents FaviconHandler from stopping iteration over a page's icon URLs
if a download for one of the icon URLs previously returned a 404. In particular,
this CL makes FaviconHandler::ScheduleDownload() call the download callback
when FaviconDriver::StartDownload() does not start a download. This is important
because the download callback is responsible for initiating the fetch for the
data for the next favicon candidate.

Sample scenario:
  Page HTML snippet:
  <link rel="icon" href="www.google.com/nothingtoseehere.png" />
  <link rel="icon" href="www.google.com/favicon.ico" />

  A previous download for "www.google.com/nothingtoseehere.png" returned a 404
  (e.g. the user previously visited a different page which also makes use of
  www.google.com/nothingtoseehere.png)

  This CL makes FaviconHandler fetch data for "www.google.com/favicon.ico"
  despite the download for the first favicon candidate
  ("www.google.com/nothingtoseehere.png") having previously failed.

BUG=474429
TEST=FaviconHandlerTest.MultipleFavicons404,
     FaviconHandlerTest.MultipleFaviconsAll404

Committed: https://crrev.com/8c6d0c6cc37a13f830d3422930a5f2f96674f046
Cr-Commit-Position: refs/heads/master@{#327339}

[pkotwicz, 2015-04-26 21:27:27]

pkotwicz@chromium.org changed reviewers:
+ rogerm@chromium.org

[pkotwicz, 2015-04-26 21:32:06]

rogerm@ PTAL

[Roger McFarlane (Chromium), 2015-04-27 14:25:48]

https://codereview.chromium.org/1079523005/diff/1/components/favicon/core/fav...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/1079523005/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:442: } else {
nit: Maybe return after ProcessCurrentUrl() ...


More importantly, add a comment block indicating...


"We get here when we are no longer going to continue downloading icons, either
because we've found a suitable one, or run out of candidates. (1) If we have a
valid candidate downloaded, save it to the
favicon database. (2) Clear the download bookkeeping structures."

Or some such...

https://codereview.chromium.org/1079523005/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:689: download_requests_[download_id]
= DownloadRequest(url, image_url, icon_type);
This reads like you could have collisions on multiple failures. Correct me if
I'm wrong, but it seems like multiple failed requests (download_id == 0) are
immediately handled to completion below? Maybe a comment to that effect?

https://codereview.chromium.org/1079523005/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:691: if (download_id == 0) {
As per above, comment that you trigger the handler with an empty image on
failure to start the download.

[pkotwicz, 2015-04-27 18:49:13]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2015-04-27 18:50:55]

rogerm@ can you please take another look?

https://codereview.chromium.org/1079523005/diff/1/components/favicon/core/fav...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/1079523005/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:691: if (download_id == 0) {
You are right, this is non-obvious. I have added a comment

[Roger McFarlane (Chromium), 2015-04-27 18:53:21]

lgtm

[pkotwicz, 2015-04-27 19:47:40]

pkotwicz@chromium.org changed reviewers:
+ sky@chromium.org

[pkotwicz, 2015-04-27 19:47:40]

Scott, can you please take a look?

[sky, 2015-04-27 21:08:52]

LGTM

[pkotwicz, 2015-04-28 17:12:38]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2015-04-28 17:12:55]

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

[commit-bot: I haz the power, 2015-04-28 18:50:24]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-04-28 18:52:11]

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