Cleanup desktop NTP metrics recording, part 2

Followup to https://codereview.chromium.org/2429523002

- Remove NTP_*_SIDE_SUGGESTION (replaced by LogMostVisitedImpression calls)
- Cleanup tests

BUG=656646
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2435683003
Cr-Commit-Position: refs/heads/master@{#427086}

chrome/browser/resources/local_ntp/most_visited_single.js

@@ -16,10 +16,6 @@
  * @const
  */
 var LOG_TYPE = {
-  // A suggestion coming from the server was rendered.
-  NTP_SERVER_SIDE_SUGGESTION: 0,
-  // A suggestion coming from the client was rendered.
-  NTP_CLIENT_SIDE_SUGGESTION: 1,
   // All NTP Tiles have finished loading (successfully or failing).
   NTP_ALL_TILES_LOADED: 11,
 };
@@ -310,7 +306,6 @@ var addTile = function(args) {
       data.faviconUrl = 'chrome-search://favicon/size/16@' +
           window.devicePixelRatio + 'x/' + data.renderViewId + '/' + data.tid;
     }
-    logEvent(LOG_TYPE.NTP_CLIENT_SIDE_SUGGESTION);
     tiles.appendChild(renderTile(data));
   } else if (args.url) {
     // If a URL is passed: a server-side suggestion.
@@ -319,7 +314,6 @@ var addTile = function(args) {
     if (/^javascript:/i.test(args.url) ||
         /^javascript:/i.test(args.thumbnailUrl))
       return;
-    logEvent(LOG_TYPE.NTP_SERVER_SIDE_SUGGESTION);
     tiles.appendChild(renderTile(args));
   } else {  // an empty tile
     tiles.appendChild(renderTile(null));

chrome/browser/ui/search/search_ipc_router_unittest.cc

@@ -259,7 +259,7 @@ TEST_F(SearchIPCRouterTest, ProcessLogEventMsg) {
   NavigateAndCommitActiveTab(GURL(chrome::kChromeSearchLocalNtpUrl));
   SetupMockDelegateAndPolicy();
   MockSearchIPCRouterPolicy* policy = GetSearchIPCRouterPolicy();
-  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_CLIENT_SIDE_SUGGESTION, delta))
+  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_ALL_TILES_LOADED, delta))
       .Times(1);
   EXPECT_CALL(*policy, ShouldProcessLogEvent()).Times(1)
       .WillOnce(testing::Return(true));
@@ -267,7 +267,7 @@ TEST_F(SearchIPCRouterTest, ProcessLogEventMsg) {
   content::WebContents* contents = web_contents();
   OnMessageReceived(ChromeViewHostMsg_LogEvent(
       contents->GetRoutingID(), GetSearchIPCRouterSeqNo(),
-      NTP_CLIENT_SIDE_SUGGESTION, delta));
+      NTP_ALL_TILES_LOADED, delta));
 }
 
 TEST_F(SearchIPCRouterTest, IgnoreLogEventMsg) {
@@ -275,7 +275,7 @@ TEST_F(SearchIPCRouterTest, IgnoreLogEventMsg) {
   NavigateAndCommitActiveTab(GURL("chrome-search://foo/bar"));
   SetupMockDelegateAndPolicy();
   MockSearchIPCRouterPolicy* policy = GetSearchIPCRouterPolicy();
-  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_CLIENT_SIDE_SUGGESTION, delta))
+  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_ALL_TILES_LOADED, delta))
       .Times(0);
   EXPECT_CALL(*policy, ShouldProcessLogEvent()).Times(1)
       .WillOnce(testing::Return(false));
