Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1727573003:
[NTP Snippets] Implement snippets merging (Closed)
|
Created:
4 years, 10 months ago by Marc Treib Modified:
4 years, 10 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@fetch_startup Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[NTP Snippets] Implement snippets merging
Detect already-existing snippets based on the URL.
(This does not actually persist the merged set of snippets yet.)
BUG=587819
Committed: https://crrev.com/c4c3dc2a193a03aedbde3e97028d71ed16b75fa4
Cr-Commit-Position: refs/heads/master@{#377858}
Patch Set 1 #
Total comments: 7
Patch Set 2 : rebase #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 14 (6 generated)
treib@chromium.org changed reviewers: + bauerb@chromium.org
Keep on stackin'! PTAL! https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (left): https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:94: JSONStringValueDeserializer deserializer(str); Not sure what the point of this class is, it just wraps over JSONReader which is just as easy to use directly.
Description was changed from ========== [NTP Snippets] Implement snippets merging Detect already-existing snippets based on the URL BUG=587819 ========== to ========== [NTP Snippets] Implement snippets merging Detect already-existing snippets based on the URL. (This does not actually persist the merged set of snippets yet.) BUG=587819 ==========
https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (left): https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:94: JSONStringValueDeserializer deserializer(str); On 2016/02/23 12:57:54, Marc Treib wrote: > Not sure what the point of this class is, it just wraps over JSONReader which is > just as easy to use directly. ¯\_(ツ)_/¯ It does return an error message and code, which are useful in some cases. https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:126: else This kind of feels like it should go into a map...
https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (left): https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:94: JSONStringValueDeserializer deserializer(str); On 2016/02/23 13:42:21, Bernhard Bauer wrote: > On 2016/02/23 12:57:54, Marc Treib wrote: > > Not sure what the point of this class is, it just wraps over JSONReader which > is > > just as easy to use directly. > > ¯\_(ツ)_/¯ It does return an error message and code, which are useful in some > cases. Which JSONReader also does, if you call ReadAndReturnError, which is what JSONStringValueDeserializer does. It provides literally no benefit. https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:126: else On 2016/02/23 13:42:21, Bernhard Bauer wrote: > This kind of feels like it should go into a map... Maybe? That would mess up the order though, and we'd need a custom comparator. And we'd probably end up storing all the URLs twice... So, I'd stay with the vector for now, especially given that the exact storage method will probably change soon anyway.
https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:126: else On 2016/02/23 13:58:33, Marc Treib wrote: > On 2016/02/23 13:42:21, Bernhard Bauer wrote: > > This kind of feels like it should go into a map... > > Maybe? That would mess up the order though, and we'd need a custom comparator. > And we'd probably end up storing all the URLs twice... Weeeellll... we could of course make this a std::set with a comparator that only looks at URLs 😃 Also, note that now you'll end up with an order that depends on the order of old versions of the snippet list. That may or may not be what you want? But if you're fine with that, LGTM. > So, I'd stay with the vector for now, especially given that the exact storage > method will probably change soon anyway.
https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1727573003/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:126: else On 2016/02/23 14:48:28, Bernhard Bauer wrote: > On 2016/02/23 13:58:33, Marc Treib wrote: > > On 2016/02/23 13:42:21, Bernhard Bauer wrote: > > > This kind of feels like it should go into a map... > > > > Maybe? That would mess up the order though, and we'd need a custom comparator. > > And we'd probably end up storing all the URLs twice... > > Weeeellll... we could of course make this a std::set with a comparator that only > looks at URLs 😃 Good point :) > Also, note that now you'll end up with an order that depends on the order of old > versions of the snippet list. That may or may not be what you want? Probably not - but AFAIK we haven't defined a "correct order" yet, so "order of appearance" is as good as any I guess :) > But if you're fine with that, LGTM. Yeah, given that it'll change anyway, I'll just keep it as is for now. > > So, I'd stay with the vector for now, especially given that the exact storage > > method will probably change soon anyway. >
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/1727573003/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1727573003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1727573003/20001
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Implement snippets merging Detect already-existing snippets based on the URL. (This does not actually persist the merged set of snippets yet.) BUG=587819 ========== to ========== [NTP Snippets] Implement snippets merging Detect already-existing snippets based on the URL. (This does not actually persist the merged set of snippets yet.) BUG=587819 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [NTP Snippets] Implement snippets merging Detect already-existing snippets based on the URL. (This does not actually persist the merged set of snippets yet.) BUG=587819 ========== to ========== [NTP Snippets] Implement snippets merging Detect already-existing snippets based on the URL. (This does not actually persist the merged set of snippets yet.) BUG=587819 Committed: https://crrev.com/c4c3dc2a193a03aedbde3e97028d71ed16b75fa4 Cr-Commit-Position: refs/heads/master@{#377858} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c4c3dc2a193a03aedbde3e97028d71ed16b75fa4 Cr-Commit-Position: refs/heads/master@{#377858} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
