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

Issue 2572133002: Add more features to ntp-tiles-internal (Closed)

Created:
4 years ago by sfiera
Modified:
3 years, 10 months ago
CC:
chromium-reviews, pkl (ping after 24h if needed), oshima+watch_chromium.org, ntp-dev+reviews_chromium.org, sdefresne+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add more features to ntp-tiles-internal Let user fetch MostLikely tiles. If that's a no-op, e.g. because the user is not signed in, print a note to that effect. Show the Popular Sites JSON file when requested (pretty-printed). BUG=655622 Review-Url: https://codereview.chromium.org/2572133002 Cr-Commit-Position: refs/heads/master@{#448593} Committed: https://chromium.googlesource.com/chromium/src/+/65cefe9d0f5cd6a7892476b55e6431a91c05d11b

Patch Set 1 #

Patch Set 2 : remove unneeded include #

Total comments: 12

Patch Set 3 : address comments #

Total comments: 2

Patch Set 4 : rebase #

Patch Set 5 : rebase #

Patch Set 6 : Clarify comment #

Patch Set 7 : Fix ios #

Patch Set 8 : Remove blocking pool #

Patch Set 9 : Fix iOS again #

Patch Set 10 : Add ntp_tiles owners to client webui owners #

Unified diffs Side-by-side diffs Delta from patch set Stats (+150 lines, -59 lines) Patch
M chrome/browser/ui/webui/OWNERS View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/ntp_tiles_internals_ui.cc View 1 2 3 4 5 6 7 3 chunks +0 lines, -11 lines 0 comments Download
M chrome/browser/ui/webui/popular_sites_internals_ui.cc View 1 2 3 4 5 6 7 2 chunks +0 lines, -6 lines 0 comments Download
M components/ntp_tiles/most_visited_sites.h View 1 2 3 4 1 chunk +13 lines, -0 lines 0 comments Download
M components/ntp_tiles/most_visited_sites.cc View 1 2 3 4 1 chunk +15 lines, -0 lines 0 comments Download
M components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h View 1 2 3 4 3 chunks +8 lines, -0 lines 0 comments Download
M components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc View 1 2 3 4 5 7 chunks +77 lines, -13 lines 0 comments Download
M components/ntp_tiles/webui/ntp_tiles_internals_message_handler_client.h View 1 2 3 4 5 6 7 2 chunks +0 lines, -4 lines 0 comments Download
M components/ntp_tiles/webui/popular_sites_internals_message_handler_client.h View 1 2 3 4 5 6 7 2 chunks +0 lines, -4 lines 0 comments Download
M components/ntp_tiles/webui/resources/ntp_tiles_internals.css View 1 chunk +1 line, -1 line 0 comments Download
M components/ntp_tiles/webui/resources/ntp_tiles_internals.html View 2 chunks +14 lines, -3 lines 0 comments Download
M components/ntp_tiles/webui/resources/ntp_tiles_internals.js View 1 chunk +15 lines, -1 line 0 comments Download
M ios/chrome/browser/ui/webui/OWNERS View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc View 1 2 3 4 5 6 7 5 chunks +2 lines, -10 lines 0 comments Download
M ios/chrome/browser/ui/webui/popular_sites_internals_ui.cc View 1 2 3 4 5 6 7 8 2 chunks +0 lines, -6 lines 0 comments Download

Messages

Total messages: 48 (32 generated)
sfiera
After this, we can probably remove chrome://popular-sites-internals. We're not far from removing chrome://suggestions either. The ...
4 years ago (2016-12-14 14:52:14 UTC) #2
Marc Treib
https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/most_visited_sites.h File components/ntp_tiles/most_visited_sites.h (right): https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/most_visited_sites.h#newcode115 components/ntp_tiles/most_visited_sites.h:115: MostVisitedSitesSupervisor* supervisor() const { return supervisor_.get(); } If these ...
4 years ago (2016-12-14 15:06:27 UTC) #3
sfiera
https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/most_visited_sites.h File components/ntp_tiles/most_visited_sites.h (right): https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/most_visited_sites.h#newcode115 components/ntp_tiles/most_visited_sites.h:115: MostVisitedSitesSupervisor* supervisor() const { return supervisor_.get(); } On 2016/12/14 ...
4 years ago (2016-12-15 09:59:13 UTC) #8
Marc Treib
lgtm https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc#newcode158 components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:158: suggestions_status_ = "not synced"; On 2016/12/15 09:59:13, sfiera ...
4 years ago (2016-12-15 10:29:17 UTC) #10
Marc Treib
On 2016/12/15 10:29:17, Marc Treib wrote: > lgtm > > https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc > File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right): ...
3 years, 11 months ago (2017-01-25 15:54:56 UTC) #13
sfiera
Still relevant. Put it aside while fixing bugs reported against ntp-tiles-internals; I think there's still ...
3 years, 11 months ago (2017-01-25 16:07:07 UTC) #14
sfiera
https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2572133002/diff/20001/components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc#newcode158 components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:158: suggestions_status_ = "not synced"; On 2016/12/15 10:29:17, Marc Treib ...
3 years, 10 months ago (2017-02-02 17:16:44 UTC) #15
sfiera
Also removed the blocking pool, since it's no longer needed to get the JSON with ...
3 years, 10 months ago (2017-02-03 11:24:35 UTC) #26
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/2572133002/180001
3 years, 10 months ago (2017-02-03 13:14:24 UTC) #36
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/356211)
3 years, 10 months ago (2017-02-03 13:21:29 UTC) #38
sfiera
+bauerb,eugenebut for client owners. I also added the ntp-tiles owners to the ntp-tiles and popular-sites ...
3 years, 10 months ago (2017-02-03 13:55:05 UTC) #40
Marc Treib
Still lgtm
3 years, 10 months ago (2017-02-03 13:59:40 UTC) #41
Eugene But (OOO till 7-30)
ios lgtm
3 years, 10 months ago (2017-02-03 18:59:55 UTC) #42
Bernhard Bauer
lgtm
3 years, 10 months ago (2017-02-07 10:18:11 UTC) #43
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/2572133002/200001
3 years, 10 months ago (2017-02-07 10:20:44 UTC) #45
commit-bot: I haz the power
3 years, 10 months ago (2017-02-07 11:03:31 UTC) #48
Message was sent while issue was closed.
Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/65cefe9d0f5cd6a7892476b55e64...

Powered by Google App Engine
This is Rietveld 408576698