[Content suggestions] Implement fetching publishers' favicons

[Content suggestions] Implement fetching publishers' favicons

This CL implements fetching publisher's favicons using LargeIconService
(it follows-up on CL 2781583002 that has only added the API function).

BUG=692975
Review-Url: https://codereview.chromium.org/2790743003
Cr-Commit-Position: refs/heads/master@{#462444}
Committed: https://chromium.googlesource.com/chromium/src/+/7b7e71f19b573ed109fba77b10271798b0ac91bb

[jkrcal, 2017-03-31 13:07:03]

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

[commit-bot: I haz the power, 2017-03-31 13:07:16]

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

[jkrcal, 2017-03-31 13:09:47]

jkrcal@chromium.org changed reviewers:
+ bauerb@chromium.org, gambard@chromium.org, pkotwicz@chromium.org

[jkrcal, 2017-03-31 13:09:48]

Bernhard, could you PTAL?

Gauthier, could you PTAL at iOS factory?
Peter, could you PTAL at DEPS? (favicon_base, favicon/core)

[commit-bot: I haz the power, 2017-03-31 13:11:20]

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

[commit-bot: I haz the power, 2017-03-31 13:11:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jkrcal, 2017-03-31 13:36:13]

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

[commit-bot: I haz the power, 2017-03-31 13:36:33]

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-31 13:47:10]

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

[commit-bot: I haz the power, 2017-03-31 13:47:11]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[gambard, 2017-03-31 14:30:45]

ios/ lgtm with one suggestion

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:157:
large_icon_service_->GetLargeIconImageOrFallbackStyle(
It would make sense to move this to a separate file so it might be easier to
reuse? Or maybe it should be part of the LargeIconService API directly?

[Bernhard Bauer, 2017-03-31 16:45:16]

LGTM w/ nits:

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/DEPS (right):

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/DEPS:4: "+components/favicon/core",
Huh. Are both the DEPS file and the GN file autoformatted? They seem to use
different lexicographical orderings.

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:141: // favicon (maybe
voluntarily).
Nit: I _think_ I know what you mean by this comment, but it reads a bit
confusing (What would involuntarily involving the provider mean? Threaten it
with physical violence unless it hands over a URL? 😃). Maybe "Allow the provider
to provide (or possibly override) the URL for looking up the favicon"?

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:155: const GURL&
publisher_url = (*position).url().GetWithEmptyPath();
This can be `position->`.

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:103: large_icon_service, 
// Can be nullptr in unittests.
Nit: I would move the comment to the previous line and keep the code on a single
line.

[jkrcal, 2017-04-03 12:37:14]

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

[commit-bot: I haz the power, 2017-04-03 12:37:25]

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

[jkrcal, 2017-04-03 12:37:36]

Thanks!

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/DEPS (right):

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/DEPS:4: "+components/favicon/core",
On 2017/03/31 16:45:16, Bernhard (slow until Apr 6) wrote:
> Huh. Are both the DEPS file and the GN file autoformatted? They seem to use
> different lexicographical orderings.

git cl format does nothing on the current content of these files. Anyway, you
are right, favicon_base should come as the second. Fixed.

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:141: // favicon (maybe
voluntarily).
On 2017/03/31 16:45:16, Bernhard (slow until Apr 6) wrote:
> Nit: I _think_ I know what you mean by this comment, but it reads a bit
> confusing (What would involuntarily involving the provider mean? Threaten it
> with physical violence unless it hands over a URL? 😃). Maybe "Allow the
provider
> to provide (or possibly override) the URL for looking up the favicon"?

:)
Done.

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:155: const GURL&
publisher_url = (*position).url().GetWithEmptyPath();
On 2017/03/31 16:45:16, Bernhard (slow until Apr 6) wrote:
> This can be `position->`.

Done.

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:157:
large_icon_service_->GetLargeIconImageOrFallbackStyle(
On 2017/03/31 14:30:44, gambard wrote:
> It would make sense to move this to a separate file so it might be easier to
> reuse? Or maybe it should be part of the LargeIconService API directly?

I agree that some wrapper fetch-from-cache-and-server-as-fallback should be part
of LargeIconService. Let me do it later, in a separate CL, when another user of
this kind appears. Landing changes in LargeIconService takes time and I really
want to squeeze this into M59.

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.h (right):

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.h:103: large_icon_service, 
// Can be nullptr in unittests.
On 2017/03/31 16:45:16, Bernhard (slow until Apr 6) wrote:
> Nit: I would move the comment to the previous line and keep the code on a
single
> line.

Done.

[jkrcal, 2017-04-03 12:38:42]

Peter, friendly ping! 
Could you PTAL at DEPS? (favicon_base, favicon/core)

[gambard, 2017-04-03 12:38:50]

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
File components/ntp_snippets/content_suggestions_service.cc (right):

https://codereview.chromium.org/2790743003/diff/40001/components/ntp_snippets...
components/ntp_snippets/content_suggestions_service.cc:157:
large_icon_service_->GetLargeIconImageOrFallbackStyle(
On 2017/04/03 12:37:36, jkrcal wrote:
> On 2017/03/31 14:30:44, gambard wrote:
> > It would make sense to move this to a separate file so it might be easier to
> > reuse? Or maybe it should be part of the LargeIconService API directly?
> 
> I agree that some wrapper fetch-from-cache-and-server-as-fallback should be
part
> of LargeIconService. Let me do it later, in a separate CL, when another user
of
> this kind appears. Landing changes in LargeIconService takes time and I really
> want to squeeze this into M59.

Acknowledged.

[commit-bot: I haz the power, 2017-04-03 13:38:54]

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

[commit-bot: I haz the power, 2017-04-03 13:38:55]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2017-04-03 14:55:02]

LGTM on DEPS change

[jkrcal, 2017-04-06 11:56:28]

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

[commit-bot: I haz the power, 2017-04-06 11:56:46]

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-04-06 13:06:43]

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

[commit-bot: I haz the power, 2017-04-06 13:06:44]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-04-06 13:07:20]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-04-06 13:07:21]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
gambard@chromium.org, pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2790743003/#ps100001
(title: "Minor fix")

[commit-bot: I haz the power, 2017-04-06 13:07:33]

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-04-06 13:12:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491484040565250,
"parent_rev": "0ffe901d4a9a8eec1419392acf6e2d28afc4d9de", "commit_rev":
"7b7e71f19b573ed109fba77b10271798b0ac91bb"}

[commit-bot: I haz the power, 2017-04-06 13:13:33]

Description was changed from

==========
[Content suggestions] Implement fetching publishers' favicons

This CL implements fetching publisher's favicons using LargeIconService
(it follows-up on CL 2781583002 that has only added the API function).

BUG=692975
==========

to

==========
[Content suggestions] Implement fetching publishers' favicons

This CL implements fetching publisher's favicons using LargeIconService
(it follows-up on CL 2781583002 that has only added the API function).

BUG=692975

Review-Url: https://codereview.chromium.org/2790743003
Cr-Commit-Position: refs/heads/master@{#462444}
Committed:
https://chromium.googlesource.com/chromium/src/+/7b7e71f19b573ed109fba77b1027...
==========

[commit-bot: I haz the power, 2017-04-06 13:13:34]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/7b7e71f19b573ed109fba77b1027...