[Content suggestions] Reveal URL with favicon in snippets-internals

[Content suggestions] Reveal URL with favicon in snippets-internals

This CL adds debugging information for fetching favicons via Content
Suggestions Service. As in the presence of AMP URLs, the URL used for
fetching favicons is different from the main URL, we display it as
another piece of information in details for a suggestions.

BUG=676259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2824173005
Cr-Commit-Position: refs/heads/master@{#465590}
Committed: https://chromium.googlesource.com/chromium/src/+/06fc44d446d9eca68c8c827439ad2145a2e0f1d3

[jkrcal, 2017-04-18 17:16:40]

Description was changed from

==========
[Content suggestions] Reveal URL with favicon in snippets-internals

This CL adds debugging information for fetching favicons via Content
Suggestions Service. As in the presence of AMP URLs, the URL used for
fetching favicons is different from the main URL, we display it as
another piece of information in details for a suggestions.

BUG=676259
==========

to

==========
[Content suggestions] Reveal URL with favicon in snippets-internals

This CL adds debugging information for fetching favicons via Content
Suggestions Service. As in the presence of AMP URLs, the URL used for
fetching favicons is different from the main URL, we display it as
another piece of information in details for a suggestions.

BUG=676259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[jkrcal, 2017-04-18 17:17:06]

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

[commit-bot: I haz the power, 2017-04-18 17:17:33]

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

jkrcal@chromium.org changed reviewers:
+ vitaliii@chromium.org

[jkrcal, 2017-04-18 17:17:34]

Vitalii, could you PTAL?

[commit-bot: I haz the power, 2017-04-18 18:57:06]

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

[commit-bot: I haz the power, 2017-04-18 18:57:07]

Dry run: This issue passed the CQ dry run.

[vitaliii, 2017-04-19 09:02:10]

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/resources/sn...
File chrome/browser/resources/snippets_internals.html (right):

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/resources/sn...
chrome/browser/resources/snippets_internals.html:147: <td>Favicon URL
This may be misleading. The URL below is "of a page that links to a favicon",
while I would expect "Favicon URL" to link to an image itself. Should we change
to "URL with favicon"?

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:57:
suggestion.url_with_favicon().GetWithEmptyPath().spec());
Why do you remove path?

[jkrcal, 2017-04-19 10:26:48]

Thanks! PTAL, again.

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/resources/sn...
File chrome/browser/resources/snippets_internals.html (right):

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/resources/sn...
chrome/browser/resources/snippets_internals.html:147: <td>Favicon URL
On 2017/04/19 09:02:10, vitaliii wrote:
> This may be misleading. The URL below is "of a page that links to a favicon",
> while I would expect "Favicon URL" to link to an image itself. Should we
change
> to "URL with favicon"?

Done.

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:57:
suggestion.url_with_favicon().GetWithEmptyPath().spec());
On 2017/04/19 09:02:10, vitaliii wrote:
> Why do you remove path?

To do the same what is done in ContentSuggestionsService. I think this is not
worth introducing another ForDebugging method in the service.

[vitaliii, 2017-04-19 10:47:44]

LGTM % nit.

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:57:
suggestion.url_with_favicon().GetWithEmptyPath().spec());
On 2017/04/19 10:26:48, jkrcal wrote:
> On 2017/04/19 09:02:10, vitaliii wrote:
> > Why do you remove path?
> 
> To do the same what is done in ContentSuggestionsService. I think this is not
> worth introducing another ForDebugging method in the service.

Do you mean that ContentSuggestionsService ignores path (I could not see
anything related in CS)?
If yes, then you duplicating the logic here and it will become obsolete and
confusing when ContentSuggestionsService starts doing something else. 

Have you considered ignoring path inside url_with_favicon()?

[jkrcal, 2017-04-19 11:44:07]

Thanks.

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/sni...
File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right):

https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/sni...
chrome/browser/ui/webui/snippets_internals_message_handler.cc:57:
suggestion.url_with_favicon().GetWithEmptyPath().spec());
On 2017/04/19 10:47:44, vitaliii wrote:
> On 2017/04/19 10:26:48, jkrcal wrote:
> > On 2017/04/19 09:02:10, vitaliii wrote:
> > > Why do you remove path?
> > 
> > To do the same what is done in ContentSuggestionsService. I think this is
not
> > worth introducing another ForDebugging method in the service.
> 
> Do you mean that ContentSuggestionsService ignores path (I could not see
> anything related in CS)?
> If yes, then you duplicating the logic here and it will become obsolete and
> confusing when ContentSuggestionsService starts doing something else. 
> 
> Have you considered ignoring path inside url_with_favicon()?

Okay, done.

[jkrcal, 2017-04-19 11:44:17]

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

[commit-bot: I haz the power, 2017-04-19 11:44:32]

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-19 12:06:54]

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

[commit-bot: I haz the power, 2017-04-19 12:07:13]

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

[vitaliii, 2017-04-19 12:08:21]

Great, thank you!

LGTM

[commit-bot: I haz the power, 2017-04-19 13:13:32]

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

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

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-04-19 14:13:59]

The CQ bit was checked by jkrcal@chromium.org

[commit-bot: I haz the power, 2017-04-19 14:14:24]

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-19 14:18:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492611239519500,
"parent_rev": "d30183ba912ec6315d8b682b5a1064f554409a13", "commit_rev":
"06fc44d446d9eca68c8c827439ad2145a2e0f1d3"}

[commit-bot: I haz the power, 2017-04-19 14:19:00]

Description was changed from

==========
[Content suggestions] Reveal URL with favicon in snippets-internals

This CL adds debugging information for fetching favicons via Content
Suggestions Service. As in the presence of AMP URLs, the URL used for
fetching favicons is different from the main URL, we display it as
another piece of information in details for a suggestions.

BUG=676259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Content suggestions] Reveal URL with favicon in snippets-internals

This CL adds debugging information for fetching favicons via Content
Suggestions Service. As in the presence of AMP URLs, the URL used for
fetching favicons is different from the main URL, we display it as
another piece of information in details for a suggestions.

BUG=676259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2824173005
Cr-Commit-Position: refs/heads/master@{#465590}
Committed:
https://chromium.googlesource.com/chromium/src/+/06fc44d446d9eca68c8c827439ad...
==========

[commit-bot: I haz the power, 2017-04-19 14:19:01]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/06fc44d446d9eca68c8c827439ad...