Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(365)

Issue 2824173005: [Content suggestions] Reveal URL with favicon in snippets-internals (Closed)

Created:
3 years, 8 months ago by jkrcal
Modified:
3 years, 8 months ago
Reviewers:
vitaliii
CC:
chromium-reviews, noyau+watch_chromium.org, arv+watch_chromium.org, ntp-dev+reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[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

Patch Set 1 #

Total comments: 6

Patch Set 2 : Vitalii's comments #

Patch Set 3 : Vitalii's comments #2 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -5 lines) Patch
M chrome/browser/resources/snippets_internals.html View 1 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/snippets_internals_message_handler.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M components/ntp_snippets/content_suggestion.h View 1 2 1 chunk +6 lines, -3 lines 0 comments Download
M components/ntp_snippets/content_suggestions_service.cc View 1 2 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 23 (15 generated)
jkrcal
Vitalii, could you PTAL?
3 years, 8 months ago (2017-04-18 17:17:34 UTC) #5
vitaliii
https://codereview.chromium.org/2824173005/diff/1/chrome/browser/resources/snippets_internals.html File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2824173005/diff/1/chrome/browser/resources/snippets_internals.html#newcode147 chrome/browser/resources/snippets_internals.html:147: <td>Favicon URL This may be misleading. The URL below ...
3 years, 8 months ago (2017-04-19 09:02:10 UTC) #8
jkrcal
Thanks! PTAL, again. https://codereview.chromium.org/2824173005/diff/1/chrome/browser/resources/snippets_internals.html File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2824173005/diff/1/chrome/browser/resources/snippets_internals.html#newcode147 chrome/browser/resources/snippets_internals.html:147: <td>Favicon URL On 2017/04/19 09:02:10, vitaliii ...
3 years, 8 months ago (2017-04-19 10:26:48 UTC) #9
vitaliii
LGTM % nit. https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/snippets_internals_message_handler.cc File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/snippets_internals_message_handler.cc#newcode57 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: ...
3 years, 8 months ago (2017-04-19 10:47:44 UTC) #10
jkrcal
Thanks. https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/snippets_internals_message_handler.cc File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2824173005/diff/1/chrome/browser/ui/webui/snippets_internals_message_handler.cc#newcode57 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 ...
3 years, 8 months ago (2017-04-19 11:44:07 UTC) #11
vitaliii
Great, thank you! LGTM
3 years, 8 months ago (2017-04-19 12:08:21 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2824173005/40001
3 years, 8 months ago (2017-04-19 14:14:24 UTC) #20
commit-bot: I haz the power
3 years, 8 months ago (2017-04-19 14:19:01 UTC) #23
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/06fc44d446d9eca68c8c827439ad...

Powered by Google App Engine
This is Rietveld 408576698