Re-enable prerender RemovingLink browser tests.

Re-enable prerender RemovingLink browser tests.

The tests have changed significantly since they were first disabled. Add a
WaitForStop or two for good measure, but leave them as-is for the most part.
They can be disabled again if they still flake. Merge their custom prerender
loader into the main one; it's mostly the same.

In addition, for better test coverage, add a new test which asserts on events
received when a <link rel=prerender> is added for an existing prerender after
that prerender has loaded. Significantly rework the prerender events logic
to allow the test framework to wait on an event being received in the loader.

BUG=167340, 128841
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251495

[davidben, 2014-02-11 20:08:49]

A fair amount of machinery to just support one test, but probably useful? Means
we can later avoid making assumptions about how OnPrerenderStart flows into the
renderer.

[davidben, 2014-02-11 20:42:38]

On 2014/02/11 20:08:49, David Benjamin wrote:
> A fair amount of machinery to just support one test, but probably useful?
Means
> we can later avoid making assumptions about how OnPrerenderStart flows into
the
> renderer.

This will collide with https://codereview.chromium.org/133073021/. I expect the
other CL to land first, so I'll revise this when it does, but I don't expect the
changes to be serious. I'll a new entry in prerender_events_common.js for
webkitprerenderdomcontentloaded and also add a WaitForPrerenderEventsCount()
call in PrerenderDeferredImage to resolve the conceivable (but I doubt it can
actually happen) flake that CL will add in one of its assertions.

[mmenke, 2014-02-13 16:34:32]

This looks pretty reasonable.

While looking over the code, I noticed that
PrerenderHandle::AdoptPrerenderDataFrom seems to be unused now?  Can we just get
rid of it.

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

