Pass DomContentLoaded event from prerenders to launching elements.

Pass DomContentLoaded event from prerenders to launching elements.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250573

[tburkard, 2014-02-10 10:36:44]

lgtm

but gavinp should take a look, too.

[gavinp, 2014-02-10 17:06:11]

LGTM. Because of the IPC change, you'll need a security review, too.

[ksimbili, 2014-02-10 17:24:44]

Adding Justin Schuh for the security review because of IPC change.

[davidben, 2014-02-10 18:22:07]

Just a drive-by comment.

https://codereview.chromium.org/133073021/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/133073021/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_contents.cc:601:
NotifyPrerenderDomContentLoaded();
Won't this fire for every frame? I expect you want to limit this to just the
main frame. Looks like chrome/browser/guestview/webview/webview_guest.cc saves
the main frame's ID on DidCommitProvisionalLoadForFrame.

[davidben, 2014-02-10 18:27:46]

On 2014/02/10 18:22:07, David Benjamin wrote:
> Just a drive-by comment.
> 
>
https://codereview.chromium.org/133073021/diff/20001/chrome/browser/prerender...
> File chrome/browser/prerender/prerender_contents.cc (right):
> 
>
https://codereview.chromium.org/133073021/diff/20001/chrome/browser/prerender...
> chrome/browser/prerender/prerender_contents.cc:601:
> NotifyPrerenderDomContentLoaded();
> Won't this fire for every frame? I expect you want to limit this to just the
> main frame. Looks like chrome/browser/guestview/webview/webview_guest.cc saves
> the main frame's ID on DidCommitProvisionalLoadForFrame.

Hrm, that also explains why some of your browser_tests are failing. :-) Though
it looks like a couple others are failing because they abort between
DOMContentLoaded and load? I usually run prerender tests locally with

in-xephyr ./out/Debug/browser_tests --gtest_filter=Prerender*.*
--test-launcher-jobs=20

Where in-xephyr is a little script to keep the windows from popping up
everywhere:
#!/bin/sh
CHILD_DISPLAY=:1
Xephyr "$CHILD_DISPLAY" &
XEPHYR_PID=$!
export DISPLAY="$CHILD_DISPLAY"
sleep 1s  # Hack to ensure Xephyr is ready.
metacity 2>/dev/null &
"$@"
kill "$XEPHYR_PID"

[jschuh, 2014-02-10 19:01:16]

ipc security lgtm: event message with no new attack surface.

[ksimbili, 2014-02-11 01:03:58]

https://codereview.chromium.org/133073021/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/133073021/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_contents.cc:601:
NotifyPrerenderDomContentLoaded();
On 2014/02/10 18:22:07, David Benjamin wrote:
> Won't this fire for every frame? I expect you want to limit this to just the
> main frame. Looks like chrome/browser/guestview/webview/webview_guest.cc saves
> the main frame's ID on DidCommitProvisionalLoadForFrame.

Done.

[ksimbili, 2014-02-11 01:04:09]

The CQ bit was checked by ksimbili@google.com

[ksimbili, 2014-02-11 01:04:14]

The CQ bit was unchecked by ksimbili@google.com

[ksimbili, 2014-02-11 03:31:53]

The CQ bit was checked by ksimbili@google.com

[commit-bot: I haz the power, 2014-02-11 03:32:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ksimbili@google.com/133073021/150001

[ksimbili, 2014-02-11 03:37:08]

The CQ bit was unchecked by ksimbili@google.com

[davidben, 2014-02-11 17:04:33]

Hrm, you seem to still have a few of the tests flaking. It's just showing up
green because the bots re-run the tests. A lot of the failure cases are not
ordered with respect to load or DOMContentLoaded events. I've tidied up the ones
that aren't ordered w.r.t. load (although I suspect that just punting the load
counts altogether in the failure case would be better).

Anyway, for now, maybe remove the machinery that checks for DOMContentLoaded
from the common case and just put it in the two cases below. That should give
you decent coverage: one to test a normal load and one to differentiate from the
load event.

https://codereview.chromium.org/133073021/diff/150001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/133073021/diff/150001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1664:
PrerenderTestURL("files/prerender/prerender_page.html", FINAL_STATUS_USED, 1);
Maybe a manual assertion after this line to test the basic case.

