Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2936793002:
ntp_tiles: Extend chrome://ntp-tiles-internals with favicon data (Closed)
Descriptionntp_tiles: Extend chrome://ntp-tiles-internals with favicon data
For each suggestion, the page URL is used to issue a lookup in the
favicon cache, and the results are displayed in the table.
BUG=655622
Review-Url: https://codereview.chromium.org/2936793002
Cr-Commit-Position: refs/heads/master@{#479288}
Committed: https://chromium.googlesource.com/chromium/src/+/fbc3cb683e35004ca90dd649076e77ad3ff8e47e
Patch Set 1 #
Total comments: 13
Patch Set 2 : Updated iOS. #Patch Set 3 : Fix build. #Patch Set 4 : Addressed comments. #Patch Set 5 : Added missing #include. #Patch Set 6 : Avoid using std::pair with constexpr. #
Messages
Total messages: 36 (27 generated)
mastiz@chromium.org changed reviewers: + sfiera@chromium.org
Quick comment before proper review: ios? CQ dry run?
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM BUG=655622 is appropriate. There, I listed: * Favicons * URL according to MostLikely * URL according to favicon service * Is it cached? but this CL is really about just the icons we have cached, and doesn't know anything about icons that aren't cached, right? Actually, I don't remember why I thought that MostLikely or the favicon service *would* know about uncached icon URLs. Do they? https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:41: icon_types_and_names_{ This looks like it could be a constexpr std::array<std::pair<>> local to the .cc file. https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:266: const NTPTilesVector& tiles) { First, cancelable_task_tracker_.CancelAll() in case we're still trying to load icons. https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:267: if (tiles.empty()) Nit: braces And early return? https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:270: auto on_lookup_done = base::Bind( BindRepeating() to make the usage clear? https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:272: base::Unretained(this), tiles, base::Owned(new FaviconResultMap()), Can you add a comment about Unretained? It's safe because of |cancelable_task_tracker_|, right? https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h (right): https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h:72: favicon::FaviconService* favicon_service_; Nit: * const https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... File components/ntp_tiles/webui/resources/ntp_tiles_internals.html (right): https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/resources/ntp_tiles_internals.html:127: <td class="value" jscontent="url"></td> Linkify this, like URL above?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== ntp_tiles: Extend chrome://ntp-tiles-internals with favicon data For each suggestion, the page URL is used to issue a lookup in the favicon cache, and the results are displayed in the table. BUG=none ========== to ========== ntp_tiles: Extend chrome://ntp-tiles-internals with favicon data For each suggestion, the page URL is used to issue a lookup in the favicon cache, and the results are displayed in the table. BUG=655622 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
> but this CL is really about just the icons we have cached, and doesn't know anything about icons that aren't cached, right? Actually, I don't remember why I thought that MostLikely or the favicon service *would* know about uncached icon URLs. Do they? The likely reason you thought about this is because (a) the chrome suggestions proto does actually populate a favicon URL; and (b) SuggestionsService&MostVisitedSites also expose that field, which they default to another URL if none was provided by the server. On mobile, the NTP always reads favicons from the FaviconService cache, powered underneath by HistoryService. Even for the smartest cases like jkrcal's fetching of icons from Google servers, the results are still written to the same favicon cache. On desktop (remote NTP only? not sure), that icon URL is indeed used by the NTP. So it is arguably a bit inconsistent with what we'd display here. https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:41: icon_types_and_names_{ On 2017/06/13 08:56:01, sfiera wrote: > This looks like it could be a constexpr std::array<std::pair<>> local to the .cc > file. Done. https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:266: const NTPTilesVector& tiles) { On 2017/06/13 08:56:01, sfiera wrote: > First, cancelable_task_tracker_.CancelAll() in case we're still trying to load > icons. Done. https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:267: if (tiles.empty()) On 2017/06/13 08:56:00, sfiera wrote: > Nit: braces > And early return? Done. https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:270: auto on_lookup_done = base::Bind( On 2017/06/13 08:56:01, sfiera wrote: > BindRepeating() to make the usage clear? Done. https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:272: base::Unretained(this), tiles, base::Owned(new FaviconResultMap()), On 2017/06/13 08:56:01, sfiera wrote: > Can you add a comment about Unretained? It's safe because of > |cancelable_task_tracker_|, right? Done. However, I fail to see why this is different to using |weak_ptr_factory_| which is not documented similarly.
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/13 11:19:40, mastiz wrote: > On desktop (remote NTP only? not sure), that icon URL is indeed used by the NTP. > So it is arguably a bit inconsistent with what we'd display here. Hmm. I guess the way I would like to resolve that inconsistency is to change desktop to use the favicon service everywhere. I assume that, in combination with jkrcal's fetcher, that would give better results for SuggestionsService tiles that relying on the SuggestionsService's own favicon support. https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2936793002/diff/1/components/ntp_tiles/webui/... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:272: base::Unretained(this), tiles, base::Owned(new FaviconResultMap()), On 2017/06/13 11:19:39, mastiz wrote: > On 2017/06/13 08:56:01, sfiera wrote: > > Can you add a comment about Unretained? It's safe because of > > |cancelable_task_tracker_|, right? > > Done. However, I fail to see why this is different to using |weak_ptr_factory_| > which is not documented similarly. Whenever I see Unretained(), I wonder what mechanism allows it to be safe. When I see weak_ptr_factory_, I know the mechanism.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mastiz@chromium.org changed reviewers: + eugenebut@chromium.org
eugenebut@chromium.org: Please review changes in ios/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ios lgtm
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org Link to the patchset: https://codereview.chromium.org/2936793002/#ps100001 (title: "Avoid using std::pair with constexpr.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1497416252730450,
"parent_rev": "d835a9885eef04f8d7560e07c2ce9fa3d61bb16f", "commit_rev":
"fbc3cb683e35004ca90dd649076e77ad3ff8e47e"}
Message was sent while issue was closed.
Description was changed from ========== ntp_tiles: Extend chrome://ntp-tiles-internals with favicon data For each suggestion, the page URL is used to issue a lookup in the favicon cache, and the results are displayed in the table. BUG=655622 ========== to ========== ntp_tiles: Extend chrome://ntp-tiles-internals with favicon data For each suggestion, the page URL is used to issue a lookup in the favicon cache, and the results are displayed in the table. BUG=655622 Review-Url: https://codereview.chromium.org/2936793002 Cr-Commit-Position: refs/heads/master@{#479288} Committed: https://chromium.googlesource.com/chromium/src/+/fbc3cb683e35004ca90dd649076e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/fbc3cb683e35004ca90dd649076e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
