Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1883523002:
chrome://snippets-internals page for debugging NTP snippets. (Closed)
|
Created:
4 years, 8 months ago by jkrcal Modified:
4 years, 8 months ago CC:
chromium-reviews, mcwilliams+watch_chromium.org, arv+watch_chromium.org, dgn+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionchrome://snippets-internals page for debugging NTP snippets.
Displays a list of NTP snippets and discarded snippets with all details
(as obtained from the ChromeReader servers). It also shows the list of
hosts by the SuggestionService to which the snippets are restricted.
The page allows to clear the list of snippets / discarded snippets as
well as to fetch new snippets from user-specified hosts.
BUG=597566
NOPRESUBMIT=true
(the presubmit check fails on javascript arrow function, see https://bugs.chromium.org/p/chromium/issues/detail?id=567770#c8)
Committed: https://crrev.com/3ccb46d44ee5a1a3a5cee851792fb01b8ef56749
Cr-Commit-Position: refs/heads/master@{#387919}
Patch Set 1 #Patch Set 2 : A few code fixes #Patch Set 3 : Rebase-update #Patch Set 4 : Removing unintentionally added files #Patch Set 5 : Unit tests #
Total comments: 54
Patch Set 6 : Code review #Patch Set 7 : Adding the chrome://snippets-internals page in the auto-suggest list. #
Total comments: 48
Patch Set 8 : Second code review #
Total comments: 12
Patch Set 9 : Third code review ( rebase-update) #
Total comments: 12
Patch Set 10 : Fourth code review #
Total comments: 6
Patch Set 11 : Implementing last suggestions by Marc #Patch Set 12 : Implementing last suggestions from Bernhard #Messages
Total messages: 36 (13 generated)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883523002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883523002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883523002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883523002/40001
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Hi Marc, could you make the first round of critique?
Very nice! A bunch of comments below, but they're all fairly minor. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:168: <if expr="is_android"> Hm, actually almost all of this page should work on desktop too - the only Android-specific thing I found is the feature flag. Would it make sense to enable this on desktop too, given that we eventually hope to launch on more platforms? https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.css (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. 2016 :) (also in the other new files) https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:44: <span class="snippet-title" jsvalues="myid:id"><span jscontent="title"></span> >></a> Line too long (a few more of those below too). Also looks broken, what's the ">></a>" doing here? https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:48: <td>Validity <td> tags should be closed. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:91: <span class="discarded-snippet-title" jsvalues="myid:id"><span jscontent="title"></span> >></a> ">></a>" ? https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.js:23: function clearDiscarded(event) { submitClearDiscarded, for consistency? https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.js:61: if (object == null || !object.list || object.list.constructor !== Array) Are these checks actually required in any legitimate case? If so, please add a comment saying in which cases this can actually happen. Otherwise, please remove the checks - if something goes wrong, we'd like to notice, rather than silently swallowing the error. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.js:94: trigger: trigger, I think "trigger" doesn't need to be listed here. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:99: // On page reloads it is necessary to first remove the observer. Why? Also: On the initial load, you're trying to remove an observer that isn't there. (Probably not really a problem, but not nice either.) And you never remove the observer when this page is closed. That might cause use-after-free crashes. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:136: base::StringPiece hosts_piece(hosts_string); I think this conversion happens automatically, i.e. you can just pass hosts_string to SplitString. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:214: "chrome.snippets_internals.receiveHostsInput", hosts_value); Any particular reason for sending this separately? Should be trivial to just build the "imploded" string in js, right? https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:219: const std::string string_value(value ? "True" : "False"); Might as well just inline this below. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.h (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.h:11: #include "base/memory/scoped_ptr.h" We can use std::unique_ptr now (from <memory>)! But this include doesn't actually seem to be required. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.h:20: // The implementation for the chrome://zine-internals page. snippets-internals https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.h:31: void NTPSnippetsServiceShutdown() override; These two can be private. It's common practice to add a comment saying where they're overridden from (as for RegisterMessages below). https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_ui.cc (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_ui.cc:5: #include "snippets_internals_ui.h" Full paths in includes, always. Also for the message handler include below. (Probably also done by that automatic include "cleanup"?) https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_ui.h (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_ui.h:11: // The implementation for the chrome://zine-internals page. snippets-internals https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (left): https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:8: #include <utility> I think these two should stay, for std::find_if and std::move respectively. https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:182: // |suggestions_service_| can be null in tests. No point for this null check here anymore - please move it into GetSuggestionsHosts where suggestions_service_ is actually used https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:211: suggestions_service_->GetSuggestionsDataFromCache()); Can you please add a "TODO(treib)" saying that this should just call GetSnippetHostsFromPrefs? (Add TODO rather than actually change it, because I'm not sure if that won't break anything, e.g. on first run). https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:226: NTPSnippetsServiceLoaded()); misaligned https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:90: const NTPSnippetStorage& GetDiscardedSnippets(); Should be const (add "const" just before the semicolon). Also since this is a trivial getter, it should be called "discarded_snippets" (yes it's weird and kinda inconsistent, but that's what the style guide says...), and implemented inline here. https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:96: std::set<std::string> GetSuggestionsHosts(); Also const please https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:173: ASSERT_TRUE(LoadFromJSONString(json_str)); Heads-up: I changed the setup of these tests slightly in https://codereview.chromium.org/1878993002/, you'll have to rebase. (e.g. LoadFromJSONString doesn't have a return value anymore) https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:230: EXPECT_EQ(0u, snippets.size()); Ah, this works because you have a reference to the member, which changed in the meantime... I find this kinda surprising/hard to read, I'd prefer to say service()->GetDiscardedSnippets().size() here.
jkrcal@chromium.org changed reviewers: + bauerb@chromium.org
Marc, could you please look again? bauerb@chromium.org: PTAL https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:168: <if expr="is_android"> On 2016/04/13 08:45:51, Marc Treib wrote: > Hm, actually almost all of this page should work on desktop too - the only > Android-specific thing I found is the feature flag. > Would it make sense to enable this on desktop too, given that we eventually hope > to launch on more platforms? Acknowledged. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.css (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/13 08:45:51, Marc Treib wrote: > 2016 :) (also in the other new files) Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:44: <span class="snippet-title" jsvalues="myid:id"><span jscontent="title"></span> >></a> On 2016/04/13 08:45:51, Marc Treib wrote: > Line too long (a few more of those below too). > Also looks broken, what's the ">></a>" doing here? Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:48: <td>Validity On 2016/04/13 08:45:51, Marc Treib wrote: > <td> tags should be closed. Not in HTML5. https://www.w3.org/TR/2014/REC-html5-20141028/tabular-data.html#the-td-element Not that the file conforms to HTML5 (jscontent attributes break it) but it is the specification I am aiming for. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:91: <span class="discarded-snippet-title" jsvalues="myid:id"><span jscontent="title"></span> >></a> On 2016/04/13 08:45:51, Marc Treib wrote: > ">></a>" ? Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.js:23: function clearDiscarded(event) { On 2016/04/13 08:45:51, Marc Treib wrote: > submitClearDiscarded, for consistency? Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.js:61: if (object == null || !object.list || object.list.constructor !== Array) On 2016/04/13 08:45:51, Marc Treib wrote: > Are these checks actually required in any legitimate case? If so, please add a > comment saying in which cases this can actually happen. Otherwise, please remove > the checks - if something goes wrong, we'd like to notice, rather than silently > swallowing the error. Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.js:94: trigger: trigger, On 2016/04/13 08:45:51, Marc Treib wrote: > I think "trigger" doesn't need to be listed here. Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:99: // On page reloads it is necessary to first remove the observer. On 2016/04/13 08:45:51, Marc Treib wrote: > Why? > Also: On the initial load, you're trying to remove an observer that isn't there. > (Probably not really a problem, but not nice either.) > And you never remove the observer when this page is closed. That might cause > use-after-free crashes. Done. On page reload, nothing is destroyed, HandleLoaded is called again on the same object. I added a check into NTPSnippetsService::AddObserver so that we do not do it twice. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:136: base::StringPiece hosts_piece(hosts_string); On 2016/04/13 08:45:51, Marc Treib wrote: > I think this conversion happens automatically, i.e. you can just pass > hosts_string to SplitString. Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:214: "chrome.snippets_internals.receiveHostsInput", hosts_value); On 2016/04/13 08:45:51, Marc Treib wrote: > Any particular reason for sending this separately? Should be trivial to just > build the "imploded" string in js, right? Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:219: const std::string string_value(value ? "True" : "False"); On 2016/04/13 08:45:51, Marc Treib wrote: > Might as well just inline this below. Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.h (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.h:11: #include "base/memory/scoped_ptr.h" On 2016/04/13 08:45:51, Marc Treib wrote: > We can use std::unique_ptr now (from <memory>)! But this include doesn't > actually seem to be required. Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.h:20: // The implementation for the chrome://zine-internals page. On 2016/04/13 08:45:51, Marc Treib wrote: > snippets-internals Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.h:31: void NTPSnippetsServiceShutdown() override; On 2016/04/13 08:45:51, Marc Treib wrote: > These two can be private. It's common practice to add a comment saying where > they're overridden from (as for RegisterMessages below). Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_ui.cc (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_ui.cc:5: #include "snippets_internals_ui.h" On 2016/04/13 08:45:51, Marc Treib wrote: > Full paths in includes, always. Also for the message handler include below. > (Probably also done by that automatic include "cleanup"?) Done. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_ui.h (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_ui.h:11: // The implementation for the chrome://zine-internals page. On 2016/04/13 08:45:52, Marc Treib wrote: > snippets-internals Done. https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (left): https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:8: #include <utility> On 2016/04/13 08:45:52, Marc Treib wrote: > I think these two should stay, for std::find_if and std::move respectively. Done. https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:182: // |suggestions_service_| can be null in tests. On 2016/04/13 08:45:52, Marc Treib wrote: > No point for this null check here anymore - please move it into > GetSuggestionsHosts where suggestions_service_ is actually used Done. https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:211: suggestions_service_->GetSuggestionsDataFromCache()); On 2016/04/13 08:45:52, Marc Treib wrote: > Can you please add a "TODO(treib)" saying that this should just call > GetSnippetHostsFromPrefs? (Add TODO rather than actually change it, because I'm > not sure if that won't break anything, e.g. on first run). Done. https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:226: NTPSnippetsServiceLoaded()); On 2016/04/13 08:45:52, Marc Treib wrote: > misaligned Done. https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:90: const NTPSnippetStorage& GetDiscardedSnippets(); On 2016/04/13 08:45:52, Marc Treib wrote: > Should be const (add "const" just before the semicolon). > Also since this is a trivial getter, it should be called "discarded_snippets" > (yes it's weird and kinda inconsistent, but that's what the style guide > says...), and implemented inline here. Done. https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:96: std::set<std::string> GetSuggestionsHosts(); On 2016/04/13 08:45:52, Marc Treib wrote: > Also const please Done. https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:173: ASSERT_TRUE(LoadFromJSONString(json_str)); On 2016/04/13 08:45:52, Marc Treib wrote: > Heads-up: I changed the setup of these tests slightly in > https://codereview.chromium.org/1878993002/, you'll have to rebase. (e.g. > LoadFromJSONString doesn't have a return value anymore) Done. https://codereview.chromium.org/1883523002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:230: EXPECT_EQ(0u, snippets.size()); On 2016/04/13 08:45:52, Marc Treib wrote: > Ah, this works because you have a reference to the member, which changed in the > meantime... I find this kinda surprising/hard to read, I'd prefer to say > service()->GetDiscardedSnippets().size() here. Done.
https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:48: <td>Validity On 2016/04/14 15:26:59, jkrcal wrote: > On 2016/04/13 08:45:51, Marc Treib wrote: > > <td> tags should be closed. > > Not in HTML5. > https://www.w3.org/TR/2014/REC-html5-20141028/tabular-data.html#the-td-element > > Not that the file conforms to HTML5 (jscontent attributes break it) but it is > the specification I am aiming for. Huh, today I learned! But then please be consistent: a bunch of the td tags above do have closing tags, those should be removed too. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:99: // On page reloads it is necessary to first remove the observer. On 2016/04/14 15:27:00, jkrcal wrote: > On 2016/04/13 08:45:51, Marc Treib wrote: > > Why? > > Also: On the initial load, you're trying to remove an observer that isn't > there. > > (Probably not really a problem, but not nice either.) > > And you never remove the observer when this page is closed. That might cause > > use-after-free crashes. > > Done. > > On page reload, nothing is destroyed, HandleLoaded is called again on the same > object. I added a check into NTPSnippetsService::AddObserver so that we do not > do it twice. IMO this class should handle not adding itself twice. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.css (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.js:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:64: ntp_snippets_service->RemoveObserver(this); You can use base/scoped_observer.h for this kind of thing - it'll take care to unregister only if you registered before. https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:212: return empty; Just return std::set<std::string>(); ? https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:243: if (!observers_.HasObserver(observer)) { This shouldn't be here. It's the responsibility of the other object to make sure it doesn't register itself twice.
Updated based on Marc's comments. (I am sorry for missing the 2015s!) https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.css (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.css:1: /* Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/14 15:43:44, Marc Treib wrote: > 2016 Done. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.js:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/14 15:43:44, Marc Treib wrote: > 2016 Done. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:64: ntp_snippets_service->RemoveObserver(this); On 2016/04/14 15:43:44, Marc Treib wrote: > You can use base/scoped_observer.h for this kind of thing - it'll take care to > unregister only if you registered before. Done. https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:212: return empty; On 2016/04/14 15:43:44, Marc Treib wrote: > Just > return std::set<std::string>(); > ? Done. https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:243: if (!observers_.HasObserver(observer)) { On 2016/04/14 15:43:44, Marc Treib wrote: > This shouldn't be here. It's the responsibility of the other object to make sure > it doesn't register itself twice. Done.
Description was changed from ========== chrome://snippets-internals page for debugging NTP snippets. Displays a list of NTP snippets and discarded snippets with all details (as obtained from the ChromeReader servers). It also shows the list of hosts by the SuggestionService to which the snippets are restricted. The page allows to clear the list of snippets / discarded snippets as well as to fetch new snippets from user-specified hosts. BUG=597566 ========== to ========== chrome://snippets-internals page for debugging NTP snippets. Displays a list of NTP snippets and discarded snippets with all details (as obtained from the ChromeReader servers). It also shows the list of hosts by the SuggestionService to which the snippets are restricted. The page allows to clear the list of snippets / discarded snippets as well as to fetch new snippets from user-specified hosts. BUG=597566 NOPRESUBMIT=true ==========
Generally, it's a good idea not to add multiple reviewers for the same areas of code at once, to avoid duplicate review comments. https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:168: <if expr="is_android"> On 2016/04/14 15:26:59, jkrcal wrote: > On 2016/04/13 08:45:51, Marc Treib wrote: > > Hm, actually almost all of this page should work on desktop too - the only > > Android-specific thing I found is the feature flag. > > Would it make sense to enable this on desktop too, given that we eventually > hope > > to launch on more platforms? > > Acknowledged. Sooo... not done? https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.css (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.css:49: .url { All of your anchors have this class. Why not just apply this style to all `a` directly? https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.css:76: input#submit-clear { #submit-clear an ID, which is supposed to be unique, so you shouldn't need the additional input selector. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:12: maximum-scale=1.0, user-scalable=no"> Indent this line four spaces. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:25: <div class="section" jsskip="true"> What's the jsskip for? https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:44: <span class="snippet-title" jsvalues="myid:id"> "myid" is not very meaningful. "snippet-id" maybe? https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:68: jscontent="faviconUrl"></a> This should be indented four spaces. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:72: <a class="url" jsvalues="href:salientImageUrl" Would it make more sense to show the image inline? https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:87: e.g. "www.wired.com www.bbc.co.uk", You could make this a hint text (placeholder="...") on the input field. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:93: <div id="discarded-snippets" class="section"> This is another disadvantage of having discarded snippets separate: the template for these is duplicated. Could we somehow process these with the same template? (Generally, this would also strike me as a good use case for Polymer, but that would obviously be a bigger change.) https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.js:5: cr.define('chrome.snippets_internals', function() { I would probably call this SnippetsInternals, to make it more like a static class than a namespace. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.js:35: if ($(propertyId)) $(propertyId).textContent = value; In contrast to C++, the Javascript style guide does always require braces for if-statements. And in contrast to Java (but similarly to C++), the body does not go on the same line. Also, why do you even check whether the node exists? It would be a bug to receive a property for a non-existing node. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.js:53: function trigger(event) { I would move this function closer to where it is used, and maybe even inline it? https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.js:56: if ($(id)) $(id).classList.toggle('snippet-hidden'); This would also work nicer with a Polymer object. But in absence of that, instead of going through the global namespace of node IDs, you could just find the element by navigating locally, like this: event.currentTarget.parentElement.getElementsByClassName('foo')[0] https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:100: DCHECK(args->empty()); Try to be consistent about whether to use DCHECK(...->empty()) or DCHECK_EQ(0u, ...->size()). I would do the latter, because it will print the size in case of an error. https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:81: // Deletes all currently stored snippets. Empty line before comment. https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:90: const NTPSnippetStorage& discarded_snippets() const { Somewhat unrelated yak-shaving: Now that we have an accessor for discarded snippets, I personally would like to put the non-discarded snippets behind an accessor as well (both for symmetry, and because I've always thought it was a bit weird to directly iterate over this server). https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:49: <td class="expiry"><span class="date" jscontent="published"> Put the <span> on a new line, as it's a child of the <td>. This is particularly important now that we don't have a closing </td> anymore to indicate the structure. https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:101: if (!observer_.IsObserving(ntp_snippets_service)) { No braces for single-line bodies. But more generally, needing to do this is a smell that something is wrong. Why don't you register as an observer in the constructor? https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.h (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.h:52: ScopedObserver<ntp_snippets::NTPSnippetsService, ntp_snippets::NTPSnippetsServiceObserver> observer_; This needs to be broken to fit into 80 columns.
https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/1883523002/diff/80001/chrome/browser/browser_... chrome/browser/browser_resources.grd:168: <if expr="is_android"> On 2016/04/14 16:50:24, Bernhard Bauer wrote: > On 2016/04/14 15:26:59, jkrcal wrote: > > On 2016/04/13 08:45:51, Marc Treib wrote: > > > Hm, actually almost all of this page should work on desktop too - the only > > > Android-specific thing I found is the feature flag. > > > Would it make sense to enable this on desktop too, given that we eventually > > hope > > > to launch on more platforms? > > > > Acknowledged. > > Sooo... not done? No - we talked about this offline. I'm not completely sure if it would just work on desktop right now, so we decided to leave it as Android-only for now to get it landed as soon as possible, then revisit desktop.
I see Bernhard is OOO. Marc could you take a (hopefully) final look in the meantime, please? https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:49: <td class="expiry"><span class="date" jscontent="published"> On 2016/04/14 16:50:25, Bernhard (OOO until Apr 18) wrote: > Put the <span> on a new line, as it's a child of the <td>. This is particularly > important now that we don't have a closing </td> anymore to indicate the > structure. Done. The style guide https://google.github.io/styleguide/htmlcssguide.xml?showone=General_Formatti... does not require new lines for inline elements. At first reading I understood it that they new lines for inline elements are not allowed. Actually, it is not there and I agree with you it is better readable now. https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:101: if (!observer_.IsObserving(ntp_snippets_service)) { On 2016/04/14 16:50:25, Bernhard (OOO until Apr 18) wrote: > No braces for single-line bodies. > > But more generally, needing to do this is a smell that something is wrong. Why > don't you register as an observer in the constructor? Done (braces). Well, I do not register it in the constructor because - at that time, the page may not be loaded yet, - I need the page to be loaded before I can send it data, - thus, I would need to have some case distinctions in HandleLoaded and NTPSnippetsServiceLoaded() to get the same behaviour. All in all I think this is clearer. Feel free to argue further :) https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.h (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.h:52: ScopedObserver<ntp_snippets::NTPSnippetsService, ntp_snippets::NTPSnippetsServiceObserver> observer_; On 2016/04/14 16:50:25, Bernhard (OOO until Apr 18) wrote: > This needs to be broken to fit into 80 columns. Done. (I wonder why I did not get an error message while uploading. Maybe an oversight of mine.)
Mostly looks good now, thanks! You haven't addressed all of Bernhard's comments yet. You seem to have compiler errors in the tests - try building components_unittests_apk locally. Why is the "NOPRESUBMIT=true" line in the description? That should definitely go away before submitting. https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:90: const NTPSnippetStorage& discarded_snippets() const { On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > Somewhat unrelated yak-shaving: Now that we have an accessor for discarded > snippets, I personally would like to put the non-discarded snippets behind an > accessor as well (both for symmetry, and because I've always thought it was a > bit weird to directly iterate over this server). +1, but IMO this can wait for another CL. https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:8: <head> Why did you remove the head and body tags? Is this also some HTML5 thing? https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.h (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.h:52: ScopedObserver<ntp_snippets::NTPSnippetsService, ntp_snippets::NTPSnippetsServiceObserver> observer_; On 2016/04/15 08:39:54, jkrcal wrote: > On 2016/04/14 16:50:25, Bernhard (OOO until Apr 18) wrote: > > This needs to be broken to fit into 80 columns. > > Done. > > (I wonder why I did not get an error message while uploading. Maybe an oversight > of mine.) No, this seems to be an error in the presubmit hooks. Unless you explicitly bypassed them (git cl upload --nohooks, or something like that), they should have caught this. https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:57: SnippetsInternalsMessageHandler::SnippetsInternalsMessageHandler() : style: the colon goes on the next line https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:101: if (!observer_.IsObserving(ntp_snippets_service)) { nit: no braces Also maybe add a comment that this is to avoid duplicate registration on reloads. https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.h (right): https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.h:43: void SendInitialData(ntp_snippets::NTPSnippetsService* service); optional: You could either store the snippets service in a member, or make a GetSnippetsService method that gets it from the factory. Then you wouldn't have to pass it around everywhere. Up to you though, it's also okay as is. https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.h:51: // For making sure this object is not added twice as an observer. Not really - IMO it's mostly for the automatic unregistration. But this doesn't really need a comment anyway :) https://codereview.chromium.org/1883523002/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1883523002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:210: if (!suggestions_service_) { nit: no braces https://codereview.chromium.org/1883523002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:243: observer->NTPSnippetsServiceLoaded(); Heads-up: dgn's https://codereview.chromium.org/1888913002/ aims to remove this line, i.e. not notify the observer anymore when it's added. Not sure if that will break anything for the internals page.
I am sorry for missing some of the comments from Bernhard! (I checked only the comments in the last path set.) I hope to have addressed all of it by now. I am afraid the "NOPRESUBMIT=true" line has to stay to suppress a bogus presubmit warning caused by using an arrow function in js https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Functions/Arro... See crbug.com/567770. Of course, I can easily avoid using the arrow function (but I like it!). If you insist, I'll remove it and the NOPRESUBMIT flag. Marc, could you please take a look on more time? https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.css (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.css:49: .url { On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > All of your anchors have this class. Why not just apply this style to all `a` > directly? At this moment, I do not have any other links. In some previous iterations, I had some. There might be some in the future. To me, this is quite a non-standard style for an `a`. The url class adds semantics to the code. Therefore, I'd prefer to keep this style. I am willing to change it if you insist. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.css:76: input#submit-clear { On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > #submit-clear an ID, which is supposed to be unique, so you shouldn't need the > additional input selector. Done. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:12: maximum-scale=1.0, user-scalable=no"> On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > Indent this line four spaces. Done. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:25: <div class="section" jsskip="true"> On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > What's the jsskip for? It informs the jstemplate library that it can skip this element with its whole subtree while scanning the DOM tree. (an optimization that is unnecessary with this file size but I'd like to keep it for the matter of consistency across the code base) https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:44: <span class="snippet-title" jsvalues="myid:id"> On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > "myid" is not very meaningful. "snippet-id" maybe? Done. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:68: jscontent="faviconUrl"></a> On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > This should be indented four spaces. Done. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:72: <a class="url" jsvalues="href:salientImageUrl" On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > Would it make more sense to show the image inline? I think for debugging it is more important to see the URL. If you want to check that for a particular snippet, the image exists, you can always click the link. I'd prefer to tune it after users' feedback. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:87: e.g. "www.wired.com www.bbc.co.uk", On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > You could make this a hint text (placeholder="...") on the input field. Done. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:93: <div id="discarded-snippets" class="section"> On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > This is another disadvantage of having discarded snippets separate: the template > for these is duplicated. Could we somehow process these with the same template? > > (Generally, this would also strike me as a good use case for Polymer, but that > would obviously be a bigger change.) I was not aware of Polymer, just used the same library that has been used in a similar internals page. I am not aware of a way how to have just one html template here with jstemplate. Even though it is quite a pain to keep the two places consistent (and constantly forget about some change), I would prefer to keep it as it is for this CL. I am also fine with switching to Polymer as an additional exercise in an additional CL. Do you think it is worth it? https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.js:5: cr.define('chrome.snippets_internals', function() { On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > I would probably call this SnippetsInternals, to make it more like a static > class than a namespace. Done. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.js:35: if ($(propertyId)) $(propertyId).textContent = value; On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > In contrast to C++, the Javascript style guide does always require braces for > if-statements. And in contrast to Java (but similarly to C++), the body does not > go on the same line. > > Also, why do you even check whether the node exists? It would be a bug to > receive a property for a non-existing node. Done (removed the check). https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.js:53: function trigger(event) { On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > I would move this function closer to where it is used, and maybe even inline it? I have moved it inside the displayList. I have not inlined it to be consistent with all the listeners above. If you think it is really worth to move all the listeners inline, let me know. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.js:56: if ($(id)) $(id).classList.toggle('snippet-hidden'); On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > This would also work nicer with a Polymer object. But in absence of that, > instead of going through the global namespace of node IDs, you could just find > the element by navigating locally, like this: > > event.currentTarget.parentElement.getElementsByClassName('foo')[0] I am not sure about this suggestion. To me the code is harder to read and more error-prone when changing the html. Is it some common policy to avoid using ids for data elements? https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:100: DCHECK(args->empty()); On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > Try to be consistent about whether to use DCHECK(...->empty()) or DCHECK_EQ(0u, > ...->size()). I would do the latter, because it will print the size in case of > an error. Done. https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:81: // Deletes all currently stored snippets. On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > Empty line before comment. Done. https://codereview.chromium.org/1883523002/diff/120001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.h:90: const NTPSnippetStorage& discarded_snippets() const { On 2016/04/15 09:00:30, Marc Treib wrote: > On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > > Somewhat unrelated yak-shaving: Now that we have an accessor for discarded > > snippets, I personally would like to put the non-discarded snippets behind an > > accessor as well (both for symmetry, and because I've always thought it was a > > bit weird to directly iterate over this server). > > +1, but IMO this can wait for another CL. I agree, I really did not like the asymmetry when writing the code. Can it wait for another CL? https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:8: <head> On 2016/04/15 09:00:30, Marc Treib wrote: > Why did you remove the head and body tags? Is this also some HTML5 thing? Yes, just trying to be more consistent with the policy of omitting unnecessary tags. https://google.github.io/styleguide/htmlcssguide.xml?showone=Optional_Tags#Op... https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:101: if (!observer_.IsObserving(ntp_snippets_service)) { On 2016/04/15 08:39:54, jkrcal wrote: > On 2016/04/14 16:50:25, Bernhard (OOO until Apr 18) wrote: > > No braces for single-line bodies. > > > > But more generally, needing to do this is a smell that something is wrong. Why > > don't you register as an observer in the constructor? > > Done (braces). > > Well, I do not register it in the constructor because > - at that time, the page may not be loaded yet, > - I need the page to be loaded before I can send it data, > - thus, I would need to have some case distinctions in HandleLoaded and > NTPSnippetsServiceLoaded() to get the same behaviour. > > All in all I think this is clearer. Feel free to argue further :) By introducing a ntp_snippets_service_ member, your suggestion fits well. Unfortunately, running the factory code to get the service in the constructor causes a crash :| Thus, I placed it back to the HandleLoaded function. https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:57: SnippetsInternalsMessageHandler::SnippetsInternalsMessageHandler() : On 2016/04/15 09:00:30, Marc Treib wrote: > style: the colon goes on the next line Done. https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:101: if (!observer_.IsObserving(ntp_snippets_service)) { On 2016/04/15 09:00:30, Marc Treib wrote: > nit: no braces > Also maybe add a comment that this is to avoid duplicate registration on > reloads. Acknowledged. (the code changed, I think it is self-explanatory now) https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.h (right): https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.h:43: void SendInitialData(ntp_snippets::NTPSnippetsService* service); On 2016/04/15 09:00:30, Marc Treib wrote: > optional: You could either store the snippets service in a member, or make a > GetSnippetsService method that gets it from the factory. Then you wouldn't have > to pass it around everywhere. Up to you though, it's also okay as is. Done. I store it in a member. https://codereview.chromium.org/1883523002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.h:51: // For making sure this object is not added twice as an observer. On 2016/04/15 09:00:30, Marc Treib wrote: > Not really - IMO it's mostly for the automatic unregistration. But this doesn't > really need a comment anyway :) Done. https://codereview.chromium.org/1883523002/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1883523002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:210: if (!suggestions_service_) { On 2016/04/15 09:00:30, Marc Treib wrote: > nit: no braces Done. https://codereview.chromium.org/1883523002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service.cc:243: observer->NTPSnippetsServiceLoaded(); On 2016/04/15 09:00:30, Marc Treib wrote: > Heads-up: dgn's https://codereview.chromium.org/1888913002/ aims to remove this > line, i.e. not notify the observer anymore when it's added. Not sure if that > will break anything for the internals page. Thanks, I need to fix it!
The CQ bit was checked by jkrcal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883523002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883523002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, LGTM! Alright, the NOPRESUBMIT is okay in that case, but please add a short explanation to the CL description. https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:93: <div id="discarded-snippets" class="section"> On 2016/04/15 14:11:50, jkrcal wrote: > On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > > This is another disadvantage of having discarded snippets separate: the > template > > for these is duplicated. Could we somehow process these with the same > template? > > > > (Generally, this would also strike me as a good use case for Polymer, but that > > would obviously be a bigger change.) > > I was not aware of Polymer, just used the same library that has been used in a > similar internals page. > I am not aware of a way how to have just one html template here with jstemplate. > > Even though it is quite a pain to keep the two places consistent (and constantly > forget about some change), I would prefer to keep it as it is for this CL. > > I am also fine with switching to Polymer as an additional exercise in an > additional CL. Do you think it is worth it? Nah, IMO it's not worth switching. Maybe the next time we create an internals page, we could do it using Polymer from the beginning. (Caveat: I have never used Polymer, so I basically have no idea what I'm talking about here :D) https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:101: if (!observer_.IsObserving(ntp_snippets_service)) { On 2016/04/15 14:11:50, jkrcal wrote: > On 2016/04/15 08:39:54, jkrcal wrote: > > On 2016/04/14 16:50:25, Bernhard (OOO until Apr 18) wrote: > > > No braces for single-line bodies. > > > > > > But more generally, needing to do this is a smell that something is wrong. > Why > > > don't you register as an observer in the constructor? > > > > Done (braces). > > > > Well, I do not register it in the constructor because > > - at that time, the page may not be loaded yet, > > - I need the page to be loaded before I can send it data, > > - thus, I would need to have some case distinctions in HandleLoaded and > > NTPSnippetsServiceLoaded() to get the same behaviour. > > > > All in all I think this is clearer. Feel free to argue further :) > > By introducing a ntp_snippets_service_ member, your suggestion fits well. > Unfortunately, running the factory code to get the service in the constructor > causes a crash :| Because web_ui() is null? Seems that is only valid when RegisterMessages gets called: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... But IMO this is okay as-is. > Thus, I placed it back to the HandleLoaded function. https://codereview.chromium.org/1883523002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:60: ntp_snippets_service_(NULL) {} nit: nullptr please https://codereview.chromium.org/1883523002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:67: if (dom_loaded_){ nit: space before brace Or, alternatively, if (dom_loaded_) return; https://codereview.chromium.org/1883523002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:99: if (ntp_snippets_service_ == NULL) { nit: if (!ntp_snippets_service_)
Description was changed from ========== chrome://snippets-internals page for debugging NTP snippets. Displays a list of NTP snippets and discarded snippets with all details (as obtained from the ChromeReader servers). It also shows the list of hosts by the SuggestionService to which the snippets are restricted. The page allows to clear the list of snippets / discarded snippets as well as to fetch new snippets from user-specified hosts. BUG=597566 NOPRESUBMIT=true ========== to ========== chrome://snippets-internals page for debugging NTP snippets. Displays a list of NTP snippets and discarded snippets with all details (as obtained from the ChromeReader servers). It also shows the list of hosts by the SuggestionService to which the snippets are restricted. The page allows to clear the list of snippets / discarded snippets as well as to fetch new snippets from user-specified hosts. BUG=597566 NOPRESUBMIT=true (the presubmit check fails on javascript arrow function, see https://bugs.chromium.org/p/chromium/issues/detail?id=567770#c8) ==========
Thanks, Marc, for the comments. I still hope for your lgtm, Bernhard! :) https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:101: if (!observer_.IsObserving(ntp_snippets_service)) { On 2016/04/15 15:51:32, Marc Treib wrote: > On 2016/04/15 14:11:50, jkrcal wrote: > > On 2016/04/15 08:39:54, jkrcal wrote: > > > On 2016/04/14 16:50:25, Bernhard (OOO until Apr 18) wrote: > > > > No braces for single-line bodies. > > > > > > > > But more generally, needing to do this is a smell that something is wrong. > > Why > > > > don't you register as an observer in the constructor? > > > > > > Done (braces). > > > > > > Well, I do not register it in the constructor because > > > - at that time, the page may not be loaded yet, > > > - I need the page to be loaded before I can send it data, > > > - thus, I would need to have some case distinctions in HandleLoaded and > > > NTPSnippetsServiceLoaded() to get the same behaviour. > > > > > > All in all I think this is clearer. Feel free to argue further :) > > > > By introducing a ntp_snippets_service_ member, your suggestion fits well. > > Unfortunately, running the factory code to get the service in the constructor > > causes a crash :| > > Because web_ui() is null? Seems that is only valid when RegisterMessages gets > called: > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > But IMO this is okay as-is. > > > Thus, I placed it back to the HandleLoaded function. > Good point. I have moved it into the RegisterMessages function. https://codereview.chromium.org/1883523002/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/1883523002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:60: ntp_snippets_service_(NULL) {} On 2016/04/15 15:51:32, Marc Treib wrote: > nit: nullptr please Done. https://codereview.chromium.org/1883523002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:67: if (dom_loaded_){ On 2016/04/15 15:51:32, Marc Treib wrote: > nit: space before brace > Or, alternatively, > if (dom_loaded_) > return; Done. https://codereview.chromium.org/1883523002/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/snippets_internals_message_handler.cc:99: if (ntp_snippets_service_ == NULL) { On 2016/04/15 15:51:32, Marc Treib wrote: > nit: if (!ntp_snippets_service_) Done.
Sorry, just some small things left: https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:25: <div class="section" jsskip="true"> On 2016/04/15 14:11:50, jkrcal wrote: > On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > > What's the jsskip for? > > It informs the jstemplate library that it can skip this element with its whole > subtree while scanning the DOM tree. > (an optimization that is unnecessary with this file size but I'd like to keep > it for the matter of consistency across the code base) Eh, it strikes me a bit as premature optimization :) https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:119: jscontent="faviconUrl"></a> This should be indented four more spaces (it's part of the opening <a>).
Thanks, Bernhard, for the comments. Do you see anything more to fix? https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:25: <div class="section" jsskip="true"> On 2016/04/18 14:01:34, Bernhard Bauer wrote: > On 2016/04/15 14:11:50, jkrcal wrote: > > On 2016/04/14 16:50:24, Bernhard (OOO until Apr 18) wrote: > > > What's the jsskip for? > > > > It informs the jstemplate library that it can skip this element with its whole > > subtree while scanning the DOM tree. > > (an optimization that is unnecessary with this file size but I'd like to keep > > it for the matter of consistency across the code base) > > Eh, it strikes me a bit as premature optimization :) Done (removed). https://codereview.chromium.org/1883523002/diff/120001/chrome/browser/resourc... chrome/browser/resources/snippets_internals.html:119: jscontent="faviconUrl"></a> On 2016/04/18 14:01:34, Bernhard Bauer wrote: > This should be indented four more spaces (it's part of the opening <a>). This is not the last version of the file, it has changed since (and has proper spacing).
LGTM, thanks!
The CQ bit was checked by jkrcal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/1883523002/#ps220001 (title: "Implementing last suggestions from Bernhard")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1883523002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1883523002/220001
Message was sent while issue was closed.
Description was changed from ========== chrome://snippets-internals page for debugging NTP snippets. Displays a list of NTP snippets and discarded snippets with all details (as obtained from the ChromeReader servers). It also shows the list of hosts by the SuggestionService to which the snippets are restricted. The page allows to clear the list of snippets / discarded snippets as well as to fetch new snippets from user-specified hosts. BUG=597566 NOPRESUBMIT=true (the presubmit check fails on javascript arrow function, see https://bugs.chromium.org/p/chromium/issues/detail?id=567770#c8) ========== to ========== chrome://snippets-internals page for debugging NTP snippets. Displays a list of NTP snippets and discarded snippets with all details (as obtained from the ChromeReader servers). It also shows the list of hosts by the SuggestionService to which the snippets are restricted. The page allows to clear the list of snippets / discarded snippets as well as to fetch new snippets from user-specified hosts. BUG=597566 NOPRESUBMIT=true (the presubmit check fails on javascript arrow function, see https://bugs.chromium.org/p/chromium/issues/detail?id=567770#c8) ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== chrome://snippets-internals page for debugging NTP snippets. Displays a list of NTP snippets and discarded snippets with all details (as obtained from the ChromeReader servers). It also shows the list of hosts by the SuggestionService to which the snippets are restricted. The page allows to clear the list of snippets / discarded snippets as well as to fetch new snippets from user-specified hosts. BUG=597566 NOPRESUBMIT=true (the presubmit check fails on javascript arrow function, see https://bugs.chromium.org/p/chromium/issues/detail?id=567770#c8) ========== to ========== chrome://snippets-internals page for debugging NTP snippets. Displays a list of NTP snippets and discarded snippets with all details (as obtained from the ChromeReader servers). It also shows the list of hosts by the SuggestionService to which the snippets are restricted. The page allows to clear the list of snippets / discarded snippets as well as to fetch new snippets from user-specified hosts. BUG=597566 NOPRESUBMIT=true (the presubmit check fails on javascript arrow function, see https://bugs.chromium.org/p/chromium/issues/detail?id=567770#c8) Committed: https://crrev.com/3ccb46d44ee5a1a3a5cee851792fb01b8ef56749 Cr-Commit-Position: refs/heads/master@{#387919} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/3ccb46d44ee5a1a3a5cee851792fb01b8ef56749 Cr-Commit-Position: refs/heads/master@{#387919} |