https://codereview.chromium.org/142013004/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:1716:
set_loader_query_and_fragment("?links_to_insert=1");
Since there's only one link, we can get rid of this.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:34:
GetPrerenderEventCount(index, 'webkitprerenderstop')) {
Can we get multiple load events?  I guess if a page does two loads?  Think it's
worth commenting on why we don't check if we already have a load event.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:39:
GetPrerenderEventCount(index, 'webkitprerenderstop')) {
Think it's worth commenting that there may or may not a load event, in this
case.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:46:
(receivedPrerenderEvents[ev.type][index] || 0) + 1;
nit:  +2 indent.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:48: // Run all callbacks.
Remove the ones that are done.
There's no documentation of how these callbacks should indicate that they're
done, that I can see.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:49:
prerenderEventCallbacks = prerenderEventCallbacks.filter(function(cb) {
nit:  "cb" is more often writen out.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
File chrome/test/data/prerender/prerender_page_pending.html (right):

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_page_pending.html:17: return
!GetPrerenderEventCount(0, 'webkitprerenderstart');
optional:  I think it may make more sense to check for event errors here and in
DidDisplay pass than in C++, just in case we ever reuse this file (Same goes for
the other one)

[davidben, 2014-02-14 01:01:13]

> While looking over the code, I noticed that
> PrerenderHandle::AdoptPrerenderDataFrom seems to be unused now?  Can we just
get
> rid of it.

Ooh, excellent. Yeah, I'll go punt that separately.

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

https://codereview.chromium.org/142013004/diff/60001/chrome/browser/prerender...
chrome/browser/prerender/prerender_browsertest.cc:1716:
set_loader_query_and_fragment("?links_to_insert=1");
On 2014/02/13 16:34:33, mmenke wrote:
> Since there's only one link, we can get rid of this.

Done.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:34:
GetPrerenderEventCount(index, 'webkitprerenderstop')) {
On 2014/02/13 16:34:33, mmenke wrote:
> Can we get multiple load events?  I guess if a page does two loads?  Think
it's
> worth commenting on why we don't check if we already have a load event.

Done. Yeah, multiple loads are possible.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:39:
GetPrerenderEventCount(index, 'webkitprerenderstop')) {
On 2014/02/13 16:34:33, mmenke wrote:
> Think it's worth commenting that there may or may not a load event, in this
> case.

Done.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:46:
(receivedPrerenderEvents[ev.type][index] || 0) + 1;
On 2014/02/13 16:34:33, mmenke wrote:
> nit:  +2 indent.

Done.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:48: // Run all callbacks.
Remove the ones that are done.
On 2014/02/13 16:34:33, mmenke wrote:
> There's no documentation of how these callbacks should indicate that they're
> done, that I can see.

Done.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:49:
prerenderEventCallbacks = prerenderEventCallbacks.filter(function(cb) {
On 2014/02/13 16:34:33, mmenke wrote:
> nit:  "cb" is more often writen out.

Done.

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
File chrome/test/data/prerender/prerender_page_pending.html (right):

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_page_pending.html:17: return
!GetPrerenderEventCount(0, 'webkitprerenderstart');
On 2014/02/13 16:34:33, mmenke wrote:
> optional:  I think it may make more sense to check for event errors here and
in
> DidDisplay pass than in C++, just in case we ever reuse this file (Same goes
for
> the other one)

Hrm. DidDisplayPass gets called before the pending prerender is necessarily
started though. Actually we currently don't test this page's prerender events at
all post-swap, which should be fixed...

This is actually an unrelated test, so I'll hold on that for a separate CL.

[mmenke, 2014-02-14 17:33:01]

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:35:
hadPrerenderEventErrors = true;
Why did you delete this block?

https://codereview.chromium.org/142013004/diff/230001/chrome/test/data/preren...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/142013004/diff/230001/chrome/test/data/preren...
chrome/test/data/prerender/prerender_events_common.js:21: // list.
"condition is satisfied" is still vague - doesn't really imply will never be
called again, and "callback" doesn't imply there's some "condition" to wait for.

Suggest just "Each callback returns true if it should never be called again, or
false if not."

Alternatively, could call them prerenderConditionCheckers or some such, but that
sounds a little weird.

[davidben, 2014-02-14 19:18:25]

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:35:
hadPrerenderEventErrors = true;
On 2014/02/14 17:33:01, mmenke wrote:
> Why did you delete this block?

The check for all the events other than webkitprerenderstart are identical, so I
just moved it to an else.

https://codereview.chromium.org/142013004/diff/230001/chrome/test/data/preren...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/142013004/diff/230001/chrome/test/data/preren...
chrome/test/data/prerender/prerender_events_common.js:21: // list.
On 2014/02/14 17:33:01, mmenke wrote:
> "condition is satisfied" is still vague - doesn't really imply will never be
> called again, and "callback" doesn't imply there's some "condition" to wait
for.
> 
> Suggest just "Each callback returns true if it should never be called again,
or
> false if not."
> 
> Alternatively, could call them prerenderConditionCheckers or some such, but
that
> sounds a little weird.

Went with the first one. prerenderConditionCheckers does sound weird.
prerenderEventConditions feels a little less wonky? shrug.

[mmenke, 2014-02-14 19:25:43]

LGTM

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/142013004/diff/60001/chrome/test/data/prerend...
chrome/test/data/prerender/prerender_events_common.js:35:
hadPrerenderEventErrors = true;
On 2014/02/14 19:18:26, David Benjamin wrote:
> On 2014/02/14 17:33:01, mmenke wrote:
> > Why did you delete this block?
> 
> The check for all the events other than webkitprerenderstart are identical, so
I
> just moved it to an else.

Good point.

https://codereview.chromium.org/142013004/diff/410002/chrome/test/data/preren...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/142013004/diff/410002/chrome/test/data/preren...
chrome/test/data/prerender/prerender_events_common.js:21: // list. These are
used to implement WaitForPrerenderEventCount.
Maybe "...  to remain in the list and be called on future events." ?

[davidben, 2014-02-14 19:38:12]

https://codereview.chromium.org/142013004/diff/410002/chrome/test/data/preren...
File chrome/test/data/prerender/prerender_events_common.js (right):

https://codereview.chromium.org/142013004/diff/410002/chrome/test/data/preren...
chrome/test/data/prerender/prerender_events_common.js:21: // list. These are
used to implement WaitForPrerenderEventCount.
On 2014/02/14 19:25:44, mmenke wrote:
> Maybe "...  to remain in the list and be called on future events." ?

Done.

[davidben, 2014-02-14 19:38:19]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-02-14 19:41:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/142013004/510001

[commit-bot: I haz the power, 2014-02-14 20:47:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 20:47:07]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[davidben, 2014-02-14 21:17:06]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-02-14 21:19:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/142013004/510001

[commit-bot: I haz the power, 2014-02-15 03:39:31]

Change committed as 251495

[Lei Zhang, 2014-02-18 06:53:30]

A revert of this CL has been created in
https://codereview.chromium.org/170173003/ by thestig@chromium.org.

The reason for reverting is: Failing on XP Tests 1, build 30187 and onwards.
Also failing on XP Tests 2 and 3.

[ RUN      ] PrerenderBrowserTest.PrerenderPageRemovingLinkWithTwoLinks
HTTP server started on 127.0.0.1:4080...
sending server_data: {"host": "127.0.0.1", "port": 4080} (35 bytes)
File not found
prerender/prerender_page.html%E2%80%93)%C3%A4%C2%B3%C3%BBh%C3%BE)%C3%ACz%C2%BB
full
path:E:\b\build\slave\XP_Tests__1_\build\src\chrome/test/data\prerender\prerender_page.html%E2%80%93)%C3%A4%C2%B3%C3%BBh%C3%BE)%C3%ACz%C2%BB
127.0.0.1 - - [14/Feb/2014 22:17:53] code 404, message Not Found
[2656:2572:0214/221753:3777828:INFO:CONSOLE(1)] "Uncaught ReferenceError:
DidPrerenderPass is not defined", source:  (1).