Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2421463002:
FetchMore functionality backend (Closed)
DescriptionThe ContentSuggestionService now provides a |FetchMore| method that asks any provider to provide more content for the chosen category.
There is an implementation for the NtpSnippetsService.
Changes integrated into and landed here:
https://codereview.chromium.org/2473483006
BUG=634892
Patch Set 1 : Strategy pattern to handle differences in fetching procedure. #
Total comments: 22
Patch Set 2 : Introduced callback, removed strategy. #
Total comments: 65
Patch Set 3 : NTBR. Rebasing a lot. #
Total comments: 40
Patch Set 4 : Added changes from CLs 2446163005 and 2466863003. #Patch Set 5 : ID set reference, Optional callback, ... (2466863003 comments). #
Total comments: 10
Messages
Total messages: 25 (12 generated)
Patchset #1 (id:1) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
fhorschig@chromium.org changed reviewers: + treib@chromium.org
Hi Marc, would you please have a look into the first steps of the FetchMore functionality? Cheers, Friedrich
First round of comments below. Apart from that, there's one major product question which AFAIK we haven't decided, which will likely influence the design here: How "sticky" are the additional suggestions? E.g.: You're on an NTP, have 10 suggestions, click "more", now you have 20. You open a new NTP - do you get 10 or 20 suggestions? I'd say probably 10. What if, on the original NTP, you click one suggestion, then navigate back? Now you probably want to see the 20 again. Have you thought about this? Does the PRD have anything on this? https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:104: // Provides only suggestions that have not already been provided. There's no "given category" - there probably should be? One provider can provide more than one category. How does it provide suggestions? There's no return value or callback. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:133: // already loaded. This shouldn't talk about providers - users of this class don't care about that. Instead, describe what it does - what's the expected result? https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.h:68: base::OnceCallback<void(OptionalFetchedCategories fetched_categories)>; If the callback is now for one fetch only, it should be passed directly to FetchSnippets - no point in a separate SetCallback. One thing to consider: What happends when FetchSnippets is called again before the previous one has completed? https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:252: /* interactive_request= */ true, nit: misaligned There's a few other formatting things around here - try running "git cl format" to automatically fix most of them. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:647: void NTPSnippetsService::AssignExpiryDates(NTPSnippet::PtrVector* snippets) { nit: this sets both publish and expiry dates. Also, it doesn't seem to access any members, so it could be a global helper function inside an anonymous namespace. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:659: void NTPSnippetsService::AddIncompleteSnippets( This has exactly the wrong name - it *removes* incomplete snippets. And same as above: Could probably be a global helper? https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:663: return; nit: braces if either the condition or the body don't fit on one line https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.h:331: friend class NewNTPSnippetsHandlingStrategy; Hm... TBH, I'm not sure I like this. Clearly, NTPSnippetsService has grown too big, and we should consider splitting it into multiple smaller classes at some point. But IMO this doesn't really help - consider: - Given that it's a friend and calls private methods, it doesn't provide any encapsulation. It's not possible to read (and understand) the code for either the service or the strategy by itself. - IMO it makes the code harder to read, because the logic is more scattered - to understand what Service::StoreSnippets does, you'll have to figure out what strategy you currently have, and then have to jump back and forth between Service and Strategy. The alternative would be to keep all the logic inline, and pass around a "bool is_more_request". I think that'd be simpler overall - WDYT?
Patchset #2 (id:100001) has been deleted
Thank you for the comments! Could you please have another look and confirm/reject my idea? https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:104: // Provides only suggestions that have not already been provided. On 2016/10/18 08:17:45, Marc Treib wrote: > There's no "given category" - there probably should be? One provider can provide > more than one category. > How does it provide suggestions? There's no return value or callback. Done. It used to notificate an Observer. Now there is a callback. The category was included. The backend doesn't support a category filter yet so all categories are fetched and unwanted ones are discarded. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:133: // already loaded. On 2016/10/18 08:17:45, Marc Treib wrote: > This shouldn't talk about providers - users of this class don't care about that. > Instead, describe what it does - what's the expected result? Done. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.h:68: base::OnceCallback<void(OptionalFetchedCategories fetched_categories)>; On 2016/10/18 08:17:45, Marc Treib wrote: > If the callback is now for one fetch only, it should be passed directly to > FetchSnippets - no point in a separate SetCallback. > > One thing to consider: What happends when FetchSnippets is called again before > the previous one has completed? The callback is passed into FetchSnippets. The descriptive comment of this function states: "If an ongoing fetch exists, it will be cancelled and a new one without triggering an additional callback [...]" I haven't verified this behavior but I have faith in whoever wrote this. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:252: /* interactive_request= */ true, On 2016/10/18 08:17:45, Marc Treib wrote: > nit: misaligned > There's a few other formatting things around here - try running "git cl format" > to automatically fix most of them. Done. Thank you! https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:647: void NTPSnippetsService::AssignExpiryDates(NTPSnippet::PtrVector* snippets) { On 2016/10/18 08:17:45, Marc Treib wrote: > nit: this sets both publish and expiry dates. Also, it doesn't seem to access > any members, so it could be a global helper function inside an anonymous > namespace. Done. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:659: void NTPSnippetsService::AddIncompleteSnippets( On 2016/10/18 08:17:45, Marc Treib wrote: > This has exactly the wrong name - it *removes* incomplete snippets. And same as > above: Could probably be a global helper? Done. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:663: return; On 2016/10/18 08:17:45, Marc Treib wrote: > nit: braces if either the condition or the body don't fit on one line Done. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.h:331: friend class NewNTPSnippetsHandlingStrategy; On 2016/10/18 08:17:45, Marc Treib wrote: > Hm... TBH, I'm not sure I like this. Clearly, NTPSnippetsService has grown too > big, and we should consider splitting it into multiple smaller classes at some > point. But IMO this doesn't really help - consider: > - Given that it's a friend and calls private methods, it doesn't provide any > encapsulation. It's not possible to read (and understand) the code for either > the service or the strategy by itself. > - IMO it makes the code harder to read, because the logic is more scattered - to > understand what Service::StoreSnippets does, you'll have to figure out what > strategy you currently have, and then have to jump back and forth between > Service and Strategy. > > The alternative would be to keep all the logic inline, and pass around a "bool > is_more_request". I think that'd be simpler overall - WDYT? I absolutely agree with the private methods and I removed the strategy for now as it depended on too much private members. I personally consider inheritance much more understandable than a high nesting depth which worsens with an additional bool parameter. Currently, the logic is inlined, which forced me to pass a total of 7 parameters to NTPSnippetsFetcher::FetchSnippetsFromHost. I would propose to introduce a data object that handles the request parameters, the callback and would function as delegate for the snippets_fetcher_. This would require logic to move out of both classes ... What is your opinion about this approach?
Bunch more comments, and kinda-product questions :) https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.h:68: base::OnceCallback<void(OptionalFetchedCategories fetched_categories)>; On 2016/10/20 13:10:38, fhorschig wrote: > On 2016/10/18 08:17:45, Marc Treib wrote: > > If the callback is now for one fetch only, it should be passed directly to > > FetchSnippets - no point in a separate SetCallback. > > > > One thing to consider: What happends when FetchSnippets is called again before > > the previous one has completed? > > > The callback is passed into FetchSnippets. > > The descriptive comment of this function states: > "If an ongoing fetch exists, it will be cancelled and a new one without > triggering an additional callback [...]" > I haven't verified this behavior but I have faith in whoever wrote this. Well, that's what the previous behavior amounted to, but you're changing that. See other comment. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.h:331: friend class NewNTPSnippetsHandlingStrategy; On 2016/10/20 13:10:38, fhorschig wrote: > On 2016/10/18 08:17:45, Marc Treib wrote: > > Hm... TBH, I'm not sure I like this. Clearly, NTPSnippetsService has grown too > > big, and we should consider splitting it into multiple smaller classes at some > > point. But IMO this doesn't really help - consider: > > - Given that it's a friend and calls private methods, it doesn't provide any > > encapsulation. It's not possible to read (and understand) the code for either > > the service or the strategy by itself. > > - IMO it makes the code harder to read, because the logic is more scattered - > to > > understand what Service::StoreSnippets does, you'll have to figure out what > > strategy you currently have, and then have to jump back and forth between > > Service and Strategy. > > > > The alternative would be to keep all the logic inline, and pass around a "bool > > is_more_request". I think that'd be simpler overall - WDYT? > > I absolutely agree with the private methods and I removed the strategy for now > as it depended on too much private members. > I personally consider inheritance much more understandable than a high nesting > depth which worsens with an additional bool parameter. > > Currently, the logic is inlined, which forced me to pass a total of 7 parameters > to NTPSnippetsFetcher::FetchSnippetsFromHost. I would propose to introduce a > data object that handles the request parameters, the callback and would function > as delegate for the snippets_fetcher_. This would require logic to move out of > both classes ... What is your opinion about this approach? I guess that might work... it's hard to judge without looking into it in detail. In any case, I'd suggest doing restructurings like that in a separate CL (before or after this one, whichever is easier). https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:157: // Ignored. Hm. In principle, the bookmarks provider (like most/all others) could implement "more" functionality. Right now there's no way to access it, because the "more" button instead opens the bookmark manager. But have you discussed this with Patrick? If we don't expect to ever get here, then put a NOTREACHED() there. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:106: // Provides only suggestions that have not been provided. The given |callback| What if I call this method twice in a row - will I get the same suggestions twice? If not, how does the provider know what to provide? Example: User is on the NTP, sees 10 suggestions. Clicks more -> 10 more (call them #11-#20). Clicks more again -> another 10 (#21-#30). However, now the user opens a new NTP, and gets the first 10 suggestions again. Clicks more -> we'd like to return #11-#20 again. I think it might be necessary to pass in a list of "known" suggestions. WDYT? https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:109: FetchedMoreCallback callback) = 0; This should be a const&. (Or alternatively, it could be a OnceCallback, but then we should also change that in other places, like ImageFetchedCallback above. Probably best to do that separately.) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:37: class NTPSnippetsFetcher; Not needed I think? https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:144: void FetchMore(const Category& category, FetchedMoreCallback callback); const& https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:589: EXPECT_CALL(*provider, FetchMore(category, testing::_)); nit: "testing::" not required (there's a "using" above) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:590: service()->FetchMore(category, MakeDummyCallback()); You can just put a null callback here, as in "FetchedMoreCallback()". https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:772: if (category_it == categories->end()) Shouldn't we erase everything in this case? https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:787: // no reason to overfetch and filter here. nit: We don't align after the TODO. wanted or unwanted? We're not modifying any request. Also typo: "soo" and "no no" https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:114: // the owner of |callback|). This is not accurate anymore: The owner of the previous callback will never be called back. (This is what I meant with the comment before.) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:126: SnippetsAvailableCallback callback, nit: I'd put callback last (all the others are input params, while callback is essentially an output) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:220: void FilterCategories(OptionalFetchedCategories* fetched); nit: const (or you could convert it to a non-member helper function in an anonymous namespace in the .cc) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:274: // Is the request supposed to fetch one category excusively? Which one? Typo: excusively (but I'm not a fan of the question style anyway... maybe "If set, the request is for this category only; otherwise it's for all categories."?) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:735: case static_cast<int>(KnownCategories::ARTICLES): Use CategoryFactory::IsKnownCategory(category.id(), ARTICLES) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:196: return NTPSnippetsService::FetchedMoreCallback(); Just inline this below. Also "NTPSnippetsService::" isn't needed. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:281: void NTPSnippetsService::FetchSnippetsFromHosts( This doesn't do anything now - might as well remove it and just call the Impl directly. (And then maybe rename that to FetchSnippetsImpl, no "FromHosts") https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:311: std::move(CollectIdsToExclude(fetch_more)), kMaxSnippetCount, std::move isn't needed here (it's movable anyway) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:706: void NTPSnippetsService::SaveSnippets(const Category& category, So this is only called once, just above? Then I wouldn't make it a separate method. (Also I find it a bit weird that a method called "SaveSnippets" would archive existing ones.) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:724: void NTPSnippetsService::ArchiveSnippets(Category category, Why did you move this? It makes the diff harder to read (I can't easily see if you changed anything here), and it's now also a different order from the header. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:1026: void NTPSnippetsService::NotifyNewSuggestions( Hm, this now kinda does two totally different things. How about pulling the (common) loop out into a separate ConvertSnippetsToSuggestions method, and making two separate Notify methods? https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:156: class NTPSnippetsFetchingStrategy; Doesn't exist anymore https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:218: FetchedMoreCallback fetched_more_callback, Hm. Should we maybe have two different FetchFinished methods for more vs. regular fetches? (I dislike parameters that are used only sometimes) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:227: // If |replace_snippets| is set, archive old snippets. s/old/existing/ ? https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:232: void SaveSnippets(const Category& category, What's the difference between IncludeSnippets and SaveSnippets? https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:290: void NotifyNewSuggestions(bool fetched_more, Heads-up: This will conflict with https://chromiumcodereview.appspot.com/2438543003/ https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:313: // For |fetch more| requests, all known snippets are returned. IDs (capitalize) "are should" "akways" fetch_more (underscore) s/all known snippets are returned/returns all known snippet IDs/ https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:314: std::set<std::string> CollectIdsToExclude(bool fetch_more); const https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:360: CategoryContentMap categories_; Any reason for this change? https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:532: MOCK_METHOD0(MustCallback, void()); No need for a class - you can use a MockFunction for the mock method, and probably just base::Bind an empty lambda for the non-checked one. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:882: LoadMoreFromJSONString(MakeSnippetsService().get(), Hm, like this, the SnippetsService gets destroyed immediately after LoadMoreFromJSONString finished. Could that cause any trouble? https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:888: SizeIs(0)); You can also do EXPECT_CALL(...).Times(0) to make sure it really isn't called.
overall, I like the direction. Some drive-by comments as this discussion uncovers some design issues. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.h:68: base::OnceCallback<void(OptionalFetchedCategories fetched_categories)>; On 2016/10/20 16:51:39, Marc Treib wrote: > On 2016/10/20 13:10:38, fhorschig wrote: > > On 2016/10/18 08:17:45, Marc Treib wrote: > > > If the callback is now for one fetch only, it should be passed directly to > > > FetchSnippets - no point in a separate SetCallback. > > > > > > One thing to consider: What happends when FetchSnippets is called again > before > > > the previous one has completed? > > > > > > The callback is passed into FetchSnippets. > > > > The descriptive comment of this function states: > > "If an ongoing fetch exists, it will be cancelled and a new one without > > triggering an additional callback [...]" > > I haven't verified this behavior but I have faith in whoever wrote this. > > Well, that's what the previous behavior amounted to, but you're changing that. > See other comment. never trust untested code ;-) A test would be really nice for that scenario. https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.h:331: friend class NewNTPSnippetsHandlingStrategy; On 2016/10/20 16:51:39, Marc Treib wrote: > On 2016/10/20 13:10:38, fhorschig wrote: > > On 2016/10/18 08:17:45, Marc Treib wrote: > > > Hm... TBH, I'm not sure I like this. Clearly, NTPSnippetsService has grown > too > > > big, and we should consider splitting it into multiple smaller classes at > some > > > point. But IMO this doesn't really help - consider: > > > - Given that it's a friend and calls private methods, it doesn't provide any > > > encapsulation. It's not possible to read (and understand) the code for > either > > > the service or the strategy by itself. > > > - IMO it makes the code harder to read, because the logic is more scattered > - > > to > > > understand what Service::StoreSnippets does, you'll have to figure out what > > > strategy you currently have, and then have to jump back and forth between > > > Service and Strategy. > > > > > > The alternative would be to keep all the logic inline, and pass around a > "bool > > > is_more_request". I think that'd be simpler overall - WDYT? > > > > I absolutely agree with the private methods and I removed the strategy for now > > as it depended on too much private members. > > I personally consider inheritance much more understandable than a high nesting > > depth which worsens with an additional bool parameter. > > > > Currently, the logic is inlined, which forced me to pass a total of 7 > parameters > > to NTPSnippetsFetcher::FetchSnippetsFromHost. I would propose to introduce a > > data object that handles the request parameters, the callback and would > function > > as delegate for the snippets_fetcher_. This would require logic to move out of > > both classes ... What is your opinion about this approach? > > I guess that might work... it's hard to judge without looking into it in detail. > In any case, I'd suggest doing restructurings like that in a separate CL (before > or after this one, whichever is easier). What about introducing a NTPSnippetsFetcher::Parameters struct which we could simply populate here? This should help with the many parameters. For the strategy, I'd introduce and enum first and implement the logic inside the service and then have a look what a good abstraction would be. As Marc pointed out, we need to introduce an additional abstraction and I'd rather see another layer on top of the ntp snippets database which hides away snippets archival and could also do replacements and extensions of categories. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (left): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:319: void NTPSnippetsFetcher::SetCallback( it's nice to see this method gone. However, i'm still not 100% sure I like the logic of "you pass a callback into FetchSnippetsFromHosts(), and we maybe call it back". Would it be hard to make this a proper contract and always call it once the request finishes? Shouldn't the higher layer (which issues multiple calls) deal with how to deal with races? Looking at the snippet fetcher implementation, there's also some room for refactoring. A lot of state of the NTPSnippetsFetcher is per-request. So we could separate the snippets fetching logic from the request state (introduce an internal request object which would also hold the callback, request parameters, etc.)
https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (left): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:319: void NTPSnippetsFetcher::SetCallback( On 2016/10/24 06:38:25, tschumann wrote: > it's nice to see this method gone. However, i'm still not 100% sure I like the > logic of "you pass a callback into FetchSnippetsFromHosts(), and we maybe call > it back". Would it be hard to make this a proper contract and always call it > once the request finishes? Shouldn't the higher layer (which issues multiple > calls) deal with how to deal with races? > > Looking at the snippet fetcher implementation, there's also some room for > refactoring. A lot of state of the NTPSnippetsFetcher is per-request. So we > could separate the snippets fetching logic from the request state (introduce an > internal request object which would also hold the callback, request parameters, > etc.) Yes, we actually discussed this, and agree it's a good idea. I'd prefer that kind of restructuring to happen in a separate CL though - let's keep restructurings separate from new functionality if possible.
Most comments addressed (in https://codereview.chromium.org/2446163005/#ps60001) https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.h:68: base::OnceCallback<void(OptionalFetchedCategories fetched_categories)>; On 2016/10/20 16:51:39, Marc Treib wrote: > On 2016/10/20 13:10:38, fhorschig wrote: > > On 2016/10/18 08:17:45, Marc Treib wrote: > > > If the callback is now for one fetch only, it should be passed directly to > > > FetchSnippets - no point in a separate SetCallback. > > > > > > One thing to consider: What happends when FetchSnippets is called again > before > > > the previous one has completed? > > > > > > The callback is passed into FetchSnippets. > > > > The descriptive comment of this function states: > > "If an ongoing fetch exists, it will be cancelled and a new one without > > triggering an additional callback [...]" > > I haven't verified this behavior but I have faith in whoever wrote this. > > Well, that's what the previous behavior amounted to, but you're changing that. > See other comment. We decided to fix this after this CL (crbug.com/659931). https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2421463002/diff/80001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.h:331: friend class NewNTPSnippetsHandlingStrategy; On 2016/10/24 06:38:25, tschumann wrote: > On 2016/10/20 16:51:39, Marc Treib wrote: > > On 2016/10/20 13:10:38, fhorschig wrote: > > > On 2016/10/18 08:17:45, Marc Treib wrote: > > > > Hm... TBH, I'm not sure I like this. Clearly, NTPSnippetsService has grown > > too > > > > big, and we should consider splitting it into multiple smaller classes at > > some > > > > point. But IMO this doesn't really help - consider: > > > > - Given that it's a friend and calls private methods, it doesn't provide > any > > > > encapsulation. It's not possible to read (and understand) the code for > > either > > > > the service or the strategy by itself. > > > > - IMO it makes the code harder to read, because the logic is more > scattered > > - > > > to > > > > understand what Service::StoreSnippets does, you'll have to figure out > what > > > > strategy you currently have, and then have to jump back and forth between > > > > Service and Strategy. > > > > > > > > The alternative would be to keep all the logic inline, and pass around a > > "bool > > > > is_more_request". I think that'd be simpler overall - WDYT? > > > > > > I absolutely agree with the private methods and I removed the strategy for > now > > > as it depended on too much private members. > > > I personally consider inheritance much more understandable than a high > nesting > > > depth which worsens with an additional bool parameter. > > > > > > Currently, the logic is inlined, which forced me to pass a total of 7 > > parameters > > > to NTPSnippetsFetcher::FetchSnippetsFromHost. I would propose to introduce a > > > data object that handles the request parameters, the callback and would > > function > > > as delegate for the snippets_fetcher_. This would require logic to move out > of > > > both classes ... What is your opinion about this approach? > > > > I guess that might work... it's hard to judge without looking into it in > detail. > > In any case, I'd suggest doing restructurings like that in a separate CL > (before > > or after this one, whichever is easier). > > What about introducing a NTPSnippetsFetcher::Parameters struct which we could > simply populate here? This should help with the many parameters. > For the strategy, I'd introduce and enum first and implement the logic inside > the service and then have a look what a good abstraction would be. As Marc > pointed out, we need to introduce an additional abstraction and I'd rather see > another layer on top of the ntp snippets database which hides away snippets > archival and could also do replacements and extensions of categories. NTPSnippetsFetcher::FetchSnippets now takes a Params struct (https://codereview.chromium.org/2454063002/) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.cc:157: // Ignored. On 2016/10/20 16:51:39, Marc Treib wrote: > Hm. In principle, the bookmarks provider (like most/all others) could implement > "more" functionality. Right now there's no way to access it, because the "more" > button instead opens the bookmark manager. But have you discussed this with > Patrick? > If we don't expect to ever get here, then put a NOTREACHED() there. I discussed this with dgn and bauerb: We'll unify "get suggestions" (currently implemented via the observer) and "fetch more". They'll fill you in on the details :) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:106: // Provides only suggestions that have not been provided. The given |callback| On 2016/10/20 16:51:39, Marc Treib wrote: > What if I call this method twice in a row - will I get the same suggestions > twice? If not, how does the provider know what to provide? > > Example: User is on the NTP, sees 10 suggestions. Clicks more -> 10 more (call > them #11-#20). Clicks more again -> another 10 (#21-#30). > However, now the user opens a new NTP, and gets the first 10 suggestions again. > Clicks more -> we'd like to return #11-#20 again. > I think it might be necessary to pass in a list of "known" suggestions. WDYT? Also discussed with dgn and bauerb: Yep, we'll pass a list of known suggestion IDs. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:37: class NTPSnippetsFetcher; On 2016/10/20 16:51:39, Marc Treib wrote: > Not needed I think? Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:144: void FetchMore(const Category& category, FetchedMoreCallback callback); On 2016/10/20 16:51:39, Marc Treib wrote: > const& Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service_unittest.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:589: EXPECT_CALL(*provider, FetchMore(category, testing::_)); On 2016/10/20 16:51:39, Marc Treib wrote: > nit: "testing::" not required (there's a "using" above) Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service_unittest.cc:590: service()->FetchMore(category, MakeDummyCallback()); On 2016/10/20 16:51:39, Marc Treib wrote: > You can just put a null callback here, as in "FetchedMoreCallback()". Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:787: // no reason to overfetch and filter here. On 2016/10/20 16:51:39, Marc Treib wrote: > nit: We don't align after the TODO. > wanted or unwanted? > We're not modifying any request. > Also typo: "soo" and "no no" Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:220: void FilterCategories(OptionalFetchedCategories* fetched); On 2016/10/20 16:51:39, Marc Treib wrote: > nit: const (or you could convert it to a non-member helper function in an > anonymous namespace in the .cc) Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:274: // Is the request supposed to fetch one category excusively? Which one? On 2016/10/20 16:51:40, Marc Treib wrote: > Typo: excusively (but I'm not a fan of the question style anyway... maybe "If > set, the request is for this category only; otherwise it's for all > categories."?) Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:735: case static_cast<int>(KnownCategories::ARTICLES): On 2016/10/20 16:51:40, Marc Treib wrote: > Use CategoryFactory::IsKnownCategory(category.id(), ARTICLES) Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:196: return NTPSnippetsService::FetchedMoreCallback(); On 2016/10/20 16:51:40, Marc Treib wrote: > Just inline this below. Also "NTPSnippetsService::" isn't needed. Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:281: void NTPSnippetsService::FetchSnippetsFromHosts( On 2016/10/20 16:51:40, Marc Treib wrote: > This doesn't do anything now - might as well remove it and just call the Impl > directly. (And then maybe rename that to FetchSnippetsImpl, no "FromHosts") ...buut it's public, and used by the internals page, so leaving it there for now. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:311: std::move(CollectIdsToExclude(fetch_more)), kMaxSnippetCount, On 2016/10/20 16:51:40, Marc Treib wrote: > std::move isn't needed here (it's movable anyway) Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:706: void NTPSnippetsService::SaveSnippets(const Category& category, On 2016/10/20 16:51:40, Marc Treib wrote: > So this is only called once, just above? Then I wouldn't make it a separate > method. (Also I find it a bit weird that a method called "SaveSnippets" would > archive existing ones.) Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:724: void NTPSnippetsService::ArchiveSnippets(Category category, On 2016/10/20 16:51:40, Marc Treib wrote: > Why did you move this? It makes the diff harder to read (I can't easily see if > you changed anything here), and it's now also a different order from the header. Done. (Moved back) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:1026: void NTPSnippetsService::NotifyNewSuggestions( On 2016/10/20 16:51:40, Marc Treib wrote: > Hm, this now kinda does two totally different things. How about pulling the > (common) loop out into a separate ConvertSnippetsToSuggestions method, and > making two separate Notify methods? Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:156: class NTPSnippetsFetchingStrategy; On 2016/10/20 16:51:40, Marc Treib wrote: > Doesn't exist anymore Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:227: // If |replace_snippets| is set, archive old snippets. On 2016/10/20 16:51:40, Marc Treib wrote: > s/old/existing/ ? Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:232: void SaveSnippets(const Category& category, On 2016/10/20 16:51:40, Marc Treib wrote: > What's the difference between IncludeSnippets and SaveSnippets? Done. (Merged SaveSnippets into IncludeSnippets) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:290: void NotifyNewSuggestions(bool fetched_more, On 2016/10/20 16:51:40, Marc Treib wrote: > Heads-up: This will conflict with > https://chromiumcodereview.appspot.com/2438543003/ Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:313: // For |fetch more| requests, all known snippets are returned. On 2016/10/20 16:51:40, Marc Treib wrote: > IDs (capitalize) > "are should" > "akways" > fetch_more (underscore) > s/all known snippets are returned/returns all known snippet IDs/ Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:314: std::set<std::string> CollectIdsToExclude(bool fetch_more); On 2016/10/20 16:51:40, Marc Treib wrote: > const Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:360: CategoryContentMap categories_; On 2016/10/20 16:51:40, Marc Treib wrote: > Any reason for this change? Done. (reverted) https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:532: MOCK_METHOD0(MustCallback, void()); On 2016/10/20 16:51:40, Marc Treib wrote: > No need for a class - you can use a MockFunction for the mock method, and > probably just base::Bind an empty lambda for the non-checked one. Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:882: LoadMoreFromJSONString(MakeSnippetsService().get(), On 2016/10/20 16:51:40, Marc Treib wrote: > Hm, like this, the SnippetsService gets destroyed immediately after > LoadMoreFromJSONString finished. Could that cause any trouble? Done. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:888: SizeIs(0)); On 2016/10/20 16:51:40, Marc Treib wrote: > You can also do > EXPECT_CALL(...).Times(0) > to make sure it really isn't called. ...except that you can't, because the observer isn't a mock :)
vitaliii@chromium.org changed reviewers: + vitaliii@chromium.org
If you are unsure about some of my comments, feel free to ignore or to wait for Marc to have a look. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:45: void FetchMore(const Category& category, nit:FetchMoreSuggestions currently it is difficult to understand what is actually fetched by looking at the declaration only. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestion.cc:31: const NTPSnippet* snippet) { Why is this a pointer? I guess a constant link would work here. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:54: static ContentSuggestion FromSnippet(Category category, Can this be a constructor instead? https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:34: using FetchedMoreCallback = nit: FetchMoreCallback or MultipleContentSuggestionCallback https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:105: // A user-triggered request to fetch more content for the given category. "User-triggered" seems to be not relevant here, the comment should tell what the function does, e.g. "Fetches new content suggestions..." https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:106: // Provides only suggestions that have not been provided. The given |callback| This sentence looks contradicting to the next line. "Provides" is ambiguous to me in this context. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:107: // is called with these suggestion, along with all existing suggestions. s/suggestion/suggestionS https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:141: // completed, the given |callback| is called with the updated content. s/completed/successful? https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:781: std::move(params_.snippets_available_callback).Run( Why do you need this external |std::move|? https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:704: for (const auto& category : *fetched_categories) { There is no need to have a loop if there is an ASSERT_THAT(size, 1). https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:706: switch (category.category.id()) { Replace with an |if|. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:747: ASSERT_THAT(fetched_categories->size(), Eq(0u)); nit: EXPECT_THAT https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:186: int num_snippets_dismissed = num_snippets - snippets->size(); s/dismissed/removed? "dismissed" is usually used in a different context (e.g. user dismissed a suggestion). https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:288: NullCallback()); Actually writing here |NTPSnippetsService::FetchedMoreCallback()| would make it easier to understand what kind of callback this is and what the function parameter is. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:349: params->excluded_ids.insert(snippet->id()); I cannot understand why this is done here. First, this contradicts the name. Second, there is CollectIdsToExclude which does precisely the same. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:223: // Add newly available snippets in |category| to the provided content. s/Add/Adds https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:224: // If |replace_snippets| is set, archive existing snippets. nit: remove "is set" https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:286: // and calls a passed callback. If no callbakc is given it notifies the s/callbakc/callbaCK s/it/, https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:319: // Returns a set of snippet ids that are should not be fetched in subsequent grammar: remove "are" https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:320: // requests. These IDs akways include dismissed snippets. IDs is inconsistent with the previous line. s/akways/aLways
Rebased CL so that it doesn't break any new tests but can still be used as base for marc's CL 2446163005. Further comments were addressed in CL 2466863003. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:106: // Provides only suggestions that have not been provided. The given |callback| On 2016/10/28 14:49:49, Marc Treib wrote: > On 2016/10/20 16:51:39, Marc Treib wrote: > > What if I call this method twice in a row - will I get the same suggestions > > twice? If not, how does the provider know what to provide? > > > > Example: User is on the NTP, sees 10 suggestions. Clicks more -> 10 more (call > > them #11-#20). Clicks more again -> another 10 (#21-#30). > > However, now the user opens a new NTP, and gets the first 10 suggestions > again. > > Clicks more -> we'd like to return #11-#20 again. > > I think it might be necessary to pass in a list of "known" suggestions. WDYT? > > Also discussed with dgn and bauerb: Yep, we'll pass a list of known suggestion > IDs. Done in CL 2466863003. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:109: FetchedMoreCallback callback) = 0; On 2016/10/20 16:51:39, Marc Treib wrote: > This should be a const&. > (Or alternatively, it could be a OnceCallback, but then we should also change > that in other places, like ImageFetchedCallback above. Probably best to do that > separately.) Done. It's const for now. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:772: if (category_it == categories->end()) On 2016/10/20 16:51:39, Marc Treib wrote: > Shouldn't we erase everything in this case? Correct. Changed and tested in CL 2466863003. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:114: // the owner of |callback|). On 2016/10/20 16:51:39, Marc Treib wrote: > This is not accurate anymore: The owner of the previous callback will never be > called back. (This is what I meant with the comment before.) As discussed offline, this behavior will change with the followup CL. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.h:126: SnippetsAvailableCallback callback, On 2016/10/20 16:51:39, Marc Treib wrote: > nit: I'd put callback last (all the others are input params, while callback is > essentially an output) Done in CL 2466863003. https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2421463002/diff/120001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:218: FetchedMoreCallback fetched_more_callback, On 2016/10/20 16:51:40, Marc Treib wrote: > Hm. Should we maybe have two different FetchFinished methods for more vs. > regular fetches? (I dislike parameters that are used only sometimes) I would make that change as soon as we decided how to handle the callbacks within the SnippetsFetcher. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/bookmarks/bookmark_suggestions_provider.h:45: void FetchMore(const Category& category, On 2016/11/01 23:29:57, vitaliii wrote: > nit:FetchMoreSuggestions > currently it is difficult to understand what is actually fetched by looking at > the declaration only. The function is inherited from the SuggestionsProvider which makes it clear that suggestions are fetched. As the function might be used for other fetches, too, the name was changed in CL 2466863003. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestion.cc:31: const NTPSnippet* snippet) { On 2016/11/01 23:29:57, vitaliii wrote: > Why is this a pointer? > I guess a constant link would work here. Dropped. (see declaration). https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:54: static ContentSuggestion FromSnippet(Category category, On 2016/11/01 23:29:57, vitaliii wrote: > Can this be a constructor instead? Yes. This deprecated code was dropped in CL 2446163005. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:34: using FetchedMoreCallback = On 2016/11/01 23:29:57, vitaliii wrote: > nit: FetchMoreCallback or MultipleContentSuggestionCallback Renamed (but shorter) in CL 2466863003. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:105: // A user-triggered request to fetch more content for the given category. On 2016/11/01 23:29:57, vitaliii wrote: > "User-triggered" seems to be not relevant here, the comment should tell what the > function does, e.g. "Fetches new content suggestions..." Done. (in CL 2466863003) https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:106: // Provides only suggestions that have not been provided. The given |callback| On 2016/11/01 23:29:57, vitaliii wrote: > This sentence looks contradicting to the next line. "Provides" is ambiguous to > me in this context. Done. (in CL 2466863003) https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:107: // is called with these suggestion, along with all existing suggestions. On 2016/11/01 23:29:57, vitaliii wrote: > s/suggestion/suggestionS Done. (in CL 2466863003) https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:141: // completed, the given |callback| is called with the updated content. On 2016/11/01 23:29:57, vitaliii wrote: > s/completed/successful? It's also called if the call was not successful (e.g. the service wasn't ready yet). https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:781: std::move(params_.snippets_available_callback).Run( On 2016/11/01 23:29:57, vitaliii wrote: > Why do you need this external |std::move|? It's a "OnceCallback" that is consumed upon call: https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:704: for (const auto& category : *fetched_categories) { On 2016/11/01 23:29:57, vitaliii wrote: > There is no need to have a loop if there is an ASSERT_THAT(size, 1). Correct. Dropped in CL 2446163005. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:706: switch (category.category.id()) { On 2016/11/01 23:29:57, vitaliii wrote: > Replace with an |if|. Also correct. Also dropped in CL 2446163005. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:747: ASSERT_THAT(fetched_categories->size(), Eq(0u)); On 2016/11/01 23:29:57, vitaliii wrote: > nit: EXPECT_THAT Done in CL 2446163005. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:186: int num_snippets_dismissed = num_snippets - snippets->size(); On 2016/11/01 23:29:57, vitaliii wrote: > s/dismissed/removed? > "dismissed" is usually used in a different context (e.g. user dismissed a > suggestion). Done in CL 2466863003. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:288: NullCallback()); On 2016/11/01 23:29:57, vitaliii wrote: > Actually writing here |NTPSnippetsService::FetchedMoreCallback()| would make it > easier to understand what kind of callback this is and what the function > parameter is. Done in 2446163005. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:349: params->excluded_ids.insert(snippet->id()); On 2016/11/01 23:29:57, vitaliii wrote: > I cannot understand why this is done here. > First, this contradicts the name. > Second, there is CollectIdsToExclude which does precisely the same. This is a false merge. It is completely dropped later on. (These changes are part of the reason why I marked it NTBR ;D) https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.h (right): https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:223: // Add newly available snippets in |category| to the provided content. On 2016/11/01 23:29:58, vitaliii wrote: > s/Add/Adds Done in CL 2446163005. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:224: // If |replace_snippets| is set, archive existing snippets. On 2016/11/01 23:29:57, vitaliii wrote: > nit: remove "is set" Done in CL 2446163005. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:286: // and calls a passed callback. If no callbakc is given it notifies the On 2016/11/01 23:29:58, vitaliii wrote: > s/callbakc/callbaCK > s/it/, Done in CL 2446163005. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:319: // Returns a set of snippet ids that are should not be fetched in subsequent On 2016/11/01 23:29:58, vitaliii wrote: > grammar: remove "are" Done in CL 2446163005. https://codereview.chromium.org/2421463002/diff/140001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.h:320: // requests. These IDs akways include dismissed snippets. On 2016/11/01 23:29:57, vitaliii wrote: > IDs is inconsistent with the previous line. > s/akways/aLways Done in CL 2446163005.
Patchset #4 (id:160001) has been deleted
Patchset #5 (id:200001) has been deleted
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
lgtm adding a few comments to keep track no follow-ups I'll do after this CL is submitted. https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... File components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc (right): https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... components/ntp_snippets/physical_web_pages/physical_web_page_suggestions_provider.cc:98: NOTREACHED(); we should still call the passed-in callback. (also in other providers). https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:744: [exclusive](const FetchedCategory& c) -> bool { this should capture by reference. https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:763: // As soo as backends support the parameter, there is no fix formatting. (and soo->soon) https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc (right): https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:187: base::Optional<Category> OptionalArticlesCategory() { we should inline this. It only obfuscates reading the tests and it's actually quite short to write out. https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:696: snippets_fetcher().FetchSnippets(params, MakeMockCallback()); it would be cleaner to pass-in mock_callback(). Otherwise it's really hard to follow how mock_callback() should get executed. Maybe the method name is just a mis-nomer. We should rename it so that it reads like: ToSnippetsAvailableCallback(mock_callback()); https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_fetcher_unittest.cc:701: for (const auto& category : *fetched_categories) { if you already enforce the size of 1, then there's no need for the loop and the switch. (keep tests simple). https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:787: content->snippets.insert(content->snippets.end(), what happens if I open a new NTP after hitting 'more' on an old one? It looks like the new NTP would now also show more articles. I believe the simplest thing for now would be to only return a truncated list of snippets https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:1080: ConvertToContentSuggestions(category, content.snippets); i believe we need to limit this to the first 10 results so that newly opened NTPs don't show more items in case we clicked More on a different one. Can sync up here later.
dgn@chromium.org changed reviewers: + dgn@chromium.org
https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:1080: ConvertToContentSuggestions(category, content.snippets); On 2016/11/03 09:20:58, tschumann wrote: > i believe we need to limit this to the first 10 results so that newly opened > NTPs don't show more items in case we clicked More on a different one. > Can sync up here later. AFAICT, NotifyNewSuggestions() reports the suggestions to the ContentSuggestionService, and that one will store them. We reach there when we replace the current list of suggestion instead of appending, so we should still have 10. When we go through NotifyMoreSuggestions(), we don't notify the ContentSuggestionService, so the regular GetSuggestionsForCategory() will not see these extra suggestions. But a DCHECK or a test would be useful to ensure that it stays correct.
https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2421463002/diff/220001/components/ntp_snippet... components/ntp_snippets/remote/ntp_snippets_service.cc:1080: ConvertToContentSuggestions(category, content.snippets); On 2016/11/03 10:28:04, dgn wrote: > On 2016/11/03 09:20:58, tschumann wrote: > > i believe we need to limit this to the first 10 results so that newly opened > > NTPs don't show more items in case we clicked More on a different one. > > Can sync up here later. > > AFAICT, NotifyNewSuggestions() reports the suggestions to the > ContentSuggestionService, and that one will store them. We reach there when we > replace the current list of suggestion instead of appending, so we should still > have 10. > > When we go through NotifyMoreSuggestions(), we don't notify the > ContentSuggestionService, so the regular GetSuggestionsForCategory() will not > see these extra suggestions. > > But a DCHECK or a test would be useful to ensure that it stays correct. Ok got it. I was a bit confused thinking we would store the results inside the NTPSnippetsService which might have turned out problematic. This way it's fine. Will remove my TODO.
I just submitted https://codereview.chromium.org/2473483006/ which makes this CL unnecessary. Please delete at your convenience (also, feel free to double-check my CL didn't miss anything).
Description was changed from ========== The ContentSuggestionService now provides a |FetchMore| method that asks any provider to provide more content for the chosen category. There is an implementation for the NtpSnippetsService. BUG=634892 ========== to ========== The ContentSuggestionService now provides a |FetchMore| method that asks any provider to provide more content for the chosen category. There is an implementation for the NtpSnippetsService. Changes integrated into and landed here: https://codereview.chromium.org/2473483006 BUG=634892 ========== |
