Split NTP_TILE_LOADED event.

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

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/webui/ntp/ntp_user_data_logger.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/metrics/histogram.h"
@@ skipping 82 matching lines @@
       content->GetController().GetVisibleEntry();
   if (entry)
     logger->ntp_url_ = entry->GetURL();
 
   return logger;
 }
 
 void NTPUserDataLogger::LogEvent(NTPLoggingEventType event,
                                  base::TimeDelta time) {
   switch (event) {
-    // It is possible that our page gets update with a different set of
-    // suggestions if the NTP is left open enough time.
-    // In either case, we want to flush our stats before recounting again.
     case NTP_SERVER_SIDE_SUGGESTION:
-      if (has_client_side_suggestions_)
-        EmitNtpStatistics(EmitReason::INTERNAL_FLUSH);
       has_server_side_suggestions_ = true;
       break;
     case NTP_CLIENT_SIDE_SUGGESTION:
-      if (has_server_side_suggestions_)
-        EmitNtpStatistics(EmitReason::INTERNAL_FLUSH);
       has_client_side_suggestions_ = true;
       break;
     case NTP_TILE:

[Marc Treib, 2016/07/12 12:40:09]

Unrelated to this CL, but: Can we get rid of NTP_TILE and instead increment
number_of_tiles_ on NTP_SERVER_SIDE_SUGGESTION and NTP_CLIENT_SIDE_SUGGESTION?

[sfiera, 2016/07/12 13:21:21]

I think so.

[Marc Treib, 2016/07/12 13:39:01]

Maybe add a TODO for this?

[sfiera, 2016/07/12 14:10:25]

Done.

       number_of_tiles_++;
       break;
     case NTP_TILE_LOADED:
-      // The time at which the last tile has loaded (title, thumbnail or single)
-      // is a good proxy for the total load time of the NTP, therefore we keep
-      // the max as the load time.
-      load_time_ = std::max(load_time_, time);
+      // We no longer emit statistics for the multi-iframe NTP.
+      break;
+    case NTP_ALL_TILES_LOADED:
+      load_time_ = time;

[Marc Treib, 2016/07/12 12:40:09]

Might as well get rid of the load_time_ member now and instead just pass it into
EmitNtpStatistics - WDYT?

[sfiera, 2016/07/12 13:21:21]

Done.

+      EmitNtpStatistics();
       break;
     default:
       NOTREACHED();
   }
 }
 
 void NTPUserDataLogger::LogMostVisitedImpression(
     int position, NTPLoggingTileSource tile_source) {
   UMA_HISTOGRAM_ENUMERATION(kMostVisitedImpressionHistogramName, position,
                             kNumMostVisited);
@@ skipping 24 matching lines @@
       kNumMostVisited,
       kNumMostVisited + 1,
       base::Histogram::kUmaTargetedHistogramFlag);
   counter->Add(position);
 
   // 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(base::UserMetricsAction("MostVisited_Clicked"));
 }
 
-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),
+    : has_server_side_suggestions_(false),
       has_client_side_suggestions_(false),
       number_of_tiles_(0),
       has_emitted_(false),
       during_startup_(false) {
   during_startup_ = !AfterStartupTaskUtils::IsBrowserStartupComplete();
 
   // We record metrics about session data here because when this class typically
   // emits metrics it is too late. This session data would theoretically have
   // been used to populate the page, and we want to learn about its state when
   // the NTP is being generated.
   if (contents) {
     ProfileSyncService* sync = ProfileSyncServiceFactory::GetForProfile(
         Profile::FromBrowserContext(contents->GetBrowserContext()));
     if (sync) {
       browser_sync::SessionsSyncManager* sessions =
           static_cast<browser_sync::SessionsSyncManager*>(
               sync->GetSessionsSyncableService());
       if (sessions) {
         sync_sessions::SyncSessionsMetrics::RecordYoungestForeignTabAgeOnNTP(
             sessions);
       }
     }
   }
 }
 
-// content::WebContentsObserver override
-void NTPUserDataLogger::NavigationEntryCommitted(
-    const content::LoadCommittedDetails& load_details) {
-  if (!load_details.previous_url.is_valid())
-    return;
-
-  if (search::MatchesOriginAndPath(ntp_url_, load_details.previous_url))
-    EmitNtpStatistics(EmitReason::NAVIGATED_AWAY);
-}
-
-void NTPUserDataLogger::EmitNtpStatistics(EmitReason reason) {
+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_)

[Marc Treib, 2016/07/12 12:40:09]

Can we get rid of the !number_of_tiles_ check now? I don't see what that serves.
(I'm guessing it was needed as a hack before, to avoid emitting before anything
was actually loaded)

Also, do we want to keep has_emitted_? Should be safe to get rid of it now, no?

[sfiera, 2016/07/12 13:21:21]

I think it was a hack and is now unneeded. Removed.
I don't think it's safe. The single-iframe NTP could get tiles multiple times,
and logs NTP_ALL_TILES_LOADED each time, with increasing load times. And in the
case where there's an in-page reload, we don't really care about load times or
number of tiles anymore, do we?

On the other hand, in the other CL, we can safely reset it with everything else
when the user goes back to the NTP.

[Marc Treib, 2016/07/12 13:39:01]

Yep, the "going back to NTP" case is what I was thinking about. Let's do that in
the other CL then.

     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);
   }
   has_server_side_suggestions_ = false;
   has_client_side_suggestions_ = false;
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "NewTabPage.NumberOfTiles", number_of_tiles_, 0, kNumMostVisited,
       kNumMostVisited + 1);
   number_of_tiles_ = 0;
   has_emitted_ = true;
   during_startup_ = false;
 }