Move WasUnableToDownload logic to FaviconHandler

Move WasUnableToDownload logic to FaviconHandler

There's no need to leak this logic to the FaviconDriver class hierarchy
and have duplicate platform-dependent code.

BUG=694312
Review-Url: https://codereview.chromium.org/2728893004
Cr-Commit-Position: refs/heads/master@{#455408}
Committed: https://chromium.googlesource.com/chromium/src/+/e14e925e1705ff0cf7bc9726afc6761459a254a1

[mastiz, 2017-03-02 13:03:57]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 13:04:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mastiz, 2017-03-02 13:23:49]

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

[commit-bot: I haz the power, 2017-03-02 14:00:44]

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

[commit-bot: I haz the power, 2017-03-02 14:00:45]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-03-02 21:20:26]

https://codereview.chromium.org/2728893004/diff/1/components/favicon/content/...
File components/favicon/content/content_favicon_driver_unittest.cc (right):

https://codereview.chromium.org/2728893004/diff/1/components/favicon/content/...
components/favicon/content/content_favicon_driver_unittest.cc:74: // start a
download.
How about: Test that a download is initiated when there isn't a favicon in the
database for either the page URL or the icon URL.

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

https://codereview.chromium.org/2728893004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:670: if (service_ &&
service_->WasUnableToDownloadFavicon(image_url)) {
Won't this cause a regression? In particular if a user starts out with nothing
in history and visits the following two pages in order

OnFaviconUpdated() won't be called for the second page

Page #1 Icons:
https://www.google.com/aerae -> 404

Page #2 Icons
10.png -> 10x10px PNG
11.png -> 11x11px PNG
https://www.google.com/aerae -> 404

https://codereview.chromium.org/2728893004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:690: if (download_id == 0) {
Looking at the implementation of DownloadImage() I think that it is no longer
possible for |download_id| == 0

[mastiz, 2017-03-03 14:21:13]

The CQ bit was checked by mastiz@chromium.org to run a CQ dry run

[mastiz, 2017-03-03 14:21:14]

PTAL.

https://codereview.chromium.org/2728893004/diff/1/components/favicon/content/...
File components/favicon/content/content_favicon_driver_unittest.cc (right):

https://codereview.chromium.org/2728893004/diff/1/components/favicon/content/...
components/favicon/content/content_favicon_driver_unittest.cc:74: // start a
download.
On 2017/03/02 21:20:26, pkotwicz wrote:
> How about: Test that a download is initiated when there isn't a favicon in the
> database for either the page URL or the icon URL.

Done.

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

https://codereview.chromium.org/2728893004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:670: if (service_ &&
service_->WasUnableToDownloadFavicon(image_url)) {
On 2017/03/02 21:20:26, pkotwicz wrote:
> Won't this cause a regression? In particular if a user starts out with nothing
> in history and visits the following two pages in order
> 
> OnFaviconUpdated() won't be called for the second page
> 
> Page #1 Icons:
> https://www.google.com/aerae -> 404
> 
> Page #2 Icons
> 10.png -> 10x10px PNG
> 11.png -> 11x11px PNG
> https://www.google.com/aerae -> 404

I think you're right. I tried to add tests that would reproduce this case
(https://codereview.chromium.org/2732653002/) but failed: 
1. If sizes are explicit: FaviconHandler will stop as soon as the 11x11 one is
fetched (because it sorts them first).
2. If sizes are not explicit:  FaviconHandler will stop as soon as the 10x10 is
fetched, because the following one doesn't have an explicit size.

So I'm not entirely sure the bug is reproducible, but anyway addressed your
concern.

https://codereview.chromium.org/2728893004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:690: if (download_id == 0) {
On 2017/03/02 21:20:26, pkotwicz wrote:
> Looking at the implementation of DownloadImage() I think that it is no longer
> possible for |download_id| == 0

Tests currently return download_id in case of failure. I will address this in
future CLs where FaviconHandler tests are rewritten.

[commit-bot: I haz the power, 2017-03-03 14:21:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 15:04:57]

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

[commit-bot: I haz the power, 2017-03-03 15:04:58]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-03-03 18:31:16]

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

https://codereview.chromium.org/2728893004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:670: if (service_ &&
service_->WasUnableToDownloadFavicon(image_url)) {
This bug is reproduceable on the desktop for a FaviconHandler constructed with
FaviconDriverObserver::NON_TOUCH_16_DIP.

It is probably possible to create a test case for which the bug reproduces for
FaviconDriverObserver::NON_TOUCH_LARGEST by having incorrects "sizes"
information in the <link> tags

https://codereview.chromium.org/2728893004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:690: if (download_id == 0) {
Please add a TODO and file a bug :)

