Pass load events from prerenders to launching elements.

Pass load events from prerenders to launching elements.

The event is already on the webkit side, this patch plumbs it through. Note that this patch is downstream from https://codereview.chromium.org/11551003/ and so it can't land until after that one.

R=mmenke@chromium.org,jschuh@chromium.org
BUG=None


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

[gavinp, 2012-12-18 20:45:55]

Matt: PTAL. I'd be happier with better testing coverage here... Oh boy, I would
be.

jschuh: PTAL. Note these events, like the last few of these you looked at, are
browser->renderer, to send a single unparameterized event onto an element.

[jschuh, 2012-12-18 21:06:27]

lgtm

[gavinp, 2012-12-19 18:26:32]

On 2012/12/18 21:06:27, Justin Schuh wrote:
> lgtm

Matt: hold off on taking a closer look at this until I get the testing better.
I'd like to be guarded against this event regressing, so I'm going to take some
time to think about a better browser test, and possibly unit test coverage
(although that's way trickier).

[gavinp, 2012-12-20 20:41:09]

Matt,

This is ready for you to look at now. I've cleaned up the event testing API as
well; it was silly.

[mmenke, 2012-12-27 19:11:08]

This LGTM, just fairly minor comments.  I'll plan to take another look once
you've responded, but think it's safe to land without the second look.

https://codereview.chromium.org/11571037/diff/16016/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/11571037/diff/16016/chrome/browser/prerender/...
chrome/browser/prerender/prerender_browsertest.cc:2439:
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderEventsNoLoad) {
Think it's worth mentioning that this test may pass flakily on regression, to
encourage filing a bug on intermittent failures.

https://codereview.chromium.org/11571037/diff/16016/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11571037/diff/16016/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:55: // The prerender should
only be null in unit tests.
Do unit tests actually simulate this event?

https://codereview.chromium.org/11571037/diff/16016/chrome/test/data/prerende...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/11571037/diff/16016/chrome/test/data/prerende...
chrome/test/data/prerender/prerender_events_common.js:10: var
hadPrerenderEventErrors = false;
Think it's worth having a comment that enumerates the errors it detects
(Currently just event ordering errors, of course).  If you think it's unlikely
we'll ever be looking for other errors, could just rename it to
"hadPrerenderEventOrderingErrors".

https://codereview.chromium.org/11571037/diff/16016/chrome/test/data/prerende...
chrome/test/data/prerender/prerender_events_common.js:36:
receivedPrerenderStopEvents[index] = true;
Just for completeness, suggest checking to make sure we've received the start
event, and not received the stop event.  That way, we could actually test just
hadPrerenderEventErrors on all tests, if we want, and have a little more test
coverage.

[gavinp, 2012-12-28 13:23:15]

Matt,

Thanks for your review. I'm not in a rush to land this, so please go ahead and
take a second look before I CQ+ it if you have the time.

Since this patch depends on https://codereview.chromium.org/11551003/ , which
hasn't been reviewed, I can't put it in the queue until that's done, without at
least a little rebasing, which seems silly to do.

https://codereview.chromium.org/11571037/diff/16016/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/11571037/diff/16016/chrome/browser/prerender/...
chrome/browser/prerender/prerender_browsertest.cc:2439:
IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderEventsNoLoad) {
On 2012/12/27 19:11:08, Matt Menke wrote:
> Think it's worth mentioning that this test may pass flakily on regression, to
> encourage filing a bug on intermittent failures.

Done.

https://codereview.chromium.org/11571037/diff/16016/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11571037/diff/16016/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:55: // The prerender should
only be null in unit tests.
On 2012/12/27 19:11:08, Matt Menke wrote:
> Do unit tests actually simulate this event?

Yes. See PrerenderDispatcherTest.PrerenderDispatcherSingleRemove.

https://codereview.chromium.org/11571037/diff/16016/chrome/test/data/prerende...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/11571037/diff/16016/chrome/test/data/prerende...
chrome/test/data/prerender/prerender_events_common.js:10: var
hadPrerenderEventErrors = false;
On 2012/12/27 19:11:08, Matt Menke wrote:
> Think it's worth having a comment that enumerates the errors it detects
> (Currently just event ordering errors, of course).  If you think it's unlikely
> we'll ever be looking for other errors, could just rename it to
> "hadPrerenderEventOrderingErrors".

Done.

https://codereview.chromium.org/11571037/diff/16016/chrome/test/data/prerende...
chrome/test/data/prerender/prerender_events_common.js:36:
receivedPrerenderStopEvents[index] = true;
On 2012/12/27 19:11:08, Matt Menke wrote:
> Just for completeness, suggest checking to make sure we've received the start
> event, and not received the stop event.  That way, we could actually test just
> hadPrerenderEventErrors on all tests, if we want, and have a little more test
> coverage.

Done. Good thinking.

[mmenke, 2012-12-28 13:59:50]

On Fri, Dec 28, 2012 at 8:23 AM, <gavinp@chromium.org> wrote:

> Matt,
>
> Thanks for your review. I'm not in a rush to land this, so please go ahead
> and
> take a second look before I CQ+ it if you have the time.
>
> Since this patch depends on
https://codereview.chromium.**org/11551003/<https://codereview.chromium.org/1...,
which
> hasn't been reviewed, I can't put it in the queue until that's done,
> without at
> least a little rebasing, which seems silly to do.
>

Ahh - thanks.  Thought I had one more in the queue, but missed it, since I
have pending comments, and it was "hidden" at the top.

[mmenke, 2012-12-28 18:48:36]

https://codereview.chromium.org/11571037/diff/16016/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11571037/diff/16016/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:55: // The prerender should
only be null in unit tests.
On 2012/12/28 13:23:15, gavinp wrote:
> On 2012/12/27 19:11:08, Matt Menke wrote:
> > Do unit tests actually simulate this event?
> 
> Yes. See PrerenderDispatcherTest.PrerenderDispatcherSingleRemove.

It calls OnPrerenderStop, not OnPrerenderStopLoading (You'd actually need a new
unit test to call this function, since it didn't exist before, and is only
called by OnControlMessageReceived)

https://codereview.chromium.org/11571037/diff/41001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/11571037/diff/41001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_browsertest.cc:2444: // so plesae be
aggressive about filing bugs when this test is failing.
nit:  please

[gavinp, 2012-12-28 20:13:16]

Gah, good catch on the dispatcher. Thanks.

I'm letting this bake as it's waiting for the slots change; but as things are
you LGTMed and I'm assuming that's still good if you don't tell me it's not.

The slots change is not yet LGTMed; once it is, expect me to CQ this not long
after LKGR advances.

https://codereview.chromium.org/11571037/diff/16016/chrome/renderer/prerender...
File chrome/renderer/prerender/prerender_dispatcher.cc (right):

https://codereview.chromium.org/11571037/diff/16016/chrome/renderer/prerender...
chrome/renderer/prerender/prerender_dispatcher.cc:55: // The prerender should
only be null in unit tests.
On 2012/12/28 18:48:36, Matt Menke wrote:
> On 2012/12/28 13:23:15, gavinp wrote:
> > On 2012/12/27 19:11:08, Matt Menke wrote:
> > > Do unit tests actually simulate this event?
> > 
> > Yes. See PrerenderDispatcherTest.PrerenderDispatcherSingleRemove.
> 
> It calls OnPrerenderStop, not OnPrerenderStopLoading (You'd actually need a
new
> unit test to call this function, since it didn't exist before, and is only
> called by OnControlMessageReceived)

Right. And since this function is a NOP and untestable from the unit test, I've
upgraded this test to a DCHECK().

https://codereview.chromium.org/11571037/diff/41001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/11571037/diff/41001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_browsertest.cc:2444: // so plesae be
aggressive about filing bugs when this test is failing.
On 2012/12/28 18:48:36, Matt Menke wrote:
> nit:  please

Done.

[mmenke, 2012-12-28 20:14:37]

On 2012/12/28 20:13:16, gavinp wrote:
> Gah, good catch on the dispatcher. Thanks.
> 
> I'm letting this bake as it's waiting for the slots change; but as things are
> you LGTMed and I'm assuming that's still good if you don't tell me it's not.
> 
> The slots change is not yet LGTMed; once it is, expect me to CQ this not long
> after LKGR advances.

Yea, this does indeed still LGTM

[commit-bot: I haz the power, 2012-12-31 15:49:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11571037/30036

[commit-bot: I haz the power, 2012-12-31 16:36:15]

Retried try job too often on linux_chromeos for step(s) unit_tests

[commit-bot: I haz the power, 2013-01-01 17:02:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11571037/30036

[commit-bot: I haz the power, 2013-01-01 18:50:54]

Retried try job too often on mac_rel for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-02 13:06:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11571037/30036

[commit-bot: I haz the power, 2013-01-02 14:56:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11571037/30036

[commit-bot: I haz the power, 2013-01-02 17:11:01]

Retried try job too often on linux_rel for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-02 18:09:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11571037/30036

[commit-bot: I haz the power, 2013-01-02 20:37:12]

Retried try job too often on linux_rel for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-02 21:57:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11571037/30036

[gavinp, 2013-01-02 21:57:58]

On 2013/01/02 20:37:12, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_rel for step(s) browser_tests

That failure looks like it might be prerender related. I'm checking again, and
running more tests. I wonder if other fixes have revealed a problem here.. The
passes on this bot were all before the recurse fix landed, for instance.

[commit-bot: I haz the power, 2013-01-02 23:35:17]

Retried try job too often on linux_rel for step(s) browser_tests

[gavinp, 2013-01-03 00:42:14]

On 2013/01/02 23:35:17, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_rel for step(s) browser_tests

Definitely this CL is adding flakiness to the browsers tests. It's a nasty
flakiness that was already there, but it must be fixed. I'm adding some waits
for destruction, that should improve things.

[gavinp, 2013-01-03 17:20:09]

I found the flakiness, it was a silly DCHECK. The print test was the perfect one
to reveal it: it stops loading by dying.

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

https://codereview.chromium.org/11571037/diff/66004/chrome/browser/prerender/...
chrome/browser/prerender/prerender_contents.cc:475: DCHECK_EQ(FINAL_STATUS_MAX,
final_status_);
This simply isn't always true.

[commit-bot: I haz the power, 2013-01-03 17:20:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/11571037/62005

[mmenke, 2013-01-03 17:23:47]

Good find.

https://chromiumcodereview.appspot.com/11571037/diff/66004/chrome/browser/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

https://chromiumcodereview.appspot.com/11571037/diff/66004/chrome/browser/pre...
chrome/browser/prerender/prerender_contents.cc:475: DCHECK_EQ(FINAL_STATUS_MAX,
final_status_);
On 2013/01/03 17:20:10, gavinp wrote:
> This simply isn't always true.

My understanding of the world will never be the same, due to this earth
shattering revelation.

[gavinp, 2013-01-03 17:30:48]

On 2013/01/03 17:23:47, Matt Menke wrote:
> Good find.
> 
>
https://chromiumcodereview.appspot.com/11571037/diff/66004/chrome/browser/pre...
> File chrome/browser/prerender/prerender_contents.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11571037/diff/66004/chrome/browser/pre...
> chrome/browser/prerender/prerender_contents.cc:475:
DCHECK_EQ(FINAL_STATUS_MAX,
> final_status_);
> On 2013/01/03 17:20:10, gavinp wrote:
> > This simply isn't always true.
> 
> My understanding of the world will never be the same, due to this earth
> shattering revelation.

I have traveled through time from 1976 to share this fact with you.

[commit-bot: I haz the power, 2013-01-03 21:05:23]

Change committed as 175011