Make EmitNtpStatistics() private.

It's still called from the same places, but through public functions
that describe what is happening and why, rather than exposing an
implementation detail.

Also, add an enum so that it's immediately obvious at the call site why
EmitNtpStatistics() is being called.

BUG=625990

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

chrome/browser/ui/search/instant_controller.cc

@@ -108,10 +108,9 @@ void InstantController::ActiveTabChanged() {
 }
 
 void InstantController::TabDeactivated(content::WebContents* contents) {
-  // If user is deactivating an NTP tab, log the number of mouseovers for this
-  // NTP session.
+  // If user is deactivating an NTP tab, give it a change to log stats.
   if (search::IsInstantNTP(contents))
-    InstantTab::EmitNtpStatistics(contents);
+    InstantTab::TabDeactivated(contents);
 }
 
 void InstantController::LogDebugEvent(const std::string& info) const {

chrome/browser/ui/search/instant_tab.cc

@@ -21,8 +21,14 @@ void InstantTab::Init(content::WebContents* contents) {
 }
 
 // static
-void InstantTab::EmitNtpStatistics(content::WebContents* contents) {
-  NTPUserDataLogger::GetOrCreateFromWebContents(contents)->EmitNtpStatistics();
+void InstantTab::TabDeactivated(content::WebContents* contents) {
+  NTPUserDataLogger::GetOrCreateFromWebContents(contents)->TabDeactivated();
+}
+
+// static
+void InstantTab::MostVisitedItemsChanged(content::WebContents* contents) {
+  NTPUserDataLogger::GetOrCreateFromWebContents(contents)
+      ->MostVisitedItemsChanged();
 }
 
 bool InstantTab::ShouldProcessAboutToNavigateMainFrame() {

chrome/browser/ui/search/instant_tab.h

@@ -22,8 +22,9 @@ class InstantTab : public InstantPage {
   // the page supports the Instant API.
   void Init(content::WebContents* contents);
 
-  // Logs a number of statistics regarding the NTP.
-  static void EmitNtpStatistics(content::WebContents* contents);
+  // Logs statistics regarding the NTP.
+  static void TabDeactivated(content::WebContents* contents);
+  static void MostVisitedItemsChanged(content::WebContents* contents);
 
  private:
   // Overridden from InstantPage:

chrome/browser/ui/search/search_tab_helper.cc

@@ -387,7 +387,7 @@ void SearchTabHelper::MostVisitedItemsChanged(
     const std::vector<InstantMostVisitedItem>& items) {
   // When most visited change, the NTP usually reloads the tiles. This means
   // our metrics get inconsistent. So we'd rather emit stats now.
-  InstantTab::EmitNtpStatistics(web_contents_);
+  InstantTab::MostVisitedItemsChanged(web_contents_);
   ipc_router_.SendMostVisitedItems(items);
   LogMostVisitedItemsSource(items);
 }

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

@@ -109,60 +109,6 @@ NTPUserDataLogger* NTPUserDataLogger::GetOrCreateFromWebContents(
   return logger;
 }
 
-void NTPUserDataLogger::EmitNtpStatistics() {
-  // We only send statistics once per page.
-  // And we don't send if there are no tiles recorded.
-  if (has_emitted_ || !number_of_tiles_)
-    return;
-
-  // LoadTime only gets update once per page, so we don't have it on reloads.
-  if (load_time_ > base::TimeDelta::FromMilliseconds(0)) {
-    logLoadTimeHistogram("NewTabPage.LoadTime", load_time_);
-
-    // Split between ML and MV.
-    std::string type = has_server_side_suggestions_ ?
-        "MostLikely" : "MostVisited";
-    logLoadTimeHistogram("NewTabPage.LoadTime." + type, load_time_);
-    // Split between Web and Local.
-    std::string source = ntp_url_.SchemeIsHTTPOrHTTPS() ? "Web" : "LocalNTP";
-    logLoadTimeHistogram("NewTabPage.LoadTime." + source, load_time_);
-
-    // Split between Startup and non-startup.
-    std::string status = during_startup_ ? "Startup" : "NewTab";
-    logLoadTimeHistogram("NewTabPage.LoadTime." + status, load_time_);
-
-    load_time_ = base::TimeDelta::FromMilliseconds(0);
-  }
-  UMA_HISTOGRAM_ENUMERATION(
-      "NewTabPage.SuggestionsType",
-      has_server_side_suggestions_ ? SERVER_SIDE : CLIENT_SIDE,
-      SUGGESTIONS_TYPE_COUNT);
-  has_server_side_suggestions_ = false;
-  has_client_side_suggestions_ = false;
-  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfTiles", number_of_tiles_);
-  number_of_tiles_ = 0;
-  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfThumbnailTiles",
-                          number_of_thumbnail_tiles_);
-  number_of_thumbnail_tiles_ = 0;
-  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfGrayTiles",
-                          number_of_gray_tiles_);
-  number_of_gray_tiles_ = 0;
-  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfExternalTiles",
-                          number_of_external_tiles_);
-  number_of_external_tiles_ = 0;
-  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfThumbnailErrors",
-                          number_of_thumbnail_errors_);
-  number_of_thumbnail_errors_ = 0;
-  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfGrayTileFallbacks",
-                          number_of_gray_tile_fallbacks_);
-  number_of_gray_tile_fallbacks_ = 0;
-  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfExternalTileFallbacks",
-                          number_of_external_tile_fallbacks_);
-  number_of_external_tile_fallbacks_ = 0;
-  has_emitted_ = true;
-  during_startup_ = false;
-}
-
 void NTPUserDataLogger::LogEvent(NTPLoggingEventType event,
                                  base::TimeDelta time) {
   switch (event) {
@@ -171,12 +117,12 @@ void NTPUserDataLogger::LogEvent(NTPLoggingEventType event,
     // In either case, we want to flush our stats before recounting again.
     case NTP_SERVER_SIDE_SUGGESTION:
       if (has_client_side_suggestions_)
-        EmitNtpStatistics();
+        EmitNtpStatistics(EmitReason::INTERNAL_FLUSH);
       has_server_side_suggestions_ = true;
       break;
     case NTP_CLIENT_SIDE_SUGGESTION:
       if (has_server_side_suggestions_)
-        EmitNtpStatistics();
+        EmitNtpStatistics(EmitReason::INTERNAL_FLUSH);
       has_client_side_suggestions_ = true;
       break;
     case NTP_TILE:
@@ -256,10 +202,18 @@ void NTPUserDataLogger::NavigationEntryCommitted(
     return;
 
   if (search::MatchesOriginAndPath(ntp_url_, load_details.previous_url)) {
-    EmitNtpStatistics();
+    EmitNtpStatistics(EmitReason::NAVIGATED_AWAY);
   }
 }
 
+void NTPUserDataLogger::TabDeactivated() {
+  EmitNtpStatistics(EmitReason::CLOSED);
+}
+
+void NTPUserDataLogger::MostVisitedItemsChanged() {
+  EmitNtpStatistics(EmitReason::MV_CHANGED);
+}
+
 NTPUserDataLogger::NTPUserDataLogger(content::WebContents* contents)
     : content::WebContentsObserver(contents),
       has_server_side_suggestions_(false),
@@ -293,3 +247,57 @@ NTPUserDataLogger::NTPUserDataLogger(content::WebContents* contents)
     }
   }
 }
