Add ContentSuggestionsCategoryFactory; Store categories as ints

components/ntp_snippets/content_suggestions_category.h

Index: components/ntp_snippets/content_suggestions_category.h
diff --git a/components/ntp_snippets/content_suggestions_category.h b/components/ntp_snippets/content_suggestions_category.h
index 70bb23d06fedc97de38433c46551ea6fc347af2c..f49af2938a5736060f0b55cc75e98e16d3f74ccb 100644
--- a/components/ntp_snippets/content_suggestions_category.h
+++ b/components/ntp_snippets/content_suggestions_category.h
@@ -5,10 +5,69 @@
 #ifndef COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_CATEGORY_H_
 #define COMPONENTS_NTP_SNIPPETS_CONTENT_SUGGESTIONS_CATEGORY_H_
 
+#include <ostream>
+
 namespace ntp_snippets {
 
-// A category groups ContentSuggestions which belong together.
-enum class ContentSuggestionsCategory { ARTICLES, OFFLINE_PAGES, COUNT };
+class ContentSuggestionsCategoryFactory;
+
+// These are the categories that the client knows about.
+// The values before LOCAL_CATEGORIES_COUNT are the categories that are provided
+// locally on the device. Categories provided by the server only need to be
+// hard-coded here if they need to be recognized by the client implementation.
+enum class KnownSuggestionsCategories {
+  OFFLINE_PAGES,
+  LOCAL_CATEGORIES_COUNT,
+
+  REMOTE_CATEGORIES_OFFSET = 10000,

[Marc Treib, 2016/07/28 11:41:45]

Should REMOTE_CATEGORIES_OFFSET itself be a valid category number? (Probably
either way is fine, but right now I can't tell which it is)

[Philipp Keck, 2016/07/28 13:50:53]

No it shouldn't and the factory enforces that. I documented it in the header
comment here.

+  ARTICLES = REMOTE_CATEGORIES_OFFSET + 1,
+};
+
+// A category groups ContentSuggestions which belong together. Use the
+// ContentSuggestionsCategoryFactory to obtain instances.
+class ContentSuggestionsCategory {
+ public:
+  int id() const { return id_; };

[Marc Treib, 2016/07/28 11:41:45]

nit: extra semicolon at the end

[Philipp Keck, 2016/07/28 13:50:54]

Done.

+
+ private:
+  friend class ContentSuggestionsCategoryFactory;
+  ContentSuggestionsCategory(int id);

[Marc Treib, 2016/07/28 11:41:45]

empty line before

[Philipp Keck, 2016/07/28 13:50:54]

Done.

+
+  int id_;
+};
+
+inline bool operator==(const ContentSuggestionsCategory& left,

[Marc Treib, 2016/07/28 11:41:45]

All these shouldn't be inline.

[Philipp Keck, 2016/07/28 13:50:54]

Done.

+                       const ContentSuggestionsCategory& right) {
+  return left.id() == right.id();
+}
+
+inline bool operator!=(const ContentSuggestionsCategory& left,
+                       const ContentSuggestionsCategory& right) {
+  return left.id() != right.id();

[Marc Treib, 2016/07/28 11:41:45]

It's a common practice to define != in terms of ==, i.e.
return !(left == right);
In this case, it doesn't save any lines of code, but it does make sure that ==
and != are consistent.

[Philipp Keck, 2016/07/28 13:50:54]

Done.

+}
+
+inline bool operator==(const KnownSuggestionsCategories& left,

[Marc Treib, 2016/07/28 11:41:45]

Hmm, not quite sure about these... maybe an explicit IsKnownCategory method
would be clearer? Depends on how much we want a Category and a KnownCategory to
be "the same" I guess...

[tschumann, 2016/07/28 12:50:14]

Yeah, this also seems a bit overkill to me. Ideally, we wouldn't support those
comparisons and you would simply create a ContentSuggestionsCategory instance
from the KnownSuggestionsCategories and compare with that.

the whole point of this class is that you can treat all categories the same.

That aside, if a IsKnownCategory function is sufficient, that's even simpler.

[Philipp Keck, 2016/07/28 13:50:53]

Done.

[Philipp Keck, 2016/07/28 13:50:54]

See below.

+                       const ContentSuggestionsCategory& right) {
+  return static_cast<int>(left) == right.id();
+}
+
+inline bool operator==(const ContentSuggestionsCategory& left,
+                       const KnownSuggestionsCategories& right) {
+  return right == left;
+}
+
+inline bool operator!=(const KnownSuggestionsCategories& left,
+                       const ContentSuggestionsCategory& right) {
+  return static_cast<int>(left) != right.id();
+}
+
+inline bool operator!=(const ContentSuggestionsCategory& left,
+                       const KnownSuggestionsCategories& right) {
+  return right != left;
+}
+
+std::ostream& operator<<(std::ostream& os,

[Marc Treib, 2016/07/28 11:41:45]

What's this needed for - output in tests?

[tschumann, 2016/07/28 12:50:14]

not sure about chrome, but in google3 you'd have a function like:
string DebugString() const;

if logging id() is not sufficient.

[Marc Treib, 2016/07/28 12:54:02]

Chrome's LOG, DCHECK etc macros make use of operator<<. You can get by without
it, but this makes things a bit more comfortable. IMO it's fine to define this.

[Philipp Keck, 2016/07/28 13:50:54]

It's used in DCHECK_EQ, but it also comes in handy for debugging outputs in
ContentSuggestionsService. As long as the operator is consistently only used for
debugging and never for assembling strings for the user (which seems to be the
case in all the code I saw so far), it makes for nicely readable debugging
output statements. The style guide also encourages that: "Don't go out of your
way to avoid defining operator overloads. For example, prefer to define ==, =,
and <<, rather than Equals(), CopyFrom(), and PrintTo()".

That's also why I initially implemented the other comparison operators here.
Normally, comparing two things of different but similar types doesn't seem to be
a bad thing and an overloaded operator makes for nicely readable code. Now, the
code reads "someCategory.IsKnownCategory(KnownSuggestionsCategories::XYZ)". I
think there is little danger in accidentally using those wrong. But then the
operators do some conversions that are based on implementation details of the
two involved types, so they're not as straightforward as the implementation
suggests. That's why I'm also fine with an IsKnownCategory method.

[tschumann, 2016/07/28 15:03:45]

ok, so this means no operator== comparing KnownSuggestionsCategories with
ContentSuggestionsCategory?

operator<<() makes sense. I wasn't up-to-date with the style guide. It used to
be discouraged.

[Philipp Keck, 2016/07/28 15:15:25]

Yes, I removed the operator== comparing KnownSuggestionsCategories with
ContentSuggestionsCategory.

+                         const ContentSuggestionsCategory& obj);
 
 }  // namespace ntp_snippets