Make ContentSuggestionsService recognize new/removed categories

components/ntp_snippets/content_suggestions_service.cc

Index: components/ntp_snippets/content_suggestions_service.cc
diff --git a/components/ntp_snippets/content_suggestions_service.cc b/components/ntp_snippets/content_suggestions_service.cc
index 934928607776f56543562bc3d330f4c9054d0a5c..86a859f648d07e9a0cad21ba64645502c02c858e 100644
--- a/components/ntp_snippets/content_suggestions_service.cc
+++ b/components/ntp_snippets/content_suggestions_service.cc
@@ -132,55 +132,63 @@ void ContentSuggestionsService::RegisterProvider(
   if (state_ == State::DISABLED)
     return;
 
+  provider->SetObserver(this);
   for (Category category : provider->GetProvidedCategories()) {
-    DCHECK_EQ(0ul, providers_.count(category));
-    providers_[category] = provider;
-    categories_.push_back(category);
-    if (IsCategoryStatusAvailable(provider->GetCategoryStatus(category))) {
-      suggestions_by_category_[category] = std::vector<ContentSuggestion>();
-    }
+    DCHECK_NE(CategoryStatus::NOT_PROVIDED,
+              provider->GetCategoryStatus(category));
+    EnsureCategoryRegistered(provider, category);
     NotifyCategoryStatusChanged(category);
   }
-  std::sort(categories_.begin(), categories_.end(),
-            [this](const Category& left, const Category& right) {
-              return category_factory_.CompareCategories(left, right);
-            });
-  provider->SetObserver(this);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Private methods
 
 void ContentSuggestionsService::OnNewSuggestions(
-    Category changed_category,
+    ContentSuggestionsProvider* provider,
+    Category category,
     std::vector<ContentSuggestion> new_suggestions) {
-  DCHECK(IsCategoryRegistered(changed_category));
+  if (EnsureCategoryRegistered(provider, category)) {
+    NotifyCategoryStatusChanged(category);

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

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?

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

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

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

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?

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

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.

[Marc Treib, 2016/08/01 16:28:51]

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.
That's a bridge we'll cross when (if) we get there.

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

Acknowledged.

+  }
 
   for (const ContentSuggestion& suggestion :
-       suggestions_by_category_[changed_category]) {
+       suggestions_by_category_[category]) {
     id_category_map_.erase(suggestion.id());
   }
 
   for (const ContentSuggestion& suggestion : new_suggestions) {
-    id_category_map_.insert(std::make_pair(suggestion.id(), changed_category));
+    id_category_map_.insert(std::make_pair(suggestion.id(), category));
   }
 
-  suggestions_by_category_[changed_category] = std::move(new_suggestions);
+  suggestions_by_category_[category] = std::move(new_suggestions);
 
   FOR_EACH_OBSERVER(Observer, observers_, OnNewSuggestions());
 }
 
 void ContentSuggestionsService::OnCategoryStatusChanged(
-    Category changed_category,
+    ContentSuggestionsProvider* provider,
+    Category category,
     CategoryStatus new_status) {
   if (!IsCategoryStatusAvailable(new_status)) {
     for (const ContentSuggestion& suggestion :
-         suggestions_by_category_[changed_category]) {
+         suggestions_by_category_[category]) {
       id_category_map_.erase(suggestion.id());
     }
-    suggestions_by_category_.erase(changed_category);
+    suggestions_by_category_.erase(category);
+  }
+  if (new_status == CategoryStatus::NOT_PROVIDED) {
+    auto providers_entry = providers_.find(category);

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

nit: the usual naming would be 'providers_it'.

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

Done.

+    DCHECK(providers_entry != providers_.end());
+    DCHECK_EQ(provider, providers_entry->second);
+    providers_.erase(providers_entry);

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

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.

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

Done.

+    categories_.erase(
+        std::find(categories_.begin(), categories_.end(), category));
+  } else {
+    EnsureCategoryRegistered(provider, category);
+    DCHECK_EQ(new_status, provider->GetCategoryStatus(category));
   }
-  NotifyCategoryStatusChanged(changed_category);
+  NotifyCategoryStatusChanged(category);
 }
 
 void ContentSuggestionsService::OnProviderShutdown(
@@ -199,9 +207,25 @@ void ContentSuggestionsService::OnProviderShutdown(
   }
 }
 
-bool ContentSuggestionsService::IsCategoryRegistered(Category category) const {
-  return std::find(categories_.begin(), categories_.end(), category) !=
-         categories_.end();
+bool ContentSuggestionsService::EnsureCategoryRegistered(

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

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()?

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

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.

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

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.

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

How about "RegisterCategory[IfRequired/IfNecessary]"?

[Marc Treib, 2016/08/01 16:28:51]

Sure, that works too.

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

Done.

+    ContentSuggestionsProvider* provider,
+    Category category) {
+  auto entry = providers_.find(category);
+  if (entry != providers_.end()) {
+    DCHECK_EQ(entry->second, provider);
+    return false;
+  }
+
+  providers_[category] = provider;
+  categories_.push_back(category);
+  std::sort(categories_.begin(), categories_.end(),
+            [this](const Category& left, const Category& right) {
+              return category_factory_.CompareCategories(left, right);
+            });
+  if (IsCategoryStatusAvailable(provider->GetCategoryStatus(category))) {
+    suggestions_by_category_[category] = std::vector<ContentSuggestion>();
+  }
+  return true;
 }
 
 void ContentSuggestionsService::NotifyCategoryStatusChanged(Category category) {