@@ -283,7 +283,7 @@ TEST_F(SearchIPCRouterTest, IgnoreLogEventMsg) {
   content::WebContents* contents = web_contents();
   OnMessageReceived(ChromeViewHostMsg_LogEvent(
       contents->GetRoutingID(), GetSearchIPCRouterSeqNo(),
-      NTP_CLIENT_SIDE_SUGGESTION, delta));
+      NTP_ALL_TILES_LOADED, delta));
 }
 
 TEST_F(SearchIPCRouterTest, ProcessLogMostVisitedImpressionMsg) {
@@ -488,12 +488,12 @@ TEST_F(SearchIPCRouterTest, IgnoreMessageIfThePageIsNotActive) {
       contents->GetRoutingID(), page_seq_no, OMNIBOX_FOCUS_VISIBLE));
 
   base::TimeDelta delta = base::TimeDelta::FromMilliseconds(123);
-  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_CLIENT_SIDE_SUGGESTION, delta))
+  EXPECT_CALL(*mock_delegate(), OnLogEvent(NTP_ALL_TILES_LOADED, delta))
       .Times(0);
   EXPECT_CALL(*policy, ShouldProcessLogEvent()).Times(0);
   OnMessageReceived(
       ChromeViewHostMsg_LogEvent(contents->GetRoutingID(), page_seq_no,
-                                 NTP_CLIENT_SIDE_SUGGESTION, delta));
+                                 NTP_ALL_TILES_LOADED, delta));
 
   base::string16 text;
   EXPECT_CALL(*mock_delegate(), PasteIntoOmnibox(text)).Times(0);

chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc

