Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2102023002:
Add ContentSuggestionsService (Closed)
|
Created:
4 years, 5 months ago by Philipp Keck Modified:
4 years, 5 months ago CC:
chromium-reviews, dewittj, ntp-dev+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ContentSuggestionsService
Add new ContentSuggestionsService, which acts as a mixer of suggestions
provided by different providers. Add the ContentSuggestionsProvider
interface to be implemented by these providers. Add the
ContentSuggestionsServiceFactory. Add CSCategoryStatus for providers to
report on the status of their categories. Rename
ContentSuggestionCategory to ContentSuggestionsCategory for consistency.
Remove ContentSuggestionsProviderType as there can only be one provider
for any category. Remove previously inserted ToContentSuggestion() from
NTPSnippet.
All new classes do not yet interact with existing code or even touch it
and the new code is not yet used by the UI. The new service is launched
by the ProfileManager but does not do anything yet.
BUG=619560
Committed: https://crrev.com/6dbb90af760531b04ca335d99cc68b448e0523b9
Cr-Commit-Position: refs/heads/master@{#404347}
Patch Set 1 #
Total comments: 87
Patch Set 2 : Rename ContentSuggestionsProvider::Delegate::OnContentChanged to OnSuggestionsChanged #
Total comments: 41
Patch Set 3 : comments #
Total comments: 8
Patch Set 4 : All comments plus results of meeting #Patch Set 5 : Minor corrections #
Total comments: 114
Patch Set 6 : Make ContentSuggestionsService::Enabled an explicit enum #Patch Set 7 : comments #
Total comments: 30
Patch Set 8 : More comments #Patch Set 9 : Add missing return statement #
Total comments: 2
Patch Set 10 : Remove ContentSuggestionsServiceType; allow at most one provider per category #
Total comments: 39
Patch Set 11 : Marks comments #
Total comments: 13
Patch Set 12 : Marks new comments; remove ContentSuggestion::category" #Patch Set 13 : Rename ContentSuggestionsState to ContentSuggestionsCategoryStatus; Add s to ContentSuggestionsCate… #
Total comments: 29
Patch Set 14 : Tims comments #Patch Set 15 : Register CSServiceFactory in ProfileManager and CSService::state getter #Patch Set 16 : Remove NTPSnippet::ToContentSuggestion and make ContentSuggestionsProvider::MakeUniqueID protected #
Total comments: 40
Patch Set 17 : Bernhard's and Marc's comments #
Total comments: 2
Patch Set 18 : Marks comments; Disable ContentSuggestionsService by default, only enable on Android #
Total comments: 2
Patch Set 19 : Remove unused imports from profile_manager.cc #Patch Set 20 : Add explicit cast from ContentSuggestionsCategory to int #Dependent Patchsets: Messages
Total messages: 74 (15 generated)
pke@google.com changed reviewers: + treib@chromium.org
PTAL. I have a working unit test, which I will commit after the offline pages provider is implemented.
https://codereview.chromium.org/2102023002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:48: base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature); Hm. I guess for now it's okay to reuse the same flag, but please add a TODO to split off a separate one. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestion.cc:18: ContentSuggestion& ContentSuggestion::operator=(ContentSuggestion&&) = default; nit: empty lines between methods please https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestion.h:33: ContentSuggestion(ContentSuggestion&&); nit: I'd move this up, just under the other ctor. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:8: #include <set> not needed https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:44: const ContentSuggestionsProvider& source, Should this just pass the provider type? Seems like that is all the service needs anyway. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:47: virtual void OnProviderShutdown(const ContentSuggestionsProvider& source){}; nit: space before "{" and no ";" (though really, this probably shouldn't have a default implementation) https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:69: // marking it as discarded internally. I'd remove the last sentence, since it's not really relevant for the interface. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:70: virtual void Discard(const std::string& suggestion_id) {} This shouldn't have a default implementation. Probably the Clear methods shouldn't either. Also, DiscardSuggestion? For (a) more clarity and (b) consistency with the Clear methods. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:73: // through the callback. This fetch may occur locally or from the Internet. nit: don't capitalize internet https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:7: #include "base/observer_list.h" Not needed, since it's already included in the header. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:14: if (enabled_) { Remove empty if. It looks like the enabled_ flag currently doesn't do anything at all? It probably should... https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:22: DCHECK(providers_.count(provider->GetProviderType()) == 0); DCHECK_EQ(0, ...) https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:48: ContentSuggestionsProviderType>::type>(provider_type); Argh, this is ugly - I guess it doesn't compile without the cast? IMO just "int(provider_type)" is good enough for a LOG statement. Or maybe it's worth adding a "string GetProviderTypeName(ProviderType)" function? https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:95: std::move(new_suggestions.begin(), new_suggestions.end(), #include <algorithm> https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:96: std::back_inserter(suggestions_[changed_category])); #include <iterator> https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:117: std::vector<ContentSuggestion>* list, Call this "suggestions" maybe? It's not actually a list. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:119: auto provider_matcher = [provider_type](const ContentSuggestion& s) { Inline this into the remove_if? https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:127: list->erase(remove_from, list->end()); You could also use the common erase(remove_if, end) idiom, and compare the return value to the old end() to see if anything changed. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:11: #include <set> not needed https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:14: #include "base/callback_forward.h" https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:29: // them to the NTP. I'd remove the reference to the NTP - that is not the service's business. Instead, you *could* give some more info on what it does, like grouping into categories, possibly ranking (though it doesn't really do that yet). https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:43: const std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>& Does this need to be exposed at all? If so, maybe worth a typedef ("using ... = std::map<...>"), though I'm leaning towards not exposing it at all. E.g. what about the order of categories? Like this, the service has no way of influencing it. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:47: const std::vector<ContentSuggestion>& suggestions( Please give this a better name, something like "GetSuggestionsForCategory"? Also, to make it useful, there really should be a "GetCategories" too. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:48: ContentSuggestionCategory category) { Should be const. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:64: void ClearSuggestions(); I think this is also used only for debugging purposes? (ClearDiscardedSuggestions has a comment saying that) Maybe rename to ClearCachedSuggestions, since that's closer to what actually happens? https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:73: void Discard(const ContentSuggestionsProviderType provider_type, No "const" if you pass by value. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:76: // Observer accessors (outgoing observers, from this service towards UI) I'd remove the part in parens - "outgoing" is obvious, that's what observers are for, and "towards the UI" is not necessarily correct, and anyway not the service's responsibility. Also, nit: period after comment. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:80: // Implementation of ContentSuggestionsProvider::Delegate (incoming data) These can (and should) be private. Also, nit: period after comment. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:87: // ContentSuggestionsProvider::setDelegate with a delegate object, through I'd remove the "The service..." sentence, since it's not really relevant for the interface. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:105: suggestions_; suggestions_by_category_? https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:117: ContentSuggestionsProviderType provider_type); Private methods come before member variables. But this probably doesn't need to be a member at all, in which case it should be a function in an anonymous namespace in the .cc file. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:122: class ContentSuggestionsServiceObserver { I'd make this an inner class of the service (just called Observer). AFAIK, the main motivation of making an observer a separate class is that it can be moved to a separate file, so clients can include only the observer without getting the whole service too. We could do that too here, but I don't see a particular reason to. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:126: // |suggestions()[changed_category]|. Please update to refer to the (proposed) GetSuggestionsForCategory method.
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
Sorry for the bystander comments. Just saw this CL flying by and wanted to clarify some topics. For most comments, I'd actually be interested in Marc's opinion as well. My goal is to figure out -- and document -- some implementation practices across the team. It's easiest to get those from real reviews ;-) So, take these with a grain of salt. They mostly represent my opinion and my goal is to distill a team's opinion =). --Tim https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.h (right): https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:17: struct DefaultSingletonTraits; never forward declare templates. It makes them pretty much impossible to change (think like someone wants to add an optional parameter). https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:21: class ContentSuggestionsService; I don't think this forward declaration is necessary. the only time when I encountered files you don't want to include in headers were pretty fat proto files (but proto.h also helps with this issue now). Mark: Is there some general practice on Chrome around forward declarations? https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:32: friend struct base::DefaultSingletonTraits<ContentSuggestionsServiceFactory>; nit: friends are usually a pretty deep implementation detail, why I usually move them to the bottom right before the DISALLOW_XXX macro. AFAIK, there's no rule about where to put them though. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:17: ContentSuggestion::ContentSuggestion(ContentSuggestion&&) = default; Why not move the default definitions into the header? I know that strikes an interesting balance between hiding implementation details, but honestly, it also documents the move semantics quite well, why I'd usually lean towards moving them to the header. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:32: class Delegate { I'm not super solid in design patterns, but wouldn't this be a observer or consumer? https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:43: virtual void OnSuggestionsChanged( sorry for nit-picking. It feels a bit weird that a provider notifies no changes. Shouldn't it notify on new data? And looking at the signature, OnNewSuggestions() looks like a better name as you don't give any information about what changed specifically. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:58: virtual void ClearSuggestions() {} wouldn't it be cleaner to make this a pure virtual class? https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:22: } necessary?
Thank you both for your feedback! Some of the code that Tim commented on is actually just copied from the current NTPSnippetsService, so then we should also rethink the implementation there. https://codereview.chromium.org/2102023002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:48: base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature); On 2016/06/28 11:29:08, Marc Treib wrote: > Hm. I guess for now it's okay to reuse the same flag, but please add a TODO to > split off a separate one. I have a separate "article-suggestions" feature in an upcoming CL. My intention is to leave the current "snippets feature", which is disabled by default, as the main toggle for the entire NTP suggestions stuff. Maybe rename it to "ntp-content-suggestions". The new "article-suggestions" and then "offline-pages-suggestions" features will be enabled by default and turn off the sub-providers. So: ContentSuggestionsService will be enabled iff "snippets" is true (exactly as coded here, possibly renamed). ArticleSuggestionsService (pka NTPSnippetsService) will be enabled iff "snippets && article-suggestions" is true (so I will add check for article-suggestions to the NTPSnippetsServiceFactory). Is that okay? Can features be renamed without further consequences? https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestion.cc:18: ContentSuggestion& ContentSuggestion::operator=(ContentSuggestion&&) = default; On 2016/06/28 11:29:08, Marc Treib wrote: > nit: empty lines between methods please Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestion.h:33: ContentSuggestion(ContentSuggestion&&); On 2016/06/28 11:29:08, Marc Treib wrote: > nit: I'd move this up, just under the other ctor. Done, I moved up both the move-constructor and the operator=. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:17: ContentSuggestion::ContentSuggestion(ContentSuggestion&&) = default; On 2016/06/28 11:47:17, tschumann wrote: > Why not move the default definitions into the header? > > I know that strikes an interesting balance between hiding implementation > details, but honestly, it also documents the move semantics quite well, why I'd > usually lean towards moving them to the header. I'm not a C++ expert, but I agree that this would indeed be nicer. It only defines the behaviour, but the code I wrote does not implement that behaviour (because obviously the default behaviour doesn't need to be implemented). But I think it's on the list of don'ts (http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts) because it is technically not a trivial constructor and the compiler throws an error if I put it in the header file. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:43: virtual void OnSuggestionsChanged( On 2016/06/28 11:47:17, tschumann wrote: > sorry for nit-picking. It feels a bit weird that a provider notifies no changes. > Shouldn't it notify on new data? And looking at the signature, > OnNewSuggestions() looks like a better name as you don't give any information > about what changed specifically. What do you mean by "notifies no changes"? This method is not only called for new suggestions, but also to delete suggestions - simply by not passing them anymore in the new list. I carefully avoided that term "new" because the current implementation should be differentiated from the fine-grained interface (Create, Update, Reorder, Delete) that we might implement in the future. The current implementation is equivalent to Android's notifyDataSetChanged (https://developer.android.com/reference/android/widget/BaseAdapter.html#notif...) in that the callee must go figure out what exactly changed on its own.
Some comments on Tim's comments :) https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.h (right): https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:17: struct DefaultSingletonTraits; On 2016/06/28 11:47:17, tschumann wrote: > never forward declare templates. It makes them pretty much impossible to change > (think like someone wants to add an optional parameter). I generally agree, though this particular thing is very common with these factory classes. That said, I'm also fine with just including the thing. https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:21: class ContentSuggestionsService; On 2016/06/28 11:47:17, tschumann wrote: > I don't think this forward declaration is necessary. the only time when I > encountered files you don't want to include in headers were pretty fat proto > files (but proto.h also helps with this issue now). > > Mark: Is there some general practice on Chrome around forward declarations? Yes. As per https://www.chromium.org/developers/coding-style: "Unlike the Google style guide, Chromium style prefers forward declarations to #includes where possible." https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:32: friend struct base::DefaultSingletonTraits<ContentSuggestionsServiceFactory>; On 2016/06/28 11:47:17, tschumann wrote: > nit: friends are usually a pretty deep implementation detail, why I usually move > them to the bottom right before the DISALLOW_XXX macro. > > AFAIK, there's no rule about where to put them though. This is the common style in Chrome AFAIK. There *is* a rule that DISALLOW_COPY_AND_ASSIGN goes last. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:17: ContentSuggestion::ContentSuggestion(ContentSuggestion&&) = default; On 2016/06/28 11:47:17, tschumann wrote: > Why not move the default definitions into the header? > > I know that strikes an interesting balance between hiding implementation > details, but honestly, it also documents the move semantics quite well, why I'd > usually lean towards moving them to the header. There's a style check that prohibits moving these into the header. ("non-trivial" ctor implementations must go into the .cc file, and "default" implementations apparently count as non-trivial. Maybe that should be changed in the style checker tool, but then again.. meh) https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:43: virtual void OnSuggestionsChanged( On 2016/06/28 11:47:17, tschumann wrote: > sorry for nit-picking. It feels a bit weird that a provider notifies no changes. Sorry, I don't get that.. it does notify, that's what this very method does...? > Shouldn't it notify on new data? And looking at the signature, > OnNewSuggestions() looks like a better name as you don't give any information > about what changed specifically. Agreed, OnNewSuggestions[Available] or something like that would probably be a better name. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:58: virtual void ClearSuggestions() {} On 2016/06/28 11:47:17, tschumann wrote: > wouldn't it be cleaner to make this a pure virtual class? Yep, probably. I commented more or less the same thing :)
You have lots of empty lines in the CL description; please remove those. https://codereview.chromium.org/2102023002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:48: base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature); On 2016/06/28 12:04:08, Philipp Keck wrote: > On 2016/06/28 11:29:08, Marc Treib wrote: > > Hm. I guess for now it's okay to reuse the same flag, but please add a TODO to > > split off a separate one. > > I have a separate "article-suggestions" feature in an upcoming CL. My intention > is to leave the current "snippets feature", which is disabled by default, as the > main toggle for the entire NTP suggestions stuff. Maybe rename it to > "ntp-content-suggestions". The new "article-suggestions" and then > "offline-pages-suggestions" features will be enabled by default and turn off the > sub-providers. Mostly SGTM, see below. > So: > ContentSuggestionsService will be enabled iff "snippets" is true (exactly as > coded here, possibly renamed). > ArticleSuggestionsService (pka NTPSnippetsService) will be enabled iff "snippets > && article-suggestions" is true (so I will add check for article-suggestions to > the NTPSnippetsServiceFactory). ArticleSuggestionsService probably shouldn't check for the "snippets" feature, but instead check if the ContentSuggestionsService exists. > Is that okay? Can features be renamed without further consequences? No, the actual string can not be renamed easily. We already have live configs that reference the existing name; we'd need to update them with both the old and new name, to support old and new Chrome versions. What we can do though is to rename only the variable (kNTPSnippetsFeature -> kNTPContentSuggestions or something like that), but keep the string as-is.
Description was changed from ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. BUG=619560 ========== to ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. BUG=619560 ==========
https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.h (right): https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:17: struct DefaultSingletonTraits; On 2016/06/28 12:05:19, Marc Treib wrote: > On 2016/06/28 11:47:17, tschumann wrote: > > never forward declare templates. It makes them pretty much impossible to > change > > (think like someone wants to add an optional parameter). > > I generally agree, though this particular thing is very common with these > factory classes. That said, I'm also fine with just including the thing. I'm not feeling strongly, but leaning towards not forward-declaring. Primarily because there might be less common types where we probably shouldn't forward declare them and cases should be rare in general. I'm also happy with an exception of commonly used templates. Your call ;-) https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:21: class ContentSuggestionsService; On 2016/06/28 12:05:19, Marc Treib wrote: > On 2016/06/28 11:47:17, tschumann wrote: > > I don't think this forward declaration is necessary. the only time when I > > encountered files you don't want to include in headers were pretty fat proto > > files (but proto.h also helps with this issue now). > > > > Mark: Is there some general practice on Chrome around forward declarations? > > Yes. As per https://www.chromium.org/developers/coding-style: > "Unlike the Google style guide, Chromium style prefers forward declarations to > #includes where possible." I see. Well it's probably because you compile a lot locally -- header includes are reasonably cheap when compiling on forge. Thanks for the explanation -- this clarifies a lot, including why "empty" in headers is considered bad! https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:32: friend struct base::DefaultSingletonTraits<ContentSuggestionsServiceFactory>; On 2016/06/28 12:05:19, Marc Treib wrote: > On 2016/06/28 11:47:17, tschumann wrote: > > nit: friends are usually a pretty deep implementation detail, why I usually > move > > them to the bottom right before the DISALLOW_XXX macro. > > > > AFAIK, there's no rule about where to put them though. > > This is the common style in Chrome AFAIK. > There *is* a rule that DISALLOW_COPY_AND_ASSIGN goes last. Ok, cool. If there's a common style, we should just apply it everywhere (as this is a pretty minor detail). https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:43: virtual void OnSuggestionsChanged( On 2016/06/28 12:05:19, Marc Treib wrote: > On 2016/06/28 11:47:17, tschumann wrote: > > sorry for nit-picking. It feels a bit weird that a provider notifies no > changes. > > Sorry, I don't get that.. it does notify, that's what this very method does...? hehe, I wasn't reading closely, i read OnSuggestionChange (and dropped the 'd'). So technically, the name is alright, but I'd prefer the OnNewSomething, in case we do want to notify about particular changes later (in which case a name like OnSuggestionChange would be an appropriate choice). > > > Shouldn't it notify on new data? And looking at the signature, > > OnNewSuggestions() looks like a better name as you don't give any information > > about what changed specifically. > > Agreed, OnNewSuggestions[Available] or something like that would probably be a > better name. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:22: } On 2016/06/28 11:47:17, tschumann wrote: > necessary? Done. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:36: ContentSuggestionsService(bool enabled); nit: usually enums are better to read here. Compare the following client code: ContentSuggestionsService(ContentSuggestionsService::kEnabled) ContentSuggestionsService(ContentSuggestionsService::kDisabled) vs. ContentSuggestionsService(true) ContentSuggestionsService(false) https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:85: void OnProviderShutdown(const ContentSuggestionsProvider& source) override; nit: please insert an empty line between methods. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:116: bool RemoveSuggestionsOfProvider( shouldn't functions go before members? (declaration order). https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:130: // Sent when the service is shutting down. please insert empty line.
https://codereview.chromium.org/2102023002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:48: base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature); On 2016/06/28 12:09:50, Marc Treib wrote: > On 2016/06/28 12:04:08, Philipp Keck wrote: > > On 2016/06/28 11:29:08, Marc Treib wrote: > > > Hm. I guess for now it's okay to reuse the same flag, but please add a TODO > to > > > split off a separate one. > > > > I have a separate "article-suggestions" feature in an upcoming CL. My > intention > > is to leave the current "snippets feature", which is disabled by default, as > the > > main toggle for the entire NTP suggestions stuff. Maybe rename it to > > "ntp-content-suggestions". The new "article-suggestions" and then > > "offline-pages-suggestions" features will be enabled by default and turn off > the > > sub-providers. > > Mostly SGTM, see below. > > > So: > > ContentSuggestionsService will be enabled iff "snippets" is true (exactly as > > coded here, possibly renamed). > > ArticleSuggestionsService (pka NTPSnippetsService) will be enabled iff > "snippets > > && article-suggestions" is true (so I will add check for article-suggestions > to > > the NTPSnippetsServiceFactory). > > ArticleSuggestionsService probably shouldn't check for the "snippets" feature, > but instead check if the ContentSuggestionsService exists. > > > Is that okay? Can features be renamed without further consequences? > > No, the actual string can not be renamed easily. We already have live configs > that reference the existing name; we'd need to update them with both the old and > new name, to support old and new Chrome versions. What we can do though is to > rename only the variable (kNTPSnippetsFeature -> kNTPContentSuggestions or > something like that), but keep the string as-is. Acknowledged. So in that case, it seems especially important to make the old "snippets" feature become the new top-level switch and introduce the "articles" as a new switch. I assume that the live configs want to control the top-level feature, not the underlying provider. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:8: #include <set> On 2016/06/28 11:29:09, Marc Treib wrote: > not needed Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:44: const ContentSuggestionsProvider& source, On 2016/06/28 11:29:08, Marc Treib wrote: > Should this just pass the provider type? Seems like that is all the service > needs anyway. That would indeed be enough, the service has the map<type, instance> anyway. The reason I just chose this variant without really thinking about it is because normally with these callback/observer patterns you just pass the instance. So to me, it's part of the design pattern. I'm happy to change it though, they seem about equal to me in terms of performance and readability. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:47: virtual void OnProviderShutdown(const ContentSuggestionsProvider& source){}; On 2016/06/28 11:29:09, Marc Treib wrote: > nit: space before "{" and no ";" (though really, this probably shouldn't have a > default implementation) Done. Replaced with "= 0". You are right, default implementation only makes sense for observers (where different observers might be interested in different things). https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:69: // marking it as discarded internally. On 2016/06/28 11:29:08, Marc Treib wrote: > I'd remove the last sentence, since it's not really relevant for the interface. Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:70: virtual void Discard(const std::string& suggestion_id) {} On 2016/06/28 11:29:08, Marc Treib wrote: > This shouldn't have a default implementation. > Probably the Clear methods shouldn't either. > > Also, DiscardSuggestion? For (a) more clarity and (b) consistency with the Clear > methods. Removed the default implementation. I left the default implementation in place for ClearSuggestions and ClearDiscardedSuggestions. Those are only for debugging purposes and some provider's might not be able to implement any functionality there. E.g. the offline page provider probably won't do anything to the stored offline pages when someone clicks a button for ClearSuggestions on the internals page (especially not delete the stored pages). Some other providers might discard suggestions by actually deleting them from the underlying model, so they won't need to keep track of previously discarded suggestions and thus they won't need ClearDiscardedSuggestions. The motivation for "ClearSuggestions()" vs. "Discard(suggestion)" is that the latter already contains the "suggestion" in the argument. For that reason, I also removed the "Snippet" from "FetchSnippetImage", so that it now reads "FetchImage(some_suggestion, ..)". But to be honest, another reason was: It saved me a lot of renaming from "Snippet" to "SuggestedContent" to "Card" to "ContentSuggestion". So now that we fixed the terms, I'm okay with adding the "Suggestion" back in: "DiscardSuggestion" and/or "FetchSuggestionImage". https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:73: // through the callback. This fetch may occur locally or from the Internet. On 2016/06/28 11:29:08, Marc Treib wrote: > nit: don't capitalize internet Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:7: #include "base/observer_list.h" On 2016/06/28 11:29:09, Marc Treib wrote: > Not needed, since it's already included in the header. Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:14: if (enabled_) { On 2016/06/28 11:29:09, Marc Treib wrote: > Remove empty if. > It looks like the enabled_ flag currently doesn't do anything at all? It > probably should... The reason that there is no code is not that the enabled flag doesn't do anything. The reason is that the constructor currently doesn't need to do anything because there is simply no setup to be done. When the service is disabled, none of the providers' factories will register a provider, so the service will remain empty. We could additionally prevent calls to functions like RegisterProvider, GetSuggestionsForCategory and possibly all others or make them return empty results. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:22: DCHECK(providers_.count(provider->GetProviderType()) == 0); On 2016/06/28 11:29:09, Marc Treib wrote: > DCHECK_EQ(0, ...) Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:48: ContentSuggestionsProviderType>::type>(provider_type); On 2016/06/28 11:29:09, Marc Treib wrote: > Argh, this is ugly - I guess it doesn't compile without the cast? > IMO just "int(provider_type)" is good enough for a LOG statement. Or maybe it's > worth adding a "string GetProviderTypeName(ProviderType)" function? Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:95: std::move(new_suggestions.begin(), new_suggestions.end(), On 2016/06/28 11:29:09, Marc Treib wrote: > #include <algorithm> Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:96: std::back_inserter(suggestions_[changed_category])); On 2016/06/28 11:29:09, Marc Treib wrote: > #include <iterator> Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:117: std::vector<ContentSuggestion>* list, On 2016/06/28 11:29:09, Marc Treib wrote: > Call this "suggestions" maybe? It's not actually a list. Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:119: auto provider_matcher = [provider_type](const ContentSuggestion& s) { On 2016/06/28 11:29:09, Marc Treib wrote: > Inline this into the remove_if? Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:127: list->erase(remove_from, list->end()); On 2016/06/28 11:29:09, Marc Treib wrote: > You could also use the common erase(remove_if, end) idiom, and compare the > return value to the old end() to see if anything changed. Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:11: #include <set> On 2016/06/28 11:29:10, Marc Treib wrote: > not needed Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:14: On 2016/06/28 11:29:10, Marc Treib wrote: > #include "base/callback_forward.h" Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:29: // them to the NTP. On 2016/06/28 11:29:09, Marc Treib wrote: > I'd remove the reference to the NTP - that is not the service's business. > Instead, you *could* give some more info on what it does, like grouping into > categories, possibly ranking (though it doesn't really do that yet). Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:43: const std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>& On 2016/06/28 11:29:09, Marc Treib wrote: > Does this need to be exposed at all? If so, maybe worth a typedef ("using ... = > std::map<...>"), though I'm leaning towards not exposing it at all. E.g. what > about the order of categories? Like this, the service has no way of influencing > it. You're right, but I'm not sure which solution is the best. A "GetCategories" would make sense if the service had an idea of which categories are enabled at all. Otherwise it would just return the categories enum, which would probably be hard-coded in the UI anyway. Iirc, we decided that the service doesn't decide the order of the suggestions (within a category). So probably it wouldn't decided the order of the categories either? Depending on how much the service knows about and does with categories, I'd either just remove this method and only use GetSuggestionsForCategory, or rethink how the category handling works. Maybe we need an "OnCategoryAdded" event, etc.? https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:47: const std::vector<ContentSuggestion>& suggestions( On 2016/06/28 11:29:10, Marc Treib wrote: > Please give this a better name, something like "GetSuggestionsForCategory"? > Also, to make it useful, there really should be a "GetCategories" too. Acknowledged. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:48: ContentSuggestionCategory category) { On 2016/06/28 11:29:09, Marc Treib wrote: > Should be const. Acknowledged. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:64: void ClearSuggestions(); On 2016/06/28 11:29:09, Marc Treib wrote: > I think this is also used only for debugging purposes? > (ClearDiscardedSuggestions has a comment saying that) > > Maybe rename to ClearCachedSuggestions, since that's closer to what actually > happens? Done, renamed here and in the provider interface. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:73: void Discard(const ContentSuggestionsProviderType provider_type, On 2016/06/28 11:29:09, Marc Treib wrote: > No "const" if you pass by value. Done, fixed here and in FetchImage() above. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:76: // Observer accessors (outgoing observers, from this service towards UI) On 2016/06/28 11:29:09, Marc Treib wrote: > I'd remove the part in parens - "outgoing" is obvious, that's what observers are > for, and "towards the UI" is not necessarily correct, and anyway not the > service's responsibility. > Also, nit: period after comment. Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:80: // Implementation of ContentSuggestionsProvider::Delegate (incoming data) On 2016/06/28 11:29:09, Marc Treib wrote: > These can (and should) be private. > Also, nit: period after comment. Aha ok? Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:87: // ContentSuggestionsProvider::setDelegate with a delegate object, through On 2016/06/28 11:29:10, Marc Treib wrote: > I'd remove the "The service..." sentence, since it's not really relevant for the > interface. Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:105: suggestions_; On 2016/06/28 11:29:09, Marc Treib wrote: > suggestions_by_category_? Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:117: ContentSuggestionsProviderType provider_type); On 2016/06/28 11:29:10, Marc Treib wrote: > Private methods come before member variables. But this probably doesn't need to > be a member at all, in which case it should be a function in an anonymous > namespace in the .cc file. Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:122: class ContentSuggestionsServiceObserver { On 2016/06/28 11:29:10, Marc Treib wrote: > I'd make this an inner class of the service (just called Observer). > > AFAIK, the main motivation of making an observer a separate class is that it can > be moved to a separate file, so clients can include only the observer without > getting the whole service too. We could do that too here, but I don't see a > particular reason to. This implementation is analogous to NTPSnippetsService, so the reasons for having a separate would be the same. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:126: // |suggestions()[changed_category]|. On 2016/06/28 11:29:09, Marc Treib wrote: > Please update to refer to the (proposed) GetSuggestionsForCategory method. Acknowledged. https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.h (right): https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:17: struct DefaultSingletonTraits; On 2016/06/28 12:32:40, tschumann wrote: > On 2016/06/28 12:05:19, Marc Treib wrote: > > On 2016/06/28 11:47:17, tschumann wrote: > > > never forward declare templates. It makes them pretty much impossible to > > change > > > (think like someone wants to add an optional parameter). > > > > I generally agree, though this particular thing is very common with these > > factory classes. That said, I'm also fine with just including the thing. > > I'm not feeling strongly, but leaning towards not forward-declaring. Primarily > because there might be less common types where we probably shouldn't forward > declare them and cases should be rare in general. > I'm also happy with an exception of commonly used templates. Your call ;-) The existing code is not consistent here (compare NTPSnippetsServiceFactory to SuggestionsServiceFactory). I'm just going to leave it as is, please let me know if you want me to change it. https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:21: class ContentSuggestionsService; On 2016/06/28 12:32:40, tschumann wrote: > > On 2016/06/28 12:05:19, Marc Treib wrote: > > On 2016/06/28 11:47:17, tschumann wrote: > > > I don't think this forward declaration is necessary. the only time when I > > > encountered files you don't want to include in headers were pretty fat proto > > > files (but proto.h also helps with this issue now). > > > > > > Mark: Is there some general practice on Chrome around forward declarations? > > > > Yes. As per https://www.chromium.org/developers/coding-style: > > "Unlike the Google style guide, Chromium style prefers forward declarations to > > #includes where possible." > > I see. Well it's probably because you compile a lot locally -- header includes > are reasonably cheap when compiling on forge. > Thanks for the explanation -- this clarifies a lot, including why "empty" in > headers is considered bad! Acknowledged. https://codereview.chromium.org/2102023002/diff/20001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.h:32: friend struct base::DefaultSingletonTraits<ContentSuggestionsServiceFactory>; On 2016/06/28 12:32:40, tschumann wrote: > On 2016/06/28 12:05:19, Marc Treib wrote: > > On 2016/06/28 11:47:17, tschumann wrote: > > > nit: friends are usually a pretty deep implementation detail, why I usually > > move > > > them to the bottom right before the DISALLOW_XXX macro. > > > > > > AFAIK, there's no rule about where to put them though. > > > > This is the common style in Chrome AFAIK. > > There *is* a rule that DISALLOW_COPY_AND_ASSIGN goes last. > Ok, cool. If there's a common style, we should just apply it everywhere (as this > is a pretty minor detail). Acknowledged, see above. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:32: class Delegate { On 2016/06/28 11:47:17, tschumann wrote: > I'm not super solid in design patterns, but wouldn't this be a observer or > consumer? It's technically not a delegate because we use the same object (the service itself). It's also not really a delegation pattern because the purpose is not to delegate. If we used two separate objects, then the service (the second object being called) would be the delegate and not the object implementing this interface. The purpose here is to expose only part of the service's interface to the provider, i.e., a provider needs to call the service for new content, but it shouldn't call the service's shutdown method or sth like that. I don't know what design pattern that would be. It's also not really an observer pattern because there is no flexible registering and unregistering of interested observers, but there when a provider is running, there is always exactly one place that it should deliver its content to, namely to the service. The Producer/Consumer pattern I've only seen in the context of parallel computing so far. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:43: virtual void OnSuggestionsChanged( On 2016/06/28 12:32:40, tschumann wrote: > On 2016/06/28 12:05:19, Marc Treib wrote: > > On 2016/06/28 11:47:17, tschumann wrote: > > > sorry for nit-picking. It feels a bit weird that a provider notifies no > > changes. > > > > Sorry, I don't get that.. it does notify, that's what this very method > does...? > hehe, I wasn't reading closely, i read OnSuggestionChange (and dropped the 'd'). > So technically, the name is alright, but I'd prefer the OnNewSomething, in case > we do want to notify about particular changes later (in which case a name like > OnSuggestionChange would be an appropriate choice). > > > > > > Shouldn't it notify on new data? And looking at the signature, > > > OnNewSuggestions() looks like a better name as you don't give any > information > > > about what changed specifically. > > > > Agreed, OnNewSuggestions[Available] or something like that would probably be a > > better name. > It's also not "OnSuggestionChanged" but "OnSuggestionsChanged", so it doesn't notify about the inner change of a single suggestion, but about a change (anywhere) in the whole list of all suggestions. In the future, we might add OnSuggestionAdd, OnSuggestionChange (no plural), OnSuggestionRemove, etc. Because "OnSuggestionsChanged" and "OnSuggestionChange" would be confusingly similar, we could call the current method "OnContentChanged"? https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:58: virtual void ClearSuggestions() {} On 2016/06/28 11:47:17, tschumann wrote: > wouldn't it be cleaner to make this a pure virtual class? Note sure, see my comment on the Discard() method over there https://codereview.chromium.org/2102023002/patch/1/10008 https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:36: ContentSuggestionsService(bool enabled); On 2016/06/28 12:32:40, tschumann wrote: > nit: usually enums are better to read here. > Compare the following client code: > ContentSuggestionsService(ContentSuggestionsService::kEnabled) > ContentSuggestionsService(ContentSuggestionsService::kDisabled) > vs. > ContentSuggestionsService(true) > ContentSuggestionsService(false) True, but wouldn't that question pretty much every boolean in the code? For example, the RemoveSuggestionsOfProvider helper method below could return "::kNothingChanged" or "::kSomethingChanged" instead of true and false. I haven't found any other service that uses an enum or labeled constants instead of true/false. But I have found sth like this in unit tests: ContentSuggestionsService( /* enabled= */ false) https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:85: void OnProviderShutdown(const ContentSuggestionsProvider& source) override; On 2016/06/28 12:32:40, tschumann wrote: > nit: please insert an empty line between methods. Ok. What are the criteria for doing that? Because AddObserver/RemoveObserver, getters/setters, constructors/destructors are usually visually grouped by not having an empty line between them. The styleguide doesn't seem to specify anything, but the example does not contain blank lines: https://google.github.io/styleguide/cppguide.html#Class_Format https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:116: bool RemoveSuggestionsOfProvider( On 2016/06/28 12:32:40, tschumann wrote: > shouldn't functions go before members? (declaration order). Acknowledged, this member was removed anyway. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:130: // Sent when the service is shutting down. On 2016/06/28 12:32:40, tschumann wrote: > please insert empty line. Done.
https://codereview.chromium.org/2102023002/diff/1/chrome/browser/ntp_snippets... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/1/chrome/browser/ntp_snippets... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:48: base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature); On 2016/06/28 14:18:55, Philipp Keck wrote: > On 2016/06/28 12:09:50, Marc Treib wrote: > > On 2016/06/28 12:04:08, Philipp Keck wrote: > > > On 2016/06/28 11:29:08, Marc Treib wrote: > > > > Hm. I guess for now it's okay to reuse the same flag, but please add a > TODO > > to > > > > split off a separate one. > > > > > > I have a separate "article-suggestions" feature in an upcoming CL. My > > intention > > > is to leave the current "snippets feature", which is disabled by default, as > > the > > > main toggle for the entire NTP suggestions stuff. Maybe rename it to > > > "ntp-content-suggestions". The new "article-suggestions" and then > > > "offline-pages-suggestions" features will be enabled by default and turn off > > the > > > sub-providers. > > > > Mostly SGTM, see below. > > > > > So: > > > ContentSuggestionsService will be enabled iff "snippets" is true (exactly as > > > coded here, possibly renamed). > > > ArticleSuggestionsService (pka NTPSnippetsService) will be enabled iff > > "snippets > > > && article-suggestions" is true (so I will add check for article-suggestions > > to > > > the NTPSnippetsServiceFactory). > > > > ArticleSuggestionsService probably shouldn't check for the "snippets" feature, > > but instead check if the ContentSuggestionsService exists. > > > > > Is that okay? Can features be renamed without further consequences? > > > > No, the actual string can not be renamed easily. We already have live configs > > that reference the existing name; we'd need to update them with both the old > and > > new name, to support old and new Chrome versions. What we can do though is to > > rename only the variable (kNTPSnippetsFeature -> kNTPContentSuggestions or > > something like that), but keep the string as-is. > > Acknowledged. So in that case, it seems especially important to make the old > "snippets" feature become the new top-level switch and introduce the "articles" > as a new switch. I assume that the live configs want to control the top-level > feature, not the underlying provider. Yup! https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:44: const ContentSuggestionsProvider& source, On 2016/06/28 14:18:55, Philipp Keck wrote: > On 2016/06/28 11:29:08, Marc Treib wrote: > > Should this just pass the provider type? Seems like that is all the service > > needs anyway. > > That would indeed be enough, the service has the map<type, instance> anyway. > The reason I just chose this variant without really thinking about it is because > normally with these callback/observer patterns you just pass the instance. So to > me, it's part of the design pattern. > > I'm happy to change it though, they seem about equal to me in terms of > performance and readability. Oh, I'm not worried about performance. But the service really only wants the ID, it doesn't do anything else with the provider. So I'd prefer to make that obvious. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:70: virtual void Discard(const std::string& suggestion_id) {} On 2016/06/28 14:18:55, Philipp Keck wrote: > On 2016/06/28 11:29:08, Marc Treib wrote: > > This shouldn't have a default implementation. > > Probably the Clear methods shouldn't either. > > > > Also, DiscardSuggestion? For (a) more clarity and (b) consistency with the > Clear > > methods. > > Removed the default implementation. > > I left the default implementation in place for ClearSuggestions and > ClearDiscardedSuggestions. Those are only for debugging purposes and some > provider's might not be able to implement any functionality there. E.g. the > offline page provider probably won't do anything to the stored offline pages > when someone clicks a button for ClearSuggestions on the internals page > (especially not delete the stored pages). Some other providers might discard > suggestions by actually deleting them from the underlying model, so they won't > need to keep track of previously discarded suggestions and thus they won't need > ClearDiscardedSuggestions. I think it's still better for those providers to explicitly say "I don't do anything about this", rather than having some potentially bad default behavior. But it's not a huge deal. > The motivation for "ClearSuggestions()" vs. "Discard(suggestion)" is that the > latter already contains the "suggestion" in the argument. For that reason, I > also removed the "Snippet" from "FetchSnippetImage", so that it now reads > "FetchImage(some_suggestion, ..)". > > But to be honest, another reason was: It saved me a lot of renaming from > "Snippet" to "SuggestedContent" to "Card" to "ContentSuggestion". So now that we > fixed the terms, I'm okay with adding the "Suggestion" back in: > "DiscardSuggestion" and/or "FetchSuggestionImage". Fair enough ;) Yup, I'd prefer adding the "Suggestion" back. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:14: if (enabled_) { On 2016/06/28 14:18:56, Philipp Keck wrote: > On 2016/06/28 11:29:09, Marc Treib wrote: > > Remove empty if. > > It looks like the enabled_ flag currently doesn't do anything at all? It > > probably should... > > The reason that there is no code is not that the enabled flag doesn't do > anything. The reason is that the constructor currently doesn't need to do > anything because there is simply no setup to be done. Alright, but then remove the empty if. > When the service is disabled, none of the providers' factories will register a > provider, so the service will remain empty. We could additionally prevent calls > to functions like RegisterProvider, GetSuggestionsForCategory and possibly all > others or make them return empty results. Hm. If the "enabled" state can change at runtime (e.g. because we expose a setting), then it might make sense to register the providers anyway, in case the service later gets enabled. WDYT? https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:43: const std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>& On 2016/06/28 14:18:56, Philipp Keck wrote: > On 2016/06/28 11:29:09, Marc Treib wrote: > > Does this need to be exposed at all? If so, maybe worth a typedef ("using ... > = > > std::map<...>"), though I'm leaning towards not exposing it at all. E.g. what > > about the order of categories? Like this, the service has no way of > influencing > > it. > > You're right, but I'm not sure which solution is the best. > A "GetCategories" would make sense if the service had an idea of which > categories are enabled at all. Otherwise it would just return the categories > enum, which would probably be hard-coded in the UI anyway. No, it would go over suggestions_by_category_ and collect all categories that have any suggestions. > Iirc, we decided that the service doesn't decide the order of the suggestions > (within a category). So probably it wouldn't decided the order of the categories > either? I think it will, because who else would? But whoever makes the decision in the end, the service will have to expose it to the UI. > Depending on how much the service knows about and does with categories, I'd > either just remove this method and only use GetSuggestionsForCategory, or > rethink how the category handling works. Maybe we need an "OnCategoryAdded" > event, etc.? I'm all for removing this method and just exposing GetSuggestionsForCategory. I don't see why we'd need OnCategoryAdded - the existing OnSuggestionsChanged should cover that already? https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:122: class ContentSuggestionsServiceObserver { On 2016/06/28 14:18:56, Philipp Keck wrote: > On 2016/06/28 11:29:10, Marc Treib wrote: > > I'd make this an inner class of the service (just called Observer). > > > > AFAIK, the main motivation of making an observer a separate class is that it > can > > be moved to a separate file, so clients can include only the observer without > > getting the whole service too. We could do that too here, but I don't see a > > particular reason to. > > This implementation is analogous to NTPSnippetsService, so the reasons for > having a separate would be the same. I don't know of any reasons for having it separate there either :) Also, the SnippetsServiceObserver will probably go away? (or rather, be replaced by Provider::Delegate) https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:85: void OnProviderShutdown(const ContentSuggestionsProvider& source) override; On 2016/06/28 14:18:57, Philipp Keck wrote: > On 2016/06/28 12:32:40, tschumann wrote: > > nit: please insert an empty line between methods. > > Ok. What are the criteria for doing that? Because AddObserver/RemoveObserver, > getters/setters, constructors/destructors are usually visually grouped by not > having an empty line between them. > > The styleguide doesn't seem to specify anything, but the example does not > contain blank lines: > https://google.github.io/styleguide/cppguide.html#Class_Format AFAIK there aren't any hard rules. Some people prefer one way, others another :) https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:20: ContentSuggestion& ContentSuggestion::operator=(ContentSuggestion&&) = default; nit: move to match the order in the header https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:126: if (RemoveSuggestionsOfProvider(&pair.second, source.GetProviderType())) { If all suggestions in that category were removed, you should also remove that from suggestions_by_category_. https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:81: // Registers a new ContentSuggestionsProvider. This method will fail when a nit: s/when/if https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:123: ContentSuggestionCategory changed_category) = 0; Hm, does this even need to include the category? At least for now, the UI doesn't care, it'll update everything or nothing anyway.
https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:44: const ContentSuggestionsProvider& source, On 2016/06/28 15:04:55, Marc Treib wrote: > On 2016/06/28 14:18:55, Philipp Keck wrote: > > On 2016/06/28 11:29:08, Marc Treib wrote: > > > Should this just pass the provider type? Seems like that is all the service > > > needs anyway. > > > > That would indeed be enough, the service has the map<type, instance> anyway. > > The reason I just chose this variant without really thinking about it is > because > > normally with these callback/observer patterns you just pass the instance. So > to > > me, it's part of the design pattern. > > > > I'm happy to change it though, they seem about equal to me in terms of > > performance and readability. > > Oh, I'm not worried about performance. But the service really only wants the ID, > it doesn't do anything else with the provider. So I'd prefer to make that > obvious. I don't want to sidetrack the whole discussion, but it feels a bit weird to me that we need to expose a ContentSuggestionsProviderType at all. Sure, for a registry we might need something like this, but this should only be some internal part. That aside, I'd hope that we can simply handle the different types by passing references/pointers of objects around. If you don't mind, a quick design discussion might be nice to collect some feedback here. Don't get me wrong, I don't want to nitpick, but I believe it would be a good point in time to gather feedback from the group and make sure people know what's going on. That said, we should keep the initial audience small to make sure the discussion stays productive and then simply present the design we're happy with to the larger team. On a side note, Markus told me he put together a document with current supported user interactions -- would be nice to play them through and see how they would be supported by this API. What do you think?
Description was changed from ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. BUG=619560 ========== to ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. Change ContentSuggestion to merge the ID from the provider with the provider's type into a single, unique ID that can be used by the UI and let ContentSuggestionsService convert back for communication with the providers. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. BUG=619560 ==========
Description was changed from ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. Change ContentSuggestion to merge the ID from the provider with the provider's type into a single, unique ID that can be used by the UI and let ContentSuggestionsService convert back for communication with the providers. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. BUG=619560 ========== to ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. Change ContentSuggestion to merge the ID from the provider with the provider's type into a single, unique ID that can be used by the UI and let ContentSuggestionsService convert back for communication with the providers. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. BUG=619560 ==========
Thank you for your input and for the meeting, ptal. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:44: const ContentSuggestionsProvider& source, On 2016/06/28 16:48:35, tschumann wrote: > On 2016/06/28 15:04:55, Marc Treib wrote: > > On 2016/06/28 14:18:55, Philipp Keck wrote: > > > On 2016/06/28 11:29:08, Marc Treib wrote: > > > > Should this just pass the provider type? Seems like that is all the > service > > > > needs anyway. > > > > > > That would indeed be enough, the service has the map<type, instance> anyway. > > > The reason I just chose this variant without really thinking about it is > > because > > > normally with these callback/observer patterns you just pass the instance. > So > > to > > > me, it's part of the design pattern. > > > > > > I'm happy to change it though, they seem about equal to me in terms of > > > performance and readability. > > > > Oh, I'm not worried about performance. But the service really only wants the > ID, > > it doesn't do anything else with the provider. So I'd prefer to make that > > obvious. > > I don't want to sidetrack the whole discussion, but it feels a bit weird to me > that we need to expose a ContentSuggestionsProviderType at all. Sure, for a > registry we might need something like this, but this should only be some > internal part. That aside, I'd hope that we can simply handle the different > types by passing references/pointers of objects around. If you don't mind, a > quick design discussion might be nice to collect some feedback here. > Don't get me wrong, I don't want to nitpick, but I believe it would be a good > point in time to gather feedback from the group and make sure people know what's > going on. That said, we should keep the initial audience small to make sure the > discussion stays productive and then simply present the design we're happy with > to the larger team. > > On a side note, Markus told me he put together a document with current supported > user interactions -- would be nice to play them through and see how they would > be supported by this API. > > What do you think? Thank you. The code here now passes the provider_type (as its the interface between the provider and the service), but the interface towards the UI merges the provider_type with the ID. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_provider.h:70: virtual void Discard(const std::string& suggestion_id) {} On 2016/06/28 15:04:55, Marc Treib wrote: > On 2016/06/28 14:18:55, Philipp Keck wrote: > > On 2016/06/28 11:29:08, Marc Treib wrote: > > > This shouldn't have a default implementation. > > > Probably the Clear methods shouldn't either. > > > > > > Also, DiscardSuggestion? For (a) more clarity and (b) consistency with the > > Clear > > > methods. > > > > Removed the default implementation. > > > > I left the default implementation in place for ClearSuggestions and > > ClearDiscardedSuggestions. Those are only for debugging purposes and some > > provider's might not be able to implement any functionality there. E.g. the > > offline page provider probably won't do anything to the stored offline pages > > when someone clicks a button for ClearSuggestions on the internals page > > (especially not delete the stored pages). Some other providers might discard > > suggestions by actually deleting them from the underlying model, so they won't > > need to keep track of previously discarded suggestions and thus they won't > need > > ClearDiscardedSuggestions. > > I think it's still better for those providers to explicitly say "I don't do > anything about this", rather than having some potentially bad default behavior. > But it's not a huge deal. > > > The motivation for "ClearSuggestions()" vs. "Discard(suggestion)" is that the > > latter already contains the "suggestion" in the argument. For that reason, I > > also removed the "Snippet" from "FetchSnippetImage", so that it now reads > > "FetchImage(some_suggestion, ..)". > > > > But to be honest, another reason was: It saved me a lot of renaming from > > "Snippet" to "SuggestedContent" to "Card" to "ContentSuggestion". So now that > we > > fixed the terms, I'm okay with adding the "Suggestion" back in: > > "DiscardSuggestion" and/or "FetchSuggestionImage". > > Fair enough ;) Yup, I'd prefer adding the "Suggestion" back. Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:14: if (enabled_) { On 2016/06/28 15:04:55, Marc Treib wrote: > On 2016/06/28 14:18:56, Philipp Keck wrote: > > On 2016/06/28 11:29:09, Marc Treib wrote: > > > Remove empty if. > > > It looks like the enabled_ flag currently doesn't do anything at all? It > > > probably should... > > > > The reason that there is no code is not that the enabled flag doesn't do > > anything. The reason is that the constructor currently doesn't need to do > > anything because there is simply no setup to be done. > > Alright, but then remove the empty if. > > > When the service is disabled, none of the providers' factories will register a > > provider, so the service will remain empty. We could additionally prevent > calls > > to functions like RegisterProvider, GetSuggestionsForCategory and possibly all > > others or make them return empty results. > > Hm. If the "enabled" state can change at runtime (e.g. because we expose a > setting), then it might make sense to register the providers anyway, in case the > service later gets enabled. WDYT? I removed the empty if for now. The question how to properly implement that depends on how status cards are handled, i.e., where they are produced and, if that's in the backend, how they are passed to the UI. Are they special suggestions (to turn some settings on), are they a separate Delegate/Callback method, or is it just a Service::is_enabled() that returns false and the UI needs to realise that? I agree: All (available) providers should always be registered with the service. If disabled, the service should clear its data stores and somehow tell the UI. Additionally, the service could make sure not to return any suggestions (except maybe the appropriate status cards). If disabled, a provider should stop fetching data and not deliver data to the service anymore and tell the service that it's disabled (and the service removes the corresponding data). How does a provider know that the main switch/feature is turned off? Maybe through a ContentSuggestionsService::is_enabled() getter and a Delete::ServiceDisabled() notification? Then we'd also need Delegate::ServiceEnabled() to re-enable. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:127: list->erase(remove_from, list->end()); On 2016/06/28 11:29:09, Marc Treib wrote: > You could also use the common erase(remove_if, end) idiom, and compare the > return value to the old end() to see if anything changed. Undone. Reverted to old code because checking whether sth changed with the usual erase(remove_if) idiom doesn't work. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:43: const std::map<ContentSuggestionCategory, std::vector<ContentSuggestion>>& On 2016/06/28 15:04:55, Marc Treib wrote: > On 2016/06/28 14:18:56, Philipp Keck wrote: > > On 2016/06/28 11:29:09, Marc Treib wrote: > > > Does this need to be exposed at all? If so, maybe worth a typedef ("using > ... > > = > > > std::map<...>"), though I'm leaning towards not exposing it at all. E.g. > what > > > about the order of categories? Like this, the service has no way of > > influencing > > > it. > > > > You're right, but I'm not sure which solution is the best. > > A "GetCategories" would make sense if the service had an idea of which > > categories are enabled at all. Otherwise it would just return the categories > > enum, which would probably be hard-coded in the UI anyway. > > No, it would go over suggestions_by_category_ and collect all categories that > have any suggestions. > > > Iirc, we decided that the service doesn't decide the order of the suggestions > > (within a category). So probably it wouldn't decided the order of the > categories > > either? > > I think it will, because who else would? But whoever makes the decision in the > end, the service will have to expose it to the UI. > > > Depending on how much the service knows about and does with categories, I'd > > either just remove this method and only use GetSuggestionsForCategory, or > > rethink how the category handling works. Maybe we need an "OnCategoryAdded" > > event, etc.? > > I'm all for removing this method and just exposing GetSuggestionsForCategory. > I don't see why we'd need OnCategoryAdded - the existing OnSuggestionsChanged > should cover that already? Done. Changed OnSuggestionsChanged to not be called per-category. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:47: const std::vector<ContentSuggestion>& suggestions( On 2016/06/28 11:29:10, Marc Treib wrote: > Please give this a better name, something like "GetSuggestionsForCategory"? > Also, to make it useful, there really should be a "GetCategories" too. Done. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.h:122: class ContentSuggestionsServiceObserver { On 2016/06/28 15:04:55, Marc Treib wrote: > On 2016/06/28 14:18:56, Philipp Keck wrote: > > On 2016/06/28 11:29:10, Marc Treib wrote: > > > I'd make this an inner class of the service (just called Observer). > > > > > > AFAIK, the main motivation of making an observer a separate class is that it > > can > > > be moved to a separate file, so clients can include only the observer > without > > > getting the whole service too. We could do that too here, but I don't see a > > > particular reason to. > > > > This implementation is analogous to NTPSnippetsService, so the reasons for > > having a separate would be the same. > > I don't know of any reasons for having it separate there either :) > Also, the SnippetsServiceObserver will probably go away? (or rather, be replaced > by Provider::Delegate) Yes, as soon as the internals page is able to get the info it needs without the NTPSnippetsServiceObserver and as soon as the UI gets the proper disabled/shutdown from the new service, the old observer will go away. So I'll just make this one an inner class and not change the existing one. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:58: virtual void ClearSuggestions() {} On 2016/06/28 14:18:57, Philipp Keck wrote: > On 2016/06/28 11:47:17, tschumann wrote: > > wouldn't it be cleaner to make this a pure virtual class? > > Note sure, see my comment on the Discard() method over there > https://codereview.chromium.org/2102023002/patch/1/10008 Done. https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:20: ContentSuggestion& ContentSuggestion::operator=(ContentSuggestion&&) = default; On 2016/06/28 15:04:55, Marc Treib wrote: > nit: move to match the order in the header Done. https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:126: if (RemoveSuggestionsOfProvider(&pair.second, source.GetProviderType())) { On 2016/06/28 15:04:55, Marc Treib wrote: > If all suggestions in that category were removed, you should also remove that > from suggestions_by_category_. Done. https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:81: // Registers a new ContentSuggestionsProvider. This method will fail when a On 2016/06/28 15:04:55, Marc Treib wrote: > nit: s/when/if Done. https://codereview.chromium.org/2102023002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:123: ContentSuggestionCategory changed_category) = 0; On 2016/06/28 15:04:55, Marc Treib wrote: > Hm, does this even need to include the category? At least for now, the UI > doesn't care, it'll update everything or nothing anyway. Done.
following up on my comments of the old patch-set. Some might not apply anymore; will fast-forward to the latest patch-set. [the logic of separating the patch-sets from each other still boggles my mind..] https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:36: ContentSuggestionsService(bool enabled); On 2016/06/28 14:18:57, Philipp Keck wrote: > On 2016/06/28 12:32:40, tschumann wrote: > > nit: usually enums are better to read here. > > Compare the following client code: > > ContentSuggestionsService(ContentSuggestionsService::kEnabled) > > ContentSuggestionsService(ContentSuggestionsService::kDisabled) > > vs. > > ContentSuggestionsService(true) > > ContentSuggestionsService(false) > > True, but wouldn't that question pretty much every boolean in the code? For > example, the RemoveSuggestionsOfProvider helper method below could return > "::kNothingChanged" or "::kSomethingChanged" instead of true and false. I > haven't found any other service that uses an enum or labeled constants instead > of true/false. But I have found sth like this in unit tests: > ContentSuggestionsService( /* enabled= */ false) It essentially applies to every boolean parameter except setters. Return values are fine as the method name should give enough context. The comment-at-call-side pattern works nicely if we have automated checks in place that makes sure they match the parameter name. Otherwise, things are easily screwed up when writing or refactoring the called method. In google3, we have a presubmit check for this, not sure about Chromium. As this is a constructor, I'd be in favor of documenting the semantics nicely with the type system (if we're indeed lacking the comment-checks in chromium). https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:85: void OnProviderShutdown(const ContentSuggestionsProvider& source) override; On 2016/06/28 15:04:55, Marc Treib wrote: > On 2016/06/28 14:18:57, Philipp Keck wrote: > > On 2016/06/28 12:32:40, tschumann wrote: > > > nit: please insert an empty line between methods. > > > > Ok. What are the criteria for doing that? Because AddObserver/RemoveObserver, > > getters/setters, constructors/destructors are usually visually grouped by not > > having an empty line between them. > > > > The styleguide doesn't seem to specify anything, but the example does not > > contain blank lines: > > https://google.github.io/styleguide/cppguide.html#Class_Format > > AFAIK there aren't any hard rules. Some people prefer one way, others another :) Good point! I agree that in the AddObserver/RemoveObserver case an empty line would not be necessary. In this case however, I find it pretty hard to parse and actually see the second method. Then again, it's simply implementing the Delegate interface and there's nothing of particular interest to the reader. I need to think a bit more about a good rule of thumb... One point worth mentioning is, that these less interesting details actually muddy the public interface of ContentSuggestionService -- they should only be called through the Delegate interface. For now, maybe move them to the end of the public section to keep the public interface together. Maybe we can actually move this onto a private nested class or into the .cc file.
https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:14: if (enabled_) { On 2016/06/30 09:35:35, Philipp Keck wrote: > On 2016/06/28 15:04:55, Marc Treib wrote: > > On 2016/06/28 14:18:56, Philipp Keck wrote: > > > On 2016/06/28 11:29:09, Marc Treib wrote: > > > > Remove empty if. > > > > It looks like the enabled_ flag currently doesn't do anything at all? It > > > > probably should... > > > > > > The reason that there is no code is not that the enabled flag doesn't do > > > anything. The reason is that the constructor currently doesn't need to do > > > anything because there is simply no setup to be done. > > > > Alright, but then remove the empty if. > > > > > When the service is disabled, none of the providers' factories will register > a > > > provider, so the service will remain empty. We could additionally prevent > > calls > > > to functions like RegisterProvider, GetSuggestionsForCategory and possibly > all > > > others or make them return empty results. > > > > Hm. If the "enabled" state can change at runtime (e.g. because we expose a > > setting), then it might make sense to register the providers anyway, in case > the > > service later gets enabled. WDYT? > > I removed the empty if for now. The question how to properly implement that > depends on how status cards are handled, i.e., where they are produced and, if > that's in the backend, how they are passed to the UI. Are they special > suggestions (to turn some settings on), are they a separate Delegate/Callback > method, or is it just a Service::is_enabled() that returns false and the UI > needs to realise that? > > I agree: All (available) providers should always be registered with the service. > If disabled, the service should clear its data stores and somehow tell the UI. > Additionally, the service could make sure not to return any suggestions (except > maybe the appropriate status cards). > If disabled, a provider should stop fetching data and not deliver data to the > service anymore and tell the service that it's disabled (and the service removes > the corresponding data). How does a provider know that the main switch/feature > is turned off? Maybe through a ContentSuggestionsService::is_enabled() getter > and a Delete::ServiceDisabled() notification? Then we'd also need > Delegate::ServiceEnabled() to re-enable. Hm, right, we need "StateChanged" notifications both ways (IMO this should be one method with a parameter, rather than separate Enabled/Disabled ones). So yes, I guess we need a method both on the ServiceObserver and the ProviderObserver/Delegate. https://codereview.chromium.org/2102023002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:48: base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature); Please add a TODO to split the feature. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:14: std::string original_id) { const std::string& https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:15: return std::to_string(int(provider_type)) + ":" + original_id; Hm, base::StringPrintf would be the common way to do this in Chrome (and it probably avoid some intermediate allocations) https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:26: ContentSuggestion(const std::string& id, Rename this to "provider_id" or something like that, to make clear that it's not the actual ID of the ContentSuggestion? And/or add a comment explaining what's going on. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:41: virtual void OnSuggestionsChanged( Did we agree on a name here? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:51: virtual ContentSuggestionsProviderType GetProviderType() const = 0; Hm, you could actually store the provider type here in the base class (passed in from the subclass via a protected ctor). That removes some duplication between the subclasses, and also makes sure they can't change their type at runtime. WDYT? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:66: // Discards the suggestion with the given ID. A provider needs to ensure that nit: I'd put the "real" API methods before the debugging-only ones. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:68: // Observer). There is no need to call the observer for an update after a This formulation leaves it open whether the observer will be called or not. We should make a decision and stick with it. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:18: ContentSuggestionsProviderType GetProviderTypeFromCombinedID(std::string id) { const std::string& https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:19: auto colon_index = id.find(":"); size_t please - if you say "auto", then it kinda looks like this was an iterator IMO https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:21: int provider_type = std::stoi(id.substr(0, colon_index)); What happens if this fails to parse as an int? std::stoi isn't allowed in Chromium yet, see https://chromium-cpp.appspot.com/. The common way to do this in Chrome is base::StringToInt. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:22: return ContentSuggestionsProviderType(provider_type); What if the int isn't a valid provider type? (Shouldn't happen, so a DCHECK is enough, no "real" error handling required.) https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:30: } I think it'd be nice if the "combine" and "extract" functions were in the same place, next to each other. Maybe static methods on ContentSuggestion? Though that kinda invites others to use them... hrm. I don't know. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:43: } else { No "else" after "return" https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:58: observers_.Clear(); Hm. Since the observers register themselves, they should probably also unregister themselves. So DCHECK(observers_.empty()) ? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:63: enabled_ = false; Also clear the cached suggestions, just to be sure? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:69: return suggestions_by_category_.at(category); Hm, so this assumes that it'll never be called for a category that doesn't have suggestions. Should it maybe return an empty vector in that case? Otherwise, at least add a comment in the header saying that this mustn't be called for categories that aren't in GetCategories. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:77: std::string original_id = GetIDFromCombinedID(suggestion_id); nit: move this into the "if" https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:113: for (auto& pair : suggestions_by_category_) { Can't you get the category from the suggestions, rather than going over all of them? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:116: [suggestion_id](const ContentSuggestion& suggestion) { You could capture this by reference, to save a string copy https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:127: ContentSuggestionsProviderType>::type>(provider_type); int(provider_type) https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:27: // them grouped into categories. Add a "not used yet" comment, similar to ContentSuggestion itself? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:105: void OnProviderShutdown( This is one of the cases where there usually isn't an empty line between methods. (I guess Tim might disagree with this style? I kinda like having all methods from one superclass grouped together, but I don't care enough to argue :) ) https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:116: suggestions_by_category_; Hm, maybe at this point just a vector of pairs would be simpler? Up to you https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:121: // The observers. This is kinda "add 1 to i" :D
https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:36: ContentSuggestionsService(bool enabled); On 2016/06/30 10:20:06, tschumann wrote: > On 2016/06/28 14:18:57, Philipp Keck wrote: > > On 2016/06/28 12:32:40, tschumann wrote: > > > nit: usually enums are better to read here. > > > Compare the following client code: > > > ContentSuggestionsService(ContentSuggestionsService::kEnabled) > > > ContentSuggestionsService(ContentSuggestionsService::kDisabled) > > > vs. > > > ContentSuggestionsService(true) > > > ContentSuggestionsService(false) > > > > True, but wouldn't that question pretty much every boolean in the code? For > > example, the RemoveSuggestionsOfProvider helper method below could return > > "::kNothingChanged" or "::kSomethingChanged" instead of true and false. I > > haven't found any other service that uses an enum or labeled constants instead > > of true/false. But I have found sth like this in unit tests: > > ContentSuggestionsService( /* enabled= */ false) > > It essentially applies to every boolean parameter except setters. Return values > are fine as the method name should give enough context. > > The comment-at-call-side pattern works nicely if we have automated checks in > place that makes sure they match the parameter name. Otherwise, things are > easily screwed up when writing or refactoring the called method. In google3, we > have a presubmit check for this, not sure about Chromium. > > As this is a constructor, I'd be in favor of documenting the semantics nicely > with the type system (if we're indeed lacking the comment-checks in chromium). Done. Unlike your code example above, I used an actual enum, not just two static fields. I'm not 100% happy with the naming of that enum, but it follows the convention of naming the enum after the property. https://codereview.chromium.org/2102023002/diff/20001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:85: void OnProviderShutdown(const ContentSuggestionsProvider& source) override; On 2016/06/30 10:20:06, tschumann wrote: > On 2016/06/28 15:04:55, Marc Treib wrote: > > On 2016/06/28 14:18:57, Philipp Keck wrote: > > > On 2016/06/28 12:32:40, tschumann wrote: > > > > nit: please insert an empty line between methods. > > > > > > Ok. What are the criteria for doing that? Because > AddObserver/RemoveObserver, > > > getters/setters, constructors/destructors are usually visually grouped by > not > > > having an empty line between them. > > > > > > The styleguide doesn't seem to specify anything, but the example does not > > > contain blank lines: > > > https://google.github.io/styleguide/cppguide.html#Class_Format > > > > AFAIK there aren't any hard rules. Some people prefer one way, others another > :) > > Good point! > I agree that in the AddObserver/RemoveObserver case an empty line would not be > necessary. > In this case however, I find it pretty hard to parse and actually see the second > method. Then again, it's simply implementing the Delegate interface and there's > nothing of particular interest to the reader. > I need to think a bit more about a good rule of thumb... > > One point worth mentioning is, that these less interesting details actually > muddy the public interface of ContentSuggestionService -- they should only be > called through the Delegate interface. For now, maybe move them to the end of > the public section to keep the public interface together. > > Maybe we can actually move this onto a private nested class or into the .cc > file. This is already resolved, see https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... and https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets...
some more nits. Usually I'd insist in having the tests in this CL, but let's make sure we have agreement on the interface first ;-) https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:18: } shouldn't the closing namespace be commented like } // namespace ? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:41: virtual void OnSuggestionsChanged( personally, I'd try to avoid the term 'change' in this call as it suggests more info about the actual change (which might be added at some point in the future or not). Given that you simply inform about new data, I'd sympathize more with the names suggested by Marc before (OnNewSuggestions[Available] or something). However, this should also be simple enough to change once we add the new functions (if ever), so I'm not feeling strongly here. Feel free to leave as is if you prefer the current name. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:46: virtual void OnProviderShutdown( can you document this function? When is it called? What happens after the call? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:58: virtual void ClearCachedSuggestions() = 0; for functions which should not be called in production code, I like the convention to include this fact in there name. This makes it visible at the call site. Something like: ClearCachedSuggestionsForDebugging(). If these functions are called in tests, I'd go with ClearCachedSuggestionsForTesting(). Same goes for ClearDiscardedSuggestions(). Also, test/debug-only methods are not really part of the public interface, so I'd move them to the end of the public section to not confuse the reader. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:18: ContentSuggestionsProviderType GetProviderTypeFromCombinedID(std::string id) { do we already have move semantics in chrome? I'd go for const std::string& (same for GetIDFromCombinedID(). https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:22: return ContentSuggestionsProviderType(provider_type); use the most restrictive cast, i.e. static_cast<ContentSuggestionsProviderType>(). https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:69: return suggestions_by_category_.at(category); hmmm... is there anything guaranteeing that we have indeed an entry for every category enum? this might be hard to enforce, so maybe handle this gracefully? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:169: // TODO(pke) Useful ordering of categories. can you explain this TODO() in a bit more detail? =) https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:38: virtual void OnSuggestionsChanged() = 0; The service interface is taking on a lot of roles. Would clients ever access the data without being notified through the observer? If not, I wonder if it wouldn't be cleaner to factor out an data access interface and hand this into the Observer methods to properly decouple the two concerns the service is doing (data access + registry). Also it seems a bit weird that the observer needs to get access to the service from somewhere else when it only wants to react on observed events. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:40: // Sent when the service is shutting down. what are the post conditions after this function? what's the responsibilty of the observer? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:112: // All current suggestion categories, in order. what's the order? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:121: // The observers. you can drop this comment -- it's not adding any value thanks to proper variable naming :-)
Some comments on Tim's comments. Also quite a bit of overlap there, which I'll take as a good sign :) https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:18: ContentSuggestionsProviderType GetProviderTypeFromCombinedID(std::string id) { On 2016/06/30 10:55:19, tschumann wrote: > do we already have move semantics in chrome? > I'd go for const std::string& (same for GetIDFromCombinedID(). We do, but there's no reason to pass in ownership here. So yes, const ref please. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:22: return ContentSuggestionsProviderType(provider_type); On 2016/06/30 10:55:19, tschumann wrote: > use the most restrictive cast, i.e. > static_cast<ContentSuggestionsProviderType>(). AFAIK that will actually do the exact same thing. I think you have to manually check the range to really make sure. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:38: virtual void OnSuggestionsChanged() = 0; On 2016/06/30 10:55:19, tschumann wrote: > The service interface is taking on a lot of roles. > Would clients ever access the data without being notified through the observer? > If not, I wonder if it wouldn't be cleaner to factor out an data access > interface and hand this into the Observer methods to properly decouple the two > concerns the service is doing (data access + registry). Hm. I'm not convinced that's worth the extra complexity at this point (but not really opposed either). > Also it seems a bit weird that the observer needs to get access to the service > from somewhere else when it only wants to react on observed events. That's fairly common: The service is more or less a singleton (per-profile), you access it through the factory. And you need the service anyway to register yourself as an observer.
https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:22: return ContentSuggestionsProviderType(provider_type); On 2016/06/30 11:34:23, Marc Treib wrote: > On 2016/06/30 10:55:19, tschumann wrote: > > use the most restrictive cast, i.e. > > static_cast<ContentSuggestionsProviderType>(). > > AFAIK that will actually do the exact same thing. I think you have to manually > check the range to really make sure. in this particular case, c-style cast does the same thing, but keep in mind that the c-style cast might even apply a reinterpret cast in general (http://en.cppreference.com/w/cpp/language/explicit_cast). Range-checks need to be done independently, as it would be unspecified behavior. The question is just what we do in out-of-range cases. I guess we don't want to crash chrome, so we need to go on with something. But yeah, it would be better to document this explicitly. (might be a bit overkill for now, but this function could simply indicate an error and the calling side could then make sure to drop/ignore an offending element). https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:30: } On 2016/06/30 10:36:54, Marc Treib wrote: > I think it'd be nice if the "combine" and "extract" functions were in the same > place, next to each other. Maybe static methods on ContentSuggestion? Though > that kinda invites others to use them... hrm. I don't know. +1 They should live together but not on the ContentSuggestion. The ContentSuggestion should not know anything about the ID encoding/decoding. This file is IMO the right place to implement both. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:58: observers_.Clear(); On 2016/06/30 10:36:54, Marc Treib wrote: > Hm. Since the observers register themselves, they should probably also > unregister themselves. So DCHECK(observers_.empty()) ? +1 https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:127: ContentSuggestionsProviderType>::type>(provider_type); On 2016/06/30 10:36:54, Marc Treib wrote: > int(provider_type) i'd vote for implicit_cast<int> as it's the least powerful cast to do the job. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:38: virtual void OnSuggestionsChanged() = 0; On 2016/06/30 11:34:23, Marc Treib wrote: > On 2016/06/30 10:55:19, tschumann wrote: > > The service interface is taking on a lot of roles. > > Would clients ever access the data without being notified through the > observer? > > If not, I wonder if it wouldn't be cleaner to factor out an data access > > interface and hand this into the Observer methods to properly decouple the two > > concerns the service is doing (data access + registry). > > Hm. I'm not convinced that's worth the extra complexity at this point (but not > really opposed either). > > > Also it seems a bit weird that the observer needs to get access to the service > > from somewhere else when it only wants to react on observed events. > > That's fairly common: The service is more or less a singleton (per-profile), you > access it through the factory. And you need the service anyway to register > yourself as an observer. While that's true, I think it would be nice if the implementation detail that this is a singleton wouldn't ripple through the layers. It's kind of a dirty if you get access to Singletons from anywhere. The context is much nicer to grasp if a function gets the state from the framework it needs to work on (the framework <-> API tradeoff. I personally try to minimize framework logic and move it into easy to use, clean APIs). Especially functionality like "service shutdown" impose a certain state on the service -- users of the framework can't use the service after shutdown anymore (I guess). That's another reason why IMO accessing the service through the singleton should be minimized. If my observer is unregistered, and only gets access to the service through the observer functions, it won't get called anymore and there's no risk of accessing the service in a non-standard state.
https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:22: return ContentSuggestionsProviderType(provider_type); On 2016/06/30 11:58:11, tschumann wrote: > On 2016/06/30 11:34:23, Marc Treib wrote: > > On 2016/06/30 10:55:19, tschumann wrote: > > > use the most restrictive cast, i.e. > > > static_cast<ContentSuggestionsProviderType>(). > > > > AFAIK that will actually do the exact same thing. I think you have to manually > > check the range to really make sure. > > in this particular case, c-style cast does the same thing, but keep in mind that > the c-style cast might even apply a reinterpret cast in general > (http://en.cppreference.com/w/cpp/language/explicit_cast). > > Range-checks need to be done independently, as it would be unspecified behavior. > The question is just what we do in out-of-range cases. I guess we don't want to > crash chrome, so we need to go on with something. But yeah, it would be better > to document this explicitly. (might be a bit overkill for now, but this function > could simply indicate an error and the calling side could then make sure to > drop/ignore an offending element). IMO, the out-of-range case is one that should *never* happen - the only case where it would happen is if there was a bug in the code that generates the combined IDs. So we should just DCHECK (i.e. assert) that it doesn't happen, and not handle the failure.
Thank you, still some open points to be discussed. I have a running unit test that is not in the CL because there is only one provider type and the unit test needs to (to test the merging and splitting logic, which is the major part of the service at the moment). https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:14: if (enabled_) { On 2016/06/30 10:36:53, Marc Treib wrote: > On 2016/06/30 09:35:35, Philipp Keck wrote: > > On 2016/06/28 15:04:55, Marc Treib wrote: > > > On 2016/06/28 14:18:56, Philipp Keck wrote: > > > > On 2016/06/28 11:29:09, Marc Treib wrote: > > > > > Remove empty if. > > > > > It looks like the enabled_ flag currently doesn't do anything at all? It > > > > > probably should... > > > > > > > > The reason that there is no code is not that the enabled flag doesn't do > > > > anything. The reason is that the constructor currently doesn't need to do > > > > anything because there is simply no setup to be done. > > > > > > Alright, but then remove the empty if. > > > > > > > When the service is disabled, none of the providers' factories will > register > > a > > > > provider, so the service will remain empty. We could additionally prevent > > > calls > > > > to functions like RegisterProvider, GetSuggestionsForCategory and possibly > > all > > > > others or make them return empty results. > > > > > > Hm. If the "enabled" state can change at runtime (e.g. because we expose a > > > setting), then it might make sense to register the providers anyway, in case > > the > > > service later gets enabled. WDYT? > > > > I removed the empty if for now. The question how to properly implement that > > depends on how status cards are handled, i.e., where they are produced and, if > > that's in the backend, how they are passed to the UI. Are they special > > suggestions (to turn some settings on), are they a separate Delegate/Callback > > method, or is it just a Service::is_enabled() that returns false and the UI > > needs to realise that? > > > > I agree: All (available) providers should always be registered with the > service. > > If disabled, the service should clear its data stores and somehow tell the UI. > > Additionally, the service could make sure not to return any suggestions > (except > > maybe the appropriate status cards). > > If disabled, a provider should stop fetching data and not deliver data to the > > service anymore and tell the service that it's disabled (and the service > removes > > the corresponding data). How does a provider know that the main switch/feature > > is turned off? Maybe through a ContentSuggestionsService::is_enabled() getter > > and a Delete::ServiceDisabled() notification? Then we'd also need > > Delegate::ServiceEnabled() to re-enable. > > Hm, right, we need "StateChanged" notifications both ways (IMO this should be > one method with a parameter, rather than separate Enabled/Disabled ones). So > yes, I guess we need a method both on the ServiceObserver and the > ProviderObserver/Delegate. Yes, but I'm not implementing that (now), right? https://codereview.chromium.org/2102023002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:48: base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature); On 2016/06/30 10:36:53, Marc Treib wrote: > Please add a TODO to split the feature. I added the TODO, a corresponding CL is already prepared. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:14: std::string original_id) { On 2016/06/30 10:36:53, Marc Treib wrote: > const std::string& Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:15: return std::to_string(int(provider_type)) + ":" + original_id; On 2016/06/30 10:36:53, Marc Treib wrote: > Hm, base::StringPrintf would be the common way to do this in Chrome (and it > probably avoid some intermediate allocations) Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.cc:18: } On 2016/06/30 10:55:18, tschumann wrote: > shouldn't the closing namespace be commented like > > } // namespace > ? Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:26: ContentSuggestion(const std::string& id, On 2016/06/30 10:36:53, Marc Treib wrote: > Rename this to "provider_id" or something like that, to make clear that it's not > the actual ID of the ContentSuggestion? And/or add a comment explaining what's > going on. Should avoid "provider_id" because it would be an ID that identifies the provider itself. I call it "original_id" here because that's also the term used elsewhere. Added comments here and in the ContentSuggestionsProvider interface. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:41: virtual void OnSuggestionsChanged( On 2016/06/30 10:55:19, tschumann wrote: > personally, I'd try to avoid the term 'change' in this call as it suggests more > info about the actual change (which might be added at some point in the future > or not). Given that you simply inform about new data, I'd sympathize more with > the names suggested by Marc before (OnNewSuggestions[Available] or something). > > However, this should also be simple enough to change once we add the new > functions (if ever), so I'm not feeling strongly here. Feel free to leave as is > if you prefer the current name. I also suggested OnContentChanged earlier. The name "OnNewSuggestions" sounds confusing to me because the method is not only called when there are new suggestions and the |suggestions| parameter does not only contain the new suggestions, but it contains all suggestions and when old ones are missing, then the result of calling this function is effectively deleting suggestions (which is the opposite of "new suggestions"). What happens is: The whole list of available suggestions has (somehow) been updated. It could be that new suggestions were added, an existing one was modified, the list was reordered or some suggestions were deleted. And it's there four more fine-grained events that I would call: OnSuggestionsAdd/OnNewSuggestions, OnSuggestionModify, OnSuggestionMove, OnSuggestionsDelete -- so the name OnNewSuggestions would already be taken for the more specific event. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:46: virtual void OnProviderShutdown( On 2016/06/30 10:55:19, tschumann wrote: > can you document this function? When is it called? What happens after the call? Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:51: virtual ContentSuggestionsProviderType GetProviderType() const = 0; On 2016/06/30 10:36:53, Marc Treib wrote: > Hm, you could actually store the provider type here in the base class (passed in > from the subclass via a protected ctor). That removes some duplication between > the subclasses, and also makes sure they can't change their type at runtime. > WDYT? Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:58: virtual void ClearCachedSuggestions() = 0; On 2016/06/30 10:55:19, tschumann wrote: > for functions which should not be called in production code, I like the > convention to include this fact in there name. This makes it visible at the call > site. > Something like: ClearCachedSuggestionsForDebugging(). > If these functions are called in tests, I'd go with > ClearCachedSuggestionsForTesting(). > > Same goes for ClearDiscardedSuggestions(). > > Also, test/debug-only methods are not really part of the public interface, so > I'd move them to the end of the public section to not confuse the reader. Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:66: // Discards the suggestion with the given ID. A provider needs to ensure that On 2016/06/30 10:36:53, Marc Treib wrote: > nit: I'd put the "real" API methods before the debugging-only ones. Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:68: // Observer). There is no need to call the observer for an update after a On 2016/06/30 10:36:53, Marc Treib wrote: > This formulation leaves it open whether the observer will be called or not. We > should make a decision and stick with it. I didn't want to require that to happen because it would be inefficient. But I also didn't want to disallow it because it might be inherent to the way a provider is implemented (e.g. they forward the deletion call to some underlying data model that is not under our control and that data model emits an event). Of course the provider could temporarily suppress the event, but that would look hacky to me. Since the UI doesn't update anyway, it's not much of a difference: Unnecessary updates will only bubble up to the cache on the service, not further on. So maybe we should require an update in any case? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:18: ContentSuggestionsProviderType GetProviderTypeFromCombinedID(std::string id) { On 2016/06/30 11:34:23, Marc Treib wrote: > On 2016/06/30 10:55:19, tschumann wrote: > > do we already have move semantics in chrome? > > I'd go for const std::string& (same for GetIDFromCombinedID(). > > We do, but there's no reason to pass in ownership here. So yes, const ref > please. Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:19: auto colon_index = id.find(":"); On 2016/06/30 10:36:54, Marc Treib wrote: > size_t please - if you say "auto", then it kinda looks like this was an iterator > IMO Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:21: int provider_type = std::stoi(id.substr(0, colon_index)); On 2016/06/30 10:36:54, Marc Treib wrote: > What happens if this fails to parse as an int? > std::stoi isn't allowed in Chromium yet, see https://chromium-cpp.appspot.com/. > The common way to do this in Chrome is base::StringToInt. Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:22: return ContentSuggestionsProviderType(provider_type); On 2016/06/30 13:36:12, Marc Treib wrote: > On 2016/06/30 11:58:11, tschumann wrote: > > On 2016/06/30 11:34:23, Marc Treib wrote: > > > On 2016/06/30 10:55:19, tschumann wrote: > > > > use the most restrictive cast, i.e. > > > > static_cast<ContentSuggestionsProviderType>(). > > > > > > AFAIK that will actually do the exact same thing. I think you have to > manually > > > check the range to really make sure. > > > > in this particular case, c-style cast does the same thing, but keep in mind > that > > the c-style cast might even apply a reinterpret cast in general > > (http://en.cppreference.com/w/cpp/language/explicit_cast). > > > > Range-checks need to be done independently, as it would be unspecified > behavior. > > The question is just what we do in out-of-range cases. I guess we don't want > to > > crash chrome, so we need to go on with something. But yeah, it would be better > > to document this explicitly. (might be a bit overkill for now, but this > function > > could simply indicate an error and the calling side could then make sure to > > drop/ignore an offending element). > > IMO, the out-of-range case is one that should *never* happen - the only case > where it would happen is if there was a bug in the code that generates the > combined IDs. So we should just DCHECK (i.e. assert) that it doesn't happen, and > not handle the failure. I agree with Marc, and equally the case when the string (the part before the colon) is not numeric should never happen, so also DCHECK that. Also, I don't see any other sensible option, merely logging the problem is not possible because something needs to be returned. Not sure if I should still change it to static_cast? Like this, it's similar to the other way "int(...)" and similar to other code around. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:30: } On 2016/06/30 11:58:11, tschumann wrote: > On 2016/06/30 10:36:54, Marc Treib wrote: > > I think it'd be nice if the "combine" and "extract" functions were in the same > > place, next to each other. Maybe static methods on ContentSuggestion? Though > > that kinda invites others to use them... hrm. I don't know. > +1 > They should live together but not on the ContentSuggestion. The > ContentSuggestion should not know anything about the ID encoding/decoding. This > file is IMO the right place to implement both. I had the same dilemma. ContentSuggestion is definetely not the place because that's where we're trying to ban the provider_type from. So if we offer a GetProviderType() there, that completely defeats the purpose. Though we could make it private (only the GetProviderType()) and have ContentSuggestionsService as a friend class? The combining conversion pretty much needs to happen in the ContentSuggestion constructor, otherwise it gets really complicated for a provider to create a new ContentSuggestion. Could move it into ContentSuggestionsService as public static members, but make the GetProviderType() a private member? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:43: } else { On 2016/06/30 10:36:54, Marc Treib wrote: > No "else" after "return" I changed it. Though I personally like the shape of code like: if (cond) return A; else return B; In other projects I tend to use if (cond) return; other_statements; when the condition is rather exceptional and the return is more like "abort" and I use the above form when the two cases are equally fine. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:58: observers_.Clear(); On 2016/06/30 11:58:11, tschumann wrote: > On 2016/06/30 10:36:54, Marc Treib wrote: > > Hm. Since the observers register themselves, they should probably also > > unregister themselves. So DCHECK(observers_.empty()) ? > > +1 Marc's precondition is not perfectly true: Observers don't register themselves, but they get registered by their factory. They don't have a reference to the service, so they couldn't call an Unregister(Provider) that is placed next to the Register(Provider) in the service. Instead, there would need to be a Unregister(ProviderType) on the Observer interface, which is somewhat similar to the OnProviderShutdown method there (except that the provider might not want to shut down, so it would be a separate method?). It would make the shutdown more inefficient in that the service would need to remove and filter out all suggestions of every provider that's shutting down instead of just doing categories_.clear() and suggestions_by_category_.clear(), which is missing here btw. So maybe the service would need a state "shutting down"? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:63: enabled_ = false; On 2016/06/30 10:36:54, Marc Treib wrote: > Also clear the cached suggestions, just to be sure? Yes, see previous comment. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:69: return suggestions_by_category_.at(category); On 2016/06/30 10:55:19, tschumann wrote: > hmmm... is there anything guaranteeing that we have indeed an entry for every > category enum? this might be hard to enforce, so maybe handle this gracefully? In the obvious, simple Android implementation, this would only be called directly after calling GetCategories(), and only be called for every entry in categories_. An empty list would be different from no list or an error. This also depends on the way the status cards work (are the suggestions themselves or not). https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:77: std::string original_id = GetIDFromCombinedID(suggestion_id); On 2016/06/30 10:36:54, Marc Treib wrote: > nit: move this into the "if" Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:113: for (auto& pair : suggestions_by_category_) { On 2016/06/30 10:36:54, Marc Treib wrote: > Can't you get the category from the suggestions, rather than going over all of > them? Then I would need to include the category in the DiscardSuggestion() signature. Or make it part of the ID. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:116: [suggestion_id](const ContentSuggestion& suggestion) { On 2016/06/30 10:36:54, Marc Treib wrote: > You could capture this by reference, to save a string copy Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:127: ContentSuggestionsProviderType>::type>(provider_type); On 2016/06/30 11:58:11, tschumann wrote: > On 2016/06/30 10:36:54, Marc Treib wrote: > > int(provider_type) > > i'd vote for implicit_cast<int> as it's the least powerful cast to do the job. Wasn't sure where to import the implicit_cast from, so it's int() for now. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:169: // TODO(pke) Useful ordering of categories. On 2016/06/30 10:55:19, tschumann wrote: > can you explain this TODO() in a bit more detail? =) Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:27: // them grouped into categories. On 2016/06/30 10:36:54, Marc Treib wrote: > Add a "not used yet" comment, similar to ContentSuggestion itself? Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:38: virtual void OnSuggestionsChanged() = 0; On 2016/06/30 11:58:11, tschumann wrote: > On 2016/06/30 11:34:23, Marc Treib wrote: > > On 2016/06/30 10:55:19, tschumann wrote: > > > The service interface is taking on a lot of roles. > > > Would clients ever access the data without being notified through the > > observer? > > > If not, I wonder if it wouldn't be cleaner to factor out an data access > > > interface and hand this into the Observer methods to properly decouple the > two > > > concerns the service is doing (data access + registry). > > > > Hm. I'm not convinced that's worth the extra complexity at this point (but not > > really opposed either). > > > > > Also it seems a bit weird that the observer needs to get access to the > service > > > from somewhere else when it only wants to react on observed events. > > > > That's fairly common: The service is more or less a singleton (per-profile), > you > > access it through the factory. And you need the service anyway to register > > yourself as an observer. > > While that's true, I think it would be nice if the implementation detail that > this is a singleton wouldn't ripple through the layers. It's kind of a dirty if > you get access to Singletons from anywhere. The context is much nicer to grasp > if a function gets the state from the framework it needs to work on (the > framework <-> API tradeoff. I personally try to minimize framework logic and > move it into easy to use, clean APIs). > Especially functionality like "service shutdown" impose a certain state on the > service -- users of the framework can't use the service after shutdown anymore > (I guess). That's another reason why IMO accessing the service through the > singleton should be minimized. If my observer is unregistered, and only gets > access to the service through the observer functions, it won't get called > anymore and there's no risk of accessing the service in a non-standard state. The observer here is the UI, really. The UI needs access to the service even outside of calls that came through the observer interface (for the FetchSuggestionImage and DiscardSuggestion methods). FetchSuggestionImage to me is also part of the data access API, but even if it's not: As soon as we build a "Refresh this section" button or infinite scroll or anything like that, the UI will need direct access to the data model, so it will need to store a pointer to the service. We could introduce a smaller interface for all the service methods that the UI needs (not Shutdown, not RegisterProvider, not the debugging stuff) and have the UI use the service through that. Currently I don't see a big advantage, but rather the need to change method signatures in yet another place whenever something changes (like FetchSnippetImage -> FetchImage -> FetchSuggestionImage -> remove provider_type argument because it's now in the ID, ...). I think it slows down code changes more than it currently helps. It should be easy to add such an interface later on. By the UI, the new factory will only be used once, namely in the snippets bridge constructor. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:40: // Sent when the service is shutting down. On 2016/06/30 10:55:19, tschumann wrote: > what are the post conditions after this function? what's the responsibilty of > the observer? Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:105: void OnProviderShutdown( On 2016/06/30 10:36:54, Marc Treib wrote: > This is one of the cases where there usually isn't an empty line between > methods. (I guess Tim might disagree with this style? I kinda like having all > methods from one superclass grouped together, but I don't care enough to argue > :) ) Yep, this empty line has been inserted because of Tim's comment (otherwise it's harder to see that there is another function, as the signature of the upper one is already 4 lines). Styleguide asks to minimize the use of empty lines though. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:112: // All current suggestion categories, in order. On 2016/06/30 10:55:19, tschumann wrote: > what's the order? That's not yet defined. It's a UX question, I guess. Currently it's simply the order in which content for the respective categories first became available. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:116: suggestions_by_category_; On 2016/06/30 10:36:54, Marc Treib wrote: > Hm, maybe at this point just a vector of pairs would be simpler? Up to you Acknowledged. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:121: // The observers. On 2016/06/30 10:55:19, tschumann wrote: > you can drop this comment -- it's not adding any value thanks to proper variable > naming :-) Done.
https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:14: if (enabled_) { On 2016/06/30 17:14:07, Philipp Keck wrote: > On 2016/06/30 10:36:53, Marc Treib wrote: > > On 2016/06/30 09:35:35, Philipp Keck wrote: > > > On 2016/06/28 15:04:55, Marc Treib wrote: > > > > On 2016/06/28 14:18:56, Philipp Keck wrote: > > > > > On 2016/06/28 11:29:09, Marc Treib wrote: > > > > > > Remove empty if. > > > > > > It looks like the enabled_ flag currently doesn't do anything at all? > It > > > > > > probably should... > > > > > > > > > > The reason that there is no code is not that the enabled flag doesn't do > > > > > anything. The reason is that the constructor currently doesn't need to > do > > > > > anything because there is simply no setup to be done. > > > > > > > > Alright, but then remove the empty if. > > > > > > > > > When the service is disabled, none of the providers' factories will > > register > > > a > > > > > provider, so the service will remain empty. We could additionally > prevent > > > > calls > > > > > to functions like RegisterProvider, GetSuggestionsForCategory and > possibly > > > all > > > > > others or make them return empty results. > > > > > > > > Hm. If the "enabled" state can change at runtime (e.g. because we expose a > > > > setting), then it might make sense to register the providers anyway, in > case > > > the > > > > service later gets enabled. WDYT? > > > > > > I removed the empty if for now. The question how to properly implement that > > > depends on how status cards are handled, i.e., where they are produced and, > if > > > that's in the backend, how they are passed to the UI. Are they special > > > suggestions (to turn some settings on), are they a separate > Delegate/Callback > > > method, or is it just a Service::is_enabled() that returns false and the UI > > > needs to realise that? > > > > > > I agree: All (available) providers should always be registered with the > > service. > > > If disabled, the service should clear its data stores and somehow tell the > UI. > > > Additionally, the service could make sure not to return any suggestions > > (except > > > maybe the appropriate status cards). > > > If disabled, a provider should stop fetching data and not deliver data to > the > > > service anymore and tell the service that it's disabled (and the service > > removes > > > the corresponding data). How does a provider know that the main > switch/feature > > > is turned off? Maybe through a ContentSuggestionsService::is_enabled() > getter > > > and a Delete::ServiceDisabled() notification? Then we'd also need > > > Delegate::ServiceEnabled() to re-enable. > > > > Hm, right, we need "StateChanged" notifications both ways (IMO this should be > > one method with a parameter, rather than separate Enabled/Disabled ones). So > > yes, I guess we need a method both on the ServiceObserver and the > > ProviderObserver/Delegate. > > Yes, but I'm not implementing that (now), right? Yeah, another CL is fine, but we'll need at least some of it before we can switch to the new service (to clear out any suggestions on sign-out). https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestion.h:26: ContentSuggestion(const std::string& id, On 2016/06/30 17:14:07, Philipp Keck wrote: > On 2016/06/30 10:36:53, Marc Treib wrote: > > Rename this to "provider_id" or something like that, to make clear that it's > not > > the actual ID of the ContentSuggestion? And/or add a comment explaining what's > > going on. > > Should avoid "provider_id" because it would be an ID that identifies the > provider itself. I call it "original_id" here because that's also the term used > elsewhere. > Added comments here and in the ContentSuggestionsProvider interface. Acknowledged. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:41: virtual void OnSuggestionsChanged( On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 10:55:19, tschumann wrote: > > personally, I'd try to avoid the term 'change' in this call as it suggests > more > > info about the actual change (which might be added at some point in the future > > or not). Given that you simply inform about new data, I'd sympathize more with > > the names suggested by Marc before (OnNewSuggestions[Available] or something). > > > > However, this should also be simple enough to change once we add the new > > functions (if ever), so I'm not feeling strongly here. Feel free to leave as > is > > if you prefer the current name. > > I also suggested OnContentChanged earlier. > > The name "OnNewSuggestions" sounds confusing to me because the method is not > only called when there are new suggestions and the |suggestions| parameter does > not only contain the new suggestions, but it contains all suggestions and when > old ones are missing, then the result of calling this function is effectively > deleting suggestions (which is the opposite of "new suggestions"). > > What happens is: The whole list of available suggestions has (somehow) been > updated. It could be that new suggestions were added, an existing one was > modified, the list was reordered or some suggestions were deleted. And it's > there four more fine-grained events that I would call: > OnSuggestionsAdd/OnNewSuggestions, OnSuggestionModify, OnSuggestionMove, > OnSuggestionsDelete -- so the name OnNewSuggestions would already be taken for > the more specific event. IMO "OnSuggestionsAdded" would be a better (because less ambiguous) name for the potential fine-grained event. Anyway, we've bike-shedded this name enough, and we can still change it later if the need arises. So either "OnSuggestionsChanged" or "OnNewSuggestions[Available]" are fine with me. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:68: // Observer). There is no need to call the observer for an update after a On 2016/06/30 17:14:07, Philipp Keck wrote: > On 2016/06/30 10:36:53, Marc Treib wrote: > > This formulation leaves it open whether the observer will be called or not. We > > should make a decision and stick with it. > > I didn't want to require that to happen because it would be inefficient. But I > also didn't want to disallow it because it might be inherent to the way a > provider is implemented (e.g. they forward the deletion call to some underlying > data model that is not under our control and that data model emits an event). Of > course the provider could temporarily suppress the event, but that would look > hacky to me. > > Since the UI doesn't update anyway, it's not much of a difference: Unnecessary > updates will only bubble up to the cache on the service, not further on. So > maybe we should require an update in any case? I'd slightly prefer sending no update, because then the event can be interpreted as "show a 'new content available' indicator". But I'm fine either way, as long as we have consistency, so either all providers send and update, or none do. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:22: return ContentSuggestionsProviderType(provider_type); On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 13:36:12, Marc Treib wrote: > > On 2016/06/30 11:58:11, tschumann wrote: > > > On 2016/06/30 11:34:23, Marc Treib wrote: > > > > On 2016/06/30 10:55:19, tschumann wrote: > > > > > use the most restrictive cast, i.e. > > > > > static_cast<ContentSuggestionsProviderType>(). > > > > > > > > AFAIK that will actually do the exact same thing. I think you have to > > manually > > > > check the range to really make sure. > > > > > > in this particular case, c-style cast does the same thing, but keep in mind > > that > > > the c-style cast might even apply a reinterpret cast in general > > > (http://en.cppreference.com/w/cpp/language/explicit_cast). > > > > > > Range-checks need to be done independently, as it would be unspecified > > behavior. > > > The question is just what we do in out-of-range cases. I guess we don't want > > to > > > crash chrome, so we need to go on with something. But yeah, it would be > better > > > to document this explicitly. (might be a bit overkill for now, but this > > function > > > could simply indicate an error and the calling side could then make sure to > > > drop/ignore an offending element). > > > > IMO, the out-of-range case is one that should *never* happen - the only case > > where it would happen is if there was a bug in the code that generates the > > combined IDs. So we should just DCHECK (i.e. assert) that it doesn't happen, > and > > not handle the failure. > > I agree with Marc, and equally the case when the string (the part before the > colon) is not numeric should never happen, so also DCHECK that. Also, I don't > see any other sensible option, merely logging the problem is not possible > because something needs to be returned. > > Not sure if I should still change it to static_cast? Like this, it's similar to > the other way "int(...)" and similar to other code around. I find "create a ContentSuggestionsProviderType out of this int", constructor-style, nicer than the static_cast (which IMO mostly makes sense for pointers, not for conversions between built-in types). But if Tim disagrees, I don't care enough to argue ;) https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:30: } On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 11:58:11, tschumann wrote: > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > I think it'd be nice if the "combine" and "extract" functions were in the > same > > > place, next to each other. Maybe static methods on ContentSuggestion? Though > > > that kinda invites others to use them... hrm. I don't know. > > +1 > > They should live together but not on the ContentSuggestion. The > > ContentSuggestion should not know anything about the ID encoding/decoding. > This > > file is IMO the right place to implement both. > > I had the same dilemma. ContentSuggestion is definetely not the place because > that's where we're trying to ban the provider_type from. So if we offer a > GetProviderType() there, that completely defeats the purpose. Though we could > make it private (only the GetProviderType()) and have ContentSuggestionsService > as a friend class? Naaah. > The combining conversion pretty much needs to happen in the ContentSuggestion > constructor, otherwise it gets really complicated for a provider to create a new > ContentSuggestion. Could move it into ContentSuggestionsService as public static > members, but make the GetProviderType() a private member? What GetProviderType? ContentSuggestion shouldn't have that at all. We could create a "content_suggestion_utils.h" or something like that, and make it private to the component (gn has a way of specifying which headers are considered "external" and may be used by clients, though I don't remember the details). https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:43: } else { On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 10:36:54, Marc Treib wrote: > > No "else" after "return" > > I changed it. Though I personally like the shape of code like: > if (cond) return A; > else return B; > > In other projects I tend to use > if (cond) return; > other_statements; > when the condition is rather exceptional and the return is more like "abort" and > I use the above form when the two cases are equally fine. I agree, in the second example, the benefit is clearer. But the code style is unambiguous: https://www.chromium.org/developers/coding-style https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:58: observers_.Clear(); On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 11:58:11, tschumann wrote: > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > Hm. Since the observers register themselves, they should probably also > > > unregister themselves. So DCHECK(observers_.empty()) ? > > > > +1 > > Marc's precondition is not perfectly true: Observers don't register themselves, > but they get registered by their factory. They don't have a reference to the > service, so they couldn't call an Unregister(Provider) that is placed next to > the Register(Provider) in the service. Instead, there would need to be a > Unregister(ProviderType) on the Observer interface, which is somewhat similar to > the OnProviderShutdown method there (except that the provider might not want to > shut down, so it would be a separate method?). > > It would make the shutdown more inefficient in that the service would need to > remove and filter out all suggestions of every provider that's shutting down > instead of just doing categories_.clear() and suggestions_by_category_.clear(), > which is missing here btw. > So maybe the service would need a state "shutting down"? But the observers need a reference to the service anyway, to get the actual data, right? Anyway, the Register and Unregister should be handled from the same place. So the factory is probably not the right place for that. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:63: enabled_ = false; On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 10:36:54, Marc Treib wrote: > > Also clear the cached suggestions, just to be sure? > > Yes, see previous comment. Hmm? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:69: return suggestions_by_category_.at(category); On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 10:55:19, tschumann wrote: > > hmmm... is there anything guaranteeing that we have indeed an entry for every > > category enum? this might be hard to enforce, so maybe handle this gracefully? > > In the obvious, simple Android implementation, this would only be called > directly after calling GetCategories(), and only be called for every entry in > categories_. > > An empty list would be different from no list or an error. This also depends on > the way the status cards work (are the suggestions themselves or not). "no list" isn't an option if you return a reference. Anyway, crashing when this is called with a currently empty category seems like a bad API. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:113: for (auto& pair : suggestions_by_category_) { On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 10:36:54, Marc Treib wrote: > > Can't you get the category from the suggestions, rather than going over all of > > them? > > Then I would need to include the category in the DiscardSuggestion() signature. > Or make it part of the ID. Ah, right, sorry. Carry on :) https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:120: pair.second.erase(position); Add a break? Once we're removed the suggestion, there's no reason to keep looking. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:123: } else { nit: Turning the if around might make things a bit easier to read (remove one level of indentation): if (no provider) LOG return carrying on... https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:127: ContentSuggestionsProviderType>::type>(provider_type); On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 11:58:11, tschumann wrote: > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > int(provider_type) > > > > i'd vote for implicit_cast<int> as it's the least powerful cast to do the job. > > Wasn't sure where to import the implicit_cast from, so it's int() for now. implicit_cast isn't a standard C++ feature, and doesn't exist in Chromium :) https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:38: virtual void OnSuggestionsChanged() = 0; On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 11:58:11, tschumann wrote: > > On 2016/06/30 11:34:23, Marc Treib wrote: > > > On 2016/06/30 10:55:19, tschumann wrote: > > > > The service interface is taking on a lot of roles. > > > > Would clients ever access the data without being notified through the > > > observer? > > > > If not, I wonder if it wouldn't be cleaner to factor out an data access > > > > interface and hand this into the Observer methods to properly decouple the > > two > > > > concerns the service is doing (data access + registry). > > > > > > Hm. I'm not convinced that's worth the extra complexity at this point (but > not > > > really opposed either). > > > > > > > Also it seems a bit weird that the observer needs to get access to the > > service > > > > from somewhere else when it only wants to react on observed events. > > > > > > That's fairly common: The service is more or less a singleton (per-profile), > > you > > > access it through the factory. And you need the service anyway to register > > > yourself as an observer. > > > > While that's true, I think it would be nice if the implementation detail that > > this is a singleton wouldn't ripple through the layers. It's kind of a dirty > if > > you get access to Singletons from anywhere. The context is much nicer to grasp > > if a function gets the state from the framework it needs to work on (the > > framework <-> API tradeoff. I personally try to minimize framework logic and > > move it into easy to use, clean APIs). > > Especially functionality like "service shutdown" impose a certain state on the > > service -- users of the framework can't use the service after shutdown anymore > > (I guess). That's another reason why IMO accessing the service through the > > singleton should be minimized. If my observer is unregistered, and only gets > > access to the service through the observer functions, it won't get called > > anymore and there's no risk of accessing the service in a non-standard state. > > The observer here is the UI, really. The UI needs access to the service even > outside of calls that came through the observer interface (for the > FetchSuggestionImage and DiscardSuggestion methods). FetchSuggestionImage to me > is also part of the data access API, but even if it's not: As soon as we build a > "Refresh this section" button or infinite scroll or anything like that, the UI > will need direct access to the data model, so it will need to store a pointer to > the service. > > We could introduce a smaller interface for all the service methods that the UI > needs (not Shutdown, not RegisterProvider, not the debugging stuff) and have the > UI use the service through that. Currently I don't see a big advantage, but > rather the need to change method signatures in yet another place whenever > something changes (like FetchSnippetImage -> FetchImage -> FetchSuggestionImage > -> remove provider_type argument because it's now in the ID, ...). I think it > slows down code changes more than it currently helps. It should be easy to add > such an interface later on. > > By the UI, the new factory will only be used once, namely in the snippets bridge > constructor. +1. I see the point of Tim's suggestion, but I think at this point the extra indirection would create more confusion than clarity. We might still decide to add it later, but IMO we're not at that point yet. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:105: void OnProviderShutdown( On 2016/06/30 17:14:09, Philipp Keck wrote: > On 2016/06/30 10:36:54, Marc Treib wrote: > > This is one of the cases where there usually isn't an empty line between > > methods. (I guess Tim might disagree with this style? I kinda like having all > > methods from one superclass grouped together, but I don't care enough to argue > > :) ) > > Yep, this empty line has been inserted because of Tim's comment (otherwise it's > harder to see that there is another function, as the signature of the upper one > is already 4 lines). Styleguide asks to minimize the use of empty lines though. I'd argue that nobody who reads this header should care about these methods. Whatever. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestion.cc:5: #include "base/strings/stringprintf.h" This should go below the include for the content_suggestion.h header (with an empty line between) https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:83: ContentSuggestionsProviderType GetProviderType() const { Style guide says to name trivial getters in variable style, i.e. provider_type() (yeah, I don't like it either) https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:29: // (see ntp_snippet_service.h). nit: NTPSnippetsService and ntp_snippets_service.h (plural) https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:52: enum Enabled : bool { I'd call the enum itself State or something like that. Also maybe enum class, since you seem to be using it in the "scoped" style anyway? https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:74: // (see ntp_snippet.h).pty image. Copy&paste fail?
few more nits. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:41: virtual void OnSuggestionsChanged( On 2016/06/30 17:14:08, Philipp Keck wrote: > On 2016/06/30 10:55:19, tschumann wrote: > > personally, I'd try to avoid the term 'change' in this call as it suggests > more > > info about the actual change (which might be added at some point in the future > > or not). Given that you simply inform about new data, I'd sympathize more with > > the names suggested by Marc before (OnNewSuggestions[Available] or something). > > > > However, this should also be simple enough to change once we add the new > > functions (if ever), so I'm not feeling strongly here. Feel free to leave as > is > > if you prefer the current name. > > I also suggested OnContentChanged earlier. > > The name "OnNewSuggestions" sounds confusing to me because the method is not > only called when there are new suggestions and the |suggestions| parameter does > not only contain the new suggestions, but it contains all suggestions and when > old ones are missing, then the result of calling this function is effectively > deleting suggestions (which is the opposite of "new suggestions"). > > What happens is: The whole list of available suggestions has (somehow) been > updated. It could be that new suggestions were added, an existing one was > modified, the list was reordered or some suggestions were deleted. And it's > there four more fine-grained events that I would call: > OnSuggestionsAdd/OnNewSuggestions, OnSuggestionModify, OnSuggestionMove, > OnSuggestionsDelete -- so the name OnNewSuggestions would already be taken for > the more specific event. I see your point. My motivation was that the Observer should not interpret the single elements and not care about partial updates at all. If there's new data (which might as well be the same that was displayed before), the existing data should be thrown away and replaced by the new data. The observer should not care about the deeper meaning of a new suggestion (i.e. whether a suggestion is new to the user or not) -- it should simply display whatever the service gives. IMO, the examples of more fine grained methods fit nicely into this model. OnSuggestionsChange() suggest that there's an actual change in data and that the observer might care about that change. But it might as well be the very same data set. Anyways, I feel we're spending too much time on this. Both are probably acceptable, just wanted to get my point across ;-) Like I said, pick one :-) https://codereview.chromium.org/2102023002/diff/120001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/120001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:20: using Enabled = ntp_snippets::ContentSuggestionsService::Enabled; move into the BuildServiceInstanceFor() function https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:31: const ContentSuggestionsProviderType provider_type, IMO the suggestion constructor should only get a single ID. The service shouldn't leak any kind of provider information. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:47: // suggestions anymore. This unregisters the provider, removes the observer "this unregisters the provider" I'm a bit confused what 'this' means. If it's meaning the OnProviderShutdown function, then it's something that implementations need to take care of -- that would be good to explicitly state (implementations need to makes sure to...). If it's meaning is the shut down process, then please state if the provider gets unregistered before or after this function (although I'm not sure where and how that would be done). Same for the rest of the comment. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:54: // Accessor for observer, which is notified about new available suggestions. nit: Now it's just a setter. So maybe rephrase to "Sets an observer which is notified ..". https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:83: ContentSuggestionsProviderType GetProviderType() const { Is this needed anywhere except in RegisterProvider()? What code is calling RegisterProvider? could we simply pass in the type into RegisterProvider and remove this method from the interface? On the topic of naming getters. Actually, it's just an option to use the lower_case variant. I'm also not a big fan of this, so we could just decide to go with CamelCase naming. from https://google.github.io/styleguide/cppguide.html#Function_Names "Functions that are very cheap to call may instead follow the style for variable names (all lower-case, with underscores between words)." https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:18: // Helper method get the provider_type from a combined ID. nit: let's keep the comment focused on the essential part. Drop the helper and write the comment in a descriptive way (i.e. 'gets' instead of 'get'), like: // Extracts the provider type from a ContentSuggestion ID. similar for GetIDFromCombinedID() and other functions. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:63: for (auto& provider : providers_) { Too bad we don't have value_view in Chrome (https://cs.corp.google.com/piper///depot/google3/util/gtl/iterator_adaptors.h...) :-/ https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:43: // the getters and other methods should not be used anymore. getters of the service? Is it safe to say, that the service shouldn't be used anymore? Would be good to get those semantics as simple as possible. How do we actually make sure all parts in the system know that the service singleton must not be touched anymore? Further thought: Is there a chance we can get rid of the shutdown semantics at all? [happy to discuss this offline] https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:53: Enable = true, shouldn't these follow the constant naming convention, i.e. kEnabled? (https://google.github.io/styleguide/cppguide.html#Enumerator_Names)
https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:43: // the getters and other methods should not be used anymore. On 2016/07/01 09:37:11, tschumann wrote: > getters of the service? > Is it safe to say, that the service shouldn't be used anymore? > Would be good to get those semantics as simple as possible. > How do we actually make sure all parts in the system know that the service > singleton must not be touched anymore? > > Further thought: Is there a chance we can get rid of the shutdown semantics at > all? [happy to discuss this offline] Probably not; these are a fixed part of the KeyedService system. That said, if the inter-service dependencies are properly set (i.e. all providers depend on the service), then all providers will be shut down before the service itself is shut down, so there's no reason to have the ContentSuggestionsServiceShutdown method. However, that requires all providers to be KeyedServices... https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:53: Enable = true, On 2016/07/01 09:37:11, tschumann wrote: > shouldn't these follow the constant naming convention, i.e. kEnabled? > (https://google.github.io/styleguide/cppguide.html#Enumerator_Names) In Chrome, MACRO_STYLE is officially preferred (https://www.chromium.org/developers/coding-style), but both styles are used in practice.
https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:127: ContentSuggestionsProviderType>::type>(provider_type); On 2016/07/01 09:15:32, Marc Treib wrote: > On 2016/06/30 17:14:08, Philipp Keck wrote: > > On 2016/06/30 11:58:11, tschumann wrote: > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > int(provider_type) > > > > > > i'd vote for implicit_cast<int> as it's the least powerful cast to do the > job. > > > > Wasn't sure where to import the implicit_cast from, so it's int() for now. > > implicit_cast isn't a standard C++ feature, and doesn't exist in Chromium :) =) argh... spent too much time in google3 it seems... so static_cast it is :-)
I increasingly like the idea of only allowing one provider per category. It solves a couple of problems, makes the code less complex, makes it run faster, and it's not an unreasonable assumption. Please see my two longer comments below. https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:14: if (enabled_) { On 2016/07/01 09:15:32, Marc Treib wrote: > On 2016/06/30 17:14:07, Philipp Keck wrote: > > On 2016/06/30 10:36:53, Marc Treib wrote: > > > On 2016/06/30 09:35:35, Philipp Keck wrote: > > > > On 2016/06/28 15:04:55, Marc Treib wrote: > > > > > On 2016/06/28 14:18:56, Philipp Keck wrote: > > > > > > On 2016/06/28 11:29:09, Marc Treib wrote: > > > > > > > Remove empty if. > > > > > > > It looks like the enabled_ flag currently doesn't do anything at > all? > > It > > > > > > > probably should... > > > > > > > > > > > > The reason that there is no code is not that the enabled flag doesn't > do > > > > > > anything. The reason is that the constructor currently doesn't need to > > do > > > > > > anything because there is simply no setup to be done. > > > > > > > > > > Alright, but then remove the empty if. > > > > > > > > > > > When the service is disabled, none of the providers' factories will > > > register > > > > a > > > > > > provider, so the service will remain empty. We could additionally > > prevent > > > > > calls > > > > > > to functions like RegisterProvider, GetSuggestionsForCategory and > > possibly > > > > all > > > > > > others or make them return empty results. > > > > > > > > > > Hm. If the "enabled" state can change at runtime (e.g. because we expose > a > > > > > setting), then it might make sense to register the providers anyway, in > > case > > > > the > > > > > service later gets enabled. WDYT? > > > > > > > > I removed the empty if for now. The question how to properly implement > that > > > > depends on how status cards are handled, i.e., where they are produced > and, > > if > > > > that's in the backend, how they are passed to the UI. Are they special > > > > suggestions (to turn some settings on), are they a separate > > Delegate/Callback > > > > method, or is it just a Service::is_enabled() that returns false and the > UI > > > > needs to realise that? > > > > > > > > I agree: All (available) providers should always be registered with the > > > service. > > > > If disabled, the service should clear its data stores and somehow tell the > > UI. > > > > Additionally, the service could make sure not to return any suggestions > > > (except > > > > maybe the appropriate status cards). > > > > If disabled, a provider should stop fetching data and not deliver data to > > the > > > > service anymore and tell the service that it's disabled (and the service > > > removes > > > > the corresponding data). How does a provider know that the main > > switch/feature > > > > is turned off? Maybe through a ContentSuggestionsService::is_enabled() > > getter > > > > and a Delete::ServiceDisabled() notification? Then we'd also need > > > > Delegate::ServiceEnabled() to re-enable. > > > > > > Hm, right, we need "StateChanged" notifications both ways (IMO this should > be > > > one method with a parameter, rather than separate Enabled/Disabled ones). So > > > yes, I guess we need a method both on the ServiceObserver and the > > > ProviderObserver/Delegate. > > > > Yes, but I'm not implementing that (now), right? > > Yeah, another CL is fine, but we'll need at least some of it before we can > switch to the new service (to clear out any suggestions on sign-out). Yes. I'm thinking about extending the new Enabled enum to make it a status enum similar to the new state handling in NTPSnippetsService. This would be a lot easier if we enforced a mapping from categories to providers (i.e. a category can be provided by at most one provider; if more providers want to, the second one will be ignored). Then we could have (slightly) category-specific code even in the UI; or generic code and just a list of possible categories in the UI. And the UI could ask "what's the status of category X?", whereupon the service could say "there's not even a provider for that" or it could ask the provider for the status of that category and forward the answer to the UI. Without such a mapping, it is unclear why a category is currently empty: Are the responsible provider(s) still loading? Or have they loaded an empty set? Have they encountered errors? Are they not registered? Another advantage of such a mapping is that we could get rid of the provider_type altogether, though we still would have to encode the category into the ID for convenience. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:41: virtual void OnSuggestionsChanged( On 2016/07/01 09:37:10, tschumann wrote: > On 2016/06/30 17:14:08, Philipp Keck wrote: > > On 2016/06/30 10:55:19, tschumann wrote: > > > personally, I'd try to avoid the term 'change' in this call as it suggests > > more > > > info about the actual change (which might be added at some point in the > future > > > or not). Given that you simply inform about new data, I'd sympathize more > with > > > the names suggested by Marc before (OnNewSuggestions[Available] or > something). > > > > > > However, this should also be simple enough to change once we add the new > > > functions (if ever), so I'm not feeling strongly here. Feel free to leave as > > is > > > if you prefer the current name. > > > > I also suggested OnContentChanged earlier. > > > > The name "OnNewSuggestions" sounds confusing to me because the method is not > > only called when there are new suggestions and the |suggestions| parameter > does > > not only contain the new suggestions, but it contains all suggestions and when > > old ones are missing, then the result of calling this function is effectively > > deleting suggestions (which is the opposite of "new suggestions"). > > > > What happens is: The whole list of available suggestions has (somehow) been > > updated. It could be that new suggestions were added, an existing one was > > modified, the list was reordered or some suggestions were deleted. And it's > > there four more fine-grained events that I would call: > > OnSuggestionsAdd/OnNewSuggestions, OnSuggestionModify, OnSuggestionMove, > > OnSuggestionsDelete -- so the name OnNewSuggestions would already be taken for > > the more specific event. > > I see your point. My motivation was that the Observer should not interpret the > single elements and not care about partial updates at all. If there's new data > (which might as well be the same that was displayed before), the existing data > should be thrown away and replaced by the new data. The observer should not care > about the deeper meaning of a new suggestion (i.e. whether a suggestion is new > to the user or not) -- it should simply display whatever the service gives. > IMO, the examples of more fine grained methods fit nicely into this model. > > OnSuggestionsChange() suggest that there's an actual change in data and that the > observer might care about that change. But it might as well be the very same > data set. > > Anyways, I feel we're spending too much time on this. Both are probably > acceptable, just wanted to get my point across ;-) Like I said, pick one :-) Ok I understand your point now. It's basically a short form of "OnNewListOfSuggestionsAvailable" and not "OnNewSuggestionsHaveBeenAddedToTheList". I can see how "OnSuggestionsChanged" (short for "OnOverallListOfSuggestionsHasChanged") can be confusing, I changed it. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:68: // Observer). There is no need to call the observer for an update after a On 2016/07/01 09:15:32, Marc Treib wrote: > On 2016/06/30 17:14:07, Philipp Keck wrote: > > On 2016/06/30 10:36:53, Marc Treib wrote: > > > This formulation leaves it open whether the observer will be called or not. > We > > > should make a decision and stick with it. > > > > I didn't want to require that to happen because it would be inefficient. But I > > also didn't want to disallow it because it might be inherent to the way a > > provider is implemented (e.g. they forward the deletion call to some > underlying > > data model that is not under our control and that data model emits an event). > Of > > course the provider could temporarily suppress the event, but that would look > > hacky to me. > > > > Since the UI doesn't update anyway, it's not much of a difference: Unnecessary > > updates will only bubble up to the cache on the service, not further on. So > > maybe we should require an update in any case? > > I'd slightly prefer sending no update, because then the event can be interpreted > as "show a 'new content available' indicator". But I'm fine either way, as long > as we have consistency, so either all providers send and update, or none do. We have to answer this question twice (for both Observers, from Providers and towards UI). For the ContentSuggestionsService::Observer (towards the UI) I agree with you: not sending an update is better. And for consistency, let's just do the same here. I changed it. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:30: } On 2016/07/01 09:15:32, Marc Treib wrote: > On 2016/06/30 17:14:08, Philipp Keck wrote: > > On 2016/06/30 11:58:11, tschumann wrote: > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > I think it'd be nice if the "combine" and "extract" functions were in the > > same > > > > place, next to each other. Maybe static methods on ContentSuggestion? > Though > > > > that kinda invites others to use them... hrm. I don't know. > > > +1 > > > They should live together but not on the ContentSuggestion. The > > > ContentSuggestion should not know anything about the ID encoding/decoding. > > This > > > file is IMO the right place to implement both. > > > > I had the same dilemma. ContentSuggestion is definetely not the place because > > that's where we're trying to ban the provider_type from. So if we offer a > > GetProviderType() there, that completely defeats the purpose. Though we could > > make it private (only the GetProviderType()) and have > ContentSuggestionsService > > as a friend class? > > Naaah. > > > The combining conversion pretty much needs to happen in the ContentSuggestion > > constructor, otherwise it gets really complicated for a provider to create a > new > > ContentSuggestion. Could move it into ContentSuggestionsService as public > static > > members, but make the GetProviderType() a private member? > > What GetProviderType? ContentSuggestion shouldn't have that at all. > > We could create a "content_suggestion_utils.h" or something like that, and make > it private to the component (gn has a way of specifying which headers are > considered "external" and may be used by clients, though I don't remember the > details). Ok. So should I do that? Btw this is also one of that problems that would be resolved if a category could be provided by at most one provider. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:43: } else { On 2016/07/01 09:15:32, Marc Treib wrote: > On 2016/06/30 17:14:08, Philipp Keck wrote: > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > No "else" after "return" > > > > I changed it. Though I personally like the shape of code like: > > if (cond) return A; > > else return B; > > > > In other projects I tend to use > > if (cond) return; > > other_statements; > > when the condition is rather exceptional and the return is more like "abort" > and > > I use the above form when the two cases are equally fine. > > I agree, in the second example, the benefit is clearer. But the code style is > unambiguous: https://www.chromium.org/developers/coding-style Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:58: observers_.Clear(); On 2016/07/01 09:15:32, Marc Treib wrote: > On 2016/06/30 17:14:08, Philipp Keck wrote: > > On 2016/06/30 11:58:11, tschumann wrote: > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > Hm. Since the observers register themselves, they should probably also > > > > unregister themselves. So DCHECK(observers_.empty()) ? > > > > > > +1 > > > > Marc's precondition is not perfectly true: Observers don't register > themselves, > > but they get registered by their factory. They don't have a reference to the > > service, so they couldn't call an Unregister(Provider) that is placed next to > > the Register(Provider) in the service. Instead, there would need to be a > > Unregister(ProviderType) on the Observer interface, which is somewhat similar > to > > the OnProviderShutdown method there (except that the provider might not want > to > > shut down, so it would be a separate method?). > > > > It would make the shutdown more inefficient in that the service would need to > > remove and filter out all suggestions of every provider that's shutting down > > instead of just doing categories_.clear() and > suggestions_by_category_.clear(), > > which is missing here btw. > > So maybe the service would need a state "shutting down"? > > But the observers need a reference to the service anyway, to get the actual > data, right? > > Anyway, the Register and Unregister should be handled from the same place. So > the factory is probably not the right place for that. The Unregistering would need to be triggered from the provider itself, once it notices that it must shut down. Is that right? Assuming that's true, the Registering would need to happen there, too (so both are in one place), and that would require us to pass a reference to the service into the providers, but we specifically don't want it there because we have the Observer (former Delegate) interface to reduce the interface of the service that the providers can use. So the provider factory would need to pass a ContentSuggestionsProvider::Observer pointer to the service into the provider and the Observer would get a Register method? To make this consistent with the Observer idea, Register and Unregister would need to be called OnStart and OnShutdown or sth. like that. This would fit in with the proposed requirement that there is at most one provider per category. The ContentSuggestionsProvider::Observer interface could be: +OnStartCategory(category): bool <-- "I want to provide data for this category. Return value says if that's okay or if there is already another provider for that" +OnStopCategory(category) <-- "I won't provide data for this category anymore." equivalent to current provider shutdown, but more finegrained. +OnSuggestionsChanged(category, data) <-- "For this category, which I have been allowed to provide, I deliver new data" When a provider receives the shutdown signal from the service, it would need to stop all of its categories. The ContentSuggestionsService::Observer::OnSuggestionInvalidated() for the UI could also be refined with a category parameter, because once a provider stops a category, we know that there cannot be any other providers for that category. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:63: enabled_ = false; On 2016/07/01 09:15:32, Marc Treib wrote: > On 2016/06/30 17:14:08, Philipp Keck wrote: > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > Also clear the cached suggestions, just to be sure? > > > > Yes, see previous comment. > > Hmm? Depending on how we change the shutdown, I would add "suggestions_by_category_.clear()" as you suggested, or even "DCHECK(suggestions_by_category_.empty())", if the require the providers to shut down cleanly. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:69: return suggestions_by_category_.at(category); On 2016/07/01 09:15:32, Marc Treib wrote: > On 2016/06/30 17:14:08, Philipp Keck wrote: > > On 2016/06/30 10:55:19, tschumann wrote: > > > hmmm... is there anything guaranteeing that we have indeed an entry for > every > > > category enum? this might be hard to enforce, so maybe handle this > gracefully? > > > > In the obvious, simple Android implementation, this would only be called > > directly after calling GetCategories(), and only be called for every entry in > > categories_. > > > > An empty list would be different from no list or an error. This also depends > on > > the way the status cards work (are the suggestions themselves or not). > > "no list" isn't an option if you return a reference. > Anyway, crashing when this is called with a currently empty category seems like > a bad API. Changed to empty list. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:113: for (auto& pair : suggestions_by_category_) { On 2016/07/01 09:15:32, Marc Treib wrote: > On 2016/06/30 17:14:08, Philipp Keck wrote: > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > Can't you get the category from the suggestions, rather than going over all > of > > > them? > > > > Then I would need to include the category in the DiscardSuggestion() > signature. > > Or make it part of the ID. > > Ah, right, sorry. Carry on :) It might be part of the ID, if we enforce that the category uniquely identifies the provider :-) So that would even make this code nicer and faster. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:120: pair.second.erase(position); On 2016/07/01 09:15:32, Marc Treib wrote: > Add a break? Once we're removed the suggestion, there's no reason to keep > looking. Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:123: } else { On 2016/07/01 09:15:32, Marc Treib wrote: > nit: Turning the if around might make things a bit easier to read (remove one > level of indentation): > if (no provider) > LOG > return > carrying on... Done. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:127: ContentSuggestionsProviderType>::type>(provider_type); On 2016/07/01 09:55:59, tschumann wrote: > On 2016/07/01 09:15:32, Marc Treib wrote: > > On 2016/06/30 17:14:08, Philipp Keck wrote: > > > On 2016/06/30 11:58:11, tschumann wrote: > > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > > int(provider_type) > > > > > > > > i'd vote for implicit_cast<int> as it's the least powerful cast to do the > > job. > > > > > > Wasn't sure where to import the implicit_cast from, so it's int() for now. > > > > implicit_cast isn't a standard C++ feature, and doesn't exist in Chromium :) > > =) argh... spent too much time in google3 it seems... so static_cast it is :-) It's "int(provider_type)" now? See discussion above (line 22). https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:38: virtual void OnSuggestionsChanged() = 0; On 2016/07/01 09:15:32, Marc Treib wrote: > On 2016/06/30 17:14:08, Philipp Keck wrote: > > On 2016/06/30 11:58:11, tschumann wrote: > > > On 2016/06/30 11:34:23, Marc Treib wrote: > > > > On 2016/06/30 10:55:19, tschumann wrote: > > > > > The service interface is taking on a lot of roles. > > > > > Would clients ever access the data without being notified through the > > > > observer? > > > > > If not, I wonder if it wouldn't be cleaner to factor out an data access > > > > > interface and hand this into the Observer methods to properly decouple > the > > > two > > > > > concerns the service is doing (data access + registry). > > > > > > > > Hm. I'm not convinced that's worth the extra complexity at this point (but > > not > > > > really opposed either). > > > > > > > > > Also it seems a bit weird that the observer needs to get access to the > > > service > > > > > from somewhere else when it only wants to react on observed events. > > > > > > > > That's fairly common: The service is more or less a singleton > (per-profile), > > > you > > > > access it through the factory. And you need the service anyway to register > > > > yourself as an observer. > > > > > > While that's true, I think it would be nice if the implementation detail > that > > > this is a singleton wouldn't ripple through the layers. It's kind of a dirty > > > if > > > you get access to Singletons from anywhere. The context is much nicer to > grasp > > > if a function gets the state from the framework it needs to work on (the > > > framework <-> API tradeoff. I personally try to minimize framework logic and > > > move it into easy to use, clean APIs). > > > Especially functionality like "service shutdown" impose a certain state on > the > > > service -- users of the framework can't use the service after shutdown > anymore > > > (I guess). That's another reason why IMO accessing the service through the > > > singleton should be minimized. If my observer is unregistered, and only gets > > > access to the service through the observer functions, it won't get called > > > anymore and there's no risk of accessing the service in a non-standard > state. > > > > The observer here is the UI, really. The UI needs access to the service even > > outside of calls that came through the observer interface (for the > > FetchSuggestionImage and DiscardSuggestion methods). FetchSuggestionImage to > me > > is also part of the data access API, but even if it's not: As soon as we build > a > > "Refresh this section" button or infinite scroll or anything like that, the UI > > will need direct access to the data model, so it will need to store a pointer > to > > the service. > > > > We could introduce a smaller interface for all the service methods that the UI > > needs (not Shutdown, not RegisterProvider, not the debugging stuff) and have > the > > UI use the service through that. Currently I don't see a big advantage, but > > rather the need to change method signatures in yet another place whenever > > something changes (like FetchSnippetImage -> FetchImage -> > FetchSuggestionImage > > -> remove provider_type argument because it's now in the ID, ...). I think it > > slows down code changes more than it currently helps. It should be easy to add > > such an interface later on. > > > > By the UI, the new factory will only be used once, namely in the snippets > bridge > > constructor. > > +1. I see the point of Tim's suggestion, but I think at this point the extra > indirection would create more confusion than clarity. We might still decide to > add it later, but IMO we're not at that point yet. Acknowledged. https://codereview.chromium.org/2102023002/diff/120001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/120001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:20: using Enabled = ntp_snippets::ContentSuggestionsService::Enabled; On 2016/07/01 09:37:10, tschumann wrote: > move into the BuildServiceInstanceFor() function Done. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.cc (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestion.cc:5: #include "base/strings/stringprintf.h" On 2016/07/01 09:15:33, Marc Treib wrote: > This should go below the include for the content_suggestion.h header (with an > empty line between) Done. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:31: const ContentSuggestionsProviderType provider_type, On 2016/07/01 09:37:10, tschumann wrote: > IMO the suggestion constructor should only get a single ID. The service > shouldn't leak any kind of provider information. I can't think of any good solution in which this constructor receives a single ID. That would require the "merging" of the two to happen before the constructor is called. The constructor is called by the provider. But we don't want to leak the merging logic into all providers, so we would need a helper function somewhere that does "my_provider_type+id=>combined_id", but that would leak just as this constructor does. The only thing that's leaking with this solution is an "incomprehensible" constructor towards the UI, but the UI won't use any constructors of this class anyway because it only consumes the instances. Again, all of this would be resolved if we enforced a category->provider_type mapping ;-) And that mapping wouldn't even need to be static, there just couldn't be any mixing of suggestions into one category from different providers at runtime. And that makes sense because "mixing" could only be done if there was a sensible sorting criterion (like score), which ContentSuggestion of course doesn't and shouldn't have. So if we mixed with the current code, what would happen is: "aaaaabbbb" (all from provider A, then all from provider B). Then provider A triggers an update and we have "bbbbaaaaa" (even if the a's and the b's still are exactly the same) and so on. Mixing into one category from different sources is not unrealistic. It could especially happen with "articles for you", where many different sources could suggest interesting articles, but they're all the same to the user. Like RSS readers mix your RSS feeds. Such mixing would still be possible if we require a category->provider_type mapping, but it would need to happen in or before the provider and not in the service. And due to the general nature of ContentSuggestion, that doesn't seem like a bad thing (imagine an ArticlesMixerProvider that knows what articles are and what scores are, etc.). Articles are also the only use case that I can think of where a "merging" of the "same content URL" from different sources, like the NTPSnippetsService does it today, makes any sense; so that logic could also live in an ArticlesMixerProvider. As long as such article mixing is required, I don't see any other use cases or reasons why multiple providers should be able to feed a single category. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:47: // suggestions anymore. This unregisters the provider, removes the observer On 2016/07/01 09:37:11, tschumann wrote: > "this unregisters the provider" > I'm a bit confused what 'this' means. If it's meaning the OnProviderShutdown > function, then it's something that implementations need to take care of -- that > would be good to explicitly state (implementations need to makes sure to...). > If it's meaning is the shut down process, then please state if the provider gets > unregistered before or after this function (although I'm not sure where and how > that would be done). > Same for the rest of the comment. Implementations need to take care of that -- but the only implementation of this interface is the ContentSuggestionsService itself. Since it's part of the Provider interface, the doc talks to the provider and explains what a provider can expect/achieve when calling this method. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:54: // Accessor for observer, which is notified about new available suggestions. On 2016/07/01 09:37:11, tschumann wrote: > nit: Now it's just a setter. So maybe rephrase to "Sets an observer which is > notified ..". Done. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:83: ContentSuggestionsProviderType GetProviderType() const { On 2016/07/01 09:15:33, Marc Treib wrote: > Style guide says to name trivial getters in variable style, i.e. provider_type() > (yeah, I don't like it either) But we changed "categories()" to "GetCategories()" and all it does it "return categories_"? Btw this getter and the entire ProviderType enum would also be obsolete if there was a category->provider_type mapping ;-) I changed it to provider_type and removed the comment (that nobody nit'ed about?) @tschumann: The getter is used in quite a few places; it might also be used by provider implementations so that they don't have to hard-code their type throughout their code (though protected would be enough for that). https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:18: // Helper method get the provider_type from a combined ID. On 2016/07/01 09:37:11, tschumann wrote: > nit: let's keep the comment focused on the essential part. Drop the helper and > write the comment in a descriptive way (i.e. 'gets' instead of 'get'), like: > // Extracts the provider type from a ContentSuggestion ID. > > similar for GetIDFromCombinedID() and other functions. Done. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:63: for (auto& provider : providers_) { On 2016/07/01 09:37:11, tschumann wrote: > Too bad we don't have value_view in Chrome > (https://cs.corp.google.com/piper///depot/google3/util/gtl/iterator_adaptors.h...) > :-/ Acknowledged. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:29: // (see ntp_snippet_service.h). On 2016/07/01 09:15:33, Marc Treib wrote: > nit: NTPSnippetsService and ntp_snippets_service.h (plural) Done. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:43: // the getters and other methods should not be used anymore. On 2016/07/01 09:51:52, Marc Treib wrote: > On 2016/07/01 09:37:11, tschumann wrote: > > getters of the service? > > Is it safe to say, that the service shouldn't be used anymore? > > Would be good to get those semantics as simple as possible. > > How do we actually make sure all parts in the system know that the service > > singleton must not be touched anymore? > > > > Further thought: Is there a chance we can get rid of the shutdown semantics at > > all? [happy to discuss this offline] > > Probably not; these are a fixed part of the KeyedService system. > That said, if the inter-service dependencies are properly set (i.e. all > providers depend on the service), then all providers will be shut down before > the service itself is shut down, so there's no reason to have the > ContentSuggestionsServiceShutdown method. However, that requires all providers > to be KeyedServices... We probably should not require providers to be KeyedServices because that would in many cases require an additional KeyedService even though there is another one right behind that (e.g. for offline pages). But we can require that every provider is owned by a KeyedService (either itself or the one behind) and that one will depend on the ContentSuggestionsService. So if the source (like the offline pages model) doesn't want to depend on the content suggestions, we need to set up a third KeyedService, otherwise we're fine with two. I would remove this event completely. There is no harm in calling the service after it has shut down. It will probably at some point be replaced with an OnStateChanged() event. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:53: Enable = true, On 2016/07/01 09:51:52, Marc Treib wrote: > On 2016/07/01 09:37:11, tschumann wrote: > > shouldn't these follow the constant naming convention, i.e. kEnabled? > > (https://google.github.io/styleguide/cppguide.html#Enumerator_Names) > > In Chrome, MACRO_STYLE is officially preferred > (https://www.chromium.org/developers/coding-style), but both styles are used in > practice. Done. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:74: // (see ntp_snippet.h).pty image. On 2016/07/01 09:15:33, Marc Treib wrote: > Copy&paste fail? Done. I need to turn off middle-click paste..
https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/1/components/ntp_snippets/con... components/ntp_snippets/content_suggestions_service.cc:14: if (enabled_) { On 2016/07/01 13:00:03, Philipp Keck wrote: > On 2016/07/01 09:15:32, Marc Treib wrote: > > On 2016/06/30 17:14:07, Philipp Keck wrote: > > > On 2016/06/30 10:36:53, Marc Treib wrote: > > > > On 2016/06/30 09:35:35, Philipp Keck wrote: > > > > > On 2016/06/28 15:04:55, Marc Treib wrote: > > > > > > On 2016/06/28 14:18:56, Philipp Keck wrote: > > > > > > > On 2016/06/28 11:29:09, Marc Treib wrote: > > > > > > > > Remove empty if. > > > > > > > > It looks like the enabled_ flag currently doesn't do anything at > > all? > > > It > > > > > > > > probably should... > > > > > > > > > > > > > > The reason that there is no code is not that the enabled flag > doesn't > > do > > > > > > > anything. The reason is that the constructor currently doesn't need > to > > > do > > > > > > > anything because there is simply no setup to be done. > > > > > > > > > > > > Alright, but then remove the empty if. > > > > > > > > > > > > > When the service is disabled, none of the providers' factories will > > > > register > > > > > a > > > > > > > provider, so the service will remain empty. We could additionally > > > prevent > > > > > > calls > > > > > > > to functions like RegisterProvider, GetSuggestionsForCategory and > > > possibly > > > > > all > > > > > > > others or make them return empty results. > > > > > > > > > > > > Hm. If the "enabled" state can change at runtime (e.g. because we > expose > > a > > > > > > setting), then it might make sense to register the providers anyway, > in > > > case > > > > > the > > > > > > service later gets enabled. WDYT? > > > > > > > > > > I removed the empty if for now. The question how to properly implement > > that > > > > > depends on how status cards are handled, i.e., where they are produced > > and, > > > if > > > > > that's in the backend, how they are passed to the UI. Are they special > > > > > suggestions (to turn some settings on), are they a separate > > > Delegate/Callback > > > > > method, or is it just a Service::is_enabled() that returns false and the > > UI > > > > > needs to realise that? > > > > > > > > > > I agree: All (available) providers should always be registered with the > > > > service. > > > > > If disabled, the service should clear its data stores and somehow tell > the > > > UI. > > > > > Additionally, the service could make sure not to return any suggestions > > > > (except > > > > > maybe the appropriate status cards). > > > > > If disabled, a provider should stop fetching data and not deliver data > to > > > the > > > > > service anymore and tell the service that it's disabled (and the service > > > > removes > > > > > the corresponding data). How does a provider know that the main > > > switch/feature > > > > > is turned off? Maybe through a ContentSuggestionsService::is_enabled() > > > getter > > > > > and a Delete::ServiceDisabled() notification? Then we'd also need > > > > > Delegate::ServiceEnabled() to re-enable. > > > > > > > > Hm, right, we need "StateChanged" notifications both ways (IMO this should > > be > > > > one method with a parameter, rather than separate Enabled/Disabled ones). > So > > > > yes, I guess we need a method both on the ServiceObserver and the > > > > ProviderObserver/Delegate. > > > > > > Yes, but I'm not implementing that (now), right? > > > > Yeah, another CL is fine, but we'll need at least some of it before we can > > switch to the new service (to clear out any suggestions on sign-out). > > Yes. I'm thinking about extending the new Enabled enum to make it a status enum > similar to the new state handling in NTPSnippetsService. This would be a lot > easier if we enforced a mapping from categories to providers (i.e. a category > can be provided by at most one provider; if more providers want to, the second > one will be ignored). Then we could have (slightly) category-specific code even > in the UI; or generic code and just a list of possible categories in the UI. And > the UI could ask "what's the status of category X?", whereupon the service could > say "there's not even a provider for that" or it could ask the provider for the > status of that category and forward the answer to the UI. > Without such a mapping, it is unclear why a category is currently empty: Are the > responsible provider(s) still loading? Or have they loaded an empty set? Have > they encountered errors? Are they not registered? > > Another advantage of such a mapping is that we could get rid of the > provider_type altogether, though we still would have to encode the category into > the ID for convenience. The category -> provider mapping sounds like a good idea to me. In particular, it'll make our lives easier with respect to the empty state and "more" cards, since those can then only come from one provider. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:30: } On 2016/07/01 13:00:03, Philipp Keck wrote: > On 2016/07/01 09:15:32, Marc Treib wrote: > > On 2016/06/30 17:14:08, Philipp Keck wrote: > > > On 2016/06/30 11:58:11, tschumann wrote: > > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > > I think it'd be nice if the "combine" and "extract" functions were in > the > > > same > > > > > place, next to each other. Maybe static methods on ContentSuggestion? > > Though > > > > > that kinda invites others to use them... hrm. I don't know. > > > > +1 > > > > They should live together but not on the ContentSuggestion. The > > > > ContentSuggestion should not know anything about the ID encoding/decoding. > > > This > > > > file is IMO the right place to implement both. > > > > > > I had the same dilemma. ContentSuggestion is definetely not the place > because > > > that's where we're trying to ban the provider_type from. So if we offer a > > > GetProviderType() there, that completely defeats the purpose. Though we > could > > > make it private (only the GetProviderType()) and have > > ContentSuggestionsService > > > as a friend class? > > > > Naaah. > > > > > The combining conversion pretty much needs to happen in the > ContentSuggestion > > > constructor, otherwise it gets really complicated for a provider to create a > > new > > > ContentSuggestion. Could move it into ContentSuggestionsService as public > > static > > > members, but make the GetProviderType() a private member? > > > > What GetProviderType? ContentSuggestion shouldn't have that at all. > > > > We could create a "content_suggestion_utils.h" or something like that, and > make > > it private to the component (gn has a way of specifying which headers are > > considered "external" and may be used by clients, though I don't remember the > > details). > > Ok. So should I do that? Btw this is also one of that problems that would be > resolved if a category could be provided by at most one provider. How exactly would that solve it? We'll still have to put the "combine" and "extract" logic somewhere..? https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:58: observers_.Clear(); On 2016/07/01 13:00:04, Philipp Keck wrote: > On 2016/07/01 09:15:32, Marc Treib wrote: > > On 2016/06/30 17:14:08, Philipp Keck wrote: > > > On 2016/06/30 11:58:11, tschumann wrote: > > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > > Hm. Since the observers register themselves, they should probably also > > > > > unregister themselves. So DCHECK(observers_.empty()) ? > > > > > > > > +1 > > > > > > Marc's precondition is not perfectly true: Observers don't register > > themselves, > > > but they get registered by their factory. They don't have a reference to the > > > service, so they couldn't call an Unregister(Provider) that is placed next > to > > > the Register(Provider) in the service. Instead, there would need to be a > > > Unregister(ProviderType) on the Observer interface, which is somewhat > similar > > to > > > the OnProviderShutdown method there (except that the provider might not want > > to > > > shut down, so it would be a separate method?). > > > > > > It would make the shutdown more inefficient in that the service would need > to > > > remove and filter out all suggestions of every provider that's shutting down > > > instead of just doing categories_.clear() and > > suggestions_by_category_.clear(), > > > which is missing here btw. > > > So maybe the service would need a state "shutting down"? > > > > But the observers need a reference to the service anyway, to get the actual > > data, right? > > > > Anyway, the Register and Unregister should be handled from the same place. So > > the factory is probably not the right place for that. > > The Unregistering would need to be triggered from the provider itself, once it > notices that it must shut down. Is that right? > Assuming that's true, the Registering would need to happen there, too (so both > are in one place), and that would require us to pass a reference to the service > into the providers, but we specifically don't want it there because we have the > Observer (former Delegate) interface to reduce the interface of the service that > the providers can use. > So the provider factory would need to pass a > ContentSuggestionsProvider::Observer pointer to the service into the provider > and the Observer would get a Register method? To make this consistent with the > Observer idea, Register and Unregister would need to be called OnStart and > OnShutdown or sth. like that. > > This would fit in with the proposed requirement that there is at most one > provider per category. The ContentSuggestionsProvider::Observer interface could > be: > +OnStartCategory(category): bool <-- "I want to provide data for this category. > Return value says if that's okay or if there is already another provider for > that" > +OnStopCategory(category) <-- "I won't provide data for this category anymore." > equivalent to current provider shutdown, but more finegrained. Hm, I think the categories should be registered "as statically as possible", since we never expect them to change at runtime, and so we also never expect conflicts. > +OnSuggestionsChanged(category, data) <-- "For this category, which I have been > allowed to provide, I deliver new data" > > When a provider receives the shutdown signal from the service, it would need to > stop all of its categories. If the providers are all KeyedServices, they would just unregister on their own shutdown. When the service itself is shut down, it would just DHCECK that there are no registered providers anymore. > The ContentSuggestionsService::Observer::OnSuggestionInvalidated() for the UI > could also be refined with a category parameter, because once a provider stops a > category, we know that there cannot be any other providers for that category. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:113: for (auto& pair : suggestions_by_category_) { On 2016/07/01 13:00:04, Philipp Keck wrote: > On 2016/07/01 09:15:32, Marc Treib wrote: > > On 2016/06/30 17:14:08, Philipp Keck wrote: > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > Can't you get the category from the suggestions, rather than going over > all > > of > > > > them? > > > > > > Then I would need to include the category in the DiscardSuggestion() > > signature. > > > Or make it part of the ID. > > > > Ah, right, sorry. Carry on :) > > It might be part of the ID, if we enforce that the category uniquely identifies > the provider :-) So that would even make this code nicer and faster. Yup, SGTM. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:31: const ContentSuggestionsProviderType provider_type, On 2016/07/01 13:00:04, Philipp Keck wrote: > On 2016/07/01 09:37:10, tschumann wrote: > > IMO the suggestion constructor should only get a single ID. The service > > shouldn't leak any kind of provider information. > > I can't think of any good solution in which this constructor receives a single > ID. That would require the "merging" of the two to happen before the constructor > is called. The constructor is called by the provider. But we don't want to leak > the merging logic into all providers, so we would need a helper function > somewhere that does "my_provider_type+id=>combined_id", but that would leak just > as this constructor does. > > The only thing that's leaking with this solution is an "incomprehensible" > constructor towards the UI, but the UI won't use any constructors of this class > anyway because it only consumes the instances. > > Again, all of this would be resolved if we enforced a category->provider_type > mapping ;-) And that mapping wouldn't even need to be static, there just > couldn't be any mixing of suggestions into one category from different providers > at runtime. And that makes sense because "mixing" could only be done if there > was a sensible sorting criterion (like score), which ContentSuggestion of course > doesn't and shouldn't have. So if we mixed with the current code, what would > happen is: "aaaaabbbb" (all from provider A, then all from provider B). Then > provider A triggers an update and we have "bbbbaaaaa" (even if the a's and the > b's still are exactly the same) and so on. > > Mixing into one category from different sources is not unrealistic. It could > especially happen with "articles for you", where many different sources could > suggest interesting articles, but they're all the same to the user. Like RSS > readers mix your RSS feeds. > Such mixing would still be possible if we require a category->provider_type > mapping, but it would need to happen in or before the provider and not in the > service. And due to the general nature of ContentSuggestion, that doesn't seem > like a bad thing (imagine an ArticlesMixerProvider that knows what articles are > and what scores are, etc.). Articles are also the only use case that I can think > of where a "merging" of the "same content URL" from different sources, like the > NTPSnippetsService does it today, makes any sense; so that logic could also live > in an ArticlesMixerProvider. > > As long as such article mixing is required, I don't see any other use cases or > reasons why multiple providers should be able to feed a single category. For "Articles for you", the mixing will happen on the server. For all other categories, we don't have any plans for mixing. So, once more: +1 for category->provider mapping :)
https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:30: } On 2016/07/01 13:29:17, Marc Treib wrote: > On 2016/07/01 13:00:03, Philipp Keck wrote: > > On 2016/07/01 09:15:32, Marc Treib wrote: > > > On 2016/06/30 17:14:08, Philipp Keck wrote: > > > > On 2016/06/30 11:58:11, tschumann wrote: > > > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > > > I think it'd be nice if the "combine" and "extract" functions were in > > the > > > > same > > > > > > place, next to each other. Maybe static methods on ContentSuggestion? > > > Though > > > > > > that kinda invites others to use them... hrm. I don't know. > > > > > +1 > > > > > They should live together but not on the ContentSuggestion. The > > > > > ContentSuggestion should not know anything about the ID > encoding/decoding. > > > > This > > > > > file is IMO the right place to implement both. > > > > > > > > I had the same dilemma. ContentSuggestion is definetely not the place > > because > > > > that's where we're trying to ban the provider_type from. So if we offer a > > > > GetProviderType() there, that completely defeats the purpose. Though we > > could > > > > make it private (only the GetProviderType()) and have > > > ContentSuggestionsService > > > > as a friend class? > > > > > > Naaah. > > > > > > > The combining conversion pretty much needs to happen in the > > ContentSuggestion > > > > constructor, otherwise it gets really complicated for a provider to create > a > > > new > > > > ContentSuggestion. Could move it into ContentSuggestionsService as public > > > static > > > > members, but make the GetProviderType() a private member? > > > > > > What GetProviderType? ContentSuggestion shouldn't have that at all. > > > > > > We could create a "content_suggestion_utils.h" or something like that, and > > make > > > it private to the component (gn has a way of specifying which headers are > > > considered "external" and may be used by clients, though I don't remember > the > > > details). > > > > Ok. So should I do that? Btw this is also one of that problems that would be > > resolved if a category could be provided by at most one provider. > > How exactly would that solve it? We'll still have to put the "combine" and > "extract" logic somewhere..? Yes, but it could be public logic because categories are not an implementation detail. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:58: observers_.Clear(); On 2016/07/01 13:29:17, Marc Treib wrote: > On 2016/07/01 13:00:04, Philipp Keck wrote: > > On 2016/07/01 09:15:32, Marc Treib wrote: > > > On 2016/06/30 17:14:08, Philipp Keck wrote: > > > > On 2016/06/30 11:58:11, tschumann wrote: > > > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > > > Hm. Since the observers register themselves, they should probably also > > > > > > unregister themselves. So DCHECK(observers_.empty()) ? > > > > > > > > > > +1 > > > > > > > > Marc's precondition is not perfectly true: Observers don't register > > > themselves, > > > > but they get registered by their factory. They don't have a reference to > the > > > > service, so they couldn't call an Unregister(Provider) that is placed next > > to > > > > the Register(Provider) in the service. Instead, there would need to be a > > > > Unregister(ProviderType) on the Observer interface, which is somewhat > > similar > > > to > > > > the OnProviderShutdown method there (except that the provider might not > want > > > to > > > > shut down, so it would be a separate method?). > > > > > > > > It would make the shutdown more inefficient in that the service would need > > to > > > > remove and filter out all suggestions of every provider that's shutting > down > > > > instead of just doing categories_.clear() and > > > suggestions_by_category_.clear(), > > > > which is missing here btw. > > > > So maybe the service would need a state "shutting down"? > > > > > > But the observers need a reference to the service anyway, to get the actual > > > data, right? > > > > > > Anyway, the Register and Unregister should be handled from the same place. > So > > > the factory is probably not the right place for that. > > > > The Unregistering would need to be triggered from the provider itself, once it > > notices that it must shut down. Is that right? > > Assuming that's true, the Registering would need to happen there, too (so both > > are in one place), and that would require us to pass a reference to the > service > > into the providers, but we specifically don't want it there because we have > the > > Observer (former Delegate) interface to reduce the interface of the service > that > > the providers can use. > > So the provider factory would need to pass a > > ContentSuggestionsProvider::Observer pointer to the service into the provider > > and the Observer would get a Register method? To make this consistent with the > > Observer idea, Register and Unregister would need to be called OnStart and > > OnShutdown or sth. like that. > > > > This would fit in with the proposed requirement that there is at most one > > provider per category. The ContentSuggestionsProvider::Observer interface > could > > be: > > +OnStartCategory(category): bool <-- "I want to provide data for this > category. > > Return value says if that's okay or if there is already another provider for > > that" > > +OnStopCategory(category) <-- "I won't provide data for this category > anymore." > > equivalent to current provider shutdown, but more finegrained. > > Hm, I think the categories should be registered "as statically as possible", > since we never expect them to change at runtime, and so we also never expect > conflicts. > > > +OnSuggestionsChanged(category, data) <-- "For this category, which I have > been > > allowed to provide, I deliver new data" > > > > When a provider receives the shutdown signal from the service, it would need > to > > stop all of its categories. > > If the providers are all KeyedServices, they would just unregister on their own > shutdown. When the service itself is shut down, it would just DHCECK that there > are no registered providers anymore. > > > The ContentSuggestionsService::Observer::OnSuggestionInvalidated() for the UI > > could also be refined with a category parameter, because once a provider stops > a > > category, we know that there cannot be any other providers for that category. > I agree about the static categories, when conflicts are detected, something went wrong. Though I'm not sure if we can already detect such conflicts at compile time (any suggestions?) or if we simply have to make sure that we don't make any mistakes and we DCHECK-fail if there is a conflict. A provider's provided categories are static and a provider needs to tell about its provided categories even before OnStartCategory is called (because the service will want to ask why the category is missing if it hasn't been started), probably through a constructor parameter? But it also needs to inform about status changes (data is available for the first time, data has changed, data cannot be available because something is disabled). For the latter I'm not sure if this should be per provider or per provider-category. In the latter case, we'd still need OnStartCategory() or something like that. For the shutdown I agree, even if the providers are not KeyedServices themselves, but just owned by one, it would work like that and when the ContentSuggestionsService shuts down, it just DCHECKs. https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:31: const ContentSuggestionsProviderType provider_type, On 2016/07/01 13:29:17, Marc Treib wrote: > On 2016/07/01 13:00:04, Philipp Keck wrote: > > On 2016/07/01 09:37:10, tschumann wrote: > > > IMO the suggestion constructor should only get a single ID. The service > > > shouldn't leak any kind of provider information. > > > > I can't think of any good solution in which this constructor receives a single > > ID. That would require the "merging" of the two to happen before the > constructor > > is called. The constructor is called by the provider. But we don't want to > leak > > the merging logic into all providers, so we would need a helper function > > somewhere that does "my_provider_type+id=>combined_id", but that would leak > just > > as this constructor does. > > > > The only thing that's leaking with this solution is an "incomprehensible" > > constructor towards the UI, but the UI won't use any constructors of this > class > > anyway because it only consumes the instances. > > > > Again, all of this would be resolved if we enforced a category->provider_type > > mapping ;-) And that mapping wouldn't even need to be static, there just > > couldn't be any mixing of suggestions into one category from different > providers > > at runtime. And that makes sense because "mixing" could only be done if there > > was a sensible sorting criterion (like score), which ContentSuggestion of > course > > doesn't and shouldn't have. So if we mixed with the current code, what would > > happen is: "aaaaabbbb" (all from provider A, then all from provider B). Then > > provider A triggers an update and we have "bbbbaaaaa" (even if the a's and the > > b's still are exactly the same) and so on. > > > > Mixing into one category from different sources is not unrealistic. It could > > especially happen with "articles for you", where many different sources could > > suggest interesting articles, but they're all the same to the user. Like RSS > > readers mix your RSS feeds. > > Such mixing would still be possible if we require a category->provider_type > > mapping, but it would need to happen in or before the provider and not in the > > service. And due to the general nature of ContentSuggestion, that doesn't seem > > like a bad thing (imagine an ArticlesMixerProvider that knows what articles > are > > and what scores are, etc.). Articles are also the only use case that I can > think > > of where a "merging" of the "same content URL" from different sources, like > the > > NTPSnippetsService does it today, makes any sense; so that logic could also > live > > in an ArticlesMixerProvider. > > > > As long as such article mixing is required, I don't see any other use cases or > > reasons why multiple providers should be able to feed a single category. > > For "Articles for you", the mixing will happen on the server. For all other > categories, we don't have any plans for mixing. So, once more: +1 for > category->provider mapping :) Acknowledged.
https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:58: observers_.Clear(); On 2016/07/01 13:45:11, Philipp Keck wrote: > On 2016/07/01 13:29:17, Marc Treib wrote: > > On 2016/07/01 13:00:04, Philipp Keck wrote: > > > On 2016/07/01 09:15:32, Marc Treib wrote: > > > > On 2016/06/30 17:14:08, Philipp Keck wrote: > > > > > On 2016/06/30 11:58:11, tschumann wrote: > > > > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > > > > Hm. Since the observers register themselves, they should probably > also > > > > > > > unregister themselves. So DCHECK(observers_.empty()) ? > > > > > > > > > > > > +1 > > > > > > > > > > Marc's precondition is not perfectly true: Observers don't register > > > > themselves, > > > > > but they get registered by their factory. They don't have a reference to > > the > > > > > service, so they couldn't call an Unregister(Provider) that is placed > next > > > to > > > > > the Register(Provider) in the service. Instead, there would need to be a > > > > > Unregister(ProviderType) on the Observer interface, which is somewhat > > > similar > > > > to > > > > > the OnProviderShutdown method there (except that the provider might not > > want > > > > to > > > > > shut down, so it would be a separate method?). > > > > > > > > > > It would make the shutdown more inefficient in that the service would > need > > > to > > > > > remove and filter out all suggestions of every provider that's shutting > > down > > > > > instead of just doing categories_.clear() and > > > > suggestions_by_category_.clear(), > > > > > which is missing here btw. > > > > > So maybe the service would need a state "shutting down"? > > > > > > > > But the observers need a reference to the service anyway, to get the > actual > > > > data, right? > > > > > > > > Anyway, the Register and Unregister should be handled from the same place. > > So > > > > the factory is probably not the right place for that. > > > > > > The Unregistering would need to be triggered from the provider itself, once > it > > > notices that it must shut down. Is that right? > > > Assuming that's true, the Registering would need to happen there, too (so > both > > > are in one place), and that would require us to pass a reference to the > > service > > > into the providers, but we specifically don't want it there because we have > > the > > > Observer (former Delegate) interface to reduce the interface of the service > > that > > > the providers can use. > > > So the provider factory would need to pass a > > > ContentSuggestionsProvider::Observer pointer to the service into the > provider > > > and the Observer would get a Register method? To make this consistent with > the > > > Observer idea, Register and Unregister would need to be called OnStart and > > > OnShutdown or sth. like that. > > > > > > This would fit in with the proposed requirement that there is at most one > > > provider per category. The ContentSuggestionsProvider::Observer interface > > could > > > be: > > > +OnStartCategory(category): bool <-- "I want to provide data for this > > category. > > > Return value says if that's okay or if there is already another provider for > > > that" > > > +OnStopCategory(category) <-- "I won't provide data for this category > > anymore." > > > equivalent to current provider shutdown, but more finegrained. > > > > Hm, I think the categories should be registered "as statically as possible", > > since we never expect them to change at runtime, and so we also never expect > > conflicts. > > > > > +OnSuggestionsChanged(category, data) <-- "For this category, which I have > > been > > > allowed to provide, I deliver new data" > > > > > > When a provider receives the shutdown signal from the service, it would need > > to > > > stop all of its categories. > > > > If the providers are all KeyedServices, they would just unregister on their > own > > shutdown. When the service itself is shut down, it would just DHCECK that > there > > are no registered providers anymore. > > > > > The ContentSuggestionsService::Observer::OnSuggestionInvalidated() for the > UI > > > could also be refined with a category parameter, because once a provider > stops > > a > > > category, we know that there cannot be any other providers for that > category. > > > > I agree about the static categories, when conflicts are detected, something went > wrong. Though I'm not sure if we can already detect such conflicts at compile > time (any suggestions?) or if we simply have to make sure that we don't make any > mistakes and we DCHECK-fail if there is a conflict. DCHECK should be fine; I don't actually expect any problems here. > A provider's provided categories are static and a provider needs to tell about > its provided categories even before OnStartCategory is called (because the > service will want to ask why the category is missing if it hasn't been started), > probably through a constructor parameter? I guess when the provider is registered with the service, it'd also list its categories? > But it also needs to inform about status changes (data is available for the > first time, data has changed, data cannot be available because something is > disabled). For the latter I'm not sure if this should be per provider or per > provider-category. In the latter case, we'd still need OnStartCategory() or > something like that. Yeah, that makes sense. Especially the "clear this category" would handle the sign-out case. > For the shutdown I agree, even if the providers are not KeyedServices > themselves, but just owned by one, it would work like that and when the > ContentSuggestionsService shuts down, it just DCHECKs. https://codereview.chromium.org/2102023002/diff/80001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:63: enabled_ = false; On 2016/07/01 13:00:04, Philipp Keck wrote: > On 2016/07/01 09:15:32, Marc Treib wrote: > > On 2016/06/30 17:14:08, Philipp Keck wrote: > > > On 2016/06/30 10:36:54, Marc Treib wrote: > > > > Also clear the cached suggestions, just to be sure? > > > > > > Yes, see previous comment. > > > > Hmm? > > Depending on how we change the shutdown, I would add > "suggestions_by_category_.clear()" as you suggested, or even > "DCHECK(suggestions_by_category_.empty())", if the require the providers to shut > down cleanly. Acknowledged.
Based on the "one provider per category" assumption I was able to remove a lot of code and I think it is now much clearer. Then I also added the ContentSuggestionsState (maybe should call it "status") that providers use to inform the UI about whether a category is available, or why it is not.
https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:102: std::string MakeUniqueID(ContentSuggestionCategory category, static? https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:111: std::vector<ContentSuggestionCategory> provided_categories); const & https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:25: DCHECK(suggestions_by_category_.empty()); Double DCHECK https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:88: LOG(WARNING) << "Requested image for unknown suggestion " << suggestion_id; Too much copypasta :) https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:107: if (position != suggestions->end()) { DCHECK that it was found? It should never happen that it's not found, right? https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:132: ContentSuggestionsState::AVAILABLE) { No numeric comparisons on enums please - make a helper function and use a switch. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:167: if (new_state <= ContentSuggestionsState::AVAILABLE) { Also here: Helper please https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:182: if (iterator != categories_.end()) { Can it ever be not found? https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:61: enum Enabled : bool { This should still be called State or something like that. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:63: DISABLE = false, And these should be ENABLED and DISABLED - they describe a state, not an action. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:102: // Registers a new ContentSuggestionsProvider. This method will fail if there "will fail" sounds like some runtime error, like "return an error code" or something like that. For hard constraints like this, use a formulation with "must", like "no provider must already be registered...". https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:128: bool CategoryExists(ContentSuggestionCategory category) const; CategoryIsRegistered? https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:140: // loading). It might be easier now if it always contained a (possibly empty) vector for each registered category? Then you could get rid of empty_categories_list_ (which is poorly named btw...) https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:152: std::map<std::string, ContentSuggestionCategory> id_category_map_; Hm. So you need this because you don't want to parse the category out of the ID? Not sure I like this any more... too many data structures here that have to be kept in sync. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_state.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_state.h:13: enum class ContentSuggestionsState : int { Should this be called ContentSuggestionsCategoryState, if that's what it is? https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippet.h:130: const std::string& id) const; Mh, this is ugly... so the caller has to get the ID, append the category, and pass that back in here? An NTPSnippet will only ever be ARTICLE, so can't this be done inside ToContentSuggestion?
https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:102: std::string MakeUniqueID(ContentSuggestionCategory category, On 2016/07/05 13:17:12, Marc Treib wrote: > static? Done. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:111: std::vector<ContentSuggestionCategory> provided_categories); On 2016/07/05 13:17:12, Marc Treib wrote: > const & Done. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:25: DCHECK(suggestions_by_category_.empty()); On 2016/07/05 13:17:12, Marc Treib wrote: > Double DCHECK Done. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:88: LOG(WARNING) << "Requested image for unknown suggestion " << suggestion_id; On 2016/07/05 13:17:12, Marc Treib wrote: > Too much copypasta :) Done. And a return statement was missing, too. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:107: if (position != suggestions->end()) { On 2016/07/05 13:17:12, Marc Treib wrote: > DCHECK that it was found? It should never happen that it's not found, right? Done. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:132: ContentSuggestionsState::AVAILABLE) { On 2016/07/05 13:17:12, Marc Treib wrote: > No numeric comparisons on enums please - make a helper function and use a > switch. Yep, I wanted a helper function but wasn't sure where to place it. Initially, it was ContentSuggestionsProvider::IsCategoryAvailable(category), which called ::GetCategoryState(). But then I started needing the same logic in the service and also in other places. It doesn't seem possible to place helper functions in the enum class? In Java, "provider.GetCategoryState(category).isAvailable()" would work... https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:167: if (new_state <= ContentSuggestionsState::AVAILABLE) { On 2016/07/05 13:17:12, Marc Treib wrote: > Also here: Helper please See above. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:182: if (iterator != categories_.end()) { On 2016/07/05 13:17:12, Marc Treib wrote: > Can it ever be not found? It's a precaution for providers that irreponsibly keep their Observer instance around even after they shut down (maybe even tied to some network callbacks or sth like that). With good design, that shouldn't happen, so this check could indeed be removed. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:61: enum Enabled : bool { On 2016/07/05 13:17:12, Marc Treib wrote: > This should still be called State or something like that. Done. Maybe should really rename ContentSuggestionsState to ContentSuggestionsStatus to avoid confusion. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:63: DISABLE = false, On 2016/07/05 13:17:12, Marc Treib wrote: > And these should be ENABLED and DISABLED - they describe a state, not an action. Done. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:102: // Registers a new ContentSuggestionsProvider. This method will fail if there On 2016/07/05 13:17:12, Marc Treib wrote: > "will fail" sounds like some runtime error, like "return an error code" or > something like that. For hard constraints like this, use a formulation with > "must", like "no provider must already be registered...". Done. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:128: bool CategoryExists(ContentSuggestionCategory category) const; On 2016/07/05 13:17:12, Marc Treib wrote: > CategoryIsRegistered? Done. IsCategoryRegistered. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:140: // loading). On 2016/07/05 13:17:12, Marc Treib wrote: > It might be easier now if it always contained a (possibly empty) vector for each > registered category? Then you could get rid of empty_categories_list_ (which is > poorly named btw...) empty_categories_list_ will need to stay because there are the cases when the service is disabled entirely and when a provider simply isn't registered. Also in these cases we wouldn't want the call to this method to fail. (At least we previously discussed that calling this method with a category that is not returned from GetCategories() shouldn't fail.) So what would be a better name for empty_categories_list_? https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:152: std::map<std::string, ContentSuggestionCategory> id_category_map_; On 2016/07/05 13:17:12, Marc Treib wrote: > Hm. So you need this because you don't want to parse the category out of the ID? > Not sure I like this any more... too many data structures here that have to be > kept in sync. Tim's suggestion :-) Tim, wdyt? I was already in doubt when he suggested it this morning. The advantage is that the format of the ID now doesn't matter to anyone anymore and providers can, but do not have to use MakeUniqueID(). One solution would be to add the parsing back in. Another solution would be to add the category-parameter back on the FetchImage and Discard methods accross the Android bridge, which is less problematic since the category is present there anyways. Though if we didn't do that, we could maybe even remove the .category() getter and field from the ContentSuggestion, so that it's category would implicitly be determined by the data structure it lives in. Currently, a provider could pass rubbish data into OnNewSuggestions() because the category information is redundant (and nobody checks for that). https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_state.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_state.h:13: enum class ContentSuggestionsState : int { On 2016/07/05 13:17:12, Marc Treib wrote: > Should this be called ContentSuggestionsCategoryState, if that's what it is? ContentSuggestionsCategoryStatus? CategoryStatus? https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/ntp_snippet.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/ntp_snippet.h:130: const std::string& id) const; On 2016/07/05 13:17:12, Marc Treib wrote: > Mh, this is ugly... so the caller has to get the ID, append the category, and > pass that back in here? > An NTPSnippet will only ever be ARTICLE, so can't this be done inside > ToContentSuggestion? Done. Now that MakeUniqueID is static, that call can move in here.
https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:152: std::map<std::string, ContentSuggestionCategory> id_category_map_; On 2016/07/05 13:52:00, Philipp Keck wrote: > On 2016/07/05 13:17:12, Marc Treib wrote: > > Hm. So you need this because you don't want to parse the category out of the > ID? > > Not sure I like this any more... too many data structures here that have to be > > kept in sync. > > Tim's suggestion :-) Tim, wdyt? > > I was already in doubt when he suggested it this morning. The advantage is that > the format of the ID now doesn't matter to anyone anymore and providers can, but > do not have to use MakeUniqueID(). > > One solution would be to add the parsing back in. > Another solution would be to add the category-parameter back on the FetchImage > and Discard methods accross the Android bridge, which is less problematic since > the category is present there anyways. Though if we didn't do that, we could > maybe even remove the .category() getter and field from the ContentSuggestion, > so that it's category would implicitly be determined by the data structure it > lives in. Currently, a provider could pass rubbish data into OnNewSuggestions() > because the category information is redundant (and nobody checks for that). Tim still prefers more data structures over parsing and notes that we can merge the data structures in the future if consistency becomes a problem.
https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:132: ContentSuggestionsState::AVAILABLE) { On 2016/07/05 13:51:59, Philipp Keck wrote: > On 2016/07/05 13:17:12, Marc Treib wrote: > > No numeric comparisons on enums please - make a helper function and use a > > switch. > > Yep, I wanted a helper function but wasn't sure where to place it. Initially, it > was ContentSuggestionsProvider::IsCategoryAvailable(category), which called > ::GetCategoryState(). But then I started needing the same logic in the service > and also in other places. It doesn't seem possible to place helper functions in > the enum class? In Java, "provider.GetCategoryState(category).isAvailable()" > would work... I'd suggest a public function, next to the enum definition. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:140: // loading). On 2016/07/05 13:52:00, Philipp Keck wrote: > On 2016/07/05 13:17:12, Marc Treib wrote: > > It might be easier now if it always contained a (possibly empty) vector for > each > > registered category? Then you could get rid of empty_categories_list_ (which > is > > poorly named btw...) > > empty_categories_list_ will need to stay because there are the cases when the > service is disabled entirely and when a provider simply isn't registered. Also > in these cases we wouldn't want the call to this method to fail. (At least we > previously discussed that calling this method with a category that is not > returned from GetCategories() shouldn't fail.) > So what would be a better name for empty_categories_list_? It's not a list of categories, but a list of suggestions. So, |no_suggestions_| maybe? (I don't much like putting the type in the name, in particular if it's not even the same type :D) https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:152: std::map<std::string, ContentSuggestionCategory> id_category_map_; On 2016/07/05 14:28:34, Philipp Keck wrote: > On 2016/07/05 13:52:00, Philipp Keck wrote: > > On 2016/07/05 13:17:12, Marc Treib wrote: > > > Hm. So you need this because you don't want to parse the category out of the > > ID? > > > Not sure I like this any more... too many data structures here that have to > be > > > kept in sync. > > > > Tim's suggestion :-) Tim, wdyt? > > > > I was already in doubt when he suggested it this morning. The advantage is > that > > the format of the ID now doesn't matter to anyone anymore and providers can, > but > > do not have to use MakeUniqueID(). > > > > One solution would be to add the parsing back in. > > Another solution would be to add the category-parameter back on the FetchImage > > and Discard methods accross the Android bridge, which is less problematic > since > > the category is present there anyways. Though if we didn't do that, we could > > maybe even remove the .category() getter and field from the ContentSuggestion, > > so that it's category would implicitly be determined by the data structure it > > lives in. Currently, a provider could pass rubbish data into > OnNewSuggestions() > > because the category information is redundant (and nobody checks for that). > > Tim still prefers more data structures over parsing and notes that we can merge > the data structures in the future if consistency becomes a problem. Alright. Removing category() from ContentSuggestion sounds like a good idea. The way the ContentSuggestionService API is built right now, it shouldn't be required. https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:25: id_category_map_.clear(); Shouldn't this also be empty already? https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:108: DCHECK(position != suggestions->end()); DCHECK_NE https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:182: DCHECK(iterator != categories_.end()); DCHECK_NE https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:194: provider->SetObserver(nullptr); Does this matter?
I renamed state to status and I included the plural s in ContentSuggestionsCategory, because category is not an attribute of the suggestion anymore, but a set of suggestions, so the s makes sense now and makes the naming more uniform (everything is plural now). https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:132: ContentSuggestionsState::AVAILABLE) { On 2016/07/05 14:50:50, Marc Treib wrote: > On 2016/07/05 13:51:59, Philipp Keck wrote: > > On 2016/07/05 13:17:12, Marc Treib wrote: > > > No numeric comparisons on enums please - make a helper function and use a > > > switch. > > > > Yep, I wanted a helper function but wasn't sure where to place it. Initially, > it > > was ContentSuggestionsProvider::IsCategoryAvailable(category), which called > > ::GetCategoryState(). But then I started needing the same logic in the service > > and also in other places. It doesn't seem possible to place helper functions > in > > the enum class? In Java, "provider.GetCategoryState(category).isAvailable()" > > would work... > > I'd suggest a public function, next to the enum definition. Done. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:140: // loading). On 2016/07/05 14:50:50, Marc Treib wrote: > On 2016/07/05 13:52:00, Philipp Keck wrote: > > On 2016/07/05 13:17:12, Marc Treib wrote: > > > It might be easier now if it always contained a (possibly empty) vector for > > each > > > registered category? Then you could get rid of empty_categories_list_ (which > > is > > > poorly named btw...) > > > > empty_categories_list_ will need to stay because there are the cases when the > > service is disabled entirely and when a provider simply isn't registered. Also > > in these cases we wouldn't want the call to this method to fail. (At least we > > previously discussed that calling this method with a category that is not > > returned from GetCategories() shouldn't fail.) > > So what would be a better name for empty_categories_list_? > > It's not a list of categories, but a list of suggestions. So, |no_suggestions_| > maybe? (I don't much like putting the type in the name, in particular if it's > not even the same type :D) Ah ok, I see what you mean. I named it after its usage, not after its (non-)contents. Done. https://codereview.chromium.org/2102023002/diff/180001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:152: std::map<std::string, ContentSuggestionCategory> id_category_map_; On 2016/07/05 14:50:50, Marc Treib wrote: > On 2016/07/05 14:28:34, Philipp Keck wrote: > > On 2016/07/05 13:52:00, Philipp Keck wrote: > > > On 2016/07/05 13:17:12, Marc Treib wrote: > > > > Hm. So you need this because you don't want to parse the category out of > the > > > ID? > > > > Not sure I like this any more... too many data structures here that have > to > > be > > > > kept in sync. > > > > > > Tim's suggestion :-) Tim, wdyt? > > > > > > I was already in doubt when he suggested it this morning. The advantage is > > that > > > the format of the ID now doesn't matter to anyone anymore and providers can, > > but > > > do not have to use MakeUniqueID(). > > > > > > One solution would be to add the parsing back in. > > > Another solution would be to add the category-parameter back on the > FetchImage > > > and Discard methods accross the Android bridge, which is less problematic > > since > > > the category is present there anyways. Though if we didn't do that, we could > > > maybe even remove the .category() getter and field from the > ContentSuggestion, > > > so that it's category would implicitly be determined by the data structure > it > > > lives in. Currently, a provider could pass rubbish data into > > OnNewSuggestions() > > > because the category information is redundant (and nobody checks for that). > > > > Tim still prefers more data structures over parsing and notes that we can > merge > > the data structures in the future if consistency becomes a problem. > > Alright. > > Removing category() from ContentSuggestion sounds like a good idea. The way the > ContentSuggestionService API is built right now, it shouldn't be required. Done. https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:65: virtual void SetObserver(Observer* observer) = 0; Couldn't this be implemented here in this class, along with an "Observer*" field that every implementation needs anyway? https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:25: id_category_map_.clear(); On 2016/07/05 14:50:50, Marc Treib wrote: > Shouldn't this also be empty already? Done. https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:108: DCHECK(position != suggestions->end()); On 2016/07/05 14:50:50, Marc Treib wrote: > DCHECK_NE This didn't work because the compiler didn't like it, so I changed it to that. https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:182: DCHECK(iterator != categories_.end()); On 2016/07/05 14:50:50, Marc Treib wrote: > DCHECK_NE Dito. https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:194: provider->SetObserver(nullptr); On 2016/07/05 14:50:50, Marc Treib wrote: > Does this matter? Maybe not. It's also a precaution to avoid callbacks/events (like another call to OnProviderShutdown or a delayed OnNewSuggestions from some asynchronous request). Equally, observers_.Clear() in line 28 probably doesn't matter, right?
https://codereview.chromium.org/2102023002/diff/160001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/160001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:51: // the getters and other methods should not be used anymore. i'd phrase this more strongly: "the service must not be accessed anymore." https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:26: // application-wide. Note: The provider should use its ::MakeUniqueID() i'd drop the Note:. That might be something to add to the Provider's interface... https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:36: // suggestions of all providers. i wouldn't mention providers at all in this class. Only the service knows about the existence of the providers. For most users of this class, they only got it from the service and shouldn't care about providers. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestion_category.h (left): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestion_category.h:14: enum class ContentSuggestionCategory : int { ARTICLE }; now that you don't do comparisons anymore you don't need to define the underlying type, right? drop the ': int'? https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_category_status.h:13: enum class ContentSuggestionsCategoryStatus : int { nit: drop the ': int'? We shouldn't overspecify if we don't need them. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:48: std::vector<ContentSuggestion> suggestions) = 0; const vector<>& ? https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:103: static std::string MakeUniqueID(ContentSuggestionsCategory category, should this be protected? https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:49: return no_suggestions_; I wonder if handing out references to the underlying data is the right thing to do... Essentially, you're giving out a live view of the data (actually ,the data itself), so if new data arrives, users of this service would already see the changes before they get notified. Even worse, data can be invalidated. I guess that practically, this shouldn't be a real issue because copies need to be made at the JNI bridge anyways, but that's more of a lucky circumstance. IMO, the cleaner approach would be to hand out a copy of the data -- you also wouldn't have to care about keeping the no_suggestions_ member around. For typical use cases (where the client would want to store a copy anyways), return-value-optimization or move semantics would kick in. In the JNI case, you'd likely cause another copy, but honestly, copying such a vector shouldn't be too much of an overhead. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:62: enum State : bool { drop ': bool' and the value assignments? https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:137: // All current suggestion categories, in order. This contains exactly the same what is this order? The order they were registered? Would be nice to explain here, if the order is important.
https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:108: DCHECK(position != suggestions->end()); On 2016/07/05 16:53:34, Philipp Keck wrote: > On 2016/07/05 14:50:50, Marc Treib wrote: > > DCHECK_NE > > This didn't work because the compiler didn't like it, so I changed it to that. Didn't like what, exactly? I guess it tried to "<<" the iterators, which doesn't work? Alright. https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:194: provider->SetObserver(nullptr); On 2016/07/05 16:53:34, Philipp Keck wrote: > On 2016/07/05 14:50:50, Marc Treib wrote: > > Does this matter? > > Maybe not. It's also a precaution to avoid callbacks/events (like another call > to OnProviderShutdown or a delayed OnNewSuggestions from some asynchronous > request). I'd say that it's the provider's responsibility to not call its observer after shutdown. > Equally, observers_.Clear() in line 28 probably doesn't matter, right? Probably, yes. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:48: std::vector<ContentSuggestion> suggestions) = 0; On 2016/07/06 06:36:12, tschumann wrote: > const vector<>& ? This is on purpose, to avoid a copy - it'll be moved in. No risk of copying accidentally since ContentSuggestion isn't copyable. Doesn't really matter too much though. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:49: return no_suggestions_; On 2016/07/06 06:36:12, tschumann wrote: > I wonder if handing out references to the underlying data is the right thing to > do... > Essentially, you're giving out a live view of the data (actually ,the data > itself), so if new data arrives, users of this service would already see the > changes before they get notified. Even worse, data can be invalidated. > I guess that practically, this shouldn't be a real issue because copies need to > be made at the JNI bridge anyways, but that's more of a lucky circumstance. > IMO, the cleaner approach would be to hand out a copy of the data -- you also > wouldn't have to care about keeping the no_suggestions_ member around. > For typical use cases (where the client would want to store a copy anyways), > return-value-optimization or move semantics would kick in. In the JNI case, > you'd likely cause another copy, but honestly, copying such a vector shouldn't > be too much of an overhead. Generally, the convention is that a reference is something you shouldn't keep around - you can use it right now, but then you should drop it. As such, I'm fine with handing out references (but I also won't insist, if you feel strongly).
https://codereview.chromium.org/2102023002/diff/160001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/160001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:51: // the getters and other methods should not be used anymore. On 2016/07/06 06:36:12, tschumann wrote: > i'd phrase this more strongly: "the service must not be accessed anymore." Your comment is on old code. The new comment in the code uses an even less strong formulation: "After the service has shut down, it will not provide any data anymore, though calling the getters is still safe." because of previous discussion with Marc. We didn't want to restrict any calls after shutdown because we're not checking for that anyway. https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:108: DCHECK(position != suggestions->end()); On 2016/07/06 08:20:44, Marc Treib wrote: > On 2016/07/05 16:53:34, Philipp Keck wrote: > > On 2016/07/05 14:50:50, Marc Treib wrote: > > > DCHECK_NE > > > > This didn't work because the compiler didn't like it, so I changed it to that. > > Didn't like what, exactly? I guess it tried to "<<" the iterators, which doesn't > work? Alright. Acknowledged. https://codereview.chromium.org/2102023002/diff/200001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:194: provider->SetObserver(nullptr); On 2016/07/06 08:20:44, Marc Treib wrote: > On 2016/07/05 16:53:34, Philipp Keck wrote: > > On 2016/07/05 14:50:50, Marc Treib wrote: > > > Does this matter? > > > > Maybe not. It's also a precaution to avoid callbacks/events (like another call > > to OnProviderShutdown or a delayed OnNewSuggestions from some asynchronous > > request). > > I'd say that it's the provider's responsibility to not call its observer after > shutdown. > > > Equally, observers_.Clear() in line 28 probably doesn't matter, right? > > Probably, yes. Done. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestion.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:26: // application-wide. Note: The provider should use its ::MakeUniqueID() On 2016/07/06 06:36:12, tschumann wrote: > i'd drop the Note:. > That might be something to add to the Provider's interface... Done. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestion.h:36: // suggestions of all providers. On 2016/07/06 06:36:12, tschumann wrote: > i wouldn't mention providers at all in this class. Only the service knows about > the existence of the providers. For most users of this class, they only got it > from the service and shouldn't care about providers. Done. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestion_category.h (left): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestion_category.h:14: enum class ContentSuggestionCategory : int { ARTICLE }; On 2016/07/06 06:36:12, tschumann wrote: > now that you don't do comparisons anymore you don't need to define the > underlying type, right? drop the ': int'? The int isn't there for comparison, but for the Android bridge (where it's passed over as an int). I'm not sure if it's necessary for that purpose, but that's why I had to add it in. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_category_status.h:13: enum class ContentSuggestionsCategoryStatus : int { On 2016/07/06 06:36:12, tschumann wrote: > nit: drop the ': int'? We shouldn't overspecify if we don't need them. Could be dropped unless the Android bridge needs them. I think the Java enum generator needs those int values to ensure that they match? https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:103: static std::string MakeUniqueID(ContentSuggestionsCategory category, On 2016/07/06 06:36:12, tschumann wrote: > should this be protected? No, NTPSnippet also calls it. If the NTPSnippet class can't call it directly, it needs an ID parameter on its NTPSnippet::ToContentSuggestion(std::string id) that would be filled in by the calling NTPSnippetsService, which in turn gets it from the ContentSuggestionsProvider::MakeUniqueID here. So ToContentSuggestion() would need a parameter, though ToXxx methods usually don't have parameters (except to further specify the format of the output). https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:62: enum State : bool { On 2016/07/06 06:36:12, tschumann wrote: > drop ': bool' and the value assignments? Done. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:137: // All current suggestion categories, in order. This contains exactly the same On 2016/07/06 06:36:12, tschumann wrote: > what is this order? The order they were registered? Would be nice to explain > here, if the order is important. Done.
lgtm giving my LGTM as I'm quite happy with the ove.rall design and implementation. Leaving the details to Marc =). https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_category_status.h:13: enum class ContentSuggestionsCategoryStatus : int { On 2016/07/06 08:34:23, Philipp Keck wrote: > On 2016/07/06 06:36:12, tschumann wrote: > > nit: drop the ': int'? We shouldn't overspecify if we don't need them. > > Could be dropped unless the Android bridge needs them. I think the Java enum > generator needs those int values to ensure that they match? I see. Uf the bridge needs it, it makes totally sense to keep =). https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:48: std::vector<ContentSuggestion> suggestions) = 0; On 2016/07/06 08:20:44, Marc Treib wrote: > On 2016/07/06 06:36:12, tschumann wrote: > > const vector<>& ? > > This is on purpose, to avoid a copy - it'll be moved in. No risk of copying > accidentally since ContentSuggestion isn't copyable. > Doesn't really matter too much though. given it's an interface, I have a slight tendency towards const & as it imposes less restrictions on the implementations. But not feeling terribly strongly. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:103: static std::string MakeUniqueID(ContentSuggestionsCategory category, On 2016/07/06 08:34:23, Philipp Keck wrote: > On 2016/07/06 06:36:12, tschumann wrote: > > should this be protected? > > No, NTPSnippet also calls it. If the NTPSnippet class can't call it directly, it > needs an ID parameter on its NTPSnippet::ToContentSuggestion(std::string id) > that would be filled in by the calling NTPSnippetsService, which in turn gets it > from the ContentSuggestionsProvider::MakeUniqueID here. So ToContentSuggestion() > would need a parameter, though ToXxx methods usually don't have parameters > (except to further specify the format of the output). As discussed offline, give that the provider is calling NTPSnippet::ToContentSuggestion(), we can either move that conversion into the provider or just pass in the ID.
https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_category_status.h:13: enum class ContentSuggestionsCategoryStatus : int { On 2016/07/06 14:49:43, tschumann wrote: > On 2016/07/06 08:34:23, Philipp Keck wrote: > > On 2016/07/06 06:36:12, tschumann wrote: > > > nit: drop the ': int'? We shouldn't overspecify if we don't need them. > > > > Could be dropped unless the Android bridge needs them. I think the Java enum > > generator needs those int values to ensure that they match? > > I see. Uf the bridge needs it, it makes totally sense to keep =). Acknowledged. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:48: std::vector<ContentSuggestion> suggestions) = 0; On 2016/07/06 14:49:44, tschumann wrote: > On 2016/07/06 08:20:44, Marc Treib wrote: > > On 2016/07/06 06:36:12, tschumann wrote: > > > const vector<>& ? > > > > This is on purpose, to avoid a copy - it'll be moved in. No risk of copying > > accidentally since ContentSuggestion isn't copyable. > > Doesn't really matter too much though. > > given it's an interface, I have a slight tendency towards const & as it imposes > less restrictions on the implementations. But not feeling terribly strongly. const-ref wouldn't work because ContentSuggestion is not copyable. The service is the only implementation of this interface and it is a reasonable assumption that it will stay the only implementation (we also only allow one observer, not multiple ones). https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:103: static std::string MakeUniqueID(ContentSuggestionsCategory category, On 2016/07/06 14:49:44, tschumann wrote: > On 2016/07/06 08:34:23, Philipp Keck wrote: > > On 2016/07/06 06:36:12, tschumann wrote: > > > should this be protected? > > > > No, NTPSnippet also calls it. If the NTPSnippet class can't call it directly, > it > > needs an ID parameter on its NTPSnippet::ToContentSuggestion(std::string id) > > that would be filled in by the calling NTPSnippetsService, which in turn gets > it > > from the ContentSuggestionsProvider::MakeUniqueID here. So > ToContentSuggestion() > > would need a parameter, though ToXxx methods usually don't have parameters > > (except to further specify the format of the output). > > As discussed offline, give that the provider is calling > NTPSnippet::ToContentSuggestion(), we can either move that conversion into the > provider or just pass in the ID. This method is now protected. It will be used by providers internally (and they can decide to pass it along to any helper methods they might need). The current use in NTPSnippet::ToContentSuggestion and in fact the entire ToContentSuggestion method have been removed. The new use will be in the conversion method in the NTPSnippetsService (aka ArticleSuggestionsProvider), which currently calls ToContentSuggestion. The changes to NTPSnippetsService are not part of this CL, so it will reappear later. https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:49: return no_suggestions_; On 2016/07/06 08:20:44, Marc Treib wrote: > On 2016/07/06 06:36:12, tschumann wrote: > > I wonder if handing out references to the underlying data is the right thing > to > > do... > > Essentially, you're giving out a live view of the data (actually ,the data > > itself), so if new data arrives, users of this service would already see the > > changes before they get notified. Even worse, data can be invalidated. > > I guess that practically, this shouldn't be a real issue because copies need > to > > be made at the JNI bridge anyways, but that's more of a lucky circumstance. > > IMO, the cleaner approach would be to hand out a copy of the data -- you also > > wouldn't have to care about keeping the no_suggestions_ member around. > > For typical use cases (where the client would want to store a copy anyways), > > return-value-optimization or move semantics would kick in. In the JNI case, > > you'd likely cause another copy, but honestly, copying such a vector shouldn't > > be too much of an overhead. > > Generally, the convention is that a reference is something you shouldn't keep > around - you can use it right now, but then you should drop it. As such, I'm > fine with handing out references (but I also won't insist, if you feel > strongly). Acknowledged.
A few more comments, mostly nits https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:38: // this callback for every category individually. The |suggestions| nit: I'd phrase this as describing the interface, i.e. something like "If a provider ..., this gets called..." https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:39: // parameter should always contain the full list of currently available Similar here: s/should always contain/always contains https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:44: // The list must not contain ContentSuggestions assigned to a category other Since ContentSuggestions don't contain a category anymore, this comment isn't needed anymore. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:79: // |original_id| that was passed to the |ContentSuggestion| constructor. I think this comment is outdated? https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:85: // |original_id| that was passed to the |ContentSuggestion| constructor. Also here https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:99: const std::vector<ContentSuggestionsCategory> provided_categories() const { ref? https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:33: return ContentSuggestionsCategoryStatus:: nit: braces if the body doesn't fit on one line https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:74: for (auto& provider : providers_) { nit: The elements are not providers, but category+provider pairs, right? So maybe call the variable category_and_provider or something like that? (Rule of thumb: Only use "auto" when the type is obvious or not interesting (e.g. iterator). Neither is really the case here, but spelling out the pair is also ugly...) https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:81: for (auto& provider : providers_) { Same here https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:123: if (state_ == State::DISABLED) Are we sure about this? We'll have to change it once the service state can change at runtime. (Then again, it can't yet, so maybe this is best for now.) https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:136: provider->SetObserver(this); This should go out of the loop https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:174: } Do we also need to do something if it is now available? Like add an entry to categories_ and/or suggestions_by_category_? https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:193: OnCategoryStatusChanged(category, GetCategoryStatus(category))); optional: You could replace all the OnCategoryStatusChanged calls by a NotifyCategoryStatusChanged method (the calls aren't exactly the same, but looks they could all just call GetCategoryStatus(category) to get the new status).
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_category_status.h:13: enum class ContentSuggestionsCategoryStatus : int { On 2016/07/06 15:53:20, Philipp Keck wrote: > On 2016/07/06 14:49:43, tschumann wrote: > > On 2016/07/06 08:34:23, Philipp Keck wrote: > > > On 2016/07/06 06:36:12, tschumann wrote: > > > > nit: drop the ': int'? We shouldn't overspecify if we don't need them. > > > > > > Could be dropped unless the Android bridge needs them. I think the Java enum > > > generator needs those int values to ensure that they match? > > > > I see. Uf the bridge needs it, it makes totally sense to keep =). > > Acknowledged. FTR: The java enum generator doesn't need the underlying type, and the JNI bridge (which is independent!) will use jint anyway (which happens to be typedef'd to int).
https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_category_status.h:13: enum class ContentSuggestionsCategoryStatus : int { On 2016/07/07 09:42:38, Bernhard Bauer wrote: > On 2016/07/06 15:53:20, Philipp Keck wrote: > > On 2016/07/06 14:49:43, tschumann wrote: > > > On 2016/07/06 08:34:23, Philipp Keck wrote: > > > > On 2016/07/06 06:36:12, tschumann wrote: > > > > > nit: drop the ': int'? We shouldn't overspecify if we don't need them. > > > > > > > > Could be dropped unless the Android bridge needs them. I think the Java > enum > > > > generator needs those int values to ensure that they match? > > > > > > I see. Uf the bridge needs it, it makes totally sense to keep =). > > > > Acknowledged. > > FTR: The java enum generator doesn't need the underlying type, and the JNI > bridge (which is independent!) will use jint anyway (which happens to be > typedef'd to int). Ok, so the Android bridge would work without the ": int"? I haven't implemented the Android part yet, so I don't know if it would work. The enum where most of these values here come from (https://codesearch.chromium.org/chromium/src/components/ntp_snippets/ntp_snip...) uses ": int" and that's why I have it here. To the DisabledReason enum it was added here https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet..., probably to make the static_cast<int> in the bridge possible https://codereview.chromium.org/2061803002/diff/120001/chrome/browser/android... ?
https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:47: State enabled = State::ENABLED; Wait, on non-Android platforms this is always enabled? https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:51: if (!base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature)) { Nit: No braces for single-line bodies. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:47: if (iterator == suggestions_by_category_.end()) { And no braces here ☺ https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:129: // TODO(pke) In the future, make sure that the categories have some useful Nit: The exact format for TODOs is TODO(ldap): ☺
https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_category_status.h:13: enum class ContentSuggestionsCategoryStatus : int { On 2016/07/07 09:52:22, Philipp Keck wrote: > On 2016/07/07 09:42:38, Bernhard Bauer wrote: > > On 2016/07/06 15:53:20, Philipp Keck wrote: > > > On 2016/07/06 14:49:43, tschumann wrote: > > > > On 2016/07/06 08:34:23, Philipp Keck wrote: > > > > > On 2016/07/06 06:36:12, tschumann wrote: > > > > > > nit: drop the ': int'? We shouldn't overspecify if we don't need them. > > > > > > > > > > Could be dropped unless the Android bridge needs them. I think the Java > > enum > > > > > generator needs those int values to ensure that they match? > > > > > > > > I see. Uf the bridge needs it, it makes totally sense to keep =). > > > > > > Acknowledged. > > > > FTR: The java enum generator doesn't need the underlying type, and the JNI > > bridge (which is independent!) will use jint anyway (which happens to be > > typedef'd to int). > > Ok, so the Android bridge would work without the ": int"? I haven't implemented > the Android part yet, so I don't know if it would work. The enum where most of > these values here come from > (https://codesearch.chromium.org/chromium/src/components/ntp_snippets/ntp_snip...) > uses ": int" and that's why I have it here. > To the DisabledReason enum it was added here > https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet..., > probably to make the static_cast<int> in the bridge possible > https://codereview.chromium.org/2061803002/diff/120001/chrome/browser/android... > ? The java enum generator works without the ": int" -- it's just a script, and it doesn't look at the underlying type. JNI doesn't need the underlying type *declaration* either, as that only depends on the type used by the compiler (not the source code), and it knows how to cast the type (and if the underlying type for the enum happens to be the same as a jint, it will be a no-op anyway).
https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_category_status.h (right): https://codereview.chromium.org/2102023002/diff/240001/components/ntp_snippet... components/ntp_snippets/content_suggestions_category_status.h:13: enum class ContentSuggestionsCategoryStatus : int { On 2016/07/07 10:29:08, Bernhard Bauer wrote: > On 2016/07/07 09:52:22, Philipp Keck wrote: > > On 2016/07/07 09:42:38, Bernhard Bauer wrote: > > > On 2016/07/06 15:53:20, Philipp Keck wrote: > > > > On 2016/07/06 14:49:43, tschumann wrote: > > > > > On 2016/07/06 08:34:23, Philipp Keck wrote: > > > > > > On 2016/07/06 06:36:12, tschumann wrote: > > > > > > > nit: drop the ': int'? We shouldn't overspecify if we don't need > them. > > > > > > > > > > > > Could be dropped unless the Android bridge needs them. I think the > Java > > > enum > > > > > > generator needs those int values to ensure that they match? > > > > > > > > > > I see. Uf the bridge needs it, it makes totally sense to keep =). > > > > > > > > Acknowledged. > > > > > > FTR: The java enum generator doesn't need the underlying type, and the JNI > > > bridge (which is independent!) will use jint anyway (which happens to be > > > typedef'd to int). > > > > Ok, so the Android bridge would work without the ": int"? I haven't > implemented > > the Android part yet, so I don't know if it would work. The enum where most of > > these values here come from > > > (https://codesearch.chromium.org/chromium/src/components/ntp_snippets/ntp_snip...) > > uses ": int" and that's why I have it here. > > To the DisabledReason enum it was added here > > > https://codereview.chromium.org/2061803002/diff/120001/components/ntp_snippet..., > > probably to make the static_cast<int> in the bridge possible > > > https://codereview.chromium.org/2061803002/diff/120001/chrome/browser/android... > > ? > > The java enum generator works without the ": int" -- it's just a script, and it > doesn't look at the underlying type. JNI doesn't need the underlying type > *declaration* either, as that only depends on the type used by the compiler (not > the source code), and it knows how to cast the type (and if the underlying type > for the enum happens to be the same as a jint, it will be a no-op anyway). Ok I removed it and so far nothing breaks :-) https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:47: State enabled = State::ENABLED; On 2016/07/07 10:02:19, Bernhard Bauer wrote: > Wait, on non-Android platforms this is always enabled? Should be the same as NTPSnippetsServiceFactory. On non-Android platforms, the ProfileManager will never call the factory, though (and also nobody else). Still I agree that only enabling on Android seems safer. https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:51: if (!base::FeatureList::IsEnabled(chrome::android::kNTPSnippetsFeature)) { On 2016/07/07 10:02:19, Bernhard Bauer wrote: > Nit: No braces for single-line bodies. A very debatable style rule ... makes it harder to extend the body. The style guide doesn't require it (https://google.github.io/styleguide/cppguide.html#Conditionals), if that's the right style guide. I still changed it for consistency. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:38: // this callback for every category individually. The |suggestions| On 2016/07/07 09:37:50, Marc Treib wrote: > nit: I'd phrase this as describing the interface, i.e. something like "If a > provider ..., this gets called..." Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:39: // parameter should always contain the full list of currently available On 2016/07/07 09:37:50, Marc Treib wrote: > Similar here: s/should always contain/always contains Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:44: // The list must not contain ContentSuggestions assigned to a category other On 2016/07/07 09:37:50, Marc Treib wrote: > Since ContentSuggestions don't contain a category anymore, this comment isn't > needed anymore. Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:79: // |original_id| that was passed to the |ContentSuggestion| constructor. On 2016/07/07 09:37:50, Marc Treib wrote: > I think this comment is outdated? Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:85: // |original_id| that was passed to the |ContentSuggestion| constructor. On 2016/07/07 09:37:50, Marc Treib wrote: > Also here Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:99: const std::vector<ContentSuggestionsCategory> provided_categories() const { On 2016/07/07 09:37:50, Marc Treib wrote: > ref? Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:33: return ContentSuggestionsCategoryStatus:: On 2016/07/07 09:37:51, Marc Treib wrote: > nit: braces if the body doesn't fit on one line Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:47: if (iterator == suggestions_by_category_.end()) { On 2016/07/07 10:02:19, Bernhard Bauer wrote: > And no braces here ☺ Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:74: for (auto& provider : providers_) { On 2016/07/07 09:37:51, Marc Treib wrote: > nit: The elements are not providers, but category+provider pairs, right? So > maybe call the variable category_and_provider or something like that? > (Rule of thumb: Only use "auto" when the type is obvious or not interesting > (e.g. iterator). Neither is really the case here, but spelling out the pair is > also ugly...) Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:81: for (auto& provider : providers_) { On 2016/07/07 09:37:51, Marc Treib wrote: > Same here Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:123: if (state_ == State::DISABLED) On 2016/07/07 09:37:51, Marc Treib wrote: > Are we sure about this? We'll have to change it once the service state can > change at runtime. (Then again, it can't yet, so maybe this is best for now.) Yes. It is here because the NTPSnippetsService will still be enabled for a while and will deliver to UI directly, but if the ContentSuggestionsService is additionally enabled, it also delivers data there (initially only for debugging, later because the UI reads the data from the new service). This "if" makes it clear also to the NTPSnippetsService whether the new service is active or not (because if it's disabled, it won't register its observer, so the NTPSnippetsService knows that it doesn't need to deliver any data). In a later stage, providers will just use the ::state() getter on the ContentSuggestionsService to see if they need to produce data or not. And then this can be removed, at the latest when the state can change at runtime, like you said. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:129: // TODO(pke) In the future, make sure that the categories have some useful On 2016/07/07 10:02:19, Bernhard Bauer wrote: > Nit: The exact format for TODOs is TODO(ldap): ☺ Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:136: provider->SetObserver(this); On 2016/07/07 09:37:51, Marc Treib wrote: > This should go out of the loop Done. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:174: } On 2016/07/07 09:37:51, Marc Treib wrote: > Do we also need to do something if it is now available? Like add an entry to > categories_ and/or suggestions_by_category_? No. id_category_map_ and suggestions_by_category_ are filled by OnNewSuggestions and otherwise behave as if they were empty - even if an empty entry is not explicitly added. categories_ always contains the category, even if it is not available (but only if a provider for it is present). The categories_ vector is supposed to inform which categories exist (i.e. which you can query the status for, and if the status is "available" then query the suggestions). https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:193: OnCategoryStatusChanged(category, GetCategoryStatus(category))); On 2016/07/07 09:37:51, Marc Treib wrote: > optional: You could replace all the OnCategoryStatusChanged calls by a > NotifyCategoryStatusChanged method (the calls aren't exactly the same, but looks > they could all just call GetCategoryStatus(category) to get the new status). Done.
lgtm https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:47: State enabled = State::ENABLED; On 2016/07/07 12:22:29, Philipp Keck wrote: > On 2016/07/07 10:02:19, Bernhard Bauer wrote: > > Wait, on non-Android platforms this is always enabled? > > Should be the same as NTPSnippetsServiceFactory. > On non-Android platforms, the ProfileManager will never call the factory, though > (and also nobody else). Still I agree that only enabling on Android seems safer. Yup, IMO we should change this. The change for NTPSnippetsServiceFactory should be in a separate CL though. https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:123: if (state_ == State::DISABLED) On 2016/07/07 12:22:30, Philipp Keck wrote: > On 2016/07/07 09:37:51, Marc Treib wrote: > > Are we sure about this? We'll have to change it once the service state can > > change at runtime. (Then again, it can't yet, so maybe this is best for now.) > > Yes. It is here because the NTPSnippetsService will still be enabled for a while > and will deliver to UI directly, but if the ContentSuggestionsService is > additionally enabled, it also delivers data there (initially only for debugging, > later because the UI reads the data from the new service). This "if" makes it > clear also to the NTPSnippetsService whether the new service is active or not > (because if it's disabled, it won't register its observer, so the > NTPSnippetsService knows that it doesn't need to deliver any data). > > In a later stage, providers will just use the ::state() getter on the > ContentSuggestionsService to see if they need to produce data or not. And then > this can be removed, at the latest when the state can change at runtime, like > you said. Alright. Add a TODO maybe? https://codereview.chromium.org/2102023002/diff/310001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/310001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:158: std::map<ContentSuggestionsCategory, ContentSuggestionsProvider*> providers_; nit: Maybe move this up, under categories_, so that the two "static" ones come first?
https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:47: State enabled = State::ENABLED; On 2016/07/07 12:32:16, Marc Treib wrote: > On 2016/07/07 12:22:29, Philipp Keck wrote: > > On 2016/07/07 10:02:19, Bernhard Bauer wrote: > > > Wait, on non-Android platforms this is always enabled? > > > > Should be the same as NTPSnippetsServiceFactory. > > On non-Android platforms, the ProfileManager will never call the factory, > though > > (and also nobody else). Still I agree that only enabling on Android seems > safer. > > Yup, IMO we should change this. The change for NTPSnippetsServiceFactory should > be in a separate CL though. Done. Should I prepare a separate CL for NTPSnippetsServiceFactory? https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2102023002/diff/290001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:123: if (state_ == State::DISABLED) On 2016/07/07 12:32:16, Marc Treib wrote: > On 2016/07/07 12:22:30, Philipp Keck wrote: > > On 2016/07/07 09:37:51, Marc Treib wrote: > > > Are we sure about this? We'll have to change it once the service state can > > > change at runtime. (Then again, it can't yet, so maybe this is best for > now.) > > > > Yes. It is here because the NTPSnippetsService will still be enabled for a > while > > and will deliver to UI directly, but if the ContentSuggestionsService is > > additionally enabled, it also delivers data there (initially only for > debugging, > > later because the UI reads the data from the new service). This "if" makes it > > clear also to the NTPSnippetsService whether the new service is active or not > > (because if it's disabled, it won't register its observer, so the > > NTPSnippetsService knows that it doesn't need to deliver any data). > > > > In a later stage, providers will just use the ::state() getter on the > > ContentSuggestionsService to see if they need to produce data or not. And then > > this can be removed, at the latest when the state can change at runtime, like > > you said. > > Alright. Add a TODO maybe? Done. https://codereview.chromium.org/2102023002/diff/310001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2102023002/diff/310001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:158: std::map<ContentSuggestionsCategory, ContentSuggestionsProvider*> providers_; On 2016/07/07 12:32:16, Marc Treib wrote: > nit: Maybe move this up, under categories_, so that the two "static" ones come > first? Done. Moved it above categories_ so that categories_ and suggestions_by_category_ are still grouped together.
https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:47: State enabled = State::ENABLED; On 2016/07/07 12:44:06, Philipp Keck wrote: > On 2016/07/07 12:32:16, Marc Treib wrote: > > On 2016/07/07 12:22:29, Philipp Keck wrote: > > > On 2016/07/07 10:02:19, Bernhard Bauer wrote: > > > > Wait, on non-Android platforms this is always enabled? > > > > > > Should be the same as NTPSnippetsServiceFactory. > > > On non-Android platforms, the ProfileManager will never call the factory, > > though > > > (and also nobody else). Still I agree that only enabling on Android seems > > safer. > > > > Yup, IMO we should change this. The change for NTPSnippetsServiceFactory > should > > be in a separate CL though. > > Done. Should I prepare a separate CL for NTPSnippetsServiceFactory? Yup, please do :)
pke@google.com changed reviewers: + anthonyvd@chromium.org
anthonyvd@chromium.org: Please review changes in profile_manager.cc
c/b/profiles/profile_manager.cc lgtm % small nit https://codereview.chromium.org/2102023002/diff/330001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2102023002/diff/330001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:112: #include "components/ntp_snippets/content_suggestions_service.h" Is this include necessary?
https://codereview.chromium.org/2102023002/diff/330001/chrome/browser/profile... File chrome/browser/profiles/profile_manager.cc (right): https://codereview.chromium.org/2102023002/diff/330001/chrome/browser/profile... chrome/browser/profiles/profile_manager.cc:112: #include "components/ntp_snippets/content_suggestions_service.h" On 2016/07/07 14:25:45, anthonyvd wrote: > Is this include necessary? It is indeed not necessary, I also remove the one below (which apparently used to be required but isn't anymore).
Description was changed from ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. Change ContentSuggestion to merge the ID from the provider with the provider's type into a single, unique ID that can be used by the UI and let ContentSuggestionsService convert back for communication with the providers. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. BUG=619560 ========== to ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. Add CSCategoryStatus for providers to report on the status of their categories. Rename ContentSuggestionCategory to ContentSuggestionsCategory for consistency. Remove ContentSuggestionsProviderType as there can only be one provider for any category. Remove previously inserted ToContentSuggestion() from NTPSnippet. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. The new service is launched by the ProfileManager but does not do anything yet. BUG=619560 ==========
https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/content_suggestions_service_factory.cc (right): https://codereview.chromium.org/2102023002/diff/290001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/content_suggestions_service_factory.cc:47: State enabled = State::ENABLED; On 2016/07/07 10:02:19, Bernhard Bauer wrote: > Wait, on non-Android platforms this is always enabled? @bauerb Please take a look again at the factory, as the CL needs your approval for these files. Thank you.
lgtm
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from tschumann@chromium.org, treib@chromium.org, anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/2102023002/#ps350001 (title: "Remove unused imports from profile_manager.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, no build URL) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/07/08 10:24:51, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, no build > URL) > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) While it compiles and runs fine on my machine, it seems like some compilers need an explicit cast after the ": int" has been removed.
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, treib@chromium.org, anthonyvd@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2102023002/#ps370001 (title: "Add explicit case from ContentSuggestionsCategory to int")
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.
Description was changed from ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. Add CSCategoryStatus for providers to report on the status of their categories. Rename ContentSuggestionCategory to ContentSuggestionsCategory for consistency. Remove ContentSuggestionsProviderType as there can only be one provider for any category. Remove previously inserted ToContentSuggestion() from NTPSnippet. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. The new service is launched by the ProfileManager but does not do anything yet. BUG=619560 ========== to ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. Add CSCategoryStatus for providers to report on the status of their categories. Rename ContentSuggestionCategory to ContentSuggestionsCategory for consistency. Remove ContentSuggestionsProviderType as there can only be one provider for any category. Remove previously inserted ToContentSuggestion() from NTPSnippet. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. The new service is launched by the ProfileManager but does not do anything yet. BUG=619560 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:370001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. Add CSCategoryStatus for providers to report on the status of their categories. Rename ContentSuggestionCategory to ContentSuggestionsCategory for consistency. Remove ContentSuggestionsProviderType as there can only be one provider for any category. Remove previously inserted ToContentSuggestion() from NTPSnippet. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. The new service is launched by the ProfileManager but does not do anything yet. BUG=619560 ========== to ========== Add ContentSuggestionsService Add new ContentSuggestionsService, which acts as a mixer of suggestions provided by different providers. Add the ContentSuggestionsProvider interface to be implemented by these providers. Add the ContentSuggestionsServiceFactory. Add CSCategoryStatus for providers to report on the status of their categories. Rename ContentSuggestionCategory to ContentSuggestionsCategory for consistency. Remove ContentSuggestionsProviderType as there can only be one provider for any category. Remove previously inserted ToContentSuggestion() from NTPSnippet. All new classes do not yet interact with existing code or even touch it and the new code is not yet used by the UI. The new service is launched by the ProfileManager but does not do anything yet. BUG=619560 Committed: https://crrev.com/6dbb90af760531b04ca335d99cc68b448e0523b9 Cr-Commit-Position: refs/heads/master@{#404347} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/6dbb90af760531b04ca335d99cc68b448e0523b9 Cr-Commit-Position: refs/heads/master@{#404347} |
