Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2355393002:
New snippets now replace old snippets and do not merge (Closed)
|
Created:
4 years, 3 months ago by jkrcal Modified:
4 years, 2 months ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNew snippets now replace old snippets and do not merge
Previously, the provider of remote content suggestions used a
complicated logic to merge suggestions across fetches and to expire
them.
After this CL,
- new (non-empty) list of suggestions for a category always replaces
the previous content of the category,
- we never expire current suggestions, only dismissed suggestions are
removed after expiry date (we do not need a timer, this can happen
before we use dismissed suggestions for filtering newly fetched
suggestions),
- we keep an archive of older suggestions in memory to support
previously opened NTPs (these are never stored to DB and thus
cleared upon restart; we trim the list after 200 suggestions to
address users that never close Chrome).
This CL does not deal with clearing orphaned suggestion thumbnails
from the DB which is necessary for the current design. This is left
for a follow-up CL.
As additional technical steps:
- The suggestions->category map was removed from the
ContentSuggestionService. This blocked image fetching queries
for archived suggestions to get to the provider (as the service
does not know about the archive). We now rely on the category
being encoded into the suggestion_id string. To this end the CL
moved the functions that build and parse suggestion_id strings
into CategoryFactory to be accessible both from the service and
the providers.
BUG=649009
Committed: https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024
Cr-Commit-Position: refs/heads/master@{#420677}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Minor polish #
Total comments: 44
Patch Set 3 : Marc's comments #
Total comments: 16
Patch Set 4 : Rebase #Patch Set 5 : Marc's comments #2 #
Total comments: 4
Patch Set 6 : Marc's comments #3 #Patch Set 7 : Rebase #Patch Set 8 : Unittest fix #
Total comments: 6
Patch Set 9 : Marc's comments + further changes to make unittests happy #
Total comments: 4
Patch Set 10 : Marc's comments #5 #Messages
Total messages: 41 (22 generated)
jkrcal@chromium.org changed reviewers: + treib@chromium.org
Marc, could you PTAL? https://codereview.chromium.org/2355393002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.cc (right): https://codereview.chromium.org/2355393002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.cc:21: return category_factory()->MakeUniqueID(category, within_category_id); I did not want to remove these API functions and touch all the providers as the situation might change again when we use more of the (category, id) instead of (suggestion_id) in the API.
I'm slightly worried about this intermediate state where we (almost) never delete images. We really need to make sure to get that in before BP. https://codereview.chromium.org/2355393002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.cc (right): https://codereview.chromium.org/2355393002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.cc:21: return category_factory()->MakeUniqueID(category, within_category_id); On 2016/09/21 17:05:12, jkrcal wrote: > I did not want to remove these API functions and touch all the providers as the > situation might change again when we use more of the (category, id) instead of > (suggestion_id) in the API. Acknowledged. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... File components/ntp_snippets/category_factory.h (right): https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory.h:58: const std::string& unique_id) const; Hm. These don't really belong in the CategoryFactory, but since they'll be going away anyway I guess it doesn't matter much. Please add a TODO(treib) and reference crbug.com/649048 :) https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_database.h (right): https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:60: // Deletes the snippet with the given ID, and its image. I guess this one doesn't delete the image either anymore? https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:62: // Deletes all the given snippets (identified by their IDs), not their images. I'd just remove the part after the comma, IMO it's more confusing than helping. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:54: const int kMaxArchivedSnippetCount = 200; Out of curiosity: Any particular reason for this number? https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:184: std::set<std::string> ids) { const & https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:194: void FilterSnippets(NTPSnippet::PtrVector* all_snippets, Hm, I find this name somewhat misleading, but I can't think of something much better... EraseMatchingSnippets or something? https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:400: database_->DeleteSnippets(content->snippets); Also delete the images? (Please go over all database_->Delete* calls and make sure if images also need to get deleted) https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:477: return it->get()->salient_image_url(); (*it)->salient_image_url() ? https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:484: return it->get()->salient_image_url(); Same here https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:506: // provided to image_fetcher_ (OnSnippetImageDecodedFromNetwork()). nit: pipes around image_fetcher_ https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:532: ClearExpiredSnippets(); Didn't you rename this to ClearExpiredDismissedSnippets? Forgot to hit "save"? :) https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:607: categories_[category].provided_by_server = true; Not your doing, but this is super hard to read, with so many variables called "category"... https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:682: return; I don't think returning here is correct, e.g. the database_->DeleteSnippets call should still happen. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:757: // Finally, actually delete the removed snippets from the DB. nitty nit: "Finally" doesn't apply anymore. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:764: continue; The "continue" doesn't do anything anymore. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:775: // TODO(jkrcal): Implement. Have a bug to reference? :) https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:20: #include "base/timer/timer.h" Not needed anymore I think :) https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:204: // among the archived snippets in |category|. Returns empty string, otherwise. nit: Returns an invalid (or empty) URL otherwise. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:309: // All suggestions from the most recent fetch (excl. the dismissed ones). nit: To me, "from the most recent fetch" isn't really important here, just that these are the currently "active" suggestions. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:312: // All previous snippets that we keep around in memory because they can be nitty nit: s/snippets/suggestions/, to be consistent with above https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:313: // on some open NTP. We do not store this list to any DB so that on new nitty nit 2: "DB" doesn't necessarily imply that they'd be persisted, and it's not the only way they could be persisted. So I'd just say that they aren't persisted :)
Thanks! PTAL, again. To address your worries, I now delete the image from the DB whenever a snippet is archived. I'll remove this when DeleteOrphanedImages() is implemented. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... File components/ntp_snippets/category_factory.h (right): https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/category_factory.h:58: const std::string& unique_id) const; On 2016/09/21 19:10:16, Marc Treib wrote: > Hm. These don't really belong in the CategoryFactory, but since they'll be going > away anyway I guess it doesn't matter much. > Please add a TODO(treib) and reference crbug.com/649048 :) Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_database.h (right): https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:60: // Deletes the snippet with the given ID, and its image. On 2016/09/21 19:10:16, Marc Treib wrote: > I guess this one doesn't delete the image either anymore? No, it does not. Comment fixed, thanks! https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_database.h:62: // Deletes all the given snippets (identified by their IDs), not their images. On 2016/09/21 19:10:16, Marc Treib wrote: > I'd just remove the part after the comma, IMO it's more confusing than helping. Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:54: const int kMaxArchivedSnippetCount = 200; On 2016/09/21 19:10:16, Marc Treib wrote: > Out of curiosity: Any particular reason for this number? Not really. I was not sure if 100 is large enough so I picked 200 :) https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:184: std::set<std::string> ids) { On 2016/09/21 19:10:16, Marc Treib wrote: > const & Oh, shit :) Thanks! https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:194: void FilterSnippets(NTPSnippet::PtrVector* all_snippets, On 2016/09/21 19:10:16, Marc Treib wrote: > Hm, I find this name somewhat misleading, but I can't think of something much > better... EraseMatchingSnippets or something? Better, done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:400: database_->DeleteSnippets(content->snippets); On 2016/09/21 19:10:17, Marc Treib wrote: > Also delete the images? (Please go over all database_->Delete* calls and make > sure if images also need to get deleted) I was not sure. But I can delete them :) Actually, I'll be now more strict about deleting before clearing orphaned images is implemented. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:477: return it->get()->salient_image_url(); On 2016/09/21 19:10:16, Marc Treib wrote: > (*it)->salient_image_url() ? Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:484: return it->get()->salient_image_url(); On 2016/09/21 19:10:17, Marc Treib wrote: > Same here Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:506: // provided to image_fetcher_ (OnSnippetImageDecodedFromNetwork()). On 2016/09/21 19:10:16, Marc Treib wrote: > nit: pipes around image_fetcher_ Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:532: ClearExpiredSnippets(); On 2016/09/21 19:10:16, Marc Treib wrote: > Didn't you rename this to ClearExpiredDismissedSnippets? Forgot to hit "save"? > :) More like forgot to hit "compile" :) https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:607: categories_[category].provided_by_server = true; On 2016/09/21 19:10:17, Marc Treib wrote: > Not your doing, but this is super hard to read, with so many variables called > "category"... Agreed. I also had hard time understanding what is going on :) Added a TODO. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:682: return; On 2016/09/21 19:10:17, Marc Treib wrote: > I don't think returning here is correct, e.g. the database_->DeleteSnippets call > should still happen. I do not agree. If we get a new fetch that is practically empty, I do not think we should change anything about the current set of suggestions. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:757: // Finally, actually delete the removed snippets from the DB. On 2016/09/21 19:10:16, Marc Treib wrote: > nitty nit: "Finally" doesn't apply anymore. Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:764: continue; On 2016/09/21 19:10:16, Marc Treib wrote: > The "continue" doesn't do anything anymore. Huh, good point :) https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:775: // TODO(jkrcal): Implement. On 2016/09/21 19:10:16, Marc Treib wrote: > Have a bug to reference? :) Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.h (right): https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:20: #include "base/timer/timer.h" On 2016/09/21 19:10:17, Marc Treib wrote: > Not needed anymore I think :) Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:204: // among the archived snippets in |category|. Returns empty string, otherwise. On 2016/09/21 19:10:17, Marc Treib wrote: > nit: Returns an invalid (or empty) URL otherwise. Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:309: // All suggestions from the most recent fetch (excl. the dismissed ones). On 2016/09/21 19:10:17, Marc Treib wrote: > nit: To me, "from the most recent fetch" isn't really important here, just that > these are the currently "active" suggestions. Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:312: // All previous snippets that we keep around in memory because they can be On 2016/09/21 19:10:17, Marc Treib wrote: > nitty nit: s/snippets/suggestions/, to be consistent with above Done. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.h:313: // on some open NTP. We do not store this list to any DB so that on new On 2016/09/21 19:10:17, Marc Treib wrote: > nitty nit 2: "DB" doesn't necessarily imply that they'd be persisted, and it's > not the only way they could be persisted. So I'd just say that they aren't > persisted :) Done.
https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:682: return; On 2016/09/22 09:12:42, jkrcal wrote: > On 2016/09/21 19:10:17, Marc Treib wrote: > > I don't think returning here is correct, e.g. the database_->DeleteSnippets > call > > should still happen. > > I do not agree. If we get a new fetch that is practically empty, I do not think > we should change anything about the current set of suggestions. Okay, makes sense - mind adding a brief comment to explain? https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_factory.h (right): https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_factory.h:49: // minimize usage of these functions. See crbug.com/649048. s/minimize usage of/eliminate/ :) https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:195: NTPSnippet::PtrVector* snippets, nit: This can go on the previous line https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:445: // Image gets deleted when each snippet gets dismissed. nit: This sounds like something that will happen in the future, but in fact it's something that should already have happened. "The image already got deleted then the suggestion was dismissed"? https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:655: database_->DeleteImages(*to_archive); Hm. So when there is an open NTP that has a snippet, but not its image, then we might download and store it again (and it'll get orphaned). So this solves only half the problem, and introduces a new one (unnecessary re-downloads). So I'm not sure this improves things. Let's just make sure to get the orphan-cleanup into 55 :) https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:723: EraseMatchingSnippets(&content->snippets, new_snippets); Hm. For this purpose, should we only check the primary ID? If only one of the "sub-IDs" matches, then we might not be able to associate e.g. an image request to the new snippet. It'd also make image management a bit easier. https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:736: std::make_move_iterator(new_snippets.end())); Would content->snippets = std::move(new_snippets); work? https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:778: // Image gets deleted when each snippet gets dismissed. Same as the identical comment above :) https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:783: if ((category != articles_category_) && !content->provided_by_server) nit: Now you could merge the two "if"s
Thanks a lot, Marc! PTAL, again. https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2355393002/diff/20001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:682: return; On 2016/09/22 11:27:06, Marc Treib wrote: > On 2016/09/22 09:12:42, jkrcal wrote: > > On 2016/09/21 19:10:17, Marc Treib wrote: > > > I don't think returning here is correct, e.g. the database_->DeleteSnippets > > call > > > should still happen. > > > > I do not agree. If we get a new fetch that is practically empty, I do not > think > > we should change anything about the current set of suggestions. > > Okay, makes sense - mind adding a brief comment to explain? Done. https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... File components/ntp_snippets/category_factory.h (right): https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/category_factory.h:49: // minimize usage of these functions. See crbug.com/649048. On 2016/09/22 11:27:06, Marc Treib wrote: > s/minimize usage of/eliminate/ :) Done. https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:195: NTPSnippet::PtrVector* snippets, On 2016/09/22 11:27:06, Marc Treib wrote: > nit: This can go on the previous line Done. https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:445: // Image gets deleted when each snippet gets dismissed. On 2016/09/22 11:27:06, Marc Treib wrote: > nit: This sounds like something that will happen in the future, but in fact it's > something that should already have happened. "The image already got deleted then > the suggestion was dismissed"? Done. https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:655: database_->DeleteImages(*to_archive); On 2016/09/22 11:27:06, Marc Treib wrote: > Hm. So when there is an open NTP that has a snippet, but not its image, then we > might download and store it again (and it'll get orphaned). So this solves only > half the problem, and introduces a new one (unnecessary re-downloads). > So I'm not sure this improves things. Let's just make sure to get the > orphan-cleanup into 55 :) Okay :) Removed the line. https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:723: EraseMatchingSnippets(&content->snippets, new_snippets); On 2016/09/22 11:27:06, Marc Treib wrote: > Hm. For this purpose, should we only check the primary ID? If only one of the > "sub-IDs" matches, then we might not be able to associate e.g. an image request > to the new snippet. It'd also make image management a bit easier. Done. https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:736: std::make_move_iterator(new_snippets.end())); On 2016/09/22 11:27:06, Marc Treib wrote: > Would > content->snippets = std::move(new_snippets); > work? Done. https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:778: // Image gets deleted when each snippet gets dismissed. On 2016/09/22 11:27:06, Marc Treib wrote: > Same as the identical comment above :) Done. https://codereview.chromium.org/2355393002/diff/40001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:783: if ((category != articles_category_) && !content->provided_by_server) On 2016/09/22 11:27:06, Marc Treib wrote: > nit: Now you could merge the two "if"s Done.
lgtm Thanks! https://codereview.chromium.org/2355393002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2355393002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:206: const std::set<std::string>& matching_ids, optional: You could also pass in the snippets to match, and call GetAllIds/GetMainIds as appropriate based on match_all_ids. Not sure if that's better... https://codereview.chromium.org/2355393002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:744: // Insert new snippets. nitty nit: "Insert" isn't correct anymore. I'd just remove the comment.
Thanks! https://codereview.chromium.org/2355393002/diff/80001/components/ntp_snippets... File components/ntp_snippets/ntp_snippets_service.cc (right): https://codereview.chromium.org/2355393002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:206: const std::set<std::string>& matching_ids, On 2016/09/22 13:05:29, Marc Treib wrote: > optional: You could also pass in the snippets to match, and call > GetAllIds/GetMainIds as appropriate based on match_all_ids. Not sure if that's > better... I think the current way it is even clearer on the site of the caller what is going on. https://codereview.chromium.org/2355393002/diff/80001/components/ntp_snippets... components/ntp_snippets/ntp_snippets_service.cc:744: // Insert new snippets. On 2016/09/22 13:05:29, Marc Treib wrote: > nitty nit: "Insert" isn't correct anymore. I'd just remove the comment. Done.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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/v2/patch-status/codereview.chromium.or...
Marc, I had to update the unittest, quite a bit. Does it still LGTY?
lgtm https://codereview.chromium.org/2355393002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2355393002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:145: std::string id_string = within_category_id ? https://codereview.chromium.org/2355393002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:217: result.emplace_back(CreateSuggestion(category, number)); nit: push_back will do the exact same thing here I think https://codereview.chromium.org/2355393002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:316: // Assuming there will never be a category with the randomly-picked id below. https://xkcd.com/221/ :) (Not an actual complaint)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/v2/patch-status/codereview.chromium.or...
Thanks, Marc! I was a bit lazy in running unit-test which backfired on me. I hope to have covered everything to fix by now. https://codereview.chromium.org/2355393002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2355393002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:145: std::string id_string = On 2016/09/23 13:31:29, Marc Treib wrote: > within_category_id ? Done. https://codereview.chromium.org/2355393002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:217: result.emplace_back(CreateSuggestion(category, number)); On 2016/09/23 13:31:29, Marc Treib wrote: > nit: push_back will do the exact same thing here I think Done, thanks! https://codereview.chromium.org/2355393002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:316: // Assuming there will never be a category with the randomly-picked id below. On 2016/09/23 13:31:29, Marc Treib wrote: > https://xkcd.com/221/ :) > (Not an actual complaint) :)
On 2016/09/23 16:11:48, jkrcal wrote: > Thanks, Marc! > > I was a bit lazy in running unit-test which backfired on me. I hope to have > covered everything to fix by now. I've been there more often than I care to count ;)
On 2016/09/23 16:16:49, Marc Treib wrote: > On 2016/09/23 16:11:48, jkrcal wrote: > > Thanks, Marc! > > > > I was a bit lazy in running unit-test which backfired on me. I hope to have > > covered everything to fix by now. > > I've been there more often than I care to count ;) Maybe I was not explicit enough. My last patch set includes several additional changes. Unless you protest within 30 minutes, I understand you last comment as "still lgtm" :)
Ah indeed, I missed that. Anyway, still lgtm! https://codereview.chromium.org/2355393002/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2355393002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:653: // The snippet loaded last should be at the first position in the list now. nit: This comment could be updated https://codereview.chromium.org/2355393002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:794: // Load a different snippet - this will clear the expired dismissed ones. This seems to test an implementation details that doesn't really matter, but I guess we don't have a better "event" to trigger the actual removal of the expired dismissed stuff?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, Marc! https://codereview.chromium.org/2355393002/diff/160001/components/ntp_snippet... File components/ntp_snippets/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2355393002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:653: // The snippet loaded last should be at the first position in the list now. On 2016/09/23 16:36:06, Marc Treib wrote: > nit: This comment could be updated Oh, thanks, I missed it! https://codereview.chromium.org/2355393002/diff/160001/components/ntp_snippet... components/ntp_snippets/ntp_snippets_service_unittest.cc:794: // Load a different snippet - this will clear the expired dismissed ones. On 2016/09/23 16:36:06, Marc Treib wrote: > This seems to test an implementation details that doesn't really matter, but I > guess we don't have a better "event" to trigger the actual removal of the > expired dismissed stuff? I agree it is an implementation detail but this way it actually also makes sure it happens frequently enough (upon each fetch).
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/2355393002/#ps180001 (title: "Marc's comments #5")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by treib@chromium.org
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 #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== New snippets now replace old snippets and do not merge Previously, the provider of remote content suggestions used a complicated logic to merge suggestions across fetches and to expire them. After this CL, - new (non-empty) list of suggestions for a category always replaces the previous content of the category, - we never expire current suggestions, only dismissed suggestions are removed after expiry date (we do not need a timer, this can happen before we use dismissed suggestions for filtering newly fetched suggestions), - we keep an archive of older suggestions in memory to support previously opened NTPs (these are never stored to DB and thus cleared upon restart; we trim the list after 200 suggestions to address users that never close Chrome). This CL does not deal with clearing orphaned suggestion thumbnails from the DB which is necessary for the current design. This is left for a follow-up CL. As additional technical steps: - The suggestions->category map was removed from the ContentSuggestionService. This blocked image fetching queries for archived suggestions to get to the provider (as the service does not know about the archive). We now rely on the category being encoded into the suggestion_id string. To this end the CL moved the functions that build and parse suggestion_id strings into CategoryFactory to be accessible both from the service and the providers. BUG=649009 ========== to ========== New snippets now replace old snippets and do not merge Previously, the provider of remote content suggestions used a complicated logic to merge suggestions across fetches and to expire them. After this CL, - new (non-empty) list of suggestions for a category always replaces the previous content of the category, - we never expire current suggestions, only dismissed suggestions are removed after expiry date (we do not need a timer, this can happen before we use dismissed suggestions for filtering newly fetched suggestions), - we keep an archive of older suggestions in memory to support previously opened NTPs (these are never stored to DB and thus cleared upon restart; we trim the list after 200 suggestions to address users that never close Chrome). This CL does not deal with clearing orphaned suggestion thumbnails from the DB which is necessary for the current design. This is left for a follow-up CL. As additional technical steps: - The suggestions->category map was removed from the ContentSuggestionService. This blocked image fetching queries for archived suggestions to get to the provider (as the service does not know about the archive). We now rely on the category being encoded into the suggestion_id string. To this end the CL moved the functions that build and parse suggestion_id strings into CategoryFactory to be accessible both from the service and the providers. BUG=649009 Committed: https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024 Cr-Commit-Position: refs/heads/master@{#420677} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/928ed3fef6431a8815e32bb2d905fc3bd21f5024 Cr-Commit-Position: refs/heads/master@{#420677} |
