Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1992803002:
Use multiple IDs when discarding or merging snippets. (Closed)
|
Created:
4 years, 7 months ago by tschumann Modified:
4 years, 7 months ago Reviewers:
Marc Treib CC:
chromium-reviews, mcwilliams+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. |
DescriptionUse multiple IDs when discarding or merging snippets.
Changes the discarding and de-duplication logic to take IDs of all snippet
sources (these are always URLs in our cases) into account.
R=treib@chromium.org
BUG=603884
Committed: https://crrev.com/ab78ef3824631e1ef5fffba7c21946d66b9ad84f
Cr-Commit-Position: refs/heads/master@{#394539}
Patch Set 1 #
Total comments: 12
Patch Set 2 : first round of comments #Patch Set 3 : rebased #
Total comments: 9
Patch Set 4 : Next round of fixes. #Patch Set 5 : fixed new tests (they didn't expect de-duping based on corpus-ids.). #
Messages
Total messages: 13 (3 generated)
https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet.h:62: // bridge) and a URL (used by snipets internal page etc.). Not quite sure how to read this comment... we already have an ID vs a set of URLs, so what's the TODO? Also, I think we'd want to continue showing the ID on the internals page? nitty nit: snipPets internalS https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (left): https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:395: nitty nit: I'd keep the empty line here between "unrelated" blocks. https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:138: void InsertAllIDs(const NTPSnippetsService::NTPSnippetStorage& snippets, Heads-up: This will require a rebase onto https://codereview.chromium.org/1976153002/ - NTPSnippetsService::NTPSnippetStorage is now NTPSnippet::PtrVector https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:143: if (source.url.is_valid()) { We check that the source URL is valid when building the snippet, no need to check here again. (You could DCHECK() it if you want) https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:365: return old_snippet_ids.find(snippet->id()) != old_snippet_ids.end(); We should probably check if *any* of the new snippet's IDs/URLs are in the old IDs? Also, optional nit: I find old_snippet_ids.count(snippet->id()) > 0 a bit easier to read. https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:781: " \"title\": \"Stolen doggie finally gets returned to owner after two " Would you mind shortening the long titles/URLs, so that everything fits onto its line? The actual values don't matter anyway, and IMO that'd make it a bit easier to read. Jan has been working on re-structuring the tests, so that we don't have so many json strings flying around: https://codereview.chromium.org/1985973003/ But since that's not landed yet, I'd keep this as-is for now, and maybe revisit later.
https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet.h:62: // bridge) and a URL (used by snipets internal page etc.). On 2016/05/18 15:19:33, Marc Treib wrote: > Not quite sure how to read this comment... we already have an ID vs a set of > URLs, so what's the TODO? Also, I think we'd want to continue showing the ID on > the internals page? IMO we should properly distinguish the ID from URLs. In the internals page, we should only show a URL (and in most other places too). The id should be used only to identify a snippet. > > nitty nit: snipPets internalS Fixed the comment. If you feel this is comment is premature, I can also remove it =) https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:143: if (source.url.is_valid()) { On 2016/05/18 15:19:33, Marc Treib wrote: > We check that the source URL is valid when building the snippet, no need to > check here again. (You could DCHECK() it if you want) i thought I saw some code where we'd fall back to an empty one... Oh, that was the AMP one =). Well I guess we can just ignore it. Just wanted to make sure we're not adding empty strings unnecessarily. https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service.cc:365: return old_snippet_ids.find(snippet->id()) != old_snippet_ids.end(); On 2016/05/18 15:19:33, Marc Treib wrote: > We should probably check if *any* of the new snippet's IDs/URLs are in the old > IDs? Good catch! > > Also, optional nit: I find > old_snippet_ids.count(snippet->id()) > 0 > a bit easier to read. Done. https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippets_service_unittest.cc:781: " \"title\": \"Stolen doggie finally gets returned to owner after two " On 2016/05/18 15:19:33, Marc Treib wrote: > Would you mind shortening the long titles/URLs, so that everything fits onto its > line? The actual values don't matter anyway, and IMO that'd make it a bit easier > to read. yes indeed. Typically I like to use essentially real-world data but transforming it into JSON already makes it hard to read, so let's make it a bit simpler =) > > Jan has been working on re-structuring the tests, so that we don't have so many > json strings flying around: https://codereview.chromium.org/1985973003/ > But since that's not landed yet, I'd keep this as-is for now, and maybe revisit > later. Interesting. I'll have a look ;-)
LGTM with some more (optional) nits https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet.h:62: // bridge) and a URL (used by snipets internal page etc.). On 2016/05/18 17:09:16, tschumann wrote: > On 2016/05/18 15:19:33, Marc Treib wrote: > > Not quite sure how to read this comment... we already have an ID vs a set of > > URLs, so what's the TODO? Also, I think we'd want to continue showing the ID > on > > the internals page? > IMO we should properly distinguish the ID from URLs. In the internals page, we > should only show a URL (and in most other places too). The id should be used > only to identify a snippet. IMO the internals page should show everything we have. That's what it's for. But that's a discussion for another day :) > > nitty nit: snipPets internalS > Fixed the comment. > If you feel this is comment is premature, I can also remove it =) I kinda think it is, but I don't feel that strongly. I wouldn't mention the JNI bridge though - if we have an ID, then it should be an actual ID, without restrictions on who can use it... https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:168: for (const SnippetSource& source : snippet->sources()) { nit: no braces required (we usually leave them out if both the condition and the body fit on one line each) https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:404: if (old_snippet_ids.count(snippet->id())) { Also here: no braces required https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:408: if (old_snippet_ids.count(source.url.spec())) { And here. (The "for" should keep the braces though, since its body has two lines) https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:787: "years\", " One more avoidable line break :)
all done. Will submit soon and happy to do follow-up comments in a new changelist. https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/1992803002/diff/1/components/ntp_snippets/ntp... components/ntp_snippets/ntp_snippet.h:62: // bridge) and a URL (used by snipets internal page etc.). On 2016/05/18 18:58:05, Marc Treib wrote: > On 2016/05/18 17:09:16, tschumann wrote: > > On 2016/05/18 15:19:33, Marc Treib wrote: > > > Not quite sure how to read this comment... we already have an ID vs a set of > > > URLs, so what's the TODO? Also, I think we'd want to continue showing the ID > > on > > > the internals page? > > IMO we should properly distinguish the ID from URLs. In the internals page, > we > > should only show a URL (and in most other places too). The id should be used > > only to identify a snippet. > > IMO the internals page should show everything we have. That's what it's for. But > that's a discussion for another day :) > > > > nitty nit: snipPets internalS > > Fixed the comment. > > If you feel this is comment is premature, I can also remove it =) > > I kinda think it is, but I don't feel that strongly. > I wouldn't mention the JNI bridge though - if we have an ID, then it should be > an actual ID, without restrictions on who can use it... Removed the comment. As it's not too clear as of today :-) (my reasoning was to keep the ID hidden as much as possible. If we need a string to display in a UI, we can ask the NTPSnippet object to give us such a string, but we shouldn't ask for implementation details. To be honest, the current design of the class is more like a data type and doesn't hide any information -- need to read more code to judge if we should hide more or not. Especially when integrating with other platforms, better data hiding would allow us more flexibility). https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:168: for (const SnippetSource& source : snippet->sources()) { On 2016/05/18 18:58:05, Marc Treib wrote: > nit: no braces required (we usually leave them out if both the condition and the > body fit on one line each) hmmm... Ok, done for consistency. One day (in the not-so-distant future) I will challenge this convention, though =) https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:404: if (old_snippet_ids.count(snippet->id())) { On 2016/05/18 18:58:05, Marc Treib wrote: > Also here: no braces required Done. https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:408: if (old_snippet_ids.count(source.url.spec())) { On 2016/05/18 18:58:05, Marc Treib wrote: > And here. (The "for" should keep the braces though, since its body has two > lines) Done. https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service_unittest.cc:787: "years\", " On 2016/05/18 18:58:05, Marc Treib wrote: > One more avoidable line break :) Done.
Still LGTM, feel free to submit :) https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/1992803002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:168: for (const SnippetSource& source : snippet->sources()) { On 2016/05/18 19:23:28, tschumann wrote: > On 2016/05/18 18:58:05, Marc Treib wrote: > > nit: no braces required (we usually leave them out if both the condition and > the > > body fit on one line each) > > hmmm... Ok, done for consistency. > One day (in the not-so-distant future) I will challenge this convention, though > =) You'll have to fight Bernhard over it :D Personally, I don't really mind either way, as long as it's consistent.
newly added unit tests got broken by this change. The fix in this CL isn't elegant but consistent with what we do so far (and Jan is working on improving this).
The CQ bit was checked by tschumann@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/1992803002/#ps80001 (title: "fixed new tests (they didn't expect de-duping based on corpus-ids.).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1992803002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1992803002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Use multiple IDs when discarding or merging snippets. Changes the discarding and de-duplication logic to take IDs of all snippet sources (these are always URLs in our cases) into account. R=treib@chromium.org BUG=603884 ========== to ========== Use multiple IDs when discarding or merging snippets. Changes the discarding and de-duplication logic to take IDs of all snippet sources (these are always URLs in our cases) into account. R=treib@chromium.org BUG=603884 Committed: https://crrev.com/ab78ef3824631e1ef5fffba7c21946d66b9ad84f Cr-Commit-Position: refs/heads/master@{#394539} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ab78ef3824631e1ef5fffba7c21946d66b9ad84f Cr-Commit-Position: refs/heads/master@{#394539} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
