[NTP Snippets] Metrics: switch over known categories

components/ntp_snippets/category.cc

Index: components/ntp_snippets/category.cc
diff --git a/components/ntp_snippets/category.cc b/components/ntp_snippets/category.cc
index c9dff3125dfc9c1ecdd92ce433f1c055f43efbe5..90bcf58d5608c3a8a996b555dd199bf26e565713 100644
--- a/components/ntp_snippets/category.cc
+++ b/components/ntp_snippets/category.cc
@@ -12,6 +12,20 @@ bool Category::IsKnownCategory(KnownCategories known_category) const {
   return id_ == static_cast<int>(known_category);
 }
 
+bool Category::IsAnyKnownCategory() const {
+  static const int kKnownCategoryIds[] = {
+      static_cast<int>(KnownCategories::RECENT_TABS),
+      static_cast<int>(KnownCategories::DOWNLOADS),
+      static_cast<int>(KnownCategories::BOOKMARKS),
+      static_cast<int>(KnownCategories::PHYSICAL_WEB_PAGES),
+      static_cast<int>(KnownCategories::ARTICLES)};

[jkrcal, 2016/09/13 16:08:05]

I am just wondering:

Cannot you add to the enum class elements SERVER_CATEGORIES_MAX and
UNKNOWN_SERVER_CATEGORIES_OFFSET in the end, consistently number unknown server
categories starting after the second offset and check here only for the
following?

0 <= id_ < LOCAL_CATEGORIES_COUNT || SERVER_CATEGORIES_OFFSET <= id_ <
SERVER_CATEGORIES_MAX

[Marc Treib, 2016/09/13 16:28:50]

SERVER_CATEGORIES_MAX would actually have to be KNOWN_SERVER_CATEGORIES_MAX, and
that would imply that all the known server categories are contiguous. We won't
necessarily be able to guarantee that.
We could do the range check for local categories, and then list only the known
server categories explicitly. WDYT?

[jkrcal, 2016/09/13 17:22:27]

How come? Is it because we can introduce some category on the server without the
client knowing it and later want to add better client-side support by adding it
to KnownCategories? In such case, we can change the number of the category, to
put it in the contiguous range, cannot we?

I do not want to introduce any real burden on the development just because of
avoiding one duplicity in the code ;) So far, I just fail to see the burden /
impossibility of what I propose.
I think this is better than what we currently have. If you are convinced that
what I propose above is impossible.

[tschumann, 2016/09/14 08:41:35]

Joining this discussion as a by-stander. Given that we explicitly ask if
something is a known category, I actually like the explicit list. However, we
should not rely on folks remembering to update this list -- compilation or test
failures would be really nice. I guess we're on the same page here.
Any idea how we could enforce this?
If we introduce a KnownCategories::NUM_CATEGORIES, then we could simply DCHECK()
that arraysize(kKnownCategoryIds) == KnownCategories::NUM_CATEGORIES.

We could hide the creation of the array in separate function and check there.

[Marc Treib, 2016/09/14 08:42:20]

Say the server adds categories 2 and 3. Later, we decide that the client should
know about 3, so the list of known remote categories would have a "hole".
Now, we *could* re-assign the IDs on the server-side, but it's potentially
problematic, and IMO just generally bad practice - we'd be re-interpreting data
which has already been sent over the wire.
Done.

+  for (int known_category_id : kKnownCategoryIds) {
+    if (id_ == known_category_id)
+      return true;
+  }
+  return false;
+}
+
 bool operator==(const Category& left, const Category& right) {
   return left.id() == right.id();
 }