Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2733873002:
Third-party NTPs: Set "instant support" flag earlier. (Closed)
|
Created:
3 years, 9 months ago by Marc Treib Modified:
3 years, 9 months ago Reviewers:
sfiera CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, donnd+watch_chromium.org, jfweitz+watch_chromium.org, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThird-party NTPs: Set "instant support" flag earlier.
This fixes a regression introduced in
https://codereview.chromium.org/2429523002, which removed some
unnecessary logging calls that turned out to have unexpected side
effects.
This CL is a workaround rather than a proper fix; as far as I can tell,
it'll still be racy, though no worse than it was before. In fact, it's
kind of incidental that this ever worked at all...
BUG=698675
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2733873002
Cr-Commit-Position: refs/heads/master@{#455416}
Committed: https://chromium.googlesource.com/chromium/src/+/196a572ea1ff4a260f80bb3f96ebd4ffeaed0f71
Patch Set 1 #Patch Set 2 : comment #
Total comments: 10
Patch Set 3 : review #
Messages
Total messages: 27 (15 generated)
Description was changed from ========== Third-party NTPs: Set "instant support" flag earlier. This fixes a regression introduced in https://codereview.chromium.org/2429523002, which removed some unnecessary logging calls that turned out to have unexpected side effects. This CL is a workaround rather than a proper fix; as far as I can tell, it'll still be racy, though no worse than it was before. In fact, it's kind of incidental that this ever worked at all... BUG=698675 ========== to ========== Third-party NTPs: Set "instant support" flag earlier. This fixes a regression introduced in https://codereview.chromium.org/2429523002, which removed some unnecessary logging calls that turned out to have unexpected side effects. This CL is a workaround rather than a proper fix; as far as I can tell, it'll still be racy, though no worse than it was before. In fact, it's kind of incidental that this ever worked at all... BUG=698675 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
treib@chromium.org changed reviewers: + sfiera@chromium.org
PTAL. Ugly hacks FTW.
Question: why don't we see this problem with Google-branded NTPs (local or remote)? Are Google NTPs slower?
https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:220: // page to true, which makes later calls to fetch the most visited items "of the page": are we setting it for the page, or the iframe, or does this question not make sense? https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:223: NTP_LOGGING_EVENT_TYPE.NTP_ALL_TILES_RECEIVED); Would it make more sense to put this right before the call to apiHandle.getMostVisitedItemData()?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/06 15:42:53, sfiera wrote: > Question: why don't we see this problem with Google-branded NTPs (local or > remote)? Are Google NTPs slower? Nope, we're not just slower :) Multiple things here: - Our script for checking "does this page support Instant" (https://cs.chromium.org/chromium/src/chrome/renderer/searchbox/searchbox_exte...) checks for a particular thing which the Bing NTP doesn't use, so it says "false" there. I tried expanding that so it covers Bing too, but I failed - apparently the script is run before the Bing page has registered its callbacks etc. (It does work on Google NTPs.) - We also set the "supports Instant" flag when the page calls *some* APIs. I'm not sure if there's any sort of system to it. Proooobably we should clean that up eventually, but I'm a bit afraid that touching that stuff will open a huge can of worms. - The Bing NTP implements only some subset of the MV APIs; in particular it doesn't seem to care about onmostvisitedchange events. (I think.) If it did, maybe that would help too. But there's not much we can do about that, and anyway, there's still a bug on our side.
On 2017/03/06 18:49:15, Marc Treib wrote: > On 2017/03/06 15:42:53, sfiera wrote: > > Question: why don't we see this problem with Google-branded NTPs (local or > > remote)? Are Google NTPs slower? > > Nope, we're not just slower :) Multiple things here: > > - Our script for checking "does this page support Instant" > (https://cs.chromium.org/chromium/src/chrome/renderer/searchbox/searchbox_exte...) > checks for a particular thing which the Bing NTP doesn't use, so it says "false" > there. I tried expanding that so it covers Bing too, but I failed - apparently > the script is run before the Bing page has registered its callbacks etc. (It > does work on Google NTPs.) > > - We also set the "supports Instant" flag when the page calls *some* APIs. I'm > not sure if there's any sort of system to it. Proooobably we should clean that > up eventually, but I'm a bit afraid that touching that stuff will open a huge > can of worms. Expanding a bit more: Actually, I don't see a good reason to have that flag at all. There's a lot of plumbing to keep it updated in various places, but it doesn't seem to do very much - it's an optimization in that we won't send MV sites to the renderer process if the page won't use them anyway, which is of questionable value at best (that would occur when a third party search engine implements an NTP, but not any of the MV APIs). Maybe there are other reasons for it that I missed, or maybe there were some in the past. Anyway, that's a separate discussion, and I don't really want to spend time on that right now. Hence this minimal quick&dirty fix. > - The Bing NTP implements only some subset of the MV APIs; in particular it > doesn't seem to care about onmostvisitedchange events. (I think.) If it did, > maybe that would help too. But there's not much we can do about that, and > anyway, there's still a bug on our side.
https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:220: // page to true, which makes later calls to fetch the most visited items On 2017/03/06 15:48:01, sfiera wrote: > "of the page": are we setting it for the page, or the iframe, or does this > question not make sense? 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. https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:223: NTP_LOGGING_EVENT_TYPE.NTP_ALL_TILES_RECEIVED); On 2017/03/06 15:48:01, sfiera wrote: > Would it make more sense to put this right before the call to > apiHandle.getMostVisitedItemData()? 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.)
https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:220: // page to true, which makes later calls to fetch the most visited items On 2017/03/06 18:59:18, Marc Treib wrote: > On 2017/03/06 15:48:01, sfiera wrote: > > "of the page": are we setting it for the page, or the iframe, or does this > > question not make sense? > > It's a property of the whole Tab/WebContents, i.e. both the page and the > iframe(s). 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? > Though the distinction really only makes sense for the host page - > the iframe(s) will only exist if the host page knows about Instant. 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. https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:223: NTP_LOGGING_EVENT_TYPE.NTP_ALL_TILES_RECEIVED); On 2017/03/06 18:59:18, Marc Treib wrote: > On 2017/03/06 15:48:01, sfiera wrote: > > Would it make more sense to put this right before the call to > > apiHandle.getMostVisitedItemData()? > > 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.) 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?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Thanks! Comments addressed. https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:220: // page to true, which makes later calls to fetch the most visited items On 2017/03/07 15:07:59, sfiera wrote: > On 2017/03/06 18:59:18, Marc Treib wrote: > > On 2017/03/06 15:48:01, sfiera wrote: > > > "of the page": are we setting it for the page, or the iframe, or does this > > > question not make sense? > > > > It's a property of the whole Tab/WebContents, i.e. both the page and the > > iframe(s). > > Then, should the comment here read "state of the tab"? Done. > 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? Yes. > > Though the distinction really only makes sense for the host page - > > the iframe(s) will only exist if the host page knows about Instant. > > 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? 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. > I still don't have a real understanding of what Instant is. 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). https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:223: NTP_LOGGING_EVENT_TYPE.NTP_ALL_TILES_RECEIVED); On 2017/03/07 15:07:59, sfiera wrote: > On 2017/03/06 18:59:18, Marc Treib wrote: > > On 2017/03/06 15:48:01, sfiera wrote: > > > Would it make more sense to put this right before the call to > > > apiHandle.getMostVisitedItemData()? > > > > 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.) > > It looks to me that the only things happening between here and the apiHandle > call are variable declarations. D'oh, somehow I missed that that other call is just a few lines below. Moved this one down there. > Shouldn't IPC messages be received and processed in-order? 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.
LGTM https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:220: // page to true, which makes later calls to fetch the most visited items On 2017/03/07 16:08:19, Marc Treib wrote: > On 2017/03/07 15:07:59, sfiera wrote: > > I still don't have a real understanding of what Instant is. > > 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. 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. > If you're really interested, I can try to explain more in person (the subset of > things that I'm aware of, anyway).
https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... File chrome/browser/resources/local_ntp/most_visited_util.js (right): https://codereview.chromium.org/2733873002/diff/20001/chrome/browser/resource... chrome/browser/resources/local_ntp/most_visited_util.js:220: // page to true, which makes later calls to fetch the most visited items On 2017/03/07 16:26:24, sfiera wrote: > On 2017/03/07 16:08:19, Marc Treib wrote: > > On 2017/03/07 15:07:59, sfiera wrote: > > > I still don't have a real understanding of what Instant is. > > > > 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. > > 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. > > > If you're really interested, I can try to explain more in person (the subset > of > > things that I'm aware of, anyway). 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.
The CQ bit was checked by treib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488965414876080,
"parent_rev": "1d7e887175cca415da1ce23eea7ca52be0adacbf", "commit_rev":
"196a572ea1ff4a260f80bb3f96ebd4ffeaed0f71"}
Message was sent while issue was closed.
Description was changed from ========== Third-party NTPs: Set "instant support" flag earlier. This fixes a regression introduced in https://codereview.chromium.org/2429523002, which removed some unnecessary logging calls that turned out to have unexpected side effects. This CL is a workaround rather than a proper fix; as far as I can tell, it'll still be racy, though no worse than it was before. In fact, it's kind of incidental that this ever worked at all... BUG=698675 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Third-party NTPs: Set "instant support" flag earlier. This fixes a regression introduced in https://codereview.chromium.org/2429523002, which removed some unnecessary logging calls that turned out to have unexpected side effects. This CL is a workaround rather than a proper fix; as far as I can tell, it'll still be racy, though no worse than it was before. In fact, it's kind of incidental that this ever worked at all... BUG=698675 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2733873002 Cr-Commit-Position: refs/heads/master@{#455416} Committed: https://chromium.googlesource.com/chromium/src/+/196a572ea1ff4a260f80bb3f96eb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/196a572ea1ff4a260f80bb3f96eb... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
