Third-party NTPs: Set "instant support" flag earlier.

chrome/browser/resources/local_ntp/most_visited_util.js

Index: chrome/browser/resources/local_ntp/most_visited_util.js
diff --git a/chrome/browser/resources/local_ntp/most_visited_util.js b/chrome/browser/resources/local_ntp/most_visited_util.js
index 5620d5f02468a648dabfe5f8028c4ab8ce664ebc..b05fad9df8c04cd689ba92491bd7f43bfd84eec7 100644
--- a/chrome/browser/resources/local_ntp/most_visited_util.js
+++ b/chrome/browser/resources/local_ntp/most_visited_util.js
@@ -7,10 +7,24 @@
  * @fileoverview Utilities for rendering most visited thumbnails and titles.
  */
 
+// Don't remove; see crbug.com/678778.
 // <include src="instant_iframe_validation.js">
 
 
 /**
+  * The different types of events that are logged from the NTP. The multi-iframe
+  * version of the NTP does *not* actually log any statistics anymore; this is
+  * only required as a workaround for crbug.com/698675.
+  * Note: Keep in sync with common/ntp_logging_events.h
+  * @enum {number}
+  * @const
+  */
+var NTP_LOGGING_EVENT_TYPE = {
+  NTP_ALL_TILES_RECEIVED: 12,
+};
+
+
+/**
  * The origin of this request.
  * @const {string}
  */
@@ -200,6 +214,14 @@ function fillMostVisited(location, fill) {
   params.rid = parseInt(params.rid, 10);
   if (!isFinite(params.rid) && !params.url)
     return;
+
+  // Note: This does not actually result in any logging; it's a workaround for
+  // crbug.com/698675. It effectively sets the "instant support" state of the
+  // page to true, which makes later calls to fetch the most visited items

[sfiera, 2017/03/06 15:48:01]

"of the page": are we setting it for the page, or the iframe, or does this
question not make sense?

[Marc Treib, 2017/03/06 18:59:18]

It's a property of the whole Tab/WebContents, i.e. both the page and the
iframe(s). Though the distinction really only makes sense for the host page -
the iframe(s) will only exist if the host page knows about Instant.

[sfiera, 2017/03/07 15:07:59]

Then, should the comment here read "state of the tab"?

This code is running in every iframe of the multi-iframe NTP, right? (and that's
the implementation we're concerned about, since this is a fix for the Bing NTP)
So, the first iframe would set it for the tab; for the others it's already true?
What does "knows about Instant" mean here? Anyone can create an iframe, but I
guess the iframe won't load without Instant. So, should we be making
GetMostVisitedItemData not care about whether Instant is enabled?

I still don't have a real understanding of what Instant is.

[Marc Treib, 2017/03/07 16:08:19]

Done.
Yes.
Not any random page can include this particular iframe, because it's served from
chrome-search://. See "Instant process" below.

GetMostVisitedItemData already doesn't care about the state of the "page
supports instant" flag. But if the flag is false, then we don't send the MV
items to the renderer process, so GetMostVisitedItemData has nothing to hand
out.
Yeah, that's because the term is used for different things, many of which don't
even make sense anymore :(

At least two things are relevant here:

"Instant process": NTPs (and possibly SERPs) belonging to the default search
engine get assigned to an "Instant process", which has some special privileges
(such as access to the chrome.embeddedSearch APIs, and the MV iframes).

"page supports Instant": Now, a page getting assigned to an Instant process
doesn't necessarily mean the page knows/cares about the embeddedSearch APIs. So
we have a few (IMO hacky) ways to detect that the page does, in fact, care, e.g.
when it calls such an API. Only after we detect that the page cares, we send the
MV items to the renderer process, so that GetMostVisitedItemData can hand them
out.

If you're really interested, I can try to explain more in person (the subset of
things that I'm aware of, anyway).

[sfiera, 2017/03/07 16:26:24]

OK, so it sounds like "page supports Instant" is not really what we should care
about here. What we're interested in is if the page is an NTP. A SERP might use
embeddedSearch APIs ("supporting instant") yet we shouldn't necessarily let it
embed MV iframes. An NTP might not use embeddedSearch (Bing) but it should get
tiles.

[Marc Treib, 2017/03/07 16:33:55]

Well, the problem is really that "Instant" means different, mostly
non-overlapping things. The embeddedSearch APIs include the MV stuff (there's
embeddedSearch.searchBox and embeddedSearch.newTabPage), even though it's really
unrelated to the original meaning of "Instant". So "does this page support
Instant" isn't even a well-formed question.

+  // succeed.
+  chrome.embeddedSearch.newTabPage.logEvent(
+      NTP_LOGGING_EVENT_TYPE.NTP_ALL_TILES_RECEIVED);

[sfiera, 2017/03/06 15:48:01]

Would it make more sense to put this right before the call to
apiHandle.getMostVisitedItemData()?

[Marc Treib, 2017/03/06 18:59:18]

I haven't tried, but I think that's too late: This triggers IPC to send the MV
sites from the browser to the renderer process. Presumably that'd take a while.
(As I mentioned in the description, I think it'll still be racy anyways, but
here it seems to work well enough.)

[sfiera, 2017/03/07 15:07:59]

It looks to me that the only things happening between here and the apiHandle
call are variable declarations.

Shouldn't IPC messages be received and processed in-order?

[Marc Treib, 2017/03/07 16:08:19]

D'oh, somehow I missed that that other call is just a few lines below. Moved
this one down there.
I.. guess? It doesn't matter here, because getMostVisitedItemData doesn't result
in any IPC. The renderer process just hands out whatever MV items it has cached.

+
   var data;
   if (params.url) {
     // Means that the suggestion data comes from the server. Create data object.