[NTP Snippets] Metrics: switch over known categories

components/ntp_snippets/category.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/category.h"
 
 namespace ntp_snippets {
 
 Category::Category(int id) : id_(id) {}
 
 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();
 }
 
 bool operator!=(const Category& left, const Category& right) {
   return !(left == right);
 }
 
 bool Category::CompareByID::operator()(const Category& left,
                                        const Category& right) const {
   return left.id() < right.id();
 }
 
 std::ostream& operator<<(std::ostream& os, const Category& obj) {
   os << obj.id();
   return os;
 }
 
 }  // namespace ntp_snippets