@@ -122,20 +122,8 @@ NTPUserDataLogger* NTPUserDataLogger::GetOrCreateFromWebContents(
 
 void NTPUserDataLogger::LogEvent(NTPLoggingEventType event,
                                  base::TimeDelta time) {
-  switch (event) {
-    case NTP_SERVER_SIDE_SUGGESTION:
-      has_server_side_suggestions_ = true;
-      number_of_tiles_++;
-      return;
-    case NTP_CLIENT_SIDE_SUGGESTION:
-      has_client_side_suggestions_ = true;
-      number_of_tiles_++;
-      return;
-    case NTP_ALL_TILES_LOADED:
-      EmitNtpStatistics(time);
-      return;
-  }
-  NOTREACHED();
+  DCHECK_EQ(NTP_ALL_TILES_LOADED, event);
+  EmitNtpStatistics(time);
 }
 
 void NTPUserDataLogger::LogMostVisitedImpression(
@@ -145,6 +133,15 @@ void NTPUserDataLogger::LogMostVisitedImpression(
   }
   impression_was_logged_[position] = true;
 
+  switch (tile_source) {
+    case NTPLoggingTileSource::CLIENT:
+      has_client_side_suggestions_ = true;
+      break;
+    case NTPLoggingTileSource::SERVER:
+      has_server_side_suggestions_ = true;
+      break;
+  }
+
   UMA_HISTOGRAM_ENUMERATION(kMostVisitedImpressionHistogramName, position,
                             kNumMostVisited);
 
@@ -185,7 +182,6 @@ NTPUserDataLogger::NTPUserDataLogger(content::WebContents* contents)
     : content::WebContentsObserver(contents),
       has_server_side_suggestions_(false),
       has_client_side_suggestions_(false),
-      number_of_tiles_(0),
       has_emitted_(false),
       during_startup_(!AfterStartupTaskUtils::IsBrowserStartupComplete()) {
   // We record metrics about session data here because when this class typically
@@ -209,7 +205,6 @@ void NTPUserDataLogger::NavigatedFromURLToURL(const GURL& from,
     DVLOG(1) << "Returning to New Tab Page";
     impression_was_logged_.reset();
     has_emitted_ = false;
-    number_of_tiles_ = 0;
     has_server_side_suggestions_ = false;
     has_client_side_suggestions_ = false;
   }
@@ -219,8 +214,10 @@ void NTPUserDataLogger::EmitNtpStatistics(base::TimeDelta load_time) {
   // We only send statistics once per page.
   if (has_emitted_)
     return;
+
+  size_t number_of_tiles = impression_was_logged_.count();
   DVLOG(1) << "Emitting NTP load time: " << load_time << ", "
-           << "number of tiles: " << number_of_tiles_;
+           << "number of tiles: " << number_of_tiles;
 
   LogLoadTimeHistogram("NewTabPage.LoadTime", load_time);
 
@@ -236,12 +233,9 @@ void NTPUserDataLogger::EmitNtpStatistics(base::TimeDelta load_time) {
   std::string status = during_startup_ ? "Startup" : "NewTab";
   LogLoadTimeHistogram("NewTabPage.LoadTime." + status, load_time);
 
-  has_server_side_suggestions_ = false;
-  has_client_side_suggestions_ = false;
   UMA_HISTOGRAM_CUSTOM_COUNTS(
-      "NewTabPage.NumberOfTiles", number_of_tiles_, 1, kNumMostVisited,
+      "NewTabPage.NumberOfTiles", number_of_tiles, 1, kNumMostVisited,
       kNumMostVisited + 1);
-  number_of_tiles_ = 0;
   has_emitted_ = true;
   during_startup_ = false;
 }

chrome/browser/ui/webui/ntp/ntp_user_data_logger.h

@@ -84,10 +84,6 @@ class NTPUserDataLogger
   // True if at least one iframe came from a client-side suggestion.
   bool has_client_side_suggestions_;
 
-  // Total number of tiles rendered, no matter if it's a thumbnail, a gray tile
-  // or an external tile.
-  size_t number_of_tiles_;
-
   // Whether we have already emitted NTP stats for this web contents.
   bool has_emitted_;
 

chrome/browser/ui/webui/ntp/ntp_user_data_logger_unittest.cc

@@ -4,83 +4,39 @@
 
 #include "chrome/browser/ui/webui/ntp/ntp_user_data_logger.h"
 
-#include "base/metrics/histogram_macros.h"
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/metrics/histogram.h"
 #include "base/metrics/statistics_recorder.h"
-#include "base/strings/utf_string_conversions.h"
+#include "base/test/histogram_tester.h"
 #include "chrome/common/search/ntp_logging_events.h"
-#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::Bucket;
+using testing::ElementsAre;
+using testing::IsEmpty;
+
 namespace {
 
-using testing::ElementsAre;
 using Sample = base::HistogramBase::Sample;
 using Samples = std::vector<Sample>;
 
 class TestNTPUserDataLogger : public NTPUserDataLogger {
  public:
-  TestNTPUserDataLogger() : NTPUserDataLogger(NULL) {}
+  TestNTPUserDataLogger() : NTPUserDataLogger(nullptr) {}
   ~TestNTPUserDataLogger() override {}
 };
 
-base::HistogramBase::Count GetTotalCount(const std::string& histogram_name) {
-  base::HistogramBase* histogram = base::StatisticsRecorder::FindHistogram(
-      histogram_name);
-  // Return 0 if history is uninitialized.
-  return histogram ? histogram->SnapshotSamples()->TotalCount() : 0;
-}
-
-base::HistogramBase::Count GetBinCount(const std::string& histogram_name,
-                                       Sample value) {
-  base::HistogramBase* histogram = base::StatisticsRecorder::FindHistogram(
-      histogram_name);
-  // Return 0 if history is uninitialized.
-  return histogram ? histogram->SnapshotSamples()->GetCount(value) : 0;
-}
-
-std::vector<int> GetBinCounts(const std::string& histogram_name,
-                              const Samples& values) {
-  std::vector<int> results;
-  for (const auto& value : values) {
-    results.push_back(GetBinCount(histogram_name, value));
-  }
-  return results;
-}
-
-std::vector<int> TotalImpressions(const Samples& values) {
-  return GetBinCounts("NewTabPage.SuggestionsImpression", values);
-}
-
-std::vector<int> ServerImpressions(const Samples& values) {
-  return GetBinCounts("NewTabPage.SuggestionsImpression.server", values);
-}
-
-std::vector<int> ClientImpressions(const Samples& values) {
-  return GetBinCounts("NewTabPage.SuggestionsImpression.client", values);
-}
-
-std::vector<int> TotalNavigations(const Samples& values) {
-  return GetBinCounts("NewTabPage.MostVisited", values);
-}
-
-std::vector<int> ServerNavigations(const Samples& values) {
-  return GetBinCounts("NewTabPage.MostVisited.server", values);
-}
-
-std::vector<int> ClientNavigations(const Samples& values) {
-  return GetBinCounts("NewTabPage.MostVisited.client", values);
-}
-
 }  // namespace
 
-class NTPUserDataLoggerTest : public testing::Test {
-  content::TestBrowserThreadBundle thread_bundle_;
-};
-
-TEST_F(NTPUserDataLoggerTest, TestNumberOfTiles) {
+TEST(NTPUserDataLoggerTest, TestNumberOfTiles) {
   base::StatisticsRecorder::Initialize();
 
+  base::HistogramTester histogram_tester;
+
   // Ensure non-zero statistics.
   TestNTPUserDataLogger logger;
   logger.ntp_url_ = GURL("chrome://newtab/");
@@ -88,16 +44,15 @@ TEST_F(NTPUserDataLoggerTest, TestNumberOfTiles) {
   base::TimeDelta delta = base::TimeDelta::FromMilliseconds(0);
 
   for (int i = 0; i < 8; ++i)
-    logger.LogEvent(NTP_SERVER_SIDE_SUGGESTION, delta);
-
+    logger.LogMostVisitedImpression(i, NTPLoggingTileSource::SERVER);
   logger.LogEvent(NTP_ALL_TILES_LOADED, delta);
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.NumberOfTiles"),
+              ElementsAre(Bucket(8, 1)));
 
-  EXPECT_EQ(1, GetTotalCount("NewTabPage.NumberOfTiles"));
-  EXPECT_EQ(1, GetBinCount("NewTabPage.NumberOfTiles", 8));
-
-  // Statistics should be reset to 0, so we should not log anything else.
+  // We should not log again for the same NTP.
   logger.LogEvent(NTP_ALL_TILES_LOADED, delta);
-  EXPECT_EQ(1, GetTotalCount("NewTabPage.NumberOfTiles"));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.NumberOfTiles"),
+              ElementsAre(Bucket(8, 1)));
 
   // Navigating away and back resets stats.
   logger.NavigatedFromURLToURL(GURL("chrome://newtab/"),
@@ -105,105 +60,175 @@ TEST_F(NTPUserDataLoggerTest, TestNumberOfTiles) {
   logger.NavigatedFromURLToURL(GURL("http://chromium.org"),
                                GURL("chrome://newtab/"));
   logger.LogEvent(NTP_ALL_TILES_LOADED, delta);
-  EXPECT_EQ(2, GetTotalCount("NewTabPage.NumberOfTiles"));
+  EXPECT_THAT(histogram_tester.GetAllSamples("NewTabPage.NumberOfTiles"),
+              ElementsAre(Bucket(0, 1), Bucket(8, 1)));
 }
 
-TEST_F(NTPUserDataLoggerTest, TestLogMostVisitedImpression) {
+TEST(NTPUserDataLoggerTest, TestLogMostVisitedImpression) {
   base::StatisticsRecorder::Initialize();
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+
+  base::HistogramTester histogram_tester;
 
   TestNTPUserDataLogger logger;
   logger.ntp_url_ = GURL("chrome://newtab/");
 
   // Impressions increment the associated bins.
 
+  logger.LogMostVisitedImpression(0, NTPLoggingTileSource::SERVER);
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression"),
+      ElementsAre(Bucket(0, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.server"),
+      ElementsAre(Bucket(0, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.client"),
+      IsEmpty());
+
   logger.LogMostVisitedImpression(1, NTPLoggingTileSource::SERVER);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(1, 0, 0, 0));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(1, 0, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.server"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.client"),
+      IsEmpty());
 
-  logger.LogMostVisitedImpression(2, NTPLoggingTileSource::SERVER);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+  logger.LogMostVisitedImpression(2, NTPLoggingTileSource::CLIENT);
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1), Bucket(2, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.server"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.client"),
+      ElementsAre(Bucket(2, 1)));
 
   logger.LogMostVisitedImpression(3, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 1, 0));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 1, 0));
-
-  logger.LogMostVisitedImpression(4, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 1, 1));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 1, 1));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1), Bucket(2, 1), Bucket(3, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.server"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.client"),
+      ElementsAre(Bucket(2, 1), Bucket(3, 1)));
 
   // But once incremented, they don't increase again unless reset.
