Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2386103009:
NTPSnippetsService: Garbage collect orphaned images at startup. (Closed)
DescriptionNTPSnippetsService: Garbage collect orphaned images at startup.
Also extends the unittest to use a proper image_decoder to allow better test coverage (and actually verifying the intended behavior).
BUG=649009
Committed: https://crrev.com/0d475e7d4ed829bd888d8ee18de17de9ef5d8684
Cr-Commit-Position: refs/heads/master@{#423529}
Patch Set 1 #Patch Set 2 : patch restored #
Total comments: 28
Patch Set 3 : addressed comments #Patch Set 4 : removed unused function #Patch Set 5 : snippet ids instead of snippets #
Total comments: 5
Patch Set 6 : final comments #
Messages
Total messages: 19 (8 generated)
tschumann@chromium.org changed reviewers: + jkrcal@chromium.org, treib@chromium.org
lgtm, thanks! https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.cc:139: std::unique_ptr<std::vector<std::string>> keys) { Maybe |image_keys| to make it even clearer?
https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.cc:136: void NTPSnippetsDatabase::DeleteUnreferencedImages( nit: Please match the order of methods in the header https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database.h (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.h:81: std::unique_ptr<std::set<std::string>> alive_snippets); Any particular reason for using std::set here? I'd pass in a const NTPSnippet::PtrVector&, to be consistent with the other methods (and build the set inside). https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.h:121: bool success, nit: load_keys_success? Took me a while to understand what that bool was doing in the middle there. https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database_unittest.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database_unittest.cc:325: void LoadExpectedImage(NTPSnippetsDatabase* db, nit: put into an anonymous namespace? https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database_unittest.cc:356: std::set<std::string>({"snippet-id-2"}))); nitty nit: I think "std::set<std::string>" isn't required here https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:496: ClearExpiredDismissedSnippets(); This won't do anything, since |categories_| is still empty at this point. https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:236: base::Callback<void(const std::string&, const gfx::Image&)> callback) { Add a using declaration for this callback type? ImageFetchedCallback? https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:358: ~FakeImageDecoder() override {} nit: = default https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:1154: LOG(INFO) << "signalling loop out"; nit: remove the LOG https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:1163: TEST_F(NTPSnippetsServiceTest, ShouldClearOrphanedImages) { nit: ShouldClearOrphanedImagesOnRestart?
https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.cc:136: void NTPSnippetsDatabase::DeleteUnreferencedImages( On 2016/10/06 10:21:30, Marc Treib wrote: > nit: Please match the order of methods in the header Done. https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.cc:139: std::unique_ptr<std::vector<std::string>> keys) { On 2016/10/06 10:12:47, jkrcal wrote: > Maybe |image_keys| to make it even clearer? Done. The context should already make 'keys' clear, but one word more doesn't hurt ;-) https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database.h (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.h:81: std::unique_ptr<std::set<std::string>> alive_snippets); On 2016/10/06 10:21:30, Marc Treib wrote: > Any particular reason for using std::set here? I'd pass in a const > NTPSnippet::PtrVector&, to be consistent with the other methods (and build the > set inside). hmmm... it feels a bit weird that the function call wants a whole snippet if the snippet-id is sufficient. I'd actually vote for changing DeleteImages() to take a vector of ids as well. if we want to go that route (i'm happy to write the patch), then passing the data in the data structure we actually need seems more plausible. If you feel strongly, I can change that of course ;-) https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.h:121: bool success, On 2016/10/06 10:21:30, Marc Treib wrote: > nit: load_keys_success? Took me a while to understand what that bool was doing > in the middle there. Done. https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database_unittest.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database_unittest.cc:325: void LoadExpectedImage(NTPSnippetsDatabase* db, On 2016/10/06 10:21:30, Marc Treib wrote: > nit: put into an anonymous namespace? Done. https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database_unittest.cc:356: std::set<std::string>({"snippet-id-2"}))); On 2016/10/06 10:21:30, Marc Treib wrote: > nitty nit: I think "std::set<std::string>" isn't required here unfortunately it is -- if I just pass in the initializer list, the compile fails. seems like MakeUnique does not support initializer lists :-/ https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:496: ClearExpiredDismissedSnippets(); On 2016/10/06 10:21:30, Marc Treib wrote: > This won't do anything, since |categories_| is still empty at this point. are you sure? we have a DCHECK() above verifying it includes the articles category... https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:236: base::Callback<void(const std::string&, const gfx::Image&)> callback) { On 2016/10/06 10:21:30, Marc Treib wrote: > Add a using declaration for this callback type? ImageFetchedCallback? not sure. IMO, this should be exposed by the ImageFetcher API -- not a big fan of introducing aliases for other libraries. Also, it's not a big burden in test code (once written ;-), and we also have it in production code. https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:358: ~FakeImageDecoder() override {} On 2016/10/06 10:21:30, Marc Treib wrote: > nit: = default Done. https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:1154: LOG(INFO) << "signalling loop out"; On 2016/10/06 10:21:30, Marc Treib wrote: > nit: remove the LOG Done. https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:1163: TEST_F(NTPSnippetsServiceTest, ShouldClearOrphanedImages) { On 2016/10/06 10:21:30, Marc Treib wrote: > nit: ShouldClearOrphanedImagesOnRestart? Done.
The CQ bit was checked by tschumann@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database.h (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.h:81: std::unique_ptr<std::set<std::string>> alive_snippets); On 2016/10/06 10:56:10, tschumann wrote: > On 2016/10/06 10:21:30, Marc Treib wrote: > > Any particular reason for using std::set here? I'd pass in a const > > NTPSnippet::PtrVector&, to be consistent with the other methods (and build the > > set inside). > > hmmm... it feels a bit weird that the function call wants a whole snippet if the > snippet-id is sufficient. I'd actually vote for changing DeleteImages() to take > a vector of ids as well. > if we want to go that route (i'm happy to write the patch), then passing the > data in the data structure we actually need seems more plausible. I think the main reason for doing it that way was laziness :) The caller had a vector of snippets anyway, so this saved some copying. > If you feel strongly, I can change that of course ;-) Not feeling too strongly either way, I just like consistency. If you promise to do a followup that makes things consistent (either way), then I'm fine with this for now. https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:496: ClearExpiredDismissedSnippets(); On 2016/10/06 10:56:11, tschumann wrote: > On 2016/10/06 10:21:30, Marc Treib wrote: > > This won't do anything, since |categories_| is still empty at this point. > > are you sure? we have a DCHECK() above verifying it includes the articles > category... Ah okay, correction: The category itself will be there, but it won't have any suggestions - those are added just below. So my point "this won't do anything" still stands :) If you make a set of IDs anyway, you might as well collect them from categories_[articles_category_]->snippets and ->dismissed directly in ClearOrphanedImages instead of passing in |snippets|, and move these two lines down again. Or am I missing something? https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:236: base::Callback<void(const std::string&, const gfx::Image&)> callback) { On 2016/10/06 10:56:11, tschumann wrote: > On 2016/10/06 10:21:30, Marc Treib wrote: > > Add a using declaration for this callback type? ImageFetchedCallback? > > not sure. IMO, this should be exposed by the ImageFetcher API -- not a big fan > of introducing aliases for other libraries. Ah indeed, good point! If you want to do this, go ahead, I'm an owner of components/image_fetcher too :) but otherwise this is fine as-is. > Also, it's not a big burden in test code (once written ;-), and we also have it > in production code.
https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database.h (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.h:81: std::unique_ptr<std::set<std::string>> alive_snippets); On 2016/10/06 11:37:20, Marc Treib wrote: > On 2016/10/06 10:56:10, tschumann wrote: > > On 2016/10/06 10:21:30, Marc Treib wrote: > > > Any particular reason for using std::set here? I'd pass in a const > > > NTPSnippet::PtrVector&, to be consistent with the other methods (and build > the > > > set inside). > > > > hmmm... it feels a bit weird that the function call wants a whole snippet if > the > > snippet-id is sufficient. I'd actually vote for changing DeleteImages() to > take > > a vector of ids as well. > > if we want to go that route (i'm happy to write the patch), then passing the > > data in the data structure we actually need seems more plausible. > > I think the main reason for doing it that way was laziness :) The caller had a > vector of snippets anyway, so this saved some copying. > > > If you feel strongly, I can change that of course ;-) > > Not feeling too strongly either way, I just like consistency. If you promise to > do a followup that makes things consistent (either way), then I'm fine with this > for now. These methods were not called in many places. Just updated within this CL. PTAL. https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:496: ClearExpiredDismissedSnippets(); On 2016/10/06 11:37:20, Marc Treib wrote: > On 2016/10/06 10:56:11, tschumann wrote: > > On 2016/10/06 10:21:30, Marc Treib wrote: > > > This won't do anything, since |categories_| is still empty at this point. > > > > are you sure? we have a DCHECK() above verifying it includes the articles > > category... > > Ah okay, correction: The category itself will be there, but it won't have any > suggestions - those are added just below. So my point "this won't do anything" > still stands :) > > If you make a set of IDs anyway, you might as well collect them from > categories_[articles_category_]->snippets and ->dismissed directly in > ClearOrphanedImages instead of passing in |snippets|, and move these two lines > down again. Or am I missing something? nope -- you're not :-) added a TODO() to increase test coverage as no test was failing. thanks again for the catch! https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2386103009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:236: base::Callback<void(const std::string&, const gfx::Image&)> callback) { On 2016/10/06 11:37:20, Marc Treib wrote: > On 2016/10/06 10:56:11, tschumann wrote: > > On 2016/10/06 10:21:30, Marc Treib wrote: > > > Add a using declaration for this callback type? ImageFetchedCallback? > > > > not sure. IMO, this should be exposed by the ImageFetcher API -- not a big fan > > of introducing aliases for other libraries. > > Ah indeed, good point! If you want to do this, go ahead, I'm an owner of > components/image_fetcher too :) but otherwise this is fine as-is. > > > Also, it's not a big burden in test code (once written ;-), and we also have > it > > in production code. > Added it to my TODO() list.
lgtm https://codereview.chromium.org/2386103009/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database.cc (right): https://codereview.chromium.org/2386103009/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.cc:301: nit: remove the extra empty line https://codereview.chromium.org/2386103009/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database.h (right): https://codereview.chromium.org/2386103009/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.h:81: std::unique_ptr<std::set<std::string>> alive_snippets); Hm, now this one takes a set, but all the others take vectors... oh well. nit: alive_snippet_ids? https://codereview.chromium.org/2386103009/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2386103009/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:144: result->emplace_back(snippet->id()); nit: push_back will actually do the same thing as emplace_back here (it has to do a copy anyway). I slightly prefer push_back unless you actually make use of the in-place construction.
https://codereview.chromium.org/2386103009/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_database.h (right): https://codereview.chromium.org/2386103009/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_database.h:81: std::unique_ptr<std::set<std::string>> alive_snippets); On 2016/10/06 12:47:35, Marc Treib wrote: > Hm, now this one takes a set, but all the others take vectors... oh well. > > nit: alive_snippet_ids? renamed. :-) IMO that's fine. If we'd ever need this differently we can always change. But there's no harm in providing the data in a format that fits the use nicely ;-) https://codereview.chromium.org/2386103009/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2386103009/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:144: result->emplace_back(snippet->id()); On 2016/10/06 12:47:35, Marc Treib wrote: > nit: push_back will actually do the same thing as emplace_back here (it has to > do a copy anyway). I slightly prefer push_back unless you actually make use of > the in-place construction. I do actually agree. I'm not a big fan of emplace_back() but too lazy to think about which to use when ;-). Changed to push_back().
The CQ bit was checked by tschumann@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org, treib@chromium.org Link to the patchset: https://codereview.chromium.org/2386103009/#ps100001 (title: "final comments")
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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== NTPSnippetsService: Garbage collect orphaned images at startup. Also extends the unittest to use a proper image_decoder to allow better test coverage (and actually verifying the intended behavior). BUG=649009 ========== to ========== NTPSnippetsService: Garbage collect orphaned images at startup. Also extends the unittest to use a proper image_decoder to allow better test coverage (and actually verifying the intended behavior). BUG=649009 Committed: https://crrev.com/0d475e7d4ed829bd888d8ee18de17de9ef5d8684 Cr-Commit-Position: refs/heads/master@{#423529} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0d475e7d4ed829bd888d8ee18de17de9ef5d8684 Cr-Commit-Position: refs/heads/master@{#423529} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
