Make ContentSuggestionsService recognize new/removed categories

components/ntp_snippets/content_suggestions_provider.h

Index: components/ntp_snippets/content_suggestions_provider.h
diff --git a/components/ntp_snippets/content_suggestions_provider.h b/components/ntp_snippets/content_suggestions_provider.h
index 3a80b68dd4d66cb1240b7ee0ffba19c18efc8b24..8957dc776d42ae4335a26adec916cb9e52a5b14d 100644
--- a/components/ntp_snippets/content_suggestions_provider.h
+++ b/components/ntp_snippets/content_suggestions_provider.h
@@ -39,20 +39,29 @@ class ContentSuggestionsProvider {
     // the full list of currently available suggestions for that category, i.e.,
     // an empty list will remove all suggestions from the given category. Note
     // that to clear them from the UI immediately, the provider needs to change
-    // the status of the respective category.
+    // the status of the respective category. This may be called with |category|
+    // values that are not yet provided by any provider to create a new
+    // category.

[Marc Treib, 2016/08/01 14:23:37]

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?

[Philipp Keck, 2016/08/01 14:59:03]

Done. Changed here and below.

     // IDs for the ContentSuggestions should be generated with
     // |MakeUniqueID(..)| below.
     virtual void OnNewSuggestions(
-        Category changed_category,
+        ContentSuggestionsProvider* provider,
+        Category category,
         std::vector<ContentSuggestion> suggestions) = 0;
 
     // Called when the status of a category changed.
     // |new_status| must be the value that is currently returned from the
-    // provider's |GetCategoryStatus(category)| below.
+    // provider's |GetCategoryStatus(category)| below (unless it is
+    // NOT_PROVIDED).

[Marc Treib, 2016/08/01 14:23:37]

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?

[Philipp Keck, 2016/08/01 14:59:03]

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.

[Marc Treib, 2016/08/01 15:17:47]

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.

[Philipp Keck, 2016/08/01 15:58:11]

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).

[tschumann, 2016/08/01 16:30:12]

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.

[Philipp Keck, 2016/08/01 17:04:57]

Acknowledged.

+    // This may be called with |category| values that are not yet provided by
+    // any provider to create a new category.
     // Whenever the status changes to an unavailable status, all suggestions in
     // that category must immediately be removed from all caches and from the
-    // UI.
-    virtual void OnCategoryStatusChanged(Category changed_category,
+    // UI, but the provider remains registered. When the status changes to
+    // NOT_PROVIDED, the category is unregistered without clearing the UI and
+    // must also be removed from |GetProvidedCategories()|.
+    virtual void OnCategoryStatusChanged(ContentSuggestionsProvider* provider,
+                                         Category category,
                                          CategoryStatus new_status) = 0;
 
     // Called when the provider needs to shut down and will not deliver any
@@ -66,8 +75,9 @@ class ContentSuggestionsProvider {
   virtual void SetObserver(Observer* observer) = 0;
 
   // Returns the categories provided by this provider.
-  // TODO(pke): "The value returned by this getter must not change unless
-  // OnXxx is called on the observer."
+  // The value returned by this getter must not change unless OnNewSuggestions

[Marc Treib, 2016/08/01 14:23:37]

I'd formulate this as something like: When the set of provided categories
changes, ... gets called on the observer.

[Philipp Keck, 2016/08/01 14:59:03]

Done.

+  // (for adding categories) or OnCategoryStatusChanged (with NOT_PROVIDED for
+  // removing categories) is called on the observer.

[tschumann, 2016/08/01 14:21:17]

i wonder if we can get rid of this function at all (not necessarily this CL) and
have all date flow through the observer.

[Philipp Keck, 2016/08/01 14:59:03]

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.

[Marc Treib, 2016/08/01 15:17:47]

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 :) )
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.
Yup, this sounds good. Definitely a separate CL though.

[Philipp Keck, 2016/08/01 15:58:11]

Acknowledged.

[tschumann, 2016/08/01 16:24:08]

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?

[Philipp Keck, 2016/08/01 17:04:57]

Acknowledged.
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.

[Marc Treib, 2016/08/01 17:30:35]

Yay!
NOT_PROVIDED seems to be just the thing for this? I'd prefer not to introduce
yet another ever-so-subtly different status.
I think it'd just delete all the providers.

   virtual std::vector<Category> GetProvidedCategories() = 0;
 
   // Determines the status of the given |category|, see CategoryStatus.