[NTP Snippets] Metrics: switch over known categories

components/ntp_snippets/category.h

 // 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.
 
 #ifndef COMPONENTS_NTP_SNIPPETS_CATEGORY_H_
 #define COMPONENTS_NTP_SNIPPETS_CATEGORY_H_
 
 #include <ostream>
 
 namespace ntp_snippets {
 
 class CategoryFactory;
 
 // 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 (IDs strictly larger
 // than REMOTE_CATEGORIES_OFFSET) only need to be hard-coded here if they need
 // to be recognized by the client implementation.
+// NOTE: If you add an entry here, also add it to |IsAnyKnownCategory|.
 // On Android builds, a Java counterpart will be generated for this enum.
 // GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.ntp.snippets
 enum class KnownCategories {
   // Pages recently downloaded during normal navigation.
   RECENT_TABS,
 
   // Pages downloaded by the user for offline consumption.
   DOWNLOADS,
 
   // Recently used bookmarks.
@@ skipping 19 matching lines @@
   // a std::map, but should not be used to order categories for other purposes.
   struct CompareByID;
 
   // Returns a non-negative identifier that is unique for the category and can
   // be converted back to a Category instance using
   // |CategoryFactory::FromIDValue(id)|.
   // Note that these IDs are not necessarily stable across multiple runs of
   // the application, so they should not be persisted.
   int id() const { return id_; }
 
+  // Returns whether this category matches the given |known_category|.
   bool IsKnownCategory(KnownCategories known_category) const;
 
+  // Returns whether this category is any of the known categories.

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

Without looking in the implementation, I would not understand what you mean by
"is any known category". Maybe expand by ", i.e. any element of the
KnowCategories enum except for LOCAL_CATEGORIES_COUNT and
REMOTE_CATEGORIES_OFFSET." 

It is also inconsistent with IsKnownCategory which accepts these two cases as
known categories. Admittedly, only an idiot would ask if this Category is the
LOCAL_CATEGORIES_COUNT-category. Still, I would prefer IsKnownCategory to
blacklist these two cases to get the code consistent and self-documented.

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

Added, except for the "except..." part. It should be impossible to create such a
Category instance (see CategoryFactory::FromKnownCategory), so IMO that part of
the comment isn't necessary.
Again, it shouldn't be possible to create those categories. Still, I've added
DCHECKs in IsKnownCategory to make clear that those aren't valid values.

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

Agreed, thanks.

+  bool IsAnyKnownCategory() const;
+
  private:
   friend class CategoryFactory;
 
   explicit Category(int id);
 
   int id_;
 
   // Allow copy and assignment.
 };
 
 bool operator==(const Category& left, const Category& right);
 
 bool operator!=(const Category& left, const Category& right);
 
 struct Category::CompareByID {
   bool operator()(const Category& left, const Category& right) const;
 };
 
 std::ostream& operator<<(std::ostream& os, const Category& obj);
 
 }  // namespace ntp_snippets
 
 #endif  // COMPONENTS_NTP_SNIPPETS_CATEGORY_H_