Commit c15e8abf authored by treib's avatar treib Committed by Commit bot

[NTP Snippets] Metrics: switch over known categories

This ensures that there'll be a compile error if we add a new category but don't update the metrics code.
Also, add a histogram suffix for PHYSICAL_WEB_PAGES which was missing.

BUG=none

Review-Url: https://codereview.chromium.org/2337103003
Cr-Commit-Position: refs/heads/master@{#419420}
parent 47a0672f
...@@ -4,11 +4,15 @@ ...@@ -4,11 +4,15 @@
#include "components/ntp_snippets/category.h" #include "components/ntp_snippets/category.h"
#include "base/logging.h"
namespace ntp_snippets { namespace ntp_snippets {
Category::Category(int id) : id_(id) {} Category::Category(int id) : id_(id) {}
bool Category::IsKnownCategory(KnownCategories known_category) const { bool Category::IsKnownCategory(KnownCategories known_category) const {
DCHECK_NE(known_category, KnownCategories::LOCAL_CATEGORIES_COUNT);
DCHECK_NE(known_category, KnownCategories::REMOTE_CATEGORIES_OFFSET);
return id_ == static_cast<int>(known_category); return id_ == static_cast<int>(known_category);
} }
......
...@@ -34,6 +34,7 @@ enum class KnownCategories { ...@@ -34,6 +34,7 @@ enum class KnownCategories {
// Follows the last local category. // Follows the last local category.
LOCAL_CATEGORIES_COUNT, LOCAL_CATEGORIES_COUNT,
// Remote categories come after this.
REMOTE_CATEGORIES_OFFSET = 10000, REMOTE_CATEGORIES_OFFSET = 10000,
// Articles for you. // Articles for you.
...@@ -55,6 +56,7 @@ class Category { ...@@ -55,6 +56,7 @@ class Category {
// the application, so they should not be persisted. // the application, so they should not be persisted.
int id() const { return id_; } int id() const { return id_; }
// Returns whether this category matches the given |known_category|.
bool IsKnownCategory(KnownCategories known_category) const; bool IsKnownCategory(KnownCategories known_category) const;
private: private:
......
...@@ -16,6 +16,9 @@ CategoryFactory::CategoryFactory() { ...@@ -16,6 +16,9 @@ CategoryFactory::CategoryFactory() {
AddKnownCategory(KnownCategories::RECENT_TABS); AddKnownCategory(KnownCategories::RECENT_TABS);
AddKnownCategory(KnownCategories::BOOKMARKS); AddKnownCategory(KnownCategories::BOOKMARKS);
AddKnownCategory(KnownCategories::PHYSICAL_WEB_PAGES); AddKnownCategory(KnownCategories::PHYSICAL_WEB_PAGES);
DCHECK_EQ(static_cast<size_t>(KnownCategories::LOCAL_CATEGORIES_COUNT),
ordered_categories_.size());
} }
CategoryFactory::~CategoryFactory() {} CategoryFactory::~CategoryFactory() {}
......
...@@ -5,10 +5,12 @@ ...@@ -5,10 +5,12 @@
#include "components/ntp_snippets/content_suggestions_metrics.h" #include "components/ntp_snippets/content_suggestions_metrics.h"
#include <string> #include <string>
#include <type_traits>
#include "base/metrics/histogram.h" #include "base/metrics/histogram.h"
#include "base/metrics/histogram_macros.h" #include "base/metrics/histogram_macros.h"
#include "base/strings/stringprintf.h" #include "base/strings/stringprintf.h"
#include "base/template_util.h"
namespace ntp_snippets { namespace ntp_snippets {
namespace metrics { namespace metrics {
...@@ -44,17 +46,32 @@ const char kHistogramMoreButtonClicked[] = ...@@ -44,17 +46,32 @@ const char kHistogramMoreButtonClicked[] =
const char kPerCategoryHistogramFormat[] = "%s.%s"; const char kPerCategoryHistogramFormat[] = "%s.%s";
std::string GetCategorySuffix(Category category) { std::string GetCategorySuffix(Category category) {
// TODO(treib): Find a way to produce a compile error if a known category static_assert(
// isn't listed here. std::is_same<decltype(category.id()), typename base::underlying_type<
if (category.IsKnownCategory(KnownCategories::RECENT_TABS)) KnownCategories>::type>::value,
return "RecentTabs"; "KnownCategories must have the same underlying type as category.id()");
if (category.IsKnownCategory(KnownCategories::DOWNLOADS)) // Note: Since the underlying type of KnownCategories is int, it's legal to
return "Downloads"; // cast from int to KnownCategories, even if the given value isn't listed in
if (category.IsKnownCategory(KnownCategories::BOOKMARKS)) // the enumeration. The switch still makes sure that all known values are
return "Bookmarks"; // listed here.
if (category.IsKnownCategory(KnownCategories::ARTICLES)) KnownCategories known_category = static_cast<KnownCategories>(category.id());
return "Articles"; switch (known_category) {
// All other categories go into a single "Experimental" bucket. case KnownCategories::RECENT_TABS:
return "RecentTabs";
case KnownCategories::DOWNLOADS:
return "Downloads";
case KnownCategories::BOOKMARKS:
return "Bookmarks";
case KnownCategories::PHYSICAL_WEB_PAGES:
return "PhysicalWeb";
case KnownCategories::ARTICLES:
return "Articles";
case KnownCategories::LOCAL_CATEGORIES_COUNT:
case KnownCategories::REMOTE_CATEGORIES_OFFSET:
NOTREACHED();
break;
}
// All other (unknown) categories go into a single "Experimental" bucket.
return "Experimental"; return "Experimental";
} }
......
...@@ -100595,6 +100595,7 @@ To add a new entry, add it with any value and run test to compute valid value. ...@@ -100595,6 +100595,7 @@ To add a new entry, add it with any value and run test to compute valid value.
<suffix name="RecentTabs" label="Recent (offline) tabs"/> <suffix name="RecentTabs" label="Recent (offline) tabs"/>
<suffix name="Downloads" label="Downloads"/> <suffix name="Downloads" label="Downloads"/>
<suffix name="Bookmarks" label="Bookmarks"/> <suffix name="Bookmarks" label="Bookmarks"/>
<suffix name="PhysicalWeb" label="Physical Web pages"/>
<suffix name="Articles" label="Articles for you"/> <suffix name="Articles" label="Articles for you"/>
<suffix name="Experimental" label="Experimental"/> <suffix name="Experimental" label="Experimental"/>
<affected-histogram name="NewTabPage.ContentSuggestions.CountOnNtpOpened"/> <affected-histogram name="NewTabPage.ContentSuggestions.CountOnNtpOpened"/>
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment