Split NTP_TILE_LOADED event.

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

Index: chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
diff --git a/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc b/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
index 6773db8f0576592b652752373fffc45cf165fe34..ff7facbfae9f13eca2ab92a9652a366e79adc7af 100644
--- a/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
+++ b/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
@@ -100,27 +100,21 @@ NTPUserDataLogger* NTPUserDataLogger::GetOrCreateFromWebContents(
 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();
@@ -165,17 +159,8 @@ void NTPUserDataLogger::LogMostVisitedNavigation(
   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),
@@ -201,17 +186,7 @@ NTPUserDataLogger::NTPUserDataLogger(content::WebContents* contents)
   }
 }
 
-// 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.