-  logger.LogMostVisitedImpression(1, NTPLoggingTileSource::SERVER);
-  logger.LogMostVisitedImpression(2, NTPLoggingTileSource::CLIENT);
-  logger.LogMostVisitedImpression(3, NTPLoggingTileSource::SERVER);
-  logger.LogMostVisitedImpression(4, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 1, 1));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 0, 1, 1));
+  logger.LogMostVisitedImpression(0, NTPLoggingTileSource::SERVER);
+  logger.LogMostVisitedImpression(1, NTPLoggingTileSource::CLIENT);
+  logger.LogMostVisitedImpression(2, NTPLoggingTileSource::SERVER);
+  logger.LogMostVisitedImpression(3, NTPLoggingTileSource::CLIENT);
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1), Bucket(2, 1), Bucket(3, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.server"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.client"),
+      ElementsAre(Bucket(2, 1), Bucket(3, 1)));
 
   // Impressions are silently ignored for tiles >= 8.
   logger.LogMostVisitedImpression(8, NTPLoggingTileSource::SERVER);
   logger.LogMostVisitedImpression(9, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalImpressions({8, 9}), ElementsAre(0, 0));
-  EXPECT_THAT(ServerImpressions({8, 9}), ElementsAre(0, 0));
-  EXPECT_THAT(ClientImpressions({8, 9}), ElementsAre(0, 0));
-
-  // Navigating away from the NTP and back resets stats.
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1), Bucket(2, 1), Bucket(3, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.server"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.client"),
+      ElementsAre(Bucket(2, 1), Bucket(3, 1)));
+
+  // After navigating away from the NTP and back, we record again.
   logger.NavigatedFromURLToURL(GURL("chrome://newtab/"),
                                GURL("http://chromium.org"));
   logger.NavigatedFromURLToURL(GURL("http://chromium.org"),
                                GURL("chrome://newtab/"));
