Cleanup desktop NTP metrics recording, part 2

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_macros.h"
@@ skipping 104 matching lines @@
     DVLOG(1) << "NTP URL changed from \"" << logger->ntp_url_ << "\" to \""
              << entry->GetURL() << "\"";
     logger->ntp_url_ = entry->GetURL();
   }
 
   return logger;
 }
 
 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;
   }
   impression_was_logged_[position] = true;
 
   UMA_HISTOGRAM_ENUMERATION(kMostVisitedImpressionHistogramName, position,
                             kNumMostVisited);
 
   // Cannot rely on UMA histograms macro because the name of the histogram is
   // generated dynamically.
@@ skipping 85 matching lines @@
 
   has_server_side_suggestions_ = false;
   has_client_side_suggestions_ = false;
   UMA_HISTOGRAM_CUSTOM_COUNTS(
       "NewTabPage.NumberOfTiles", number_of_tiles_, 1, kNumMostVisited,
       kNumMostVisited + 1);
   number_of_tiles_ = 0;
   has_emitted_ = true;
   during_startup_ = false;
 }