[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..e4364703632332a7038c10afc960e76ed378974b 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"
@@ -23,6 +24,22 @@ const char kThumbnailJSPath[] = "/thumbnail.js";
 const char kUtilJSPath[] = "/util.js";
 const char kCommonCSSPath[] = "/common.css";
 const char kLogHTMLPath[] = "/log.html";
+const char kMostVisitedHistogramWithProvider[] = "NewTabPage.MostVisited.%s";
+
+// This macro is needed because the histogram name changes according to the
+// provider, and UMA_HISTOGRAM_ENUMERATION caches a given histogram object at

[Jered, 2013/12/13 19:39:38]

Where are you using UMA_HISTOGRAM_ENUMERATION here? Can this not just be an
inline function with the text of this macro, minus the do...while?

[Mathieu, 2013/12/13 20:00:38]

You're right, done. Made it a private function so that it can use my other
private function, GetHistogramNameForProvider.

+// call site, which will not work in this scenario.
+#define MV_PROVIDER_CLICK_HISTOGRAM(position, provider)                        \
+  do {                                                                         \
+    std::string histogram_name =                                               \
+        MostVisitedIframeSource::GetHistogramNameForProvider(provider);        \
+    base::HistogramBase* counter = base::LinearHistogram::FactoryGet(          \
+        histogram_name, 1,                                                     \
+        MostVisitedIframeSource::kNumMostVisited,                              \
+        MostVisitedIframeSource::kNumMostVisited + 1,                          \
+        base::Histogram::kUmaTargetedHistogramFlag);                           \
+    counter->Add(position);                                                    \
+  } while (0)
 
 }  // namespace
 
@@ -69,8 +86,15 @@ void MostVisitedIframeSource::StartDataRequest(
     int position;
     if (net::GetValueForKeyInQuery(url, "pos", &str_position) &&
         base::StringToInt(str_position, &position)) {
+      // Log the Most Visited click.
       UMA_HISTOGRAM_ENUMERATION(kMostVisitedHistogramName, position,
                                 kNumMostVisited);
+      // If a specific provider is specified, log the metric specific to the
+      // provider.
+      std::string provider;
+      if (net::GetValueForKeyInQuery(url, "pr", &provider))
+        MV_PROVIDER_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"));
@@ -87,3 +111,9 @@ bool MostVisitedIframeSource::ServesPath(const std::string& path) const {
          path == kThumbnailCSSPath || path == kThumbnailJSPath ||
          path == kUtilJSPath || path == kCommonCSSPath || path == kLogHTMLPath;
 }
+
+// static
+std::string MostVisitedIframeSource::GetHistogramNameForProvider(
+    const char* provider) {
+  return base::StringPrintf(kMostVisitedHistogramWithProvider, provider);
+}