-  logger.LogMostVisitedImpression(1, NTPLoggingTileSource::SERVER);
-  logger.LogMostVisitedImpression(2, NTPLoggingTileSource::CLIENT);
-  logger.LogMostVisitedImpression(3, NTPLoggingTileSource::SERVER);
-  logger.LogMostVisitedImpression(4, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalImpressions({1, 2, 3, 4}), ElementsAre(2, 2, 2, 2));
-  EXPECT_THAT(ServerImpressions({1, 2, 3, 4}), ElementsAre(2, 1, 1, 0));
-  EXPECT_THAT(ClientImpressions({1, 2, 3, 4}), ElementsAre(0, 1, 1, 2));
+  logger.LogMostVisitedImpression(0, NTPLoggingTileSource::SERVER);
+  logger.LogMostVisitedImpression(1, NTPLoggingTileSource::CLIENT);
+  logger.LogMostVisitedImpression(2, NTPLoggingTileSource::SERVER);
+  logger.LogMostVisitedImpression(3, NTPLoggingTileSource::CLIENT);
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression"),
+      ElementsAre(Bucket(0, 2), Bucket(1, 2), Bucket(2, 2), Bucket(3, 2)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.server"),
+      ElementsAre(Bucket(0, 2), Bucket(1, 1), Bucket(2, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.SuggestionsImpression.client"),
+      ElementsAre(Bucket(1, 1), Bucket(2, 1), Bucket(3, 2)));
 }
 
-TEST_F(NTPUserDataLoggerTest, TestLogMostVisitedNavigation) {
+TEST(NTPUserDataLoggerTest, TestLogMostVisitedNavigation) {
   base::StatisticsRecorder::Initialize();
 
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+  base::HistogramTester histogram_tester;
 
   TestNTPUserDataLogger logger;
 
+  logger.LogMostVisitedNavigation(0, NTPLoggingTileSource::SERVER);
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited"),
+      ElementsAre(Bucket(0, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited.server"),
+      ElementsAre(Bucket(0, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited.client"),
+      IsEmpty());
+
   logger.LogMostVisitedNavigation(1, NTPLoggingTileSource::SERVER);
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(1, 0, 0, 0));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(1, 0, 0, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited.server"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited.client"),
+      IsEmpty());
 
-  logger.LogMostVisitedNavigation(2, NTPLoggingTileSource::SERVER);
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 0, 0));
+  logger.LogMostVisitedNavigation(2, NTPLoggingTileSource::CLIENT);
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1), Bucket(2, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited.server"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited.client"),
+      ElementsAre(Bucket(2, 1)));
 
   logger.LogMostVisitedNavigation(3, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 1, 0));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 1, 0));
