Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2145563002:
Add ContentSuggestionsService to SnippetsInternals (Closed)
|
Created:
4 years, 5 months ago by Philipp Keck Modified:
4 years, 5 months ago CC:
chromium-reviews, arv+watch_chromium.org, ntp-dev+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@articleprovider2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ContentSuggestionsService to SnippetsInternals
The ContentSuggestionsService retrieves suggestions from the
NTPSnippetsService as a provider. The latter currently delivers its
snippets to the chrome://snippets-internals/ page for debugging.
Before the NTP UI will be changed to retrieve suggestions from the
ContentSuggestionsService instead of directly from the
NTPSnippetsService, this adds the output of the
ContentSuggestionsService to the snippets-internals page to allow
debugging both and compare the data converted data to the original.
This also adds controls for clearing cached/discarded suggestions.
BUG=619560
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/f397acb853c3eaf3791e43ecedd22610d954a9d9
Cr-Commit-Position: refs/heads/master@{#405149}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Bernhard's and Marc's comments #
Total comments: 8
Patch Set 3 : Bernhard's comments #Patch Set 4 : Rebase on 2131943002/#ps260001 #Patch Set 5 : Changed switch statements #Patch Set 6 : Rebased with automerge (this should be the same as the previous patch set) #Patch Set 7 : Refactor NTPSnippetsService::UpdateStateForStatus switch statement #
Dependent Patchsets: Messages
Total messages: 21 (6 generated)
Description was changed from ========== Add ContentSuggestionsService to SnippetsInternals The ContentSuggestionsService retrieves suggestions from the NTPSnippetsService as a provider. The latter currently delivers its snippets to the chrome://snippets-internals/ page for debugging. Before the NTP UI will be changed to retrieve suggestions from the ContentSuggestionsService instead of directly from the NTPSnippetsService, this adds the output of the ContentSuggestionsService to the snippets-internals page to allow debugging both and compare the data converted data to the original. This also adds controls for clearing cached/discarded suggestions. BUG=619560 ========== to ========== Add ContentSuggestionsService to SnippetsInternals The ContentSuggestionsService retrieves suggestions from the NTPSnippetsService as a provider. The latter currently delivers its snippets to the chrome://snippets-internals/ page for debugging. Before the NTP UI will be changed to retrieve suggestions from the ContentSuggestionsService instead of directly from the NTPSnippetsService, this adds the output of the ContentSuggestionsService to the snippets-internals page to allow debugging both and compare the data converted data to the original. This also adds controls for clearing cached/discarded suggestions. BUG=619560 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
pke@google.com changed reviewers: + bauerb@chromium.org, treib@chromium.org
PTAL
treib@chromium.org changed reviewers: + jkrcal@chromium.org
A few nits; mostly LG. +jkrcal who wrote most of this, and should know better :) https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... chrome/browser/resources/snippets_internals.js:130: link.onclick = function(event) { Forgive my ignorance, but is there any difference between onclick and addEventListener('click' ? https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:61: const ntp_snippets::ContentSuggestion& suggestion, nit: ntp_snippets:: not needed, you have a "using" above https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:146: void SnippetsInternalsMessageHandler::OnCategoryStatusChanged( nit: newlines between methods please
https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... chrome/browser/resources/snippets_internals.html:171: jsvalues="hidden-id:id"> Nit: don't align HTML attributes; just indent them four spaces. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... chrome/browser/resources/snippets_internals.js:130: link.onclick = function(event) { I would either use addEventListener everywhere or onclick everywhere. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:63: std::unique_ptr<base::DictionaryValue> entry(new base::DictionaryValue); You might be able to do `auto entry = base::MakeUnique<base::DictionaryValue>`? https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:80: default: If you remove the default case from the switch statement, the compiler will warn (which is then turned into an error) if you forget a value, which might come in handy when you add more categories. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:109: default: Same here. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:110: return "Unknown status: " + base::IntToString(int(status)); Should this be a static_cast<int>? The style guide prefers those over C-style casts, but then again this isn't really a cast at all... If static_cast<int> isn't outright wrong here, I would go with that, I think, just for sake of being consistent with the majority of Chrome code.
https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... chrome/browser/resources/snippets_internals.html:171: jsvalues="hidden-id:id"> On 2016/07/12 13:09:45, Bernhard Bauer wrote: > Nit: don't align HTML attributes; just indent them four spaces. Done. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... chrome/browser/resources/snippets_internals.js:130: link.onclick = function(event) { On 2016/07/12 13:09:45, Bernhard Bauer wrote: > I would either use addEventListener everywhere or onclick everywhere. onclick allows for one, removal by ".onclick = null;" whereas addEventListener can add multiple and remove them with removeEventListener. However, to remove you still need the reference to the function you added previously and there is no "remove all". Removing/Overwriting is important here because otherwise it is executed twice after a reload (and three times after the next), causing the thing to open and immediately close again. Don't know why that wasn't an issue before? The same problem doesn't occur with the addEventListeners in initialize() above, but the idea there is also more of the "onclick" than the "addEventListener" kind, so I could change it there, too. However, the addEventListener below can't be changed because there is no equivalent onxyz event, afaik. To be consistent with existing code, I just make the old lambda a named function, then addEventListener will avoid registering it twice automatically. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:61: const ntp_snippets::ContentSuggestion& suggestion, On 2016/07/12 13:04:52, Marc Treib wrote: > nit: ntp_snippets:: not needed, you have a "using" above Done. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:63: std::unique_ptr<base::DictionaryValue> entry(new base::DictionaryValue); On 2016/07/12 13:09:45, Bernhard Bauer wrote: > You might be able to do `auto entry = base::MakeUnique<base::DictionaryValue>`? Done. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:80: default: On 2016/07/12 13:09:45, Bernhard Bauer wrote: > If you remove the default case from the switch statement, the compiler will warn > (which is then turned into an error) if you forget a value, which might come in > handy when you add more categories. Done. Oh great. I didn't know it would warn and that's exactly why I added the default in the first place (even though it's currently dead code). So I can remove it here, too, I assume: https://codereview.chromium.org/2131943002/diff/200001/components/ntp_snippet... And now there is a case for the "COUNT" dummy value... https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:109: default: On 2016/07/12 13:09:45, Bernhard Bauer wrote: > Same here. Done. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:110: return "Unknown status: " + base::IntToString(int(status)); On 2016/07/12 13:09:45, Bernhard Bauer wrote: > Should this be a static_cast<int>? The style guide prefers those over C-style > casts, but then again this isn't really a cast at all... If static_cast<int> > isn't outright wrong here, I would go with that, I think, just for sake of being > consistent with the majority of Chrome code. I delete this line anyway, but I use `int(...)` and `SomeEnumsName(someInt)` elsewhere, so I could change those for consistency. See also the discussion here: https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:146: void SnippetsInternalsMessageHandler::OnCategoryStatusChanged( On 2016/07/12 13:04:53, Marc Treib wrote: > nit: newlines between methods please Done.
https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... chrome/browser/resources/snippets_internals.js:130: link.onclick = function(event) { On 2016/07/12 13:45:02, Philipp Keck wrote: > On 2016/07/12 13:09:45, Bernhard Bauer wrote: > > I would either use addEventListener everywhere or onclick everywhere. > > onclick allows for one, removal by ".onclick = null;" whereas addEventListener > can add multiple and remove them with removeEventListener. However, to remove > you still need the reference to the function you added previously and there is > no "remove all". Removing/Overwriting is important here because otherwise it is > executed twice after a reload (and three times after the next), causing the > thing to open and immediately close again. Don't know why that wasn't an issue > before? I'm moderately sure that it *was* actually an issue before - "sometimes" the hide/unhide toggle wouldn't work, I think nobody ever investigated why :D > The same problem doesn't occur with the addEventListeners in initialize() above, > but the idea there is also more of the "onclick" than the "addEventListener" > kind, so I could change it there, too. > > However, the addEventListener below can't be changed because there is no > equivalent onxyz event, afaik. > > To be consistent with existing code, I just make the old lambda a named > function, then addEventListener will avoid registering it twice automatically. SGTM
https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:80: default: On 2016/07/12 13:45:02, Philipp Keck wrote: > On 2016/07/12 13:09:45, Bernhard Bauer wrote: > > If you remove the default case from the switch statement, the compiler will > warn > > (which is then turned into an error) if you forget a value, which might come > in > > handy when you add more categories. > > Done. Oh great. I didn't know it would warn and that's exactly why I added the > default in the first place (even though it's currently dead code). > > So I can remove it here, too, I assume: > https://codereview.chromium.org/2131943002/diff/200001/components/ntp_snippet... Indeed! > And now there is a case for the "COUNT" dummy value... Yup, that's one of the disadvantages, but reeallly, the COUNT value shouldn't be part of the enum... :) I think you'll have to move the old default handling code outside of the switch now though (you can keep the section for COUNT empty), otherwise the compiler will complain that you're missing a return statement. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:110: return "Unknown status: " + base::IntToString(int(status)); On 2016/07/12 13:45:02, Philipp Keck wrote: > On 2016/07/12 13:09:45, Bernhard Bauer wrote: > > Should this be a static_cast<int>? The style guide prefers those over C-style > > casts, but then again this isn't really a cast at all... If static_cast<int> > > isn't outright wrong here, I would go with that, I think, just for sake of > being > > consistent with the majority of Chrome code. > > I delete this line anyway, but I use `int(...)` and `SomeEnumsName(someInt)` > elsewhere, so I could change those for consistency. > > See also the discussion here: > https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... Hm. I do prefer static_cast<>, but I don't want to reopen that discussion either, so I would go with consistency and either change the other places, or leave things as they are. https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:172: <span jscontent="title"></span>>></span> Move the last closing tag to a new line so it align the with opening one? https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:83: return ""; Nit: Use an empty std::string() constructor instead of a literal here to save some instructions.
https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:80: default: On 2016/07/12 14:44:51, Bernhard Bauer wrote: > On 2016/07/12 13:45:02, Philipp Keck wrote: > > On 2016/07/12 13:09:45, Bernhard Bauer wrote: > > > If you remove the default case from the switch statement, the compiler will > > warn > > > (which is then turned into an error) if you forget a value, which might come > > in > > > handy when you add more categories. > > > > Done. Oh great. I didn't know it would warn and that's exactly why I added the > > default in the first place (even though it's currently dead code). > > > > So I can remove it here, too, I assume: > > > https://codereview.chromium.org/2131943002/diff/200001/components/ntp_snippet... > > Indeed! Done. (other CL) > > And now there is a case for the "COUNT" dummy value... > > Yup, that's one of the disadvantages, but reeallly, the COUNT value shouldn't be > part of the enum... :) > > I think you'll have to move the old default handling code outside of the switch > now though (you can keep the section for COUNT empty), otherwise the compiler > will complain that you're missing a return statement. If the COUNT value wasn't there, how would you check whether a given "int" is a valid value for the enum? (Not sure if the empty COUNT section with the return statement after the switch is more desirable than the current code -- but not having a COUNT section at all would be nice.) https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:110: return "Unknown status: " + base::IntToString(int(status)); On 2016/07/12 14:44:51, Bernhard Bauer wrote: > On 2016/07/12 13:45:02, Philipp Keck wrote: > > On 2016/07/12 13:09:45, Bernhard Bauer wrote: > > > Should this be a static_cast<int>? The style guide prefers those over > C-style > > > casts, but then again this isn't really a cast at all... If static_cast<int> > > > isn't outright wrong here, I would go with that, I think, just for sake of > > being > > > consistent with the majority of Chrome code. > > > > I delete this line anyway, but I use `int(...)` and `SomeEnumsName(someInt)` > > elsewhere, so I could change those for consistency. > > > > See also the discussion here: > > > https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... > > Hm. I do prefer static_cast<>, but I don't want to reopen that discussion > either, so I would go with consistency and either change the other places, or > leave things as they are. Done. Actually the style guide is quite clear on this, I changed it to static_cast<> on the other CL (the C-style cast was deleted here anyway). https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:172: <span jscontent="title"></span>>></span> On 2016/07/12 14:44:51, Bernhard Bauer wrote: > Move the last closing tag to a new line so it align the with opening one? Good idea. I get confused by this HTML style where you leave out some closing tags. Why does Chromium do that? https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/snippets_internals_message_handler.cc:83: return ""; On 2016/07/12 14:44:51, Bernhard Bauer wrote: > Nit: Use an empty std::string() constructor instead of a literal here to save > some instructions. Done.
LGTM when it builds. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:80: default: On 2016/07/12 15:19:56, Philipp Keck wrote: > On 2016/07/12 14:44:51, Bernhard Bauer wrote: > > On 2016/07/12 13:45:02, Philipp Keck wrote: > > > On 2016/07/12 13:09:45, Bernhard Bauer wrote: > > > > If you remove the default case from the switch statement, the compiler > will > > > warn > > > > (which is then turned into an error) if you forget a value, which might > come > > > in > > > > handy when you add more categories. > > > > > > Done. Oh great. I didn't know it would warn and that's exactly why I added > the > > > default in the first place (even though it's currently dead code). > > > > > > So I can remove it here, too, I assume: > > > > > > https://codereview.chromium.org/2131943002/diff/200001/components/ntp_snippet... > > > > Indeed! > > Done. (other CL) > > > > And now there is a case for the "COUNT" dummy value... > > > > Yup, that's one of the disadvantages, but reeallly, the COUNT value shouldn't > be > > part of the enum... :) > > > > I think you'll have to move the old default handling code outside of the > switch > > now though (you can keep the section for COUNT empty), otherwise the compiler > > will complain that you're missing a return statement. > > If the COUNT value wasn't there, how would you check whether a given "int" is a > valid value for the enum? You could have an int (not an enum) that represents the maximum value (or the maximum + 1, or something). I don't know of a way at the moment though to make that work automatically, so my comment was more hypothetical. Sorry for not making that clear! > (Not sure if the empty COUNT section with the return statement after the switch > is more desirable than the current code -- but not having a COUNT section at all > would be nice.) https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:172: <span jscontent="title"></span>>></span> On 2016/07/12 15:19:56, Philipp Keck wrote: > On 2016/07/12 14:44:51, Bernhard Bauer wrote: > > Move the last closing tag to a new line so it align the with opening one? > > Good idea. I get confused by this HTML style where you leave out some closing > tags. Why does Chromium do that? Because HTML5 doesn't require closing tags for certain kinds of tags, so this way we minimize the HTML. This is rather new though -- I think jkrcal@ was the one who introduced that style.
https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:80: default: On 2016/07/12 15:50:31, Bernhard Bauer wrote: > On 2016/07/12 15:19:56, Philipp Keck wrote: > > On 2016/07/12 14:44:51, Bernhard Bauer wrote: > > > On 2016/07/12 13:45:02, Philipp Keck wrote: > > > > On 2016/07/12 13:09:45, Bernhard Bauer wrote: > > > > > If you remove the default case from the switch statement, the compiler > > will > > > > warn > > > > > (which is then turned into an error) if you forget a value, which might > > come > > > > in > > > > > handy when you add more categories. > > > > > > > > Done. Oh great. I didn't know it would warn and that's exactly why I added > > the > > > > default in the first place (even though it's currently dead code). > > > > > > > > So I can remove it here, too, I assume: > > > > > > > > > > https://codereview.chromium.org/2131943002/diff/200001/components/ntp_snippet... > > > > > > Indeed! > > > > Done. (other CL) > > > > > > And now there is a case for the "COUNT" dummy value... > > > > > > Yup, that's one of the disadvantages, but reeallly, the COUNT value > shouldn't > > be > > > part of the enum... :) > > > > > > I think you'll have to move the old default handling code outside of the > > switch > > > now though (you can keep the section for COUNT empty), otherwise the > compiler > > > will complain that you're missing a return statement. > > > > If the COUNT value wasn't there, how would you check whether a given "int" is > a > > valid value for the enum? > > You could have an int (not an enum) that represents the maximum value (or the > maximum + 1, or something). I don't know of a way at the moment though to make > that work automatically, so my comment was more hypothetical. Sorry for not > making that clear! > > > (Not sure if the empty COUNT section with the return statement after the > switch > > is more desirable than the current code -- but not having a COUNT section at > all > > would be nice.) > You'd always have to remember to update that int, though, when you change the contents of the enum. In my opinion, C++ should support sth like that out of the box, at least with their "enum class" ... The only solutions without compromises that I saw on the web use macros. I'll leave the code here as is (not as seen here, but there: https://codereview.chromium.org/2145563002/diff/40001/chrome/browser/ui/webui...) https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:172: <span jscontent="title"></span>>></span> On 2016/07/12 15:50:31, Bernhard Bauer wrote: > On 2016/07/12 15:19:56, Philipp Keck wrote: > > On 2016/07/12 14:44:51, Bernhard Bauer wrote: > > > Move the last closing tag to a new line so it align the with opening one? > > > > Good idea. I get confused by this HTML style where you leave out some closing > > tags. Why does Chromium do that? > > Because HTML5 doesn't require closing tags for certain kinds of tags, so this > way we minimize the HTML. This is rather new though -- I think jkrcal@ was the > one who introduced that style. So minimize HTML to reduce the binary size? Removing unnecessary whitespace and quotes could also be a good idea, but wouldn't you rather have a compiler do all of that and write the code in the way you like to read it?
https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... File chrome/browser/ui/webui/snippets_internals_message_handler.cc (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/ui/webui/sni... chrome/browser/ui/webui/snippets_internals_message_handler.cc:80: default: On 2016/07/12 16:05:36, Philipp Keck wrote: > On 2016/07/12 15:50:31, Bernhard Bauer wrote: > > On 2016/07/12 15:19:56, Philipp Keck wrote: > > > On 2016/07/12 14:44:51, Bernhard Bauer wrote: > > > > On 2016/07/12 13:45:02, Philipp Keck wrote: > > > > > On 2016/07/12 13:09:45, Bernhard Bauer wrote: > > > > > > If you remove the default case from the switch statement, the compiler > > > will > > > > > warn > > > > > > (which is then turned into an error) if you forget a value, which > might > > > come > > > > > in > > > > > > handy when you add more categories. > > > > > > > > > > Done. Oh great. I didn't know it would warn and that's exactly why I > added > > > the > > > > > default in the first place (even though it's currently dead code). > > > > > > > > > > So I can remove it here, too, I assume: > > > > > > > > > > > > > > > https://codereview.chromium.org/2131943002/diff/200001/components/ntp_snippet... > > > > > > > > Indeed! > > > > > > Done. (other CL) > > > > > > > > And now there is a case for the "COUNT" dummy value... > > > > > > > > Yup, that's one of the disadvantages, but reeallly, the COUNT value > > shouldn't > > > be > > > > part of the enum... :) > > > > > > > > I think you'll have to move the old default handling code outside of the > > > switch > > > > now though (you can keep the section for COUNT empty), otherwise the > > compiler > > > > will complain that you're missing a return statement. > > > > > > If the COUNT value wasn't there, how would you check whether a given "int" > is > > a > > > valid value for the enum? > > > > You could have an int (not an enum) that represents the maximum value (or the > > maximum + 1, or something). I don't know of a way at the moment though to make > > that work automatically, so my comment was more hypothetical. Sorry for not > > making that clear! > > > > > (Not sure if the empty COUNT section with the return statement after the > > switch > > > is more desirable than the current code -- but not having a COUNT section at > > all > > > would be nice.) > > > > You'd always have to remember to update that int, though, when you change the > contents of the enum. In my opinion, C++ should support sth like that out of the > box, at least with their "enum class" ... > The only solutions without compromises that I saw on the web use macros. Yes, precisely. > I'll leave the code here as is (not as seen here, but there: > https://codereview.chromium.org/2145563002/diff/40001/chrome/browser/ui/webui...) https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:172: <span jscontent="title"></span>>></span> On 2016/07/12 16:05:37, Philipp Keck wrote: > On 2016/07/12 15:50:31, Bernhard Bauer wrote: > > On 2016/07/12 15:19:56, Philipp Keck wrote: > > > On 2016/07/12 14:44:51, Bernhard Bauer wrote: > > > > Move the last closing tag to a new line so it align the with opening one? > > > > > > Good idea. I get confused by this HTML style where you leave out some > closing > > > tags. Why does Chromium do that? > > > > Because HTML5 doesn't require closing tags for certain kinds of tags, so this > > way we minimize the HTML. This is rather new though -- I think jkrcal@ was the > > one who introduced that style. > > So minimize HTML to reduce the binary size? Removing unnecessary whitespace and > quotes could also be a good idea, but wouldn't you rather have a compiler do all > of that and write the code in the way you like to read it? Coincidentally, aberent@ is working on exactly that. But I think that leaving out the closing tags is in fact quite readable (assuming that the indentation is correct, but that should be the case anyway).
@treib PTAL again
Sorry for jumping in so late, LGTM. https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... File chrome/browser/resources/snippets_internals.js (right): https://codereview.chromium.org/2145563002/diff/1/chrome/browser/resources/sn... chrome/browser/resources/snippets_internals.js:130: link.onclick = function(event) { On 2016/07/12 13:49:47, Marc Treib wrote: > On 2016/07/12 13:45:02, Philipp Keck wrote: > > On 2016/07/12 13:09:45, Bernhard Bauer wrote: > > > I would either use addEventListener everywhere or onclick everywhere. > > > > onclick allows for one, removal by ".onclick = null;" whereas addEventListener > > can add multiple and remove them with removeEventListener. However, to remove > > you still need the reference to the function you added previously and there is > > no "remove all". Removing/Overwriting is important here because otherwise it > is > > executed twice after a reload (and three times after the next), causing the > > thing to open and immediately close again. Don't know why that wasn't an issue > > before? > > I'm moderately sure that it *was* actually an issue before - "sometimes" the > hide/unhide toggle wouldn't work, I think nobody ever investigated why :D > > > The same problem doesn't occur with the addEventListeners in initialize() > above, > > but the idea there is also more of the "onclick" than the "addEventListener" > > kind, so I could change it there, too. > > > > However, the addEventListener below can't be changed because there is no > > equivalent onxyz event, afaik. > > > > To be consistent with existing code, I just make the old lambda a named > > function, then addEventListener will avoid registering it twice automatically. > > SGTM I can confirm that the toggle behaved weirdly sometimes. I did a bit of investigation but have been dragged away by some more pushing issues :) https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... File chrome/browser/resources/snippets_internals.html (right): https://codereview.chromium.org/2145563002/diff/20001/chrome/browser/resource... chrome/browser/resources/snippets_internals.html:172: <span jscontent="title"></span>>></span> On 2016/07/12 16:16:27, Bernhard Bauer wrote: > On 2016/07/12 16:05:37, Philipp Keck wrote: > > On 2016/07/12 15:50:31, Bernhard Bauer wrote: > > > On 2016/07/12 15:19:56, Philipp Keck wrote: > > > > On 2016/07/12 14:44:51, Bernhard Bauer wrote: > > > > > Move the last closing tag to a new line so it align the with opening > one? > > > > > > > > Good idea. I get confused by this HTML style where you leave out some > > closing > > > > tags. Why does Chromium do that? > > > > > > Because HTML5 doesn't require closing tags for certain kinds of tags, so > this > > > way we minimize the HTML. This is rather new though -- I think jkrcal@ was > the > > > one who introduced that style. > > > > So minimize HTML to reduce the binary size? Removing unnecessary whitespace > and > > quotes could also be a good idea, but wouldn't you rather have a compiler do > all > > of that and write the code in the way you like to read it? > > Coincidentally, aberent@ is working on exactly that. But I think that leaving > out the closing tags is in fact quite readable (assuming that the indentation is > correct, but that should be the case anyway). Indeed, minimizing the code was not my primary goal. My primary goal was getting used to the new html5 world :)
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, jkrcal@chromium.org Link to the patchset: https://codereview.chromium.org/2145563002/#ps120001 (title: "Refactor NTPSnippetsService::UpdateStateForStatus switch statement")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add ContentSuggestionsService to SnippetsInternals The ContentSuggestionsService retrieves suggestions from the NTPSnippetsService as a provider. The latter currently delivers its snippets to the chrome://snippets-internals/ page for debugging. Before the NTP UI will be changed to retrieve suggestions from the ContentSuggestionsService instead of directly from the NTPSnippetsService, this adds the output of the ContentSuggestionsService to the snippets-internals page to allow debugging both and compare the data converted data to the original. This also adds controls for clearing cached/discarded suggestions. BUG=619560 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Add ContentSuggestionsService to SnippetsInternals The ContentSuggestionsService retrieves suggestions from the NTPSnippetsService as a provider. The latter currently delivers its snippets to the chrome://snippets-internals/ page for debugging. Before the NTP UI will be changed to retrieve suggestions from the ContentSuggestionsService instead of directly from the NTPSnippetsService, this adds the output of the ContentSuggestionsService to the snippets-internals page to allow debugging both and compare the data converted data to the original. This also adds controls for clearing cached/discarded suggestions. BUG=619560 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/f397acb853c3eaf3791e43ecedd22610d954a9d9 Cr-Commit-Position: refs/heads/master@{#405149} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f397acb853c3eaf3791e43ecedd22610d954a9d9 Cr-Commit-Position: refs/heads/master@{#405149} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
