[Most Visited] Log suggestion provider to UMA

chrome/browser/search/most_visited_iframe_source.cc

Index: chrome/browser/search/most_visited_iframe_source.cc
diff --git a/chrome/browser/search/most_visited_iframe_source.cc b/chrome/browser/search/most_visited_iframe_source.cc
index 7afd64cd5ee485a148d10cd9665bf890328dc934..cca834158749d5803d46c0c4ddbc2343d7bcc5a8 100644
--- a/chrome/browser/search/most_visited_iframe_source.cc
+++ b/chrome/browser/search/most_visited_iframe_source.cc
@@ -6,6 +6,7 @@
 
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/stringprintf.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/user_metrics.h"
 #include "grit/browser_resources.h"
@@ -24,6 +25,19 @@ const char kUtilJSPath[] = "/util.js";
 const char kCommonCSSPath[] = "/common.css";
 const char kLogHTMLPath[] = "/log.html";
 
+#define MV_CLICK_HISTOGRAM(position, provider)                              \

[Alexei Svitkine (slow), 2013/12/12 22:15:44]

Add a comment that mentions why this can't use the standard histogram macros and
documents params.

[Mathieu, 2013/12/13 15:01:38]

Done.

+  do {                                                                      \
+    std::string histogram_name = provider ?                                 \
+        base::StringPrintf(                                                 \
+            MostVisitedIframeSource::kMostVisitedHistogramWithProvider,     \
+            provider) : MostVisitedIframeSource::kMostVisitedHistogramName; \

[Alexei Svitkine (slow), 2013/12/12 22:15:44]

Shouldn't you log the non-suffixed histogram too even when there is a provider?
That can be done with a regular macro.

[Mathieu, 2013/12/13 15:01:38]

I was combining both in this macro and passing NULL as provider for the other
call. I've changed it so that the non-provider case is back to using the regular
macro and this macro is more specific to the provider.

+    base::HistogramBase* counter = base::LinearHistogram::FactoryGet(       \
+        histogram_name, 1, MostVisitedIframeSource::kNumMostVisited,        \
+        MostVisitedIframeSource::kNumMostVisited + 1,                       \
+        base::Histogram::kUmaTargetedHistogramFlag);                        \
+    counter->Add(position);                                                 \
+  } while (0)
+
 }  // namespace
 
 MostVisitedIframeSource::MostVisitedIframeSource() {
@@ -35,6 +49,8 @@ MostVisitedIframeSource::~MostVisitedIframeSource() {
 const int MostVisitedIframeSource::kNumMostVisited = 8;
 const char MostVisitedIframeSource::kMostVisitedHistogramName[] =
     "NewTabPage.MostVisited";
+const char MostVisitedIframeSource::kMostVisitedHistogramWithProvider[] =
+    "NewTabPage.MostVisited.%s";
 
 std::string MostVisitedIframeSource::GetSource() const {
   return chrome::kChromeSearchMostVisitedHost;
@@ -69,8 +85,14 @@ void MostVisitedIframeSource::StartDataRequest(
     int position;
     if (net::GetValueForKeyInQuery(url, "pos", &str_position) &&
         base::StringToInt(str_position, &position)) {
-      UMA_HISTOGRAM_ENUMERATION(kMostVisitedHistogramName, position,
-                                kNumMostVisited);
+      // Log the Most Visited click.
+      MV_CLICK_HISTOGRAM(position, NULL);
+      // If a specific provider is specified, log the metric specific to the
+      // provider.
+      std::string provider;
+      if (net::GetValueForKeyInQuery(url, "pr", &provider)) {

[Alexei Svitkine (slow), 2013/12/12 22:15:44]

Nit: {} not needed.

[Mathieu, 2013/12/13 15:01:38]

Done.

+        MV_CLICK_HISTOGRAM(position, provider.c_str());
+      }
       // Records the action. This will be available as a time-stamped stream
       // server-side and can be used to compute time-to-long-dwell.
       content::RecordAction(content::UserMetricsAction("MostVisited_Clicked"));