Cleanup desktop NTP metrics recording, part 2

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 d249c9df5633eddf0a5135c3b1e74dfa8263d85c..eceaa31352271a8fccae0e9d9f28b6fb4f635340 100644
--- a/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
+++ b/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
@@ -122,24 +122,22 @@ 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(
     int position, NTPLoggingTileSource tile_source) {
+  switch (tile_source) {
+    case NTPLoggingTileSource::CLIENT:
+      has_client_side_suggestions_ = true;
+      break;
+    case NTPLoggingTileSource::SERVER:
+      has_server_side_suggestions_ = true;
+      break;
+  }
+  number_of_tiles_++;

[Marc Treib, 2016/10/24 11:51:43]

This preserves the previous behavior: set has_client/server and increment
number_of_tiles *before* checking if we already had an impression at this index.
Should we change that? Or, can you see any reason why we shouldn't?

[sfiera, 2016/10/24 13:01:16]

Does it matter? number_of_tiles_ is only relevant to EmitNtpStatistics(), which
runs only once for any particular NTP. We shouldn't see multiple impressions for
any particular index until we've already emitted the number_of_tiles_ stat.
(impression metrics are logged separately from EmitNtpStatistics(), which is why
there's separate behavior to avoid resending them)

Given that it shouldn't matter, I'd be inclined to move number_of_tiles++ below
the conditional, just because it feels better there. Could go either way about
has_*_suggestions_.

[Marc Treib, 2016/10/24 14:29:11]

Exactly, it *shouldn't* matter. If for whatever reason, the JS was sending
multiple impressions for the same index, then the old code would count a larger
number of tiles.
Then again, we don't see evidence of that in UMA, so I suppose it's fine to
change. In fact, that lets us get rid of number_of_tiles_ completely, we can
just count the set bits in impression_was_logged_ :)
I've moved has_*_suggestions_ too, for consistency - it also shouldn't matter,
and no reason to keep this one thing separate.

[sfiera, 2016/10/24 14:41:11]

Even better!

+
   if ((position >= kNumMostVisited) || impression_was_logged_[position]) {
     return;
   }