Add CategoryInfo for meta information of content suggestions 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 1b9ecb839cb7d4cedb90f40dc88d6be2ce605fd9..3e52d1311bcece1757be9d202ca6d40c0d57c69a 100644
--- a/components/ntp_snippets/content_suggestions_service.cc
+++ b/components/ntp_snippets/content_suggestions_service.cc
@@ -48,6 +48,15 @@ CategoryStatus ContentSuggestionsService::GetCategoryStatus(
   return iterator->second->GetCategoryStatus(category);
 }
 
+CategoryInfo ContentSuggestionsService::GetCategoryInfo(
+    Category category) const {
+  auto iterator = providers_by_category_.find(category);
+  if (iterator == providers_by_category_.end())
+    return CategoryInfo(base::string16());

[tschumann, 2016/08/04 08:59:39]

it might be better to explicitly signal the lookup-miss. That way the caller can
decide how to handle this case.
Change signature to 
bool GetCategoryInfo(Category category, CategoryInfo* info);
?

[Marc Treib, 2016/08/04 09:46:30]

Explicitly signaling the failure might be good, but I'd prefer returning a
base::Optional<CategoryInfo> - out params are ugly :)

[Philipp Keck, 2016/08/04 11:48:18]

base::Optional sounds good. We could so something similar for
GetSuggestionsForCategory.
It would make the subtle difference between "empty category" and "no category"
more explicit. Just as with GetCategoryInfo(), the caller can distinguish the
cases using GetCategoryStatus.

I think we would guarantee that the CategoryInfo is available when the status is
anything but NOT_PROVIDED (that requirement would also go onto
ContentSuggestionsProvider::GetCategoryInfo). And we could guarantee that the
ContentSuggestions are available when the status is one of the AVAILABLE
statuses. The caller (the snippets bridge) would then do sth like this:
if (IsAvailable(GetCategoryStatus(category))) {
  DoSomethingWith(GetCategoryInfo(category).value());
}
So it would call .value() directly, because the status implies that it's
available. Otherwise, a DCHECK fails in .value().

[tschumann, 2016/08/04 11:51:24]

While that's possible, it would be nice to not rely on this in code. IMO it's
just bad practice to not verify the Optional before accessing it's value and
it's also not a big inconvenience. You could also use the value accessor which
takes a default value.

[Philipp Keck, 2016/08/08 14:50:18]

Done.

+
+  return iterator->second->GetCategoryInfo(category);
+}
+
 const std::vector<ContentSuggestion>&
 ContentSuggestionsService::GetSuggestionsForCategory(Category category) const {
   auto iterator = suggestions_by_category_.find(category);