Record impressions/navigations only once per tile.

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 16f07deb6d65b1268e970763d13c5cfb7f3c306b..b11c8f514ba99941a87a096c7edd0adaafaf62a2 100644
--- a/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
+++ b/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
@@ -81,6 +81,8 @@ NTPUserDataLogger::~NTPUserDataLogger() {}
 // static
 NTPUserDataLogger* NTPUserDataLogger::GetOrCreateFromWebContents(
       content::WebContents* content) {
+  DCHECK(search::IsInstantNTP(content));
+
   // Calling CreateForWebContents when an instance is already attached has no
   // effect, so we can do this.
   NTPUserDataLogger::CreateForWebContents(content);
@@ -89,10 +91,19 @@ NTPUserDataLogger* NTPUserDataLogger::GetOrCreateFromWebContents(
   // We record the URL of this NTP in order to identify navigations that
   // originate from it. We use the NavigationController's URL since it might
   // differ from the WebContents URL which is usually chrome://newtab/.
+  //
+  // We update the NTP URL every time this function is called, because the NTP
+  // URL sometimes changes while it is open, and we care about the final one for
+  // detecting when the user leaves or returns to the NTP. In particular, if the
+  // Google URL changes (e.g. google.com -> google.de), then we fall back to the
+  // local NTP.
   const content::NavigationEntry* entry =
       content->GetController().GetVisibleEntry();
-  if (entry)
+  if (entry && (logger->ntp_url_ != entry->GetURL())) {
+    DVLOG(1) << "NTP URL changed from \"" << logger->ntp_url_ << "\" to \""
+             << entry->GetURL() << "\"";
     logger->ntp_url_ = entry->GetURL();
+  }
 
   return logger;
 }
@@ -123,6 +134,12 @@ void NTPUserDataLogger::LogEvent(NTPLoggingEventType event,
 
 void NTPUserDataLogger::LogMostVisitedImpression(
     int position, NTPLoggingTileSource tile_source) {
+  if ((position >= static_cast<int>(impression_was_logged_.size())) ||
+      (impression_was_logged_[position])) {
+    return;
+  }
+  impression_was_logged_[position] = true;
+
   UMA_HISTOGRAM_ENUMERATION(kMostVisitedImpressionHistogramName, position,
                             kNumMostVisited);
 
@@ -160,7 +177,8 @@ void NTPUserDataLogger::LogMostVisitedNavigation(
 }
 
 NTPUserDataLogger::NTPUserDataLogger(content::WebContents* contents)
-    : has_server_side_suggestions_(false),
+    : content::WebContentsObserver(contents),
+      has_server_side_suggestions_(false),
       has_client_side_suggestions_(false),
       number_of_tiles_(0),
       has_emitted_(false),
@@ -186,10 +204,28 @@ NTPUserDataLogger::NTPUserDataLogger(content::WebContents* contents)
   }
 }
 
+// content::WebContentsObserver override
+void NTPUserDataLogger::NavigationEntryCommitted(
+    const content::LoadCommittedDetails& load_details) {
+  // User is returning to NTP, probably via the back button; reset stats.
+  if (load_details.previous_url.is_valid() &&
+      load_details.entry->GetURL().is_valid() &&
+      (load_details.entry->GetURL() == ntp_url_)) {
+    DVLOG(1) << "Returning to New Tab Page";
+    impression_was_logged_.reset();
+    has_emitted_ = false;
+    number_of_tiles_ = 0;
+    has_server_side_suggestions_ = true;
+    has_client_side_suggestions_ = true;

[Marc Treib, 2016/07/15 10:04:46]

These should be set to false

[sfiera, 2016/07/15 10:26:00]

Hein, that's embarrassing.

I had looked for a way to trigger NavigationEntryCommitted() from the test to
check this behavior, but I didn't see any way to create load_details.entry.
Found a workaround.

(although it's still not properly tested, because there's no test on load times
and I don't know the right test APIs for histograms to add it)

[Marc Treib, 2016/07/15 11:47:37]

Nice, I actually find that easier to read than the previous code.
There's some examples of timing-related tests here:
https://cs.chromium.org/chromium/src/components/ntp_snippets/ntp_snippets_fet...

You don't have to add any LoadTime tests in this CL though.

+  }
+}
+
 void NTPUserDataLogger::EmitNtpStatistics(base::TimeDelta load_time) {
   // We only send statistics once per page.
   if (has_emitted_)
     return;
+  DVLOG(1) << "Emitting NTP load time: " << load_time << ", "
+           << "number of tiles: " << number_of_tiles_;
 
   logLoadTimeHistogram("NewTabPage.LoadTime", load_time);