Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557103004:
Add chrome://site-tiles-internals/ (Closed)
DescriptionAdd chrome://ntp-tiles-internals/
Implement it for Android, Desktop, and iOS.
This largely supersedes chrome://popular-sites-internals/. Relative to
that, it lacks a way to see the cached JSON and the status of a manual
fetch.
BUG=655622
Committed: https://crrev.com/876fe79ca447c79d6b81738df6e756716faa9458
Cr-Commit-Position: refs/heads/master@{#438205}
Patch Set 1 #Patch Set 2 : git cl format #Patch Set 3 : Fix gn check #
Total comments: 46
Patch Set 4 : Support iOS #
Total comments: 4
Patch Set 5 : Address review comments #
Total comments: 22
Patch Set 6 : Address comments #
Total comments: 4
Patch Set 7 : ntp-tiles-internals #
Total comments: 20
Patch Set 8 : Fix ntp_tiles_resources.grdp ownership #Patch Set 9 : rebase #Patch Set 10 : IsSourceEnabled() #Patch Set 11 : Don't check ShouldShowPopularSites() #Patch Set 12 : rebase #Messages
Total messages: 71 (38 generated)
sfiera@chromium.org changed reviewers: + treib@chromium.org
Happy to demo this to you on Android or Desktop. Not yet happy to demo the iOS version because I haven't yet made it. I figured I'd send the meaty bits first; the iOS-only part should be mostly straight-forward.
Also note that the change to Chrome prefs will be obsoleted by your in-flight CL. I'd also like to have favicon stuff in the page, but I think that should wait until your change too.
The CQ bit was checked by sfiera@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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_ozone_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_...)
The CQ bit was checked by sfiera@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: CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true and NOPRESUBMIT=true in order for the CQ to process them
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by sfiera@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...
iOS now included. Easy-peasy.
First batch of comments, haven't looked at the ios part yet. https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/site_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:9: #include "chrome/browser/browser_process.h" Needed? https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:36: class ChromeSiteTilesInternalsMessageHandlerBridge s/Bridge/Client/ ? https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:40: ChromeSiteTilesInternalsMessageHandlerBridge() : handler_(this) {} This is a bit dangerous - if the handler does anything with |this|, things will probably fail in interesting ways. Could we maybe pass the pointer over in RegisterMessages? https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:82: default: What about WHITELIST? Can we get rid of the default, and list all enum values explicitly? https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:126: web_ui()->CallJavascriptFunctionUnsafe(name, values); CallJavascriptFunction from WebUIMessageHandler seems to be preferred over this. https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:131: content::WebUIDataSource* CreateSiteTilesInternalsHTMLSource() { Should this be in the anon namespace? https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/site_tiles_internals_ui.h (right): https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 :) https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/po... File components/ntp_tiles/popular_sites.h (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/po... components/ntp_tiles/popular_sites.h:100: std::string GetVersionToUse(); const? https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/resources/site_tiles_internals.html (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/resources/site_tiles_internals.html:14: <title>Popular Sites Internals</title> :) https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/resources/site_tiles_internals.js (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/resources/site_tiles_internals.js:31: */ Either implement or remove all the commented-out parts https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/site_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:26: result.clear(); nit: Does/should our new "always use braces" rule apply here too? https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:41: : web_ui_(web_ui), site_count_(8), weak_ptr_factory_(this) {} Any plans to make site_count_ non-constant? If not, just make it a kSiteCount constant? https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:118: const base::FilePath& path = web_ui_->MakePopularSites()->local_path(); Should we maybe expose the PopularSites instance from MostVisitedSites? Then there wouldn't be so many instances of it flying around, and I think MakePopularSites would become unnecessary. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:163: value.SetBoolean("popular", false); It doesn't need to be set to true in the enabled case? https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:179: tile.whitelist_icon_path.AsUTF8Unsafe()); Huh, contrary to what the comment on NTPTile says, this can actually be set for other sources too. So maybe always set it (and remove/update that comment)? Also, maybe LossyDisplayName rather than AsUTF8Unsafe? https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/site_tiles_internals_message_handler.h (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.h:29: class SiteTilesInternalsMessageHandler : public MostVisitedSites::Observer { I'm not entirely happy with the name, since it's not actually a WebUIMessageHandler (or whatever iOS's equivalent is called). But I don't really have a good suggestion either... https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.h:55: SiteTilesInternalsMessageHandlerClient* web_ui_; |client_|? IMO web_ui_ is a bit confusing. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:6: #define COMPONENTS_NTP_TILES_WEBUI_SITE_TILES_HANDLER_CLIENT_H_ Doesn't match the file name https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:32: // Returns the blocking pool for hte embedder. s/hte/the/ https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:42: // Creates a new MostVisited based on the context pf the WebUI page. s/pf/of/, also below https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:62: CallJavascriptFunctionVector(name, {&arg...}); This is a bit yucky... since we only seem to have single-argument CallJavascriptFunction invocations anyway, maybe get rid of this?
Ah, and one top-level comment: Why "site tiles"? Why not "NTP tiles", to stay consistent with how everything else is named?
noyau@chromium.org changed reviewers: + noyau@chromium.org
It would be useful to have a eg_test making sure the page loads properly on iOS. WebUI pages tends to break quite often on iOS as the underline technology is different (not the same rendering engine, not the same js). A small change to use a new chrome feature will break the iOS implementation silently... https://codereview.chromium.org/2557103004/diff/80001/components/ntp_tiles/we... File components/ntp_tiles/webui/resources/site_tiles_internals.html (right): https://codereview.chromium.org/2557103004/diff/80001/components/ntp_tiles/we... components/ntp_tiles/webui/resources/site_tiles_internals.html:12: maximum-scale=1.0, user-scalable=no"> Not allowing for zoom makes for pages that are sometimes really hard to read. Any reason why you don't want the user to be able to zoom? https://codereview.chromium.org/2557103004/diff/80001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/site_tiles_internals_ui.h (right): https://codereview.chromium.org/2557103004/diff/80001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/site_tiles_internals_ui.h:13: // The WebUI handler for chrome://physical-web. Change the comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/08 15:42:41, Marc Treib wrote: > Ah, and one top-level comment: Why "site tiles"? Why not "NTP tiles", to stay > consistent with how everything else is named? That's how things are named now, but it will look dated if these show up in places that aren't, properly speaking, new tab pages. On 2016/12/08 15:51:12, noyau wrote: > It would be useful to have a eg_test making sure the page loads properly on iOS. > WebUI pages tends to break quite often on iOS as the underline technology is > different (not the same rendering engine, not the same js). A small change to > use a new chrome feature will break the iOS implementation silently... Sure. That belongs in the internal CL, right?
https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/site_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:9: #include "chrome/browser/browser_process.h" On 2016/12/08 15:41:49, Marc Treib wrote: > Needed? Removed. https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:36: class ChromeSiteTilesInternalsMessageHandlerBridge On 2016/12/08 15:41:49, Marc Treib wrote: > s/Bridge/Client/ ? Done. https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:40: ChromeSiteTilesInternalsMessageHandlerBridge() : handler_(this) {} On 2016/12/08 15:41:49, Marc Treib wrote: > This is a bit dangerous - if the handler does anything with |this|, things will > probably fail in interesting ways. > > Could we maybe pass the pointer over in RegisterMessages? Sure, done. https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:82: default: On 2016/12/08 15:41:49, Marc Treib wrote: > What about WHITELIST? > Can we get rid of the default, and list all enum values explicitly? Done. https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:126: web_ui()->CallJavascriptFunctionUnsafe(name, values); On 2016/12/08 15:41:49, Marc Treib wrote: > CallJavascriptFunction from WebUIMessageHandler seems to be preferred over this. Unfortunately it only seems to take arguments by template, not by vector. Some of the reasoning behind the way things are done in this CL is that I would like to move the generic stuff that switches between content/ios to some common location eventually. It's not going to happen until after snippets-internals is on iOS too and it's clearer what the common part is. https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:131: content::WebUIDataSource* CreateSiteTilesInternalsHTMLSource() { On 2016/12/08 15:41:49, Marc Treib wrote: > Should this be in the anon namespace? Done. https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/site_tiles_internals_ui.h (right): https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/12/08 15:41:49, Marc Treib wrote: > 2016 :) Done. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/resources/site_tiles_internals.html (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/resources/site_tiles_internals.html:14: <title>Popular Sites Internals</title> On 2016/12/08 15:41:49, Marc Treib wrote: > :) Done. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/resources/site_tiles_internals.js (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/resources/site_tiles_internals.js:31: */ On 2016/12/08 15:41:50, Marc Treib wrote: > Either implement or remove all the commented-out parts Removed. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/site_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:26: result.clear(); On 2016/12/08 15:41:50, Marc Treib wrote: > nit: Does/should our new "always use braces" rule apply here too? Yes, I think so. I copy-pasted this function, but it's not actually used now because of the removal of viewing the JSON, so removed. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:41: : web_ui_(web_ui), site_count_(8), weak_ptr_factory_(this) {} On 2016/12/08 15:41:50, Marc Treib wrote: > Any plans to make site_count_ non-constant? If not, just make it a kSiteCount > constant? It should have that feature, since we sometimes fetch 12 (Android, non-Google DSE). I didn't consider it worth more effort than this yet, though. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:118: const base::FilePath& path = web_ui_->MakePopularSites()->local_path(); On 2016/12/08 15:41:50, Marc Treib wrote: > Should we maybe expose the PopularSites instance from MostVisitedSites? Then > there wouldn't be so many instances of it flying around, and I think > MakePopularSites would become unnecessary. Well, the alternate plan is to make PopularSites long-lived, so we don't need to create it and read a file every time the NTP is opened. I'd rather not widen MostVisitedSites' interface unnecessarily, and I'd rather not put in the work to change PopularSites, so this was an easy middle ground :) https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:163: value.SetBoolean("popular", false); On 2016/12/08 15:41:50, Marc Treib wrote: > It doesn't need to be set to true in the enabled case? It is true in the enabled case, because objects are true :) https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:179: tile.whitelist_icon_path.AsUTF8Unsafe()); On 2016/12/08 15:41:50, Marc Treib wrote: > Huh, contrary to what the comment on NTPTile says, this can actually be set for > other sources too. So maybe always set it (and remove/update that comment)? > > Also, maybe LossyDisplayName rather than AsUTF8Unsafe? Done, not done, and done. (set for other sources, update comment, use LossyDisplayName) I'll update the comment separately. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/site_tiles_internals_message_handler.h (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.h:29: class SiteTilesInternalsMessageHandler : public MostVisitedSites::Observer { On 2016/12/08 15:41:50, Marc Treib wrote: > I'm not entirely happy with the name, since it's not actually a > WebUIMessageHandler (or whatever iOS's equivalent is called). But I don't really > have a good suggestion either... If you were looking for the message handler for chrome://site-tiles-internals/, you would be more interested in this class than one that implements a specific platform's interface, right? https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.h:55: SiteTilesInternalsMessageHandlerClient* web_ui_; On 2016/12/08 15:41:50, Marc Treib wrote: > |client_|? IMO web_ui_ is a bit confusing. Done. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:6: #define COMPONENTS_NTP_TILES_WEBUI_SITE_TILES_HANDLER_CLIENT_H_ On 2016/12/08 15:41:50, Marc Treib wrote: > Doesn't match the file name Done. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:32: // Returns the blocking pool for hte embedder. On 2016/12/08 15:41:50, Marc Treib wrote: > s/hte/the/ Done. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:62: CallJavascriptFunctionVector(name, {&arg...}); On 2016/12/08 15:41:50, Marc Treib wrote: > This is a bit yucky... since we only seem to have single-argument > CallJavascriptFunction invocations anyway, maybe get rid of this? (discussed elsewhere) https://codereview.chromium.org/2557103004/diff/80001/components/ntp_tiles/we... File components/ntp_tiles/webui/resources/site_tiles_internals.html (right): https://codereview.chromium.org/2557103004/diff/80001/components/ntp_tiles/we... components/ntp_tiles/webui/resources/site_tiles_internals.html:12: maximum-scale=1.0, user-scalable=no"> On 2016/12/08 15:51:12, noyau wrote: > Not allowing for zoom makes for pages that are sometimes really hard to read. > Any reason why you don't want the user to be able to zoom? No, there isn't. This is how chrome://{snippets,popular-sites}-internals were done, probably on the model of some other place. We can change those too. https://codereview.chromium.org/2557103004/diff/80001/ios/chrome/browser/ui/w... File ios/chrome/browser/ui/webui/site_tiles_internals_ui.h (right): https://codereview.chromium.org/2557103004/diff/80001/ios/chrome/browser/ui/w... ios/chrome/browser/ui/webui/site_tiles_internals_ui.h:13: // The WebUI handler for chrome://physical-web. On 2016/12/08 15:51:12, noyau wrote: > Change the comment. Done.
On 2016/12/08 16:44:25, sfiera wrote: > On 2016/12/08 15:51:12, noyau wrote: > > It would be useful to have a eg_test making sure the page loads properly on > iOS. > > WebUI pages tends to break quite often on iOS as the underline technology is > > different (not the same rendering engine, not the same js). A small change to > > use a new chrome feature will break the iOS implementation silently... > > Sure. That belongs in the internal CL, right? For the next week or so, yes. After that, it will all be moved upstream.
On 2016/12/08 16:44:25, sfiera wrote: > On 2016/12/08 15:42:41, Marc Treib wrote: > > Ah, and one top-level comment: Why "site tiles"? Why not "NTP tiles", to stay > > consistent with how everything else is named? > > That's how things are named now, but it will look dated if these show up in > places that aren't, properly speaking, new tab pages. I'd still go with consistency. (I speak from experience: Supervised users were at one point going by three different names in the code, simultaneously. That was not fun.)
btw, is the plan to eventually remove popular-sites-internals? https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/site_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/site_tiles_internals_ui.cc:126: web_ui()->CallJavascriptFunctionUnsafe(name, values); On 2016/12/08 16:44:34, sfiera wrote: > On 2016/12/08 15:41:49, Marc Treib wrote: > > CallJavascriptFunction from WebUIMessageHandler seems to be preferred over > this. > > Unfortunately it only seems to take arguments by template, not by vector. Maybe the right solution would be to add a vector overload there? But alright, can't shave all the yaks :) > Some of the reasoning behind the way things are done in this CL is that I would > like to move the generic stuff that switches between content/ios to some common > location eventually. It's not going to happen until after snippets-internals is > on iOS too and it's clearer what the common part is. Alright, fair enough. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/site_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:41: : web_ui_(web_ui), site_count_(8), weak_ptr_factory_(this) {} On 2016/12/08 16:44:35, sfiera wrote: > On 2016/12/08 15:41:50, Marc Treib wrote: > > Any plans to make site_count_ non-constant? If not, just make it a kSiteCount > > constant? > > It should have that feature, since we sometimes fetch 12 (Android, non-Google > DSE). I didn't consider it worth more effort than this yet, though. I'd argue that as long as it doesn't have that feature, site_count shouldn't be a member :) But okay, fair enough. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:118: const base::FilePath& path = web_ui_->MakePopularSites()->local_path(); On 2016/12/08 16:44:35, sfiera wrote: > On 2016/12/08 15:41:50, Marc Treib wrote: > > Should we maybe expose the PopularSites instance from MostVisitedSites? Then > > there wouldn't be so many instances of it flying around, and I think > > MakePopularSites would become unnecessary. > > Well, the alternate plan is to make PopularSites long-lived, so we don't need to > create it and read a file every time the NTP is opened. I'd rather not widen > MostVisitedSites' interface unnecessarily, and I'd rather not put in the work to > change PopularSites, so this was an easy middle ground :) Well, the alternate alternate plan is to make MostVisitedSites long-lived (that's how it'll be used on desktop anyway). Then it could continue to own PopularSites, and that would be the one and only instance. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:163: value.SetBoolean("popular", false); On 2016/12/08 16:44:35, sfiera wrote: > On 2016/12/08 15:41:50, Marc Treib wrote: > > It doesn't need to be set to true in the enabled case? > > It is true in the enabled case, because objects are true :) Ewwww. Oh well.
https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/site_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:163: value.SetBoolean("popular", false); On 2016/12/08 17:33:22, Marc Treib wrote: > On 2016/12/08 16:44:35, sfiera wrote: > > On 2016/12/08 15:41:50, Marc Treib wrote: > > > It doesn't need to be set to true in the enabled case? > > > > It is true in the enabled case, because objects are true :) > > Ewwww. Oh well. (I suppose, if you want, the state could be { "topSites": {"enabled": true}, "suggestionsService": {"enabled": true}, "popular": {"enabled": false}, "whitelist": {"enabled": false}, })
Soo, what about the naming? I'm not opposed to "s/ntp_tiles/site_tiles/g", but IMO consistency > accuracy, so if we want to go with "site_tiles", we should do it everywhere. https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... File components/ntp_tiles/webui/site_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2557103004/diff/40001/components/ntp_tiles/we... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:163: value.SetBoolean("popular", false); On 2016/12/08 18:00:48, sfiera wrote: > On 2016/12/08 17:33:22, Marc Treib wrote: > > On 2016/12/08 16:44:35, sfiera wrote: > > > On 2016/12/08 15:41:50, Marc Treib wrote: > > > > It doesn't need to be set to true in the enabled case? > > > > > > It is true in the enabled case, because objects are true :) > > > > Ewwww. Oh well. > > (I suppose, if you want, the state could be { > "topSites": {"enabled": true}, > "suggestionsService": {"enabled": true}, > "popular": {"enabled": false}, > "whitelist": {"enabled": false}, > }) Naaah, it's okay. I just don't like JS :) https://codereview.chromium.org/2557103004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/site_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/site_tiles_internals_ui.cc:79: #endif I think this needs an "#else"? The enum entry exists on all platforms. It's also not accurate; it's not enabled everywhere on Android. (Or I guess I'm misinterpreting what "enabled" means here. Maybe "DoesSourceExist" or something?) https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... File components/ntp_tiles/webui/resources/site_tiles_internals.css (right): https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/resources/site_tiles_internals.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. 2016 (also in a few more files) https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... File components/ntp_tiles/webui/site_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:47: "getFavicon", This doesn't seem to be used yet? https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:66: DCHECK(args->GetDictionary(0, &dict)); This will break in release builds. https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:73: if (url.empty()) Braces :) https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:95: most_visited_sites_ = client_->MakeMostVisitedSites(); After https://codereview.chromium.org/2532103002/, MostVisitedSites will have a Refresh(). Would that also do the job? If so, mind adding a TODO or cherry-picking that change from my CL? https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... File components/ntp_tiles/webui/site_tiles_internals_message_handler.h (right): https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.h:6: #define COMPONENTS_NTP_TILES_WEBUI_SITE_TILES_HANDLER_H_ Doesn't match the file name https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.h:58: base::WeakPtrFactory<SiteTilesInternalsMessageHandler> weak_ptr_factory_; Unused I think https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... File components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h (right): https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:42: // Creates a new MostVisited based on the context pf the WebUI page. s/MostVisited/MostVisitedSites/ https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:59: // Helper function for CallJavascriptFunctionVector(). nit: This is really the main API, but the comment makes it sound like an internal helper thingie.
On Fri, Dec 9, 2016 at 11:52 AM, <treib@chromium.org> wrote: > Soo, what about the naming? I'm not opposed to "s/ntp_tiles/site_tiles/g", > but > IMO consistency > accuracy, so if we want to go with "site_tiles", we should > do > it everywhere. I would like to s/ntp_tiles/site_tiles/g but the approvals to make that happen are not going to come quickly, so I guess this will have to be ntp-tiles-internals for now. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That really only needs an approval from any components/ owner, everything else can be TBRed. But I think the new name deserves some more discussion - "tiles" is also not really accurate, it's a UI element that's really independent from the components/ntp_tiles code. One actual example: The plan is to have these also show up in Omnibox ZeroSuggest (which currently uses TopSites directly). "site_suggestions" maybe? On Fri, Dec 9, 2016 at 12:03 PM Chris Pickel <sfiera@chromium.org> wrote: > On Fri, Dec 9, 2016 at 11:52 AM, <treib@chromium.org> wrote: > > Soo, what about the naming? I'm not opposed to > "s/ntp_tiles/site_tiles/g", > > but > > IMO consistency > accuracy, so if we want to go with "site_tiles", we > should > > do > > it everywhere. > > I would like to s/ntp_tiles/site_tiles/g but the approvals to make > that happen are not going to come quickly, so I guess this will have > to be ntp-tiles-internals for now. > > -- > You received this message because you are subscribed to the Google Groups > "New Tab Page development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to ntp-dev+unsubscribe@chromium.org. > To post to this group, send email to ntp-dev@chromium.org. > To view this discussion on the web visit > https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/CABppz5krkwH4iqFHW2k... > . > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Add chrome://site-tiles-internals/ Implement it for Android, Desktop, and iOS. This largely supersedes chrome://popular-sites-internals/. Relative to that, it lacks a way to see the cached JSON and the status of a manual fetch. BUG=655622 ========== to ========== Add chrome://ntp-tiles-internals/ Implement it for Android, Desktop, and iOS. This largely supersedes chrome://popular-sites-internals/. Relative to that, it lacks a way to see the cached JSON and the status of a manual fetch. BUG=655622 ==========
https://codereview.chromium.org/2557103004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/site_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/site_tiles_internals_ui.cc:79: #endif On 2016/12/09 10:52:11, Marc Treib wrote: > I think this needs an "#else"? The enum entry exists on all platforms. Ah, right, this is wrong now that the WHITELIST case is in. (which also should have been true on Android) > It's also not accurate; it's not enabled everywhere on Android. (Or I guess I'm > misinterpreting what "enabled" means here. Maybe "DoesSourceExist" or > something?) This is also right. Feels like a good argument for IsSourceEnabled() to be on the MostVisitedSites object, where I can't get it wrong. That's also a good argument for MostVisitedSites to be long-lived. (I want a proper factory for MostVisitedSites, but am deferring that until your CL is in) https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... File components/ntp_tiles/webui/resources/site_tiles_internals.css (right): https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/resources/site_tiles_internals.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. On 2016/12/09 10:52:11, Marc Treib wrote: > 2016 (also in a few more files) Grepped for all 2015s, they should be gone! (just in time for 2017) https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... File components/ntp_tiles/webui/site_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:47: "getFavicon", On 2016/12/09 10:52:11, Marc Treib wrote: > This doesn't seem to be used yet? Removed. As you can see, this CL started out more ambitious… https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:66: DCHECK(args->GetDictionary(0, &dict)); On 2016/12/09 10:52:11, Marc Treib wrote: > This will break in release builds. Done. https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:73: if (url.empty()) On 2016/12/09 10:52:11, Marc Treib wrote: > Braces :) Done. https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:95: most_visited_sites_ = client_->MakeMostVisitedSites(); On 2016/12/09 10:52:11, Marc Treib wrote: > After https://codereview.chromium.org/2532103002/, MostVisitedSites will have a > Refresh(). Would that also do the job? If so, mind adding a TODO or > cherry-picking that change from my CL? Is it going to have the effect of recreating PopularSites to pick up new URL/country/version overrides (yet)? I added a TODO but I'm assuming I will need to wait a little. https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... File components/ntp_tiles/webui/site_tiles_internals_message_handler.h (right): https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.h:58: base::WeakPtrFactory<SiteTilesInternalsMessageHandler> weak_ptr_factory_; On 2016/12/09 10:52:12, Marc Treib wrote: > Unused I think Ah, yes, it was just for the removed JSON reading code (as was GetBlockingPool(), now removed from the client). https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... File components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h (right): https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:42: // Creates a new MostVisited based on the context pf the WebUI page. On 2016/12/09 10:52:12, Marc Treib wrote: > s/MostVisited/MostVisitedSites/ Done. https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:59: // Helper function for CallJavascriptFunctionVector(). On 2016/12/09 10:52:12, Marc Treib wrote: > nit: This is really the main API, but the comment makes it sound like an > internal helper thingie. Ah, I meant "helper for calling", not "helper for implementing." Does "Convenience function" make it clear to you?
The CQ bit was checked by sfiera@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: This issue passed the CQ dry run.
Thanks, looks good now, except for the naming - did we agree on "ntp-tiles" for now? https://codereview.chromium.org/2557103004/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/site_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/site_tiles_internals_ui.cc:79: #endif On 2016/12/09 17:35:28, sfiera wrote: > On 2016/12/09 10:52:11, Marc Treib wrote: > > I think this needs an "#else"? The enum entry exists on all platforms. > > Ah, right, this is wrong now that the WHITELIST case is in. > (which also should have been true on Android) > > > It's also not accurate; it's not enabled everywhere on Android. (Or I guess > I'm > > misinterpreting what "enabled" means here. Maybe "DoesSourceExist" or > > something?) > > This is also right. Feels like a good argument for IsSourceEnabled() to be on > the MostVisitedSites object, where I can't get it wrong. That's also a good > argument for MostVisitedSites to be long-lived. SG > (I want a proper factory for MostVisitedSites, but am deferring that until your > CL is in) My CL is in now! (Though the MVSites factory should probably still go into a separate CL) https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... File components/ntp_tiles/webui/site_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler.cc:95: most_visited_sites_ = client_->MakeMostVisitedSites(); On 2016/12/09 17:35:28, sfiera wrote: > On 2016/12/09 10:52:11, Marc Treib wrote: > > After https://codereview.chromium.org/2532103002/, MostVisitedSites will have > a > > Refresh(). Would that also do the job? If so, mind adding a TODO or > > cherry-picking that change from my CL? > > Is it going to have the effect of recreating PopularSites to pick up new > URL/country/version overrides (yet)? I added a TODO but I'm assuming I will need > to wait a little. It doesn't do that yet, but it definitely should. https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... File components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h (right): https://codereview.chromium.org/2557103004/diff/100001/components/ntp_tiles/w... components/ntp_tiles/webui/site_tiles_internals_message_handler_client.h:59: // Helper function for CallJavascriptFunctionVector(). On 2016/12/09 17:35:28, sfiera wrote: > On 2016/12/09 10:52:12, Marc Treib wrote: > > nit: This is really the main API, but the comment makes it sound like an > > internal helper thingie. > > Ah, I meant "helper for calling", not "helper for implementing." > > Does "Convenience function" make it clear to you? Yup SG, thanks! https://codereview.chromium.org/2557103004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/site_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/site_tiles_internals_ui.cc:82: // TODO(sfiera): support WHITELIST. I guess this is because desktop doesn't pass in the supervisor yet? https://codereview.chromium.org/2557103004/diff/120001/components/ntp_tiles/w... File components/ntp_tiles/webui/resources/site_tiles_internals.html (right): https://codereview.chromium.org/2557103004/diff/120001/components/ntp_tiles/w... components/ntp_tiles/webui/resources/site_tiles_internals.html:29: <div> Might be useful to add a top-level "is ntp_tiles active" - always true on Android, but depending on the field trial on desktop.
Yes, ntp-tiles-internals. Uploaded the rename, about to kick off the rebase. https://codereview.chromium.org/2557103004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/site_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/site_tiles_internals_ui.cc:82: // TODO(sfiera): support WHITELIST. On 2016/12/12 09:42:18, Marc Treib wrote: > I guess this is because desktop doesn't pass in the supervisor yet? Right. I guess we're not going to see WHITELIST tiles on this page on Android either because MakeMostVisitedSites() below doesn't pass the supervisor, but I'm inclined to let that slide until the factory exists. https://codereview.chromium.org/2557103004/diff/120001/components/ntp_tiles/w... File components/ntp_tiles/webui/resources/site_tiles_internals.html (right): https://codereview.chromium.org/2557103004/diff/120001/components/ntp_tiles/w... components/ntp_tiles/webui/resources/site_tiles_internals.html:29: <div> On 2016/12/12 09:42:18, Marc Treib wrote: > Might be useful to add a top-level "is ntp_tiles active" - always true on > Android, but depending on the field trial on desktop. I'll add that to the list of things still needed.
LGTM with some last nits. Thanks! https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:9: #include "base/files/file_util.h" Not needed I think https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:12: #include "base/task_runner_util.h" Not needed I think https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:26: ~NTPTilesInternalsMessageHandlerClient() = default; These should go into their own .cc file. (Or in the header, but that probably triggers some presubmit check) https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:29: : site_count_(8) {} client_(nullptr) https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h (right): https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h:6: #define COMPONENTS_NTP_TILES_WEBUI_NTP_TILES_HANDLER_H_ Doesn't match the file name. https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h:31: explicit NTPTilesInternalsMessageHandler(); nit: explicit not needed https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h:36: void RegisterMessages(NTPTilesInternalsMessageHandlerClient* client); |client| must outlive us.
lgtm, but with a comment. https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc:61: return true; The name suggestion service here is highly confusing. This means most likely right? Should this return true/false depending on the signin state of the user?
https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc:61: return true; On 2016/12/12 11:47:40, noyau wrote: > The name suggestion service here is highly confusing. This means most likely > right? Should this return true/false depending on the signin state of the user? I don't think so - this methods really just returns "does this source exist" (and maybe it should be renamed accordingly). That SuggestionsService (aka Most Likely) will return empty results for signed-out users is IMO not something that its clients should have to know about. E.g. ntp_tiles::MostVisitedSites doesn't know or care about signin status.
https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc:61: return true; On 2016/12/12 11:47:40, noyau wrote: > The name suggestion service here is highly confusing. This means most likely > right? Yes. Would you have expected MOST_LIKELY? This enum is shared in several places but it could be changed. We also use [CLIENT, SERVER, POPULAR, WHITELIST] in some places, which I like the brevity of. > Should this return true/false depending on the signin state of the user? No, for a signed-out user, we have a SuggestionsService, so it is enabled, even though it won't return tiles. This is how the NTPTilesInternalsMessageHandlerClient interface is defined. "DoesSourceExist()" sounds better to me. It would be useful to have the webui get information about the suggestions service, such as sync and sign-in state. We would first need to know that we have a valid suggestions service to ask those questions of, though, which is what this method determines.
https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc:61: return true; On 2016/12/12 12:15:38, sfiera wrote: > On 2016/12/12 11:47:40, noyau wrote: > > The name suggestion service here is highly confusing. This means most likely > > right? > > Yes. Would you have expected MOST_LIKELY? This enum is shared in several places > but it could be changed. > > We also use [CLIENT, SERVER, POPULAR, WHITELIST] in some places, which I like > the brevity of. I think those places are now (mostly?) gone :D But yes, CLIENT/SERVER is IMO better than TOP_SITES/SUGGESTIONS_SERVICE. Let's rename the enum at some point. > > Should this return true/false depending on the signin state of the user? > > No, for a signed-out user, we have a SuggestionsService, so it is enabled, even > though it won't return tiles. This is how the > NTPTilesInternalsMessageHandlerClient interface is defined. "DoesSourceExist()" > sounds better to me. > > It would be useful to have the webui get information about the suggestions > service, such as sync and sign-in state. We would first need to know that we > have a valid suggestions service to ask those questions of, though, which is > what this method determines.
https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc:61: return true; On 2016/12/12 12:18:45, Marc Treib wrote: > On 2016/12/12 12:15:38, sfiera wrote: > > On 2016/12/12 11:47:40, noyau wrote: > > > The name suggestion service here is highly confusing. This means most likely > > > right? > > > > Yes. Would you have expected MOST_LIKELY? This enum is shared in several > places > > but it could be changed. > > > > We also use [CLIENT, SERVER, POPULAR, WHITELIST] in some places, which I like > > the brevity of. > > I think those places are now (mostly?) gone :D They're in the histogram names, right? Those feel much more permanent than the source to me.
https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc (right): https://codereview.chromium.org/2557103004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/webui/ntp_tiles_internals_ui.cc:61: return true; On 2016/12/12 12:29:26, sfiera wrote: > On 2016/12/12 12:18:45, Marc Treib wrote: > > On 2016/12/12 12:15:38, sfiera wrote: > > > On 2016/12/12 11:47:40, noyau wrote: > > > > The name suggestion service here is highly confusing. This means most > likely > > > > right? > > > > > > Yes. Would you have expected MOST_LIKELY? This enum is shared in several > > places > > > but it could be changed. > > > > > > We also use [CLIENT, SERVER, POPULAR, WHITELIST] in some places, which I > like > > > the brevity of. > > > > I think those places are now (mostly?) gone :D > > They're in the histogram names, right? Those feel much more permanent than the > source to me. Ah, I thought you were referring to the old NTPLoggingTileSource enum. Yes, the histograms say "client"/"server".
sfiera@chromium.org changed reviewers: + bauerb@chromium.org, sdefresne@chromium.org
+bauerb: for linking this into //chrome/browser/ui/webui/… +sdefresne: for //components/resources/… (and fixing OWNERS there) https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc (right): https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:9: #include "base/files/file_util.h" On 2016/12/12 10:51:22, Marc Treib wrote: > Not needed I think Done. https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:12: #include "base/task_runner_util.h" On 2016/12/12 10:51:22, Marc Treib wrote: > Not needed I think Done. https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:26: ~NTPTilesInternalsMessageHandlerClient() = default; On 2016/12/12 10:51:22, Marc Treib wrote: > These should go into their own .cc file. (Or in the header, but that probably > triggers some presubmit check) Done. https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.cc:29: : site_count_(8) {} On 2016/12/12 10:51:22, Marc Treib wrote: > client_(nullptr) Done. https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... File components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h (right): https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h:6: #define COMPONENTS_NTP_TILES_WEBUI_NTP_TILES_HANDLER_H_ On 2016/12/12 10:51:22, Marc Treib wrote: > Doesn't match the file name. Done. https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h:31: explicit NTPTilesInternalsMessageHandler(); On 2016/12/12 10:51:22, Marc Treib wrote: > nit: explicit not needed Done. https://codereview.chromium.org/2557103004/diff/140001/components/ntp_tiles/w... components/ntp_tiles/webui/ntp_tiles_internals_message_handler.h:36: void RegisterMessages(NTPTilesInternalsMessageHandlerClient* client); On 2016/12/12 10:51:22, Marc Treib wrote: > |client| must outlive us. Done.
The CQ bit was checked by sfiera@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...
components/resources/ lgtm
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sfiera@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: This issue passed the CQ dry run.
I realized one tricky thing: according to the definition I gave for DoesSourceExist(), popular sites always exist on Android and iOS, irrespective of whether ShouldShowPopularSites() is true. It's instantiated, but maybe not queried, based on Finch and whether backfilling is necessary. We also don't want to check ShouldShowPopularSites() before instantiating PopularSites, because that would mean opting users into the experiment, even when backfilling is not necessary and they will never see popular sites. In fact, I shouldn't have been checking ShouldShowPopularSites() in DoesSourceExist(), because that opts users in to the experiment, too. In the latest version of the CL, DoesSourceExist(POPULAR) is unconditionally true on Android and iOS. Is there a name for ShouldShowPopularSites() that would make it more obvious that it can have side effects?
On 2016/12/13 13:10:04, sfiera wrote: > I realized one tricky thing: according to the definition I gave for > DoesSourceExist(), popular sites always exist on Android and iOS, irrespective > of whether ShouldShowPopularSites() is true. It's instantiated, but maybe not > queried, based on Finch and whether backfilling is necessary. We also don't want > to check ShouldShowPopularSites() before instantiating PopularSites, because > that would mean opting users into the experiment, even when backfilling is not > necessary and they will never see popular sites. > > In fact, I shouldn't have been checking ShouldShowPopularSites() in > DoesSourceExist(), because that opts users in to the experiment, too. In the > latest version of the CL, DoesSourceExist(POPULAR) is unconditionally true on > Android and iOS. > > Is there a name for ShouldShowPopularSites() that would make it more obvious > that it can have side effects? Hm. Maybe something more explicit like "InitPopularSitesFieldTrial"? But then it's not clear what it would return. (Aside: Opting users into the experiment when they access the internals page is probably not a huge deal.)
The CQ bit was checked by sfiera@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...
webui/ LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, noyau@chromium.org, sdefresne@chromium.org Link to the patchset: https://codereview.chromium.org/2557103004/#ps240001 (title: "rebase")
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": 240001, "attempt_start_ts": 1481649831007280,
"parent_rev": "0e55627d6089f57fd40521a7bb9683881e703249", "commit_rev":
"d231c60cd1f8f33b788bef5734214ef3841e4d7e"}
Message was sent while issue was closed.
Description was changed from ========== Add chrome://ntp-tiles-internals/ Implement it for Android, Desktop, and iOS. This largely supersedes chrome://popular-sites-internals/. Relative to that, it lacks a way to see the cached JSON and the status of a manual fetch. BUG=655622 ========== to ========== Add chrome://ntp-tiles-internals/ Implement it for Android, Desktop, and iOS. This largely supersedes chrome://popular-sites-internals/. Relative to that, it lacks a way to see the cached JSON and the status of a manual fetch. BUG=655622 Review-Url: https://codereview.chromium.org/2557103004 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Add chrome://ntp-tiles-internals/ Implement it for Android, Desktop, and iOS. This largely supersedes chrome://popular-sites-internals/. Relative to that, it lacks a way to see the cached JSON and the status of a manual fetch. BUG=655622 Review-Url: https://codereview.chromium.org/2557103004 ========== to ========== Add chrome://ntp-tiles-internals/ Implement it for Android, Desktop, and iOS. This largely supersedes chrome://popular-sites-internals/. Relative to that, it lacks a way to see the cached JSON and the status of a manual fetch. BUG=655622 Committed: https://crrev.com/876fe79ca447c79d6b81738df6e756716faa9458 Cr-Commit-Position: refs/heads/master@{#438205} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/876fe79ca447c79d6b81738df6e756716faa9458 Cr-Commit-Position: refs/heads/master@{#438205} |
