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 6773db8f0576592b652752373fffc45cf165fe34..ab4587c3049889f6e1a6dcbc89083b530712b12a 100644
--- a/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
+++ b/chrome/browser/ui/webui/ntp/ntp_user_data_logger.cc
@@ -89,10 +89,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.

[Marc Treib, 2016/07/12 08:17:46]

...wait, what? It can change while the NTP is open?!

[sfiera, 2016/07/12 08:37:33]

Yep. Reproduction:

1. Start Chromium with a clean profile
2. Switch to the second tab ("Getting Started")
3. Go to chromium.org or wherever
4. Switch back to the first tab
5. Wait (1 to 20 seconds)
6. Ta-da! Tiles have changed.

In fact, new tiles are sent every time TopSites/MostLikely changes, even if
there isn't an NTP active in the tab, it's just that without the NTP nothing
will be listening for them. (that's where http://crbug.com/626681 comes from)

[Marc Treib, 2016/07/12 08:58:47]

The *tiles* can change, but the NTP URL? I guess it can happen when there's a
redirect from remote to local NTP? (That happens when the default search
provider changes, which happens a few seconds after startup - from google.com to
google.de.)
Yup, but that should be fixed by this CL, right?

[sfiera, 2016/07/12 09:08:58]

Oh, misread that. But still yes. On first launch, I get:

[89280:89280:0712/105122:VERBOSE1:ntp_user_data_logger.cc(101)] NTP URL changed
from "https://www.google.com/_/chrome/newtab?espv=2&ie=UTF-8" to
"chrome-search://local-ntp/local-ntp.html"

I don't get it on subsequent runs.


Weirdly, on changing the default search engine, and therefore the NTP, the tab
remains open but ceases to be an NTP: the Bing NTP is redirected to bing.com;
the Google NTP redirects to google.com. That's detected correctly as "leaving",
here.
Not fixed as far as I understand it. SearchTabHelper::MostVisitedItemsChanged()
will still call ipc_router_.SendMostVisitedItems(), but there's nothing
listening on the other end.

[Marc Treib, 2016/07/14 14:40:52]

On subsequent runs, the Google TLD doesn't change anymore :)
The code that does the redirect is here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_instant_contro...
That's also expected: The same code I linked above reloads the tab; the remote
NTP JS detects that the embeddedSearch API isn't available, and redirects to the
home page.
I meant that http://crbug.com/626681 (wrong NTP URL stored here) should be
fixed.
The MV items will still be sent which is kinda pointless, but shouldn't actually
hurt, right?

+  //
+  // TODO(sfiera): move to a world where the NTP URL is always chrome://newtab/.

[Marc Treib, 2016/07/12 08:17:46]

hahahahaha... wait, you're serious. Let me laugh even harder :D

[sfiera, 2016/07/14 14:11:08]

Restricted the TODO somewhat--I expect it's a non-goal to move external NTPs to
chrome://newtab/ anyway.

[Marc Treib, 2016/07/14 14:40:52]

External as in remote? No.
We might conceivably move the local NTP to chrome://newtab, but that might be
more trouble than it's worth - quite a lot of stuff builds on the
chrome-search:// scheme.

   const content::NavigationEntry* entry =
       content->GetController().GetVisibleEntry();
-  if (entry)
+  if (entry && (logger->ntp_url_ != entry->GetURL())) {
+    VLOG(1) << "NTP URL changed from \"" << logger->ntp_url_ << "\" to \""
+            << entry->GetURL() << "\"";
     logger->ntp_url_ = entry->GetURL();
+  }
 
   return logger;
 }
@@ -129,6 +138,13 @@ void NTPUserDataLogger::LogEvent(NTPLoggingEventType event,
 
 void NTPUserDataLogger::LogMostVisitedImpression(
     int position, NTPLoggingTileSource tile_source) {
+  if (position >= static_cast<int>(impression_was_logged_.size())) {

[Marc Treib, 2016/07/12 08:17:46]

Hm, might as well just resize this to kNumMostVisited once

[sfiera, 2016/07/14 14:11:07]

Do we have a guarantee that we can't be called with >= 8? If an external NTP
creates a 9th tile, could we get here? (external NTPs still log impressions, I
think)

[Marc Treib, 2016/07/14 14:40:52]

No guarantee, but the histograms support only 8 bins anyway. So if the position
is >= 8, we might just early-out.

[sfiera, 2016/07/15 09:55:10]

Fixed to be 8 always.

+    impression_was_logged_.resize(position + 1);
+  } else if (impression_was_logged_[position]) {
+    return;
+  }
+  impression_was_logged_[position] = true;
+
   UMA_HISTOGRAM_ENUMERATION(kMostVisitedImpressionHistogramName, position,
                             kNumMostVisited);
 
@@ -165,14 +181,30 @@ void NTPUserDataLogger::LogMostVisitedNavigation(
   content::RecordAction(base::UserMetricsAction("MostVisited_Clicked"));
 }
 
+// content::WebContentsObserver override

[Marc Treib, 2016/07/12 08:17:46]

Any reason for moving this up here? (Bad rebase? I recently moved it down, when
I made it private)

[sfiera, 2016/07/14 14:11:07]

Yep, that. Down.

+void NTPUserDataLogger::NavigationEntryCommitted(
+    const content::LoadCommittedDetails& load_details) {
+  // User is navigating away from NTP; log stats.
+  if (load_details.previous_url.is_valid()

[Marc Treib, 2016/07/12 08:17:46]

We could keep the early-out if the previous URL isn't valid, and make things a
bit easier to read.

[sfiera, 2016/07/14 14:11:07]

This conditional is now gone, because we don't emit stats here, so I left the
other one untouched.

+      && search::MatchesOriginAndPath(ntp_url_, load_details.previous_url)) {
+    VLOG(1) << "Leaving New Tab Page";
+    EmitNtpStatistics(EmitReason::NAVIGATED_AWAY);

[Marc Treib, 2016/07/12 08:17:46]

Is there no "return" here on purpose? (So that a reload will both emit and reset
stats?) If so, that might be worth a comment.

[sfiera, 2016/07/14 14:11:07]

Conditional is gone. (this did handle reload)

+  }
+
+  // User is returning to NTP, probably via the back button; reset stats for
+  // impressions.
+  if (load_details.previous_url.is_valid() &&
+      load_details.entry->GetURL().is_valid() &&
+      search::MatchesOriginAndPath(load_details.entry->GetURL(), ntp_url_)) {

[Marc Treib, 2016/07/12 08:17:46]

Fun fact: search::MatchesOriginAndPath isn't symmetric - it'll consider "http"
equal to "https" in only one direction; I can't remember which.
But do we even need this here? Couldn't we just compare the URLs for equality?

[sfiera, 2016/07/14 14:11:07]

Changed to ==.

+    VLOG(1) << "Returning to New Tab Page";
+    impression_was_logged_.clear();

[Marc Treib, 2016/07/12 08:17:46]

So LoadTime and NumberOfTiles won't be logged again, because has_emitted_ is
still true. Can we simply reset that too? Would be nice to be consistent.

[sfiera, 2016/07/14 14:11:07]

Reset has_emitted_.

+  }
+}
+
 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),
@@ -201,16 +233,6 @@ 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) {
   // We only send statistics once per page.
   // And we don't send if there are no tiles recorded.