https://codereview.chromium.org/2728893004/diff/20001/components/favicon/core...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2728893004/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler.cc:668: if (service_ &&
service_->WasUnableToDownloadFavicon(image_url)) {
Can you call OnDidDownloadFavicon() instead?

This would reduce the number of possible code paths. That way ScheduleDownload()
always calls OnDidDownloadFavicon().

[mastiz, 2017-03-03 20:37:01]

PTAL.

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

https://codereview.chromium.org/2728893004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:670: if (service_ &&
service_->WasUnableToDownloadFavicon(image_url)) {
On 2017/03/03 18:31:16, pkotwicz wrote:
> This bug is reproduceable on the desktop for a FaviconHandler constructed with
> FaviconDriverObserver::NON_TOUCH_16_DIP.
> 
> It is probably possible to create a test case for which the bug reproduces for
> FaviconDriverObserver::NON_TOUCH_LARGEST by having incorrects "sizes"
> information in the <link> tags

This seems so unlikely that I'd rather not add tests for it. If you do want
tests, I'll add them in either of the two ongoing CLs that improve test coverage
(preferably as part of the FaviconHandler tests).

https://codereview.chromium.org/2728893004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:690: if (download_id == 0) {
On 2017/03/03 18:31:16, pkotwicz wrote:
> Please add a TODO and file a bug :)

Added a TODO, not filing a bug because it's a minor task and already addressed
by one of my CLs under review.

https://codereview.chromium.org/2728893004/diff/20001/components/favicon/core...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2728893004/diff/20001/components/favicon/core...
components/favicon/core/favicon_handler.cc:668: if (service_ &&
service_->WasUnableToDownloadFavicon(image_url)) {
On 2017/03/03 18:31:16, pkotwicz wrote:
> Can you call OnDidDownloadFavicon() instead?
> 
> This would reduce the number of possible code paths. That way
ScheduleDownload()
> always calls OnDidDownloadFavicon().

Done.

[pkotwicz, 2017-03-03 20:56:14]

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

https://codereview.chromium.org/2728893004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:670: if (service_ &&
service_->WasUnableToDownloadFavicon(image_url)) {
Fair enough

https://codereview.chromium.org/2728893004/diff/1/components/favicon/core/fav...
components/favicon/core/favicon_handler.cc:690: if (download_id == 0) {
Fair enough

https://codereview.chromium.org/2728893004/diff/40001/components/favicon/core...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2728893004/diff/40001/components/favicon/core...
components/favicon/core/favicon_handler.cc:664: if (service_ &&
service_->WasUnableToDownloadFavicon(image_url)) {
You also need to add the entry in download_requests_. Otherwise
OnDidDownloadFavicon() will return early and not proceed to the next icon

[mastiz, 2017-03-06 09:11:42]

PTAL.

https://codereview.chromium.org/2728893004/diff/40001/components/favicon/core...
File components/favicon/core/favicon_handler.cc (right):

https://codereview.chromium.org/2728893004/diff/40001/components/favicon/core...
components/favicon/core/favicon_handler.cc:664: if (service_ &&
service_->WasUnableToDownloadFavicon(image_url)) {
On 2017/03/03 20:56:14, pkotwicz wrote:
> You also need to add the entry in download_requests_. Otherwise
> OnDidDownloadFavicon() will return early and not proceed to the next icon

Done, thanks.

I managed to reproduce this case in tests and will be part of
https://codereview.chromium.org/2703603002/

[mastiz, 2017-03-07 09:37:27]

Friendly ping, I believe this could be ready for landing, thx.

[pkotwicz, 2017-03-08 05:44:33]

LGTM. Sorry for the delay

[mastiz, 2017-03-08 08:05:46]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2017-03-08 08:06:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 09:03:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488960346536820,
"parent_rev": "511c810d540422faf221bd82a4532313cabcc4c4", "commit_rev":
"e14e925e1705ff0cf7bc9726afc6761459a254a1"}

[commit-bot: I haz the power, 2017-03-08 09:04:08]

Description was changed from

==========
Move WasUnableToDownload logic to FaviconHandler

There's no need to leak this logic to the FaviconDriver class hierarchy
and have duplicate platform-dependent code.

BUG=694312
==========

to

==========
Move WasUnableToDownload logic to FaviconHandler

There's no need to leak this logic to the FaviconDriver class hierarchy
and have duplicate platform-dependent code.

BUG=694312

Review-Url: https://codereview.chromium.org/2728893004
Cr-Commit-Position: refs/heads/master@{#455408}
Committed:
https://chromium.googlesource.com/chromium/src/+/e14e925e1705ff0cf7bc9726afc6...
==========

[commit-bot: I haz the power, 2017-03-08 09:04:10]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/e14e925e1705ff0cf7bc9726afc6...