Add CategoryInfo for meta information of content suggestions Categories

components/ntp_snippets/content_suggestions_service.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/ntp_snippets/content_suggestions_service.h"
 
 #include <algorithm>
 #include <iterator>
 
 #include "base/bind.h"
@@ skipping 30 matching lines @@
     return CategoryStatus::ALL_SUGGESTIONS_EXPLICITLY_DISABLED;
   }
 
   auto iterator = providers_by_category_.find(category);
   if (iterator == providers_by_category_.end())
     return CategoryStatus::NOT_PROVIDED;
 
   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);
   if (iterator == suggestions_by_category_.end())
     return no_suggestions_;
   return iterator->second;
 }
 
 void ContentSuggestionsService::FetchSuggestionImage(
     const std::string& suggestion_id,
@@ skipping 151 matching lines @@
   return true;
 }
 
 void ContentSuggestionsService::NotifyCategoryStatusChanged(Category category) {
   FOR_EACH_OBSERVER(
       Observer, observers_,
       OnCategoryStatusChanged(category, GetCategoryStatus(category)));
 }
 
 }  // namespace ntp_snippets