-
-  logger.LogMostVisitedNavigation(4, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 1, 1));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(1, 1, 0, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 0, 1, 1));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1), Bucket(2, 1), Bucket(3, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited.server"),
+      ElementsAre(Bucket(0, 1), Bucket(1, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited.client"),
+      ElementsAre(Bucket(2, 1), Bucket(3, 1)));
 
   // Navigations always increase.
-  logger.LogMostVisitedNavigation(1, NTPLoggingTileSource::SERVER);
-  logger.LogMostVisitedNavigation(2, NTPLoggingTileSource::CLIENT);
-  logger.LogMostVisitedNavigation(3, NTPLoggingTileSource::SERVER);
-  logger.LogMostVisitedNavigation(4, NTPLoggingTileSource::CLIENT);
-  EXPECT_THAT(TotalNavigations({1, 2, 3, 4}), ElementsAre(2, 2, 2, 2));
-  EXPECT_THAT(ServerNavigations({1, 2, 3, 4}), ElementsAre(2, 1, 1, 0));
-  EXPECT_THAT(ClientNavigations({1, 2, 3, 4}), ElementsAre(0, 1, 1, 2));
+  logger.LogMostVisitedNavigation(0, NTPLoggingTileSource::SERVER);
+  logger.LogMostVisitedNavigation(1, NTPLoggingTileSource::CLIENT);
+  logger.LogMostVisitedNavigation(2, NTPLoggingTileSource::SERVER);
+  logger.LogMostVisitedNavigation(3, NTPLoggingTileSource::CLIENT);
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited"),
+      ElementsAre(Bucket(0, 2), Bucket(1, 2), Bucket(2, 2), Bucket(3, 2)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited.server"),
+      ElementsAre(Bucket(0, 2), Bucket(1, 1), Bucket(2, 1)));
+  EXPECT_THAT(
+      histogram_tester.GetAllSamples("NewTabPage.MostVisited.client"),
+      ElementsAre(Bucket(1, 1), Bucket(2, 1), Bucket(3, 2)));
 }

chrome/common/search/ntp_logging_events.h

@@ -11,10 +11,10 @@
 // Note: Keep in sync with browser/resources/local_ntp/most_visited_single.js
 enum NTPLoggingEventType {
   // A suggestion coming from the server was rendered.
-  NTP_SERVER_SIDE_SUGGESTION = 0,
+  // Deleted: NTP_SERVER_SIDE_SUGGESTION = 0,
 
   // A suggestion coming from the client was rendered.
-  NTP_CLIENT_SIDE_SUGGESTION = 1,
+  // Deleted: NTP_CLIENT_SIDE_SUGGESTION = 1,
 
   // Indicates a tile was rendered, no matter if it's a thumbnail, a gray tile
   // or an external tile.