Add CategoryInfo for meta information of content suggestions Categories

components/ntp_snippets/content_suggestions_service.h

Index: components/ntp_snippets/content_suggestions_service.h
diff --git a/components/ntp_snippets/content_suggestions_service.h b/components/ntp_snippets/content_suggestions_service.h
index 488aeab7388c3e1219a5eca831d327b0b2f6d5ce..7f771b5f4130be52b298381096fe0155c603981e 100644
--- a/components/ntp_snippets/content_suggestions_service.h
+++ b/components/ntp_snippets/content_suggestions_service.h
@@ -12,6 +12,7 @@
 
 #include "base/callback_forward.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/ntp_snippets/category_factory.h"
 #include "components/ntp_snippets/category_status.h"
@@ -78,6 +79,9 @@ class ContentSuggestionsService : public KeyedService,
   // Gets the status of a category.
   CategoryStatus GetCategoryStatus(Category category) const;
 
+  // Gets the meta information of a category.
+  base::Optional<CategoryInfo> GetCategoryInfo(Category category) const;

[Bernhard Bauer, 2016/08/09 09:18:33]

Hum. You go to all these lengths to implement a move constructor for
CategoryInfo (presumably because copying it expensive), and then you return an
Optional here, which makes a copy of the value. Why not just return a pointer?
Really, that *is* the common idiom in Chromium C++ for returning optional "big"
values.

[Marc Treib, 2016/08/09 09:45:12]

CategoryInfo doesn't have a copy ctor, so it can't make a copy :)

Anyway, I'd argue "pointer" is the common idiom only because Optional hasn't
been around for long. It's certainly the clearer thing.

[Philipp Keck, 2016/08/09 10:03:06]

Indeed, the only relevant call we have
(https://codereview.chromium.org/2207493002/diff/120001/components/ntp_snippet...)
results in a call to the move constructor. I agree that base::Optional is more
readable, plus it works as intended.

[Bernhard Bauer, 2016/08/09 10:25:41]

If you are saying that over time Optional<> will replace pointers for passing
optional non-copyable things around, I'm fairly certain that will not be the
case. To me, Optional<T> signifies optional _values_, and T* signifies optional
(non-copyable) objects.
Ah right. It still feels a bit strange to construct a new string every time we
call GetCategoryInfo, even if it's not copied around...
Meh. It's very explicit about the fact that the lookup might fail, but at the
only current callsite we are iterating over the categories, so we should never
call it with a missing category. Really, this smacks a bit of handling
conditions that are not supposed to happen, which the style guide explicitly
argues against.

[Philipp Keck, 2016/08/09 11:24:27]

The callsites will change soon:
https://codereview.chromium.org/2230473002/diff/1/chrome/browser/android/ntp/...
https://codereview.chromium.org/2230473002/diff/1/chrome/android/java/src/org...
That could be called at any time, though currently it is still called right
after getCategories(), I think.

That being said, I'm also fine with using a pointer (and nullptr indicating
unavailability). That's also how it's treated behind the bridge: null means not
available.

+
   // Gets the available suggestions for a category. The result is empty if the
   // category is available and empty, but also if the category is unavailable
   // for any reason, see |GetCategoryStatus()|.