+
+void NTPUserDataLogger::EmitNtpStatistics(EmitReason reason) {
+  // We only send statistics once per page.
+  // And we don't send if there are no tiles recorded.
+  if (has_emitted_ || !number_of_tiles_)
+    return;
+
+  // LoadTime only gets update once per page, so we don't have it on reloads.
+  if (load_time_ > base::TimeDelta::FromMilliseconds(0)) {
+    logLoadTimeHistogram("NewTabPage.LoadTime", load_time_);
+
+    // Split between ML and MV.
+    std::string type = has_server_side_suggestions_ ?
+        "MostLikely" : "MostVisited";
+    logLoadTimeHistogram("NewTabPage.LoadTime." + type, load_time_);
+    // Split between Web and Local.
+    std::string source = ntp_url_.SchemeIsHTTPOrHTTPS() ? "Web" : "LocalNTP";
+    logLoadTimeHistogram("NewTabPage.LoadTime." + source, load_time_);
+
+    // Split between Startup and non-startup.
+    std::string status = during_startup_ ? "Startup" : "NewTab";
+    logLoadTimeHistogram("NewTabPage.LoadTime." + status, load_time_);
+
+    load_time_ = base::TimeDelta::FromMilliseconds(0);
+  }
+  UMA_HISTOGRAM_ENUMERATION(
+      "NewTabPage.SuggestionsType",
+      has_server_side_suggestions_ ? SERVER_SIDE : CLIENT_SIDE,
+      SUGGESTIONS_TYPE_COUNT);
+  has_server_side_suggestions_ = false;
+  has_client_side_suggestions_ = false;
+  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfTiles", number_of_tiles_);
+  number_of_tiles_ = 0;
+  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfThumbnailTiles",
+                          number_of_thumbnail_tiles_);
+  number_of_thumbnail_tiles_ = 0;
+  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfGrayTiles",
+                          number_of_gray_tiles_);
+  number_of_gray_tiles_ = 0;
+  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfExternalTiles",
+                          number_of_external_tiles_);
+  number_of_external_tiles_ = 0;
+  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfThumbnailErrors",
+                          number_of_thumbnail_errors_);
+  number_of_thumbnail_errors_ = 0;
+  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfGrayTileFallbacks",
+                          number_of_gray_tile_fallbacks_);
+  number_of_gray_tile_fallbacks_ = 0;
+  UMA_HISTOGRAM_NTP_TILES("NewTabPage.NumberOfExternalTileFallbacks",
+                          number_of_external_tile_fallbacks_);
+  number_of_external_tile_fallbacks_ = 0;
+  has_emitted_ = true;
+  during_startup_ = false;
+}

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

