Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2194203002:
Make ContentSuggestionsService recognize new/removed categories (Closed)
|
Created:
4 years, 4 months ago by Philipp Keck Modified:
4 years, 4 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@renaming Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake ContentSuggestionsService recognize new/removed categories
Change the OnNewSuggestions and OnCategoryStatusChanged methods in
ContentSuggestionsService so that they recognize new categories being
added and OnCategoryStatusChanged recognizes categories being removed
when their status changes to NOT_PROVIDED. This allows providers to
change their provided categories dynamically.
Little fix: Make Category(int) constructor explicit
BUG=632320
Committed: https://crrev.com/4d3a4d66ed9290eec918786cd7457ef1bf46e0dc
Cr-Commit-Position: refs/heads/master@{#409160}
Patch Set 1 #Patch Set 2 : Adjust ContentSuggestionsServiceTest #Patch Set 3 : Put the status-change notification back in RegisterProvider() #
Total comments: 35
Patch Set 4 : Tim's and Marc's comments #Patch Set 5 : Rename providers_by_category_ in unit test #Patch Set 6 : Adjust OnCategoryStatusChanged comment #Patch Set 7 : Rename EnsureCategoryRegistered to RegisterCategoryIfRequired #
Total comments: 8
Patch Set 8 : Marc's comments #
Messages
Total messages: 25 (7 generated)
pke@google.com changed reviewers: + treib@chromium.org
PTAL
tschumann@chromium.org changed reviewers: + tschumann@chromium.org
https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:80: // removing categories) is called on the observer. i wonder if we can get rid of this function at all (not necessarily this CL) and have all date flow through the observer. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:181: auto providers_entry = providers_.find(category); nit: the usual naming would be 'providers_it'. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:210: bool ContentSuggestionsService::EnsureCategoryRegistered( nit: it feels weird for EnsureCategoryRegistered to return true in the first place; especially returning 'false' if it's laready registered. What about renaming to something like UpdateCategoryRegistry? or UpdateCategoryMapping()?
https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:44: // category. I find this a bit confusing... it's not called with multiple category values, and it doesn't actually create the category. How about: The given |category| is registered if it isn't known yet. or something like that? https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:55: // NOT_PROVIDED). I don't understand this - for NOT_PROVIDED, whatever is passed here should still be consistent with GetCategoryStatus, no? Or does it become illegal to call GetCategoryStatus? That seems a bit weird/fragile... maybe GetCategoryStatus should just return NOT_PROVIDED for categories it doesn't know about? https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:78: // The value returned by this getter must not change unless OnNewSuggestions I'd formulate this as something like: When the set of provided categories changes, ... gets called on the observer. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:152: NotifyCategoryStatusChanged(category); Hm, you call this before the new suggestions are available through the getters... I guess the UI would only react to this if a category is removed? https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:184: providers_.erase(providers_entry); Hm, maybe providers_ should be renamed to providers_by_category_? This confused me at first, since it looks like you're just removing the provider completely. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:140: bool EnsureCategoryRegistered(ContentSuggestionsProvider* provider, EnsureCategoryIsRegistered?
https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:44: // category. On 2016/08/01 14:23:37, Marc Treib wrote: > I find this a bit confusing... it's not called with multiple category values, > and it doesn't actually create the category. How about: > The given |category| is registered if it isn't known yet. > or something like that? Done. Changed here and below. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:55: // NOT_PROVIDED). On 2016/08/01 14:23:37, Marc Treib wrote: > I don't understand this - for NOT_PROVIDED, whatever is passed here should still > be consistent with GetCategoryStatus, no? > Or does it become illegal to call GetCategoryStatus? That seems a bit > weird/fragile... maybe GetCategoryStatus should just return NOT_PROVIDED for > categories it doesn't know about? Well, the current provider implementations just DCHECK that it's their category. So currently the assumption is that it's only called with the categories that the provider actually provides. We could change that, but I think it should be consistent with the contract that other methods follow. What would GetCategoryInfo() (coming up soon) return if you call it for a category that the provider doesn't provide (anymore)? Something like a "null" struct? I think ensuring that the service doesn't call any methods on the provider for weird categories is important. "Just unregistered" is certainly a corner case that's not really "weird", but it falls under the same definition of categories that the provider is not responsible for. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:78: // The value returned by this getter must not change unless OnNewSuggestions On 2016/08/01 14:23:37, Marc Treib wrote: > I'd formulate this as something like: When the set of provided categories > changes, ... gets called on the observer. Done. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:80: // removing categories) is called on the observer. On 2016/08/01 14:21:17, tschumann wrote: > i wonder if we can get rid of this function at all (not necessarily this CL) and > have all date flow through the observer. Yes we can, though I'm not sure about how we should best design the "registering" then. The method is currently used when a provider is registered and when it is unregistered. For the unregistered, it's just used to list all the provider's categories and unregister those. Instead, we could just list ALL the present categories, check which ones are provided by the provider that is about to leave, and then unregister those. For the registering, we could simply "register" a provider by setting the Observer. At some point, the provider would need to start notifying about the categories it provides (e.g. setting all of them to INITIALIZING through OnCategoryStatusChanged). The question is: When is the right time to do that. The only possibility I see is in SetObserver. So: void SetObserver(Observer* observer) { if (observer == observer_) return; observer_ = observer; if (observer_ == null) return; // Now call the observer for all the categories we currently have. observer_->OnCategoryStatusChanged(ARTICLES, articles_status_); } And articles_status_ would be initialized with INITIALIZING. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:152: NotifyCategoryStatusChanged(category); On 2016/08/01 14:23:37, Marc Treib wrote: > Hm, you call this before the new suggestions are available through the > getters... I guess the UI would only react to this if a category is removed? Related question: If a category is registered while it is INITIALIZING and then the data becomes AVAILABLE, what's the right order to notify: StatusChanged(AVAILABLE); OnNewSuggestions(here comes the data); Or the other way round? It's essentially the same question/problem, but in the case where the category was there before (so it's not about the "if" in this code here anymore). We need to solve both cases somehow. I talked to Michael about this last week. Each section would wait for content from its section until it becomes available for the first time, grab that and then not care about further AVAILABLE-updates, only other updates. And "AVAILABLE" with an empty set of suggestions means that the category is fully loaded, but no content available. So the UI must hide the section, because it can't know that there is a OnNewSuggestions call following immediately. We could enforce that the status of a category reported through GetCategoryStatus() must always be AVAILABLE (or AVAILABLE_LOADING?) when OnNewSuggestions is called. But to set that status, calling OnNewSuggestions implies OnCategoryStatusChanged(AVAILABLE). Though I think we'd end up calling OnCategoryStatusChanged(AVAILABLE) every time because we have no way of knowing if it actually changed ... https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:181: auto providers_entry = providers_.find(category); On 2016/08/01 14:21:17, tschumann wrote: > nit: the usual naming would be 'providers_it'. Done. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:184: providers_.erase(providers_entry); On 2016/08/01 14:23:37, Marc Treib wrote: > Hm, maybe providers_ should be renamed to providers_by_category_? This confused > me at first, since it looks like you're just removing the provider completely. Done. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:210: bool ContentSuggestionsService::EnsureCategoryRegistered( On 2016/08/01 14:21:17, tschumann wrote: > nit: it feels weird for EnsureCategoryRegistered to return true in the first > place; especially returning 'false' if it's laready registered. > > What about renaming to something like UpdateCategoryRegistry? > or UpdateCategoryMapping()? Marc suggested EnsureCategoryIsRegistered. The logic of the return value is like the one of "put" or "remove" from maps (Java). They always ensure the data structure is in a certain state after they return (after "put", an element is in the map, after remove it's definitely gone), and their return value tells you if they had to change something in order to achieve that. I agree that the way this works is a bit confusing, even without the naming. Another solution is: EnsureCategoryRegistered calls NotifyCategoryStatusChanged directly (fair enough, since it just registered a new category), unless you set some parameter "notifyChange" to false, in which case it won't and the caller can do that himself. That way, EnsureCategoryRegistered would not have a return value anymore. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.h:140: bool EnsureCategoryRegistered(ContentSuggestionsProvider* provider, On 2016/08/01 14:23:38, Marc Treib wrote: > EnsureCategoryIsRegistered? Acknowledged.
https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:55: // NOT_PROVIDED). On 2016/08/01 14:59:03, Philipp Keck wrote: > On 2016/08/01 14:23:37, Marc Treib wrote: > > I don't understand this - for NOT_PROVIDED, whatever is passed here should > still > > be consistent with GetCategoryStatus, no? > > Or does it become illegal to call GetCategoryStatus? That seems a bit > > weird/fragile... maybe GetCategoryStatus should just return NOT_PROVIDED for > > categories it doesn't know about? > > Well, the current provider implementations just DCHECK that it's their category. > So currently the assumption is that it's only called with the categories that > the provider actually provides. > > We could change that, but I think it should be consistent with the contract that > other methods follow. What would GetCategoryInfo() (coming up soon) return if > you call it for a category that the provider doesn't provide (anymore)? > Something like a "null" struct? > I think ensuring that the service doesn't call any methods on the provider for > weird categories is important. "Just unregistered" is certainly a corner case > that's not really "weird", but it falls under the same definition of categories > that the provider is not responsible for. The consistent behavior with GetCategoryInfo() is a good point. IMO it's worth pointing out more explicitly though: A comment at GetCategoryStatus saying that it must only be called for "existing" categories, and a more explicit explanation here for the NOT_PROVIDED case. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:80: // removing categories) is called on the observer. On 2016/08/01 14:59:03, Philipp Keck wrote: > On 2016/08/01 14:21:17, tschumann wrote: > > i wonder if we can get rid of this function at all (not necessarily this CL) > and > > have all date flow through the observer. > > Yes we can, though I'm not sure about how we should best design the > "registering" then. > > The method is currently used when a provider is registered and when it is > unregistered. For the unregistered, it's just used to list all the provider's > categories and unregister those. Instead, we could just list ALL the present > categories, check which ones are provided by the provider that is about to > leave, and then unregister those. The service has the list categories per provider anyway, so it doesn't need this info from the provider. (I think that's what you're saying :) ) > For the registering, we could simply "register" a provider by setting the > Observer. At some point, the provider would need to start notifying about the > categories it provides (e.g. setting all of them to INITIALIZING through > OnCategoryStatusChanged). > The question is: When is the right time to do that. The only possibility I see > is in SetObserver. So: > > void SetObserver(Observer* observer) { > if (observer == observer_) return; There can only be one observer, and it should never get overridden. So we should just DCHECK that it only changes from null to non-null or the other way around. > observer_ = observer; > if (observer_ == null) return; > > // Now call the observer for all the categories we currently have. > observer_->OnCategoryStatusChanged(ARTICLES, articles_status_); > } > > And articles_status_ would be initialized with INITIALIZING. Yup, this sounds good. Definitely a separate CL though. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:152: NotifyCategoryStatusChanged(category); On 2016/08/01 14:59:03, Philipp Keck wrote: > On 2016/08/01 14:23:37, Marc Treib wrote: > > Hm, you call this before the new suggestions are available through the > > getters... I guess the UI would only react to this if a category is removed? > > Related question: > > If a category is registered while it is INITIALIZING and then the data becomes > AVAILABLE, what's the right order to notify: > StatusChanged(AVAILABLE); > OnNewSuggestions(here comes the data); > Or the other way round? This way; the other way around is even weirder (the UI would get suggestions for a category that, as far as it knows, isn't available yet). I guess the other option would be to send both at once, i.e. OnCategoryUpdated(new_status, new_data), but I'm not sure that's better. WDYT? > It's essentially the same question/problem, but in the case where the category > was there before (so it's not about the "if" in this code here anymore). We need > to solve both cases somehow. > > I talked to Michael about this last week. Each section would wait for content > from its section until it becomes available for the first time, grab that and > then not care about further AVAILABLE-updates, only other updates. > And "AVAILABLE" with an empty set of suggestions means that the category is > fully loaded, but no content available. So the UI must hide the section, because > it can't know that there is a OnNewSuggestions call following immediately. > > We could enforce that the status of a category reported through > GetCategoryStatus() must always be AVAILABLE (or AVAILABLE_LOADING?) when > OnNewSuggestions is called. But to set that status, calling OnNewSuggestions > implies OnCategoryStatusChanged(AVAILABLE). Though I think we'd end up calling > OnCategoryStatusChanged(AVAILABLE) every time because we have no way of knowing > if it actually changed ... https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:210: bool ContentSuggestionsService::EnsureCategoryRegistered( On 2016/08/01 14:59:03, Philipp Keck wrote: > On 2016/08/01 14:21:17, tschumann wrote: > > nit: it feels weird for EnsureCategoryRegistered to return true in the first > > place; especially returning 'false' if it's laready registered. > > > > What about renaming to something like UpdateCategoryRegistry? > > or UpdateCategoryMapping()? > > Marc suggested EnsureCategoryIsRegistered. > > The logic of the return value is like the one of "put" or "remove" from maps > (Java). They always ensure the data structure is in a certain state after they > return (after "put", an element is in the map, after remove it's definitely > gone), and their return value tells you if they had to change something in order > to achieve that. > > I agree that the way this works is a bit confusing, even without the naming. > Another solution is: > EnsureCategoryRegistered calls NotifyCategoryStatusChanged directly (fair > enough, since it just registered a new category), unless you set some parameter > "notifyChange" to false, in which case it won't and the caller can do that > himself. That way, EnsureCategoryRegistered would not have a return value > anymore. I'd prefer if it either always or never notified. A name like Update...() IMO makes the return value a bit clearer: true if it was actually updated.
Ah, and please add a bug number to the description, probably 632320.
Description was changed from ========== Make ContentSuggestionsService recognize new/removed categories Change the OnNewSuggestions and OnCategoryStatusChanged methods in ContentSuggestionsService so that they recognize new categories being added and OnCategoryStatusChanged recognizes categories being removed when their status changes to NOT_PROVIDED. This allows providers to change their provided categories dynamically. Little fix: Make Category(int) constructor explicit BUG= ========== to ========== Make ContentSuggestionsService recognize new/removed categories Change the OnNewSuggestions and OnCategoryStatusChanged methods in ContentSuggestionsService so that they recognize new categories being added and OnCategoryStatusChanged recognizes categories being removed when their status changes to NOT_PROVIDED. This allows providers to change their provided categories dynamically. Little fix: Make Category(int) constructor explicit BUG=632320 ==========
https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:55: // NOT_PROVIDED). On 2016/08/01 15:17:47, Marc Treib wrote: > On 2016/08/01 14:59:03, Philipp Keck wrote: > > On 2016/08/01 14:23:37, Marc Treib wrote: > > > I don't understand this - for NOT_PROVIDED, whatever is passed here should > > still > > > be consistent with GetCategoryStatus, no? > > > Or does it become illegal to call GetCategoryStatus? That seems a bit > > > weird/fragile... maybe GetCategoryStatus should just return NOT_PROVIDED for > > > categories it doesn't know about? > > > > Well, the current provider implementations just DCHECK that it's their > category. > > So currently the assumption is that it's only called with the categories that > > the provider actually provides. > > > > We could change that, but I think it should be consistent with the contract > that > > other methods follow. What would GetCategoryInfo() (coming up soon) return if > > you call it for a category that the provider doesn't provide (anymore)? > > Something like a "null" struct? > > I think ensuring that the service doesn't call any methods on the provider for > > weird categories is important. "Just unregistered" is certainly a corner case > > that's not really "weird", but it falls under the same definition of > categories > > that the provider is not responsible for. > > The consistent behavior with GetCategoryInfo() is a good point. > IMO it's worth pointing out more explicitly though: A comment at > GetCategoryStatus saying that it must only be called for "existing" categories, > and a more explicit explanation here for the NOT_PROVIDED case. The notion of "existing" is relatively cumbersome to define, though. It's something like "After OnSuggestions of OnCategoryStatusChanged was called, but OnCategoryStatusChanged(NOT_PROVIDED) wasn't called". That's the advantage of having an explicit GetProvidedCategories() ;-) I now put some vague formulation there, but the restriction is now documented in GetCategoryStatus (and will also be for GetCategoryInfo). This problem would also be solved if GetCategoryStatus didn't exist (see other comments). https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:80: // removing categories) is called on the observer. On 2016/08/01 15:17:47, Marc Treib wrote: > On 2016/08/01 14:59:03, Philipp Keck wrote: > > On 2016/08/01 14:21:17, tschumann wrote: > > > i wonder if we can get rid of this function at all (not necessarily this CL) > > and > > > have all date flow through the observer. > > > > Yes we can, though I'm not sure about how we should best design the > > "registering" then. > > > > The method is currently used when a provider is registered and when it is > > unregistered. For the unregistered, it's just used to list all the provider's > > categories and unregister those. Instead, we could just list ALL the present > > categories, check which ones are provided by the provider that is about to > > leave, and then unregister those. > > The service has the list categories per provider anyway, so it doesn't need this > info from the provider. (I think that's what you're saying :) ) > > > For the registering, we could simply "register" a provider by setting the > > Observer. At some point, the provider would need to start notifying about the > > categories it provides (e.g. setting all of them to INITIALIZING through > > OnCategoryStatusChanged). > > The question is: When is the right time to do that. The only possibility I see > > is in SetObserver. So: > > > > void SetObserver(Observer* observer) { > > if (observer == observer_) return; > > There can only be one observer, and it should never get overridden. So we should > just DCHECK that it only changes from null to non-null or the other way around. > > > observer_ = observer; > > if (observer_ == null) return; > > > > // Now call the observer for all the categories we currently have. > > observer_->OnCategoryStatusChanged(ARTICLES, articles_status_); > > } > > > > And articles_status_ would be initialized with INITIALIZING. > > Yup, this sounds good. Definitely a separate CL though. Acknowledged. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:152: NotifyCategoryStatusChanged(category); On 2016/08/01 15:17:47, Marc Treib wrote: > On 2016/08/01 14:59:03, Philipp Keck wrote: > > On 2016/08/01 14:23:37, Marc Treib wrote: > > > Hm, you call this before the new suggestions are available through the > > > getters... I guess the UI would only react to this if a category is removed? > > > > Related question: > > > > If a category is registered while it is INITIALIZING and then the data becomes > > AVAILABLE, what's the right order to notify: > > StatusChanged(AVAILABLE); > > OnNewSuggestions(here comes the data); > > Or the other way round? > > This way; the other way around is even weirder (the UI would get suggestions for > a category that, as far as it knows, isn't available yet). > > I guess the other option would be to send both at once, i.e. > OnCategoryUpdated(new_status, new_data), but I'm not sure that's better. WDYT? > > > It's essentially the same question/problem, but in the case where the category > > was there before (so it's not about the "if" in this code here anymore). We > need > > to solve both cases somehow. > > > > I talked to Michael about this last week. Each section would wait for content > > from its section until it becomes available for the first time, grab that and > > then not care about further AVAILABLE-updates, only other updates. > > And "AVAILABLE" with an empty set of suggestions means that the category is > > fully loaded, but no content available. So the UI must hide the section, > because > > it can't know that there is a OnNewSuggestions call following immediately. > > > > We could enforce that the status of a category reported through > > GetCategoryStatus() must always be AVAILABLE (or AVAILABLE_LOADING?) when > > OnNewSuggestions is called. But to set that status, calling OnNewSuggestions > > implies OnCategoryStatusChanged(AVAILABLE). Though I think we'd end up calling > > OnCategoryStatusChanged(AVAILABLE) every time because we have no way of > knowing > > if it actually changed ... > I'm not opposed to that, we could also get rid of some duplicate code in the service. The problem I see with this: What if a provider wants to switch from "AVAILABLE" to "AVAILABLE_LOADING" (or back) without changing the available content suggestions? This would always be the case when a provider starts running an update in the background. Maybe passing a "null" collection (!= empty collection) of suggestions means "no change"? Otherwise, the method sounds good. Shorter method name, we could enforce that the list of suggestions is empty if the status is unavailable, we don't have the ordering problem for the calls, etc. And there is "GetCategoryInfo" coming. We now assume that there is no dynamic information in there and that there won't be much else other than the content and the status of a category that changes. Otherwise, we risk a huge OnCategoryUpdated method in the future. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:210: bool ContentSuggestionsService::EnsureCategoryRegistered( On 2016/08/01 15:17:47, Marc Treib wrote: > On 2016/08/01 14:59:03, Philipp Keck wrote: > > On 2016/08/01 14:21:17, tschumann wrote: > > > nit: it feels weird for EnsureCategoryRegistered to return true in the first > > > place; especially returning 'false' if it's laready registered. > > > > > > What about renaming to something like UpdateCategoryRegistry? > > > or UpdateCategoryMapping()? > > > > Marc suggested EnsureCategoryIsRegistered. > > > > The logic of the return value is like the one of "put" or "remove" from maps > > (Java). They always ensure the data structure is in a certain state after they > > return (after "put", an element is in the map, after remove it's definitely > > gone), and their return value tells you if they had to change something in > order > > to achieve that. > > > > I agree that the way this works is a bit confusing, even without the naming. > > Another solution is: > > EnsureCategoryRegistered calls NotifyCategoryStatusChanged directly (fair > > enough, since it just registered a new category), unless you set some > parameter > > "notifyChange" to false, in which case it won't and the caller can do that > > himself. That way, EnsureCategoryRegistered would not have a return value > > anymore. > > I'd prefer if it either always or never notified. > > A name like Update...() IMO makes the return value a bit clearer: true if it was > actually updated. How about "RegisterCategory[IfRequired/IfNecessary]"?
lgtm https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:80: // removing categories) is called on the observer. On 2016/08/01 15:58:11, Philipp Keck wrote: > On 2016/08/01 15:17:47, Marc Treib wrote: > > On 2016/08/01 14:59:03, Philipp Keck wrote: > > > On 2016/08/01 14:21:17, tschumann wrote: > > > > i wonder if we can get rid of this function at all (not necessarily this > CL) > > > and > > > > have all date flow through the observer. > > > > > > Yes we can, though I'm not sure about how we should best design the > > > "registering" then. > > > > > > The method is currently used when a provider is registered and when it is > > > unregistered. For the unregistered, it's just used to list all the > provider's > > > categories and unregister those. Instead, we could just list ALL the present > > > categories, check which ones are provided by the provider that is about to > > > leave, and then unregister those. > > > > The service has the list categories per provider anyway, so it doesn't need > this > > info from the provider. (I think that's what you're saying :) ) > > > > > For the registering, we could simply "register" a provider by setting the > > > Observer. At some point, the provider would need to start notifying about > the > > > categories it provides (e.g. setting all of them to INITIALIZING through > > > OnCategoryStatusChanged). > > > The question is: When is the right time to do that. The only possibility I > see > > > is in SetObserver. So: > > > > > > void SetObserver(Observer* observer) { > > > if (observer == observer_) return; > > > > There can only be one observer, and it should never get overridden. So we > should > > just DCHECK that it only changes from null to non-null or the other way > around. > > > > > observer_ = observer; > > > if (observer_ == null) return; > > > > > > // Now call the observer for all the categories we currently have. > > > observer_->OnCategoryStatusChanged(ARTICLES, articles_status_); > > > } > > > > > > And articles_status_ would be initialized with INITIALIZING. > > > > Yup, this sounds good. Definitely a separate CL though. > > Acknowledged. To follow up briefly (we can move on offline): The nice thing is that we could get rid of the double registration. We could simply hand in the service into the provider's constructor -- no SetObserver() function anymore :-) For the unregistering, why not have the provider use OnCategoryStatusChanged() to unregister each category?
https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:152: NotifyCategoryStatusChanged(category); On 2016/08/01 15:58:12, Philipp Keck wrote: > On 2016/08/01 15:17:47, Marc Treib wrote: > > On 2016/08/01 14:59:03, Philipp Keck wrote: > > > On 2016/08/01 14:23:37, Marc Treib wrote: > > > > Hm, you call this before the new suggestions are available through the > > > > getters... I guess the UI would only react to this if a category is > removed? > > > > > > Related question: > > > > > > If a category is registered while it is INITIALIZING and then the data > becomes > > > AVAILABLE, what's the right order to notify: > > > StatusChanged(AVAILABLE); > > > OnNewSuggestions(here comes the data); > > > Or the other way round? > > > > This way; the other way around is even weirder (the UI would get suggestions > for > > a category that, as far as it knows, isn't available yet). > > > > I guess the other option would be to send both at once, i.e. > > OnCategoryUpdated(new_status, new_data), but I'm not sure that's better. WDYT? > > > > > It's essentially the same question/problem, but in the case where the > category > > > was there before (so it's not about the "if" in this code here anymore). We > > need > > > to solve both cases somehow. > > > > > > I talked to Michael about this last week. Each section would wait for > content > > > from its section until it becomes available for the first time, grab that > and > > > then not care about further AVAILABLE-updates, only other updates. > > > And "AVAILABLE" with an empty set of suggestions means that the category is > > > fully loaded, but no content available. So the UI must hide the section, > > because > > > it can't know that there is a OnNewSuggestions call following immediately. > > > > > > We could enforce that the status of a category reported through > > > GetCategoryStatus() must always be AVAILABLE (or AVAILABLE_LOADING?) when > > > OnNewSuggestions is called. But to set that status, calling OnNewSuggestions > > > implies OnCategoryStatusChanged(AVAILABLE). Though I think we'd end up > calling > > > OnCategoryStatusChanged(AVAILABLE) every time because we have no way of > > knowing > > > if it actually changed ... > > > > I'm not opposed to that, we could also get rid of some duplicate code in the > service. > > The problem I see with this: > What if a provider wants to switch from "AVAILABLE" to "AVAILABLE_LOADING" (or > back) without changing the available content suggestions? This would always be > the case when a provider starts running an update in the background. > Maybe passing a "null" collection (!= empty collection) of suggestions means "no > change"? I thought it should only be in AVAILABLE_LOADING if there aren't any suggestions currently? Anyway, -1 for differentiating null vs empty, that's just confusing. > Otherwise, the method sounds good. Shorter method name, we could enforce that > the list of suggestions is empty if the status is unavailable, we don't have the > ordering problem for the calls, etc. > > And there is "GetCategoryInfo" coming. We now assume that there is no dynamic > information in there and that there won't be much else other than the content > and the status of a category that changes. Otherwise, we risk a huge > OnCategoryUpdated method in the future. That's a bridge we'll cross when (if) we get there. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:210: bool ContentSuggestionsService::EnsureCategoryRegistered( On 2016/08/01 15:58:12, Philipp Keck wrote: > On 2016/08/01 15:17:47, Marc Treib wrote: > > On 2016/08/01 14:59:03, Philipp Keck wrote: > > > On 2016/08/01 14:21:17, tschumann wrote: > > > > nit: it feels weird for EnsureCategoryRegistered to return true in the > first > > > > place; especially returning 'false' if it's laready registered. > > > > > > > > What about renaming to something like UpdateCategoryRegistry? > > > > or UpdateCategoryMapping()? > > > > > > Marc suggested EnsureCategoryIsRegistered. > > > > > > The logic of the return value is like the one of "put" or "remove" from maps > > > (Java). They always ensure the data structure is in a certain state after > they > > > return (after "put", an element is in the map, after remove it's definitely > > > gone), and their return value tells you if they had to change something in > > order > > > to achieve that. > > > > > > I agree that the way this works is a bit confusing, even without the naming. > > > Another solution is: > > > EnsureCategoryRegistered calls NotifyCategoryStatusChanged directly (fair > > > enough, since it just registered a new category), unless you set some > > parameter > > > "notifyChange" to false, in which case it won't and the caller can do that > > > himself. That way, EnsureCategoryRegistered would not have a return value > > > anymore. > > > > I'd prefer if it either always or never notified. > > > > A name like Update...() IMO makes the return value a bit clearer: true if it > was > > actually updated. > > How about "RegisterCategory[IfRequired/IfNecessary]"? Sure, that works too.
https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:55: // NOT_PROVIDED). On 2016/08/01 15:58:11, Philipp Keck wrote: > On 2016/08/01 15:17:47, Marc Treib wrote: > > On 2016/08/01 14:59:03, Philipp Keck wrote: > > > On 2016/08/01 14:23:37, Marc Treib wrote: > > > > I don't understand this - for NOT_PROVIDED, whatever is passed here should > > > still > > > > be consistent with GetCategoryStatus, no? > > > > Or does it become illegal to call GetCategoryStatus? That seems a bit > > > > weird/fragile... maybe GetCategoryStatus should just return NOT_PROVIDED > for > > > > categories it doesn't know about? > > > > > > Well, the current provider implementations just DCHECK that it's their > > category. > > > So currently the assumption is that it's only called with the categories > that > > > the provider actually provides. > > > > > > We could change that, but I think it should be consistent with the contract > > that > > > other methods follow. What would GetCategoryInfo() (coming up soon) return > if > > > you call it for a category that the provider doesn't provide (anymore)? > > > Something like a "null" struct? > > > I think ensuring that the service doesn't call any methods on the provider > for > > > weird categories is important. "Just unregistered" is certainly a corner > case > > > that's not really "weird", but it falls under the same definition of > > categories > > > that the provider is not responsible for. > > > > The consistent behavior with GetCategoryInfo() is a good point. > > IMO it's worth pointing out more explicitly though: A comment at > > GetCategoryStatus saying that it must only be called for "existing" > categories, > > and a more explicit explanation here for the NOT_PROVIDED case. > > The notion of "existing" is relatively cumbersome to define, though. It's > something like "After OnSuggestions of OnCategoryStatusChanged was called, but > OnCategoryStatusChanged(NOT_PROVIDED) wasn't called". That's the advantage of > having an explicit GetProvidedCategories() ;-) > > I now put some vague formulation there, but the restriction is now documented in > GetCategoryStatus (and will also be for GetCategoryInfo). We should work towards a sound model, where the observer methods are enough to communicate a consistent state. Maybe it's best to discuss that with the mentioned GetCategoryInfo() (do you have a CL I could have a look at?). It feels weird to have an incremental protocol which still relies on querying the whole model. but like I said, I'm fine with this CL as is. > > This problem would also be solved if GetCategoryStatus didn't exist (see other > comments).
Renamed the method. So shall I still land this CL and do the refactoring later? Maybe even along with https://bugs.chromium.org/p/chromium/issues/detail?id=633139 ? https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:55: // NOT_PROVIDED). On 2016/08/01 16:30:12, tschumann wrote: > On 2016/08/01 15:58:11, Philipp Keck wrote: > > On 2016/08/01 15:17:47, Marc Treib wrote: > > > On 2016/08/01 14:59:03, Philipp Keck wrote: > > > > On 2016/08/01 14:23:37, Marc Treib wrote: > > > > > I don't understand this - for NOT_PROVIDED, whatever is passed here > should > > > > still > > > > > be consistent with GetCategoryStatus, no? > > > > > Or does it become illegal to call GetCategoryStatus? That seems a bit > > > > > weird/fragile... maybe GetCategoryStatus should just return NOT_PROVIDED > > for > > > > > categories it doesn't know about? > > > > > > > > Well, the current provider implementations just DCHECK that it's their > > > category. > > > > So currently the assumption is that it's only called with the categories > > that > > > > the provider actually provides. > > > > > > > > We could change that, but I think it should be consistent with the > contract > > > that > > > > other methods follow. What would GetCategoryInfo() (coming up soon) return > > if > > > > you call it for a category that the provider doesn't provide (anymore)? > > > > Something like a "null" struct? > > > > I think ensuring that the service doesn't call any methods on the provider > > for > > > > weird categories is important. "Just unregistered" is certainly a corner > > case > > > > that's not really "weird", but it falls under the same definition of > > > categories > > > > that the provider is not responsible for. > > > > > > The consistent behavior with GetCategoryInfo() is a good point. > > > IMO it's worth pointing out more explicitly though: A comment at > > > GetCategoryStatus saying that it must only be called for "existing" > > categories, > > > and a more explicit explanation here for the NOT_PROVIDED case. > > > > The notion of "existing" is relatively cumbersome to define, though. It's > > something like "After OnSuggestions of OnCategoryStatusChanged was called, but > > OnCategoryStatusChanged(NOT_PROVIDED) wasn't called". That's the advantage of > > having an explicit GetProvidedCategories() ;-) > > > > I now put some vague formulation there, but the restriction is now documented > in > > GetCategoryStatus (and will also be for GetCategoryInfo). > > We should work towards a sound model, where the observer methods are enough to > communicate a consistent state. Maybe it's best to discuss that with the > mentioned GetCategoryInfo() (do you have a CL I could have a look at?). > > It feels weird to have an incremental protocol which still relies on querying > the whole model. but like I said, I'm fine with this CL as is. > > > > This problem would also be solved if GetCategoryStatus didn't exist (see other > > comments). > Acknowledged. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:80: // removing categories) is called on the observer. On 2016/08/01 16:24:08, tschumann wrote: > On 2016/08/01 15:58:11, Philipp Keck wrote: > > On 2016/08/01 15:17:47, Marc Treib wrote: > > > On 2016/08/01 14:59:03, Philipp Keck wrote: > > > > On 2016/08/01 14:21:17, tschumann wrote: > > > > > i wonder if we can get rid of this function at all (not necessarily this > > CL) > > > > and > > > > > have all date flow through the observer. > > > > > > > > Yes we can, though I'm not sure about how we should best design the > > > > "registering" then. > > > > > > > > The method is currently used when a provider is registered and when it is > > > > unregistered. For the unregistered, it's just used to list all the > > provider's > > > > categories and unregister those. Instead, we could just list ALL the > present > > > > categories, check which ones are provided by the provider that is about to > > > > leave, and then unregister those. > > > > > > The service has the list categories per provider anyway, so it doesn't need > > this > > > info from the provider. (I think that's what you're saying :) ) > > > > > > > For the registering, we could simply "register" a provider by setting the > > > > Observer. At some point, the provider would need to start notifying about > > the > > > > categories it provides (e.g. setting all of them to INITIALIZING through > > > > OnCategoryStatusChanged). > > > > The question is: When is the right time to do that. The only possibility I > > see > > > > is in SetObserver. So: > > > > > > > > void SetObserver(Observer* observer) { > > > > if (observer == observer_) return; > > > > > > There can only be one observer, and it should never get overridden. So we > > should > > > just DCHECK that it only changes from null to non-null or the other way > > around. > > > > > > > observer_ = observer; > > > > if (observer_ == null) return; > > > > > > > > // Now call the observer for all the categories we currently have. > > > > observer_->OnCategoryStatusChanged(ARTICLES, articles_status_); > > > > } > > > > > > > > And articles_status_ would be initialized with INITIALIZING. > > > > > > Yup, this sounds good. Definitely a separate CL though. > > > > Acknowledged. > > To follow up briefly (we can move on offline): > The nice thing is that we could get rid of the double registration. We could > simply hand in the service into the provider's constructor -- no SetObserver() > function anymore :-) Acknowledged. > For the unregistering, why not have the provider use OnCategoryStatusChanged() > to unregister each category? Actually the way we handle this might change because we will make providers fully owned by the service, as discussed on Friday (here's the bug for that btw. https://bugs.chromium.org/p/chromium/issues/detail?id=633139). Since the service owns all the providers, they don't have an independent lifecycle, so we might just not have the Unregister or ProviderShutdown method. We could simply not allow providers to shut down. If their category is gone, they can set it to some "NOT_AVAILABLE" status, maybe we need to introduce a new one for that, or they can unregister the whole category with "NOT_PROVIDED", but we could still keep the provider registered (with "Observer" or a direct service reference). So the service would not only keep a category->provider map, but also a simple "providers" list of all registered providers -- because a provider can be registered, but not have any categories (yet). For shutdown the service just calls all the registered providers and tells them to shut down, too. No fine-grained provider unregistering. Let's talk about this tomorrow. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:152: NotifyCategoryStatusChanged(category); On 2016/08/01 16:28:51, Marc Treib wrote: > On 2016/08/01 15:58:12, Philipp Keck wrote: > > On 2016/08/01 15:17:47, Marc Treib wrote: > > > On 2016/08/01 14:59:03, Philipp Keck wrote: > > > > On 2016/08/01 14:23:37, Marc Treib wrote: > > > > > Hm, you call this before the new suggestions are available through the > > > > > getters... I guess the UI would only react to this if a category is > > removed? > > > > > > > > Related question: > > > > > > > > If a category is registered while it is INITIALIZING and then the data > > becomes > > > > AVAILABLE, what's the right order to notify: > > > > StatusChanged(AVAILABLE); > > > > OnNewSuggestions(here comes the data); > > > > Or the other way round? > > > > > > This way; the other way around is even weirder (the UI would get suggestions > > for > > > a category that, as far as it knows, isn't available yet). > > > > > > I guess the other option would be to send both at once, i.e. > > > OnCategoryUpdated(new_status, new_data), but I'm not sure that's better. > WDYT? > > > > > > > It's essentially the same question/problem, but in the case where the > > category > > > > was there before (so it's not about the "if" in this code here anymore). > We > > > need > > > > to solve both cases somehow. > > > > > > > > I talked to Michael about this last week. Each section would wait for > > content > > > > from its section until it becomes available for the first time, grab that > > and > > > > then not care about further AVAILABLE-updates, only other updates. > > > > And "AVAILABLE" with an empty set of suggestions means that the category > is > > > > fully loaded, but no content available. So the UI must hide the section, > > > because > > > > it can't know that there is a OnNewSuggestions call following immediately. > > > > > > > > We could enforce that the status of a category reported through > > > > GetCategoryStatus() must always be AVAILABLE (or AVAILABLE_LOADING?) when > > > > OnNewSuggestions is called. But to set that status, calling > OnNewSuggestions > > > > implies OnCategoryStatusChanged(AVAILABLE). Though I think we'd end up > > calling > > > > OnCategoryStatusChanged(AVAILABLE) every time because we have no way of > > > knowing > > > > if it actually changed ... > > > > > > > I'm not opposed to that, we could also get rid of some duplicate code in the > > service. > > > > The problem I see with this: > > What if a provider wants to switch from "AVAILABLE" to "AVAILABLE_LOADING" (or > > back) without changing the available content suggestions? This would always be > > the case when a provider starts running an update in the background. > > Maybe passing a "null" collection (!= empty collection) of suggestions means > "no > > change"? > > I thought it should only be in AVAILABLE_LOADING if there aren't any suggestions > currently? > > Anyway, -1 for differentiating null vs empty, that's just confusing. > > > Otherwise, the method sounds good. Shorter method name, we could enforce that > > the list of suggestions is empty if the status is unavailable, we don't have > the > > ordering problem for the calls, etc. > > > > And there is "GetCategoryInfo" coming. We now assume that there is no dynamic > > information in there and that there won't be much else other than the content > > and the status of a category that changes. Otherwise, we risk a huge > > OnCategoryUpdated method in the future. > > That's a bridge we'll cross when (if) we get there. Acknowledged. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_service.cc:210: bool ContentSuggestionsService::EnsureCategoryRegistered( On 2016/08/01 16:28:51, Marc Treib wrote: > On 2016/08/01 15:58:12, Philipp Keck wrote: > > On 2016/08/01 15:17:47, Marc Treib wrote: > > > On 2016/08/01 14:59:03, Philipp Keck wrote: > > > > On 2016/08/01 14:21:17, tschumann wrote: > > > > > nit: it feels weird for EnsureCategoryRegistered to return true in the > > first > > > > > place; especially returning 'false' if it's laready registered. > > > > > > > > > > What about renaming to something like UpdateCategoryRegistry? > > > > > or UpdateCategoryMapping()? > > > > > > > > Marc suggested EnsureCategoryIsRegistered. > > > > > > > > The logic of the return value is like the one of "put" or "remove" from > maps > > > > (Java). They always ensure the data structure is in a certain state after > > they > > > > return (after "put", an element is in the map, after remove it's > definitely > > > > gone), and their return value tells you if they had to change something in > > > order > > > > to achieve that. > > > > > > > > I agree that the way this works is a bit confusing, even without the > naming. > > > > Another solution is: > > > > EnsureCategoryRegistered calls NotifyCategoryStatusChanged directly (fair > > > > enough, since it just registered a new category), unless you set some > > > parameter > > > > "notifyChange" to false, in which case it won't and the caller can do that > > > > himself. That way, EnsureCategoryRegistered would not have a return value > > > > anymore. > > > > > > I'd prefer if it either always or never notified. > > > > > > A name like Update...() IMO makes the return value a bit clearer: true if it > > was > > > actually updated. > > > > How about "RegisterCategory[IfRequired/IfNecessary]"? > > Sure, that works too. Done.
lgtm lgtm On 2016/08/01 17:04:58, Philipp Keck wrote: > Renamed the method. > > So shall I still land this CL and do the refactoring later? Maybe even along > with https://bugs.chromium.org/p/chromium/issues/detail?id=633139 ? Sounds good to me. > > https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... > File components/ntp_snippets/content_suggestions_provider.h (right): > >
LGTM with a few more nits - I'm also fine with landing this as-is and doing the refactorings in follow-up CLs. https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/40001/components/ntp_snippets... components/ntp_snippets/content_suggestions_provider.h:80: // removing categories) is called on the observer. On 2016/08/01 17:04:57, Philipp Keck wrote: > On 2016/08/01 16:24:08, tschumann wrote: > > On 2016/08/01 15:58:11, Philipp Keck wrote: > > > On 2016/08/01 15:17:47, Marc Treib wrote: > > > > On 2016/08/01 14:59:03, Philipp Keck wrote: > > > > > On 2016/08/01 14:21:17, tschumann wrote: > > > > > > i wonder if we can get rid of this function at all (not necessarily > this > > > CL) > > > > > and > > > > > > have all date flow through the observer. > > > > > > > > > > Yes we can, though I'm not sure about how we should best design the > > > > > "registering" then. > > > > > > > > > > The method is currently used when a provider is registered and when it > is > > > > > unregistered. For the unregistered, it's just used to list all the > > > provider's > > > > > categories and unregister those. Instead, we could just list ALL the > > present > > > > > categories, check which ones are provided by the provider that is about > to > > > > > leave, and then unregister those. > > > > > > > > The service has the list categories per provider anyway, so it doesn't > need > > > this > > > > info from the provider. (I think that's what you're saying :) ) > > > > > > > > > For the registering, we could simply "register" a provider by setting > the > > > > > Observer. At some point, the provider would need to start notifying > about > > > the > > > > > categories it provides (e.g. setting all of them to INITIALIZING through > > > > > OnCategoryStatusChanged). > > > > > The question is: When is the right time to do that. The only possibility > I > > > see > > > > > is in SetObserver. So: > > > > > > > > > > void SetObserver(Observer* observer) { > > > > > if (observer == observer_) return; > > > > > > > > There can only be one observer, and it should never get overridden. So we > > > should > > > > just DCHECK that it only changes from null to non-null or the other way > > > around. > > > > > > > > > observer_ = observer; > > > > > if (observer_ == null) return; > > > > > > > > > > // Now call the observer for all the categories we currently have. > > > > > observer_->OnCategoryStatusChanged(ARTICLES, articles_status_); > > > > > } > > > > > > > > > > And articles_status_ would be initialized with INITIALIZING. > > > > > > > > Yup, this sounds good. Definitely a separate CL though. > > > > > > Acknowledged. > > > > To follow up briefly (we can move on offline): > > The nice thing is that we could get rid of the double registration. We could > > simply hand in the service into the provider's constructor -- no SetObserver() > > function anymore :-) > > Acknowledged. > > > For the unregistering, why not have the provider use OnCategoryStatusChanged() > > to unregister each category? > > Actually the way we handle this might change because we will make providers > fully owned by the service, as discussed on Friday (here's the bug for that btw. > https://bugs.chromium.org/p/chromium/issues/detail?id=633139). > Since the service owns all the providers, they don't have an independent > lifecycle, so we might just not have the Unregister or ProviderShutdown method. Yay! > We could simply not allow providers to shut down. If their category is gone, > they can set it to some "NOT_AVAILABLE" status, maybe we need to introduce a new > one for that, or they can unregister the whole category with "NOT_PROVIDED", but > we could still keep the provider registered (with "Observer" or a direct service > reference). NOT_PROVIDED seems to be just the thing for this? I'd prefer not to introduce yet another ever-so-subtly different status. > So the service would not only keep a category->provider map, but also a simple > "providers" list of all registered providers -- because a provider can be > registered, but not have any categories (yet). For shutdown the service just > calls all the registered providers and tells them to shut down, too. No > fine-grained provider unregistering. I think it'd just delete all the providers. > Let's talk about this tomorrow. https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:43: // known yet, the calling |provider| will be registered as its provider. Wait, can this actually happen? Wouldn't OnCategoryStatusChanged always be called first? https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:214: auto entry = providers_by_category_.find(category); nit: call this "it"? That's the common convention for iterators https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:227: suggestions_by_category_[category] = std::vector<ContentSuggestion>(); nit: "suggestions_by_category_[category]" will create a new empty vector, which you then promptly overwrite with another empty vector :) Maybe use .insert(...) instead? https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:141: Category category); misaligned
https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_provider.h (right): https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_provider.h:43: // known yet, the calling |provider| will be registered as its provider. On 2016/08/01 17:30:35, Marc Treib wrote: > Wait, can this actually happen? Wouldn't OnCategoryStatusChanged always be > called first? We currently don't require it. A provider could call OnNewSuggestions for a new category and the service would ask GetCategoryStatus for the status, so no need for the changed event. It works the way it is, but we could also require that OnCategoryStatusChanged needs to be called first. That's another issue which disappears once we only have one method for both.
https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.cc (right): https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:214: auto entry = providers_by_category_.find(category); On 2016/08/01 17:30:35, Marc Treib wrote: > nit: call this "it"? That's the common convention for iterators Done. https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.cc:227: suggestions_by_category_[category] = std::vector<ContentSuggestion>(); On 2016/08/01 17:30:35, Marc Treib wrote: > nit: "suggestions_by_category_[category]" will create a new empty vector, which > you then promptly overwrite with another empty vector :) > Maybe use .insert(...) instead? Done. https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... File components/ntp_snippets/content_suggestions_service.h (right): https://codereview.chromium.org/2194203002/diff/120001/components/ntp_snippet... components/ntp_snippets/content_suggestions_service.h:141: Category category); On 2016/08/01 17:30:35, Marc Treib wrote: > misaligned Done.
The CQ bit was checked by pke@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, tschumann@chromium.org Link to the patchset: https://codereview.chromium.org/2194203002/#ps140001 (title: "Marc's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Make ContentSuggestionsService recognize new/removed categories Change the OnNewSuggestions and OnCategoryStatusChanged methods in ContentSuggestionsService so that they recognize new categories being added and OnCategoryStatusChanged recognizes categories being removed when their status changes to NOT_PROVIDED. This allows providers to change their provided categories dynamically. Little fix: Make Category(int) constructor explicit BUG=632320 ========== to ========== Make ContentSuggestionsService recognize new/removed categories Change the OnNewSuggestions and OnCategoryStatusChanged methods in ContentSuggestionsService so that they recognize new categories being added and OnCategoryStatusChanged recognizes categories being removed when their status changes to NOT_PROVIDED. This allows providers to change their provided categories dynamically. Little fix: Make Category(int) constructor explicit BUG=632320 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Make ContentSuggestionsService recognize new/removed categories Change the OnNewSuggestions and OnCategoryStatusChanged methods in ContentSuggestionsService so that they recognize new categories being added and OnCategoryStatusChanged recognizes categories being removed when their status changes to NOT_PROVIDED. This allows providers to change their provided categories dynamically. Little fix: Make Category(int) constructor explicit BUG=632320 ========== to ========== Make ContentSuggestionsService recognize new/removed categories Change the OnNewSuggestions and OnCategoryStatusChanged methods in ContentSuggestionsService so that they recognize new categories being added and OnCategoryStatusChanged recognizes categories being removed when their status changes to NOT_PROVIDED. This allows providers to change their provided categories dynamically. Little fix: Make Category(int) constructor explicit BUG=632320 Committed: https://crrev.com/4d3a4d66ed9290eec918786cd7457ef1bf46e0dc Cr-Commit-Position: refs/heads/master@{#409160} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4d3a4d66ed9290eec918786cd7457ef1bf46e0dc Cr-Commit-Position: refs/heads/master@{#409160} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