https://codereview.chromium.org/133073021/diff/150001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:3711:
WaitForASCIITitle(prerender->contents()->prerender_contents(), kReadyTitle);
This would be a pretty good place to assert DOMContentLoaded. This test is one
where, before the swap, DOMContentLoaded will eventually run, but load will
never run before the swap. So after the WaitForASCIITitle, assert that you've
gotten one webkitprerenderdomcontentloaded event.

Strictly speaking, image2 in prerender_deferred_image.html and the DOM parse
aren't ordered either. Maybe add a WaitForDOMContentLoads() to mirror
WaitForLoads in TestPrerender to be safe? Though I would be kinda surprised if
that one flaked.

I'm working on a CL as part of fixing some tests that'll let us wait on the
loader receiving the events which is probably closer to what we want anyway, but
that's not uploaded yet...

[ksimbili, 2014-02-11 20:05:56]

https://codereview.chromium.org/133073021/diff/150001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/133073021/diff/150001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1664:
PrerenderTestURL("files/prerender/prerender_page.html", FINAL_STATUS_USED, 1);
On 2014/02/11 17:04:33, David Benjamin wrote:
> Maybe a manual assertion after this line to test the basic case.

Done.

https://codereview.chromium.org/133073021/diff/150001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:3711:
WaitForASCIITitle(prerender->contents()->prerender_contents(), kReadyTitle);
On 2014/02/11 17:04:33, David Benjamin wrote:
> This would be a pretty good place to assert DOMContentLoaded. This test is one
> where, before the swap, DOMContentLoaded will eventually run, but load will
> never run before the swap. So after the WaitForASCIITitle, assert that you've
> gotten one webkitprerenderdomcontentloaded event.
> 
> Strictly speaking, image2 in prerender_deferred_image.html and the DOM parse
> aren't ordered either. Maybe add a WaitForDOMContentLoads() to mirror
> WaitForLoads in TestPrerender to be safe? Though I would be kinda surprised if
> that one flaked.
> 
> I'm working on a CL as part of fixing some tests that'll let us wait on the
> loader receiving the events which is probably closer to what we want anyway,
but
> that's not uploaded yet...

Added test here. I'll add WaitForDomContentLoads, if this is flaky.

[davidben, 2014-02-11 20:18:39]

LGTM with minor nits.

Also queued up a few try bot runs for you... at least, I think I did? I've never
actually started try runs from Rietveld before. :-)

https://codereview.chromium.org/133073021/diff/340001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/133073021/diff/340001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1634:
EXPECT_EQ(GetPrerenderDomContentLoadedEventCountForLinkNumber(0), 1);
Nit: EXPECT_EQ(1, GetPrerenderblah). The first argument is the expected value,
the second is the actual. (They display differently in the case of failures.)

https://codereview.chromium.org/133073021/diff/340001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:3676:
EXPECT_EQ(GetPrerenderDomContentLoadedEventCountForLinkNumber(0), 1);
Nit: Ditto.

[ksimbili, 2014-02-11 21:16:58]

https://codereview.chromium.org/133073021/diff/340001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/133073021/diff/340001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1634:
EXPECT_EQ(GetPrerenderDomContentLoadedEventCountForLinkNumber(0), 1);
On 2014/02/11 20:18:40, David Benjamin wrote:
> Nit: EXPECT_EQ(1, GetPrerenderblah). The first argument is the expected value,
> the second is the actual. (They display differently in the case of failures.)

Done.

https://codereview.chromium.org/133073021/diff/340001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:3676:
EXPECT_EQ(GetPrerenderDomContentLoadedEventCountForLinkNumber(0), 1);
On 2014/02/11 20:18:40, David Benjamin wrote:
> Nit: Ditto.

Done.

[ksimbili, 2014-02-11 22:08:36]

The CQ bit was checked by ksimbili@google.com

[commit-bot: I haz the power, 2014-02-11 22:23:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ksimbili@google.com/133073021/430001

[commit-bot: I haz the power, 2014-02-12 00:32:12]

Change committed as 250573