@@ -31,11 +31,6 @@ class NTPUserDataLogger
   static NTPUserDataLogger* GetOrCreateFromWebContents(
       content::WebContents* content);
 
-  // Logs a number of statistics regarding the NTP. Called when an NTP tab is
-  // about to be deactivated (be it by switching tabs, losing focus or closing
-  // the tab/shutting down Chrome), or when the user navigates to a URL.
-  void EmitNtpStatistics();
-
   // Called when an event occurs on the NTP that requires a counter to be
   // incremented. |time| is the delta time in ms from navigation start until
   // this event happened.
@@ -51,6 +46,12 @@ class NTPUserDataLogger
   void NavigationEntryCommitted(
       const content::LoadCommittedDetails& load_details) override;
 
+  // Called when the tab is closed. Logs statistics.
+  void TabDeactivated();
+
+  // Called when the set of most visited sites changes. Flushes statistics.
+  void MostVisitedItemsChanged();
+
  protected:
   explicit NTPUserDataLogger(content::WebContents* contents);
 
@@ -61,6 +62,19 @@ class NTPUserDataLogger
                            OnMostVisitedItemsChangedFromServer);
   FRIEND_TEST_ALL_PREFIXES(SearchTabHelperTest,
                            OnMostVisitedItemsChangedFromClient);
+  FRIEND_TEST_ALL_PREFIXES(NTPUserDataLoggerTest,
+                           TestLogging);
+
+  // To clarify at the call site whether we are calling EmitNtpStatistics() for
+  // a good reason (CLOSED, NAVIGATED_AWAY) or a questionable one (MV_CHANGED,
+  // INTERNAL_FLUSH).
+  // TODO(sfiera): remove MV_CHANGED, INTERNAL_FLUSH.
+  enum class EmitReason { CLOSED, NAVIGATED_AWAY, MV_CHANGED, INTERNAL_FLUSH };
+
+  // Logs a number of statistics regarding the NTP. Called when an NTP tab is
+  // about to be deactivated (be it by switching tabs, losing focus or closing
+  // the tab/shutting down Chrome), or when the user navigates to a URL.
+  void EmitNtpStatistics(EmitReason reason);
 
   // True if at least one iframe came from a server-side suggestion.
   bool has_server_side_suggestions_;

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

@@ -62,7 +62,7 @@ TEST_F(NTPUserDataLoggerTest, TestLogging) {
     logger.LogEvent(NTP_GRAY_TILE, delta);
   logger.LogEvent(NTP_SERVER_SIDE_SUGGESTION, delta);
 
-  logger.EmitNtpStatistics();
+  logger.EmitNtpStatistics(NTPUserDataLogger::EmitReason::NAVIGATED_AWAY);
 
   EXPECT_EQ(1, GetTotalCount("NewTabPage.NumberOfTiles"));
   EXPECT_EQ(1, GetBinCount("NewTabPage.NumberOfTiles", 8));
@@ -82,7 +82,7 @@ TEST_F(NTPUserDataLoggerTest, TestLogging) {
   EXPECT_EQ(1, GetBinCount("NewTabPage.SuggestionsType", 1));
 
   // Statistics should be reset to 0, so we should not log anything else.
-  logger.EmitNtpStatistics();
+  logger.EmitNtpStatistics(NTPUserDataLogger::EmitReason::NAVIGATED_AWAY);
   EXPECT_EQ(1, GetTotalCount("NewTabPage.NumberOfTiles"));
   EXPECT_EQ(1, GetTotalCount("NewTabPage.NumberOfThumbnailTiles"));
   EXPECT_EQ(1, GetTotalCount("NewTabPage.NumberOfThumbnailErrors"));