Better test coverage for prerender sessionStorage merging.

Better test coverage for prerender sessionStorage merging.

Actually test that the sessionStorage behaved correctly. Also, add a test for a
conflict case. Ensure the page still loads.

BUG=none
TEST=PrerenderBrowserTest.PrerenderPageNewTab,
     PrerenderBrowserTest.PrerenderSessionStorageConflict

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

[davidben, 2014-01-08 21:22:50]

Started fixing up some other stuff, but I think I'm going to hold on some
further test refactoring first. In the meantime, here are some tests split off
from that branch.

[mmenke, 2014-01-09 16:17:01]

https://codereview.chromium.org/129383002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/129383002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:3454: // the swap does not
occur synchronously.
Now that we have the swap_observer, can't we just expect the swap to succeed? 
In fact, without the swap observer, these tests could fail due to the
asynchronous swap, no?

[mmenke, 2014-01-09 16:20:26]

https://codereview.chromium.org/129383002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/129383002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:3454: // the swap does not
occur synchronously.
On 2014/01/09 16:17:01, mmenke wrote:
> Now that we have the swap_observer, can't we just expect the swap to succeed? 
> In fact, without the swap observer, these tests could fail due to the
> asynchronous swap, no?

Oh, is the issue that OpenURL() returns the wrong WebContents?  If that's the
case, I think that's very confusing, and wonder if we could do better, since
passing false for a swap that does in fact succeed before the function returns
is really weird.

Maybe have the swap_observer get the right web_contents?

[davidben, 2014-01-09 17:39:15]

https://codereview.chromium.org/129383002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/129383002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:3454: // the swap does not
occur synchronously.
On 2014/01/09 16:20:27, mmenke wrote:
> On 2014/01/09 16:17:01, mmenke wrote:
> > Now that we have the swap_observer, can't we just expect the swap to
succeed? 
> > In fact, without the swap observer, these tests could fail due to the
> > asynchronous swap, no?
> 
> Oh, is the issue that OpenURL() returns the wrong WebContents?  If that's the
> case, I think that's very confusing, and wonder if we could do better, since
> passing false for a swap that does in fact succeed before the function returns
> is really weird.
> 
> Maybe have the swap_observer get the right web_contents?

Yeah, it's really confusing. This isn't new from this CL though;
NavigateToDestURLInNewTab also did the same thing. (It never even called
DidDisplayPass.) My plan was to, when making all prerenders asynchronous for
beforeunload purposes, remove that WebContents check and then this can be a
normal NavigateToDestURL.

I can add a TODO to that effect or even just punt on the WebContents assert
right now if you think that's preferable. Doing it in the observer would be a
little screwy since it's not whether the swap happens or not. It's whether the
swap happens before or after OpenURL returns.

[mmenke, 2014-01-09 21:48:35]

LGTM

https://codereview.chromium.org/129383002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/129383002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:3454: // the swap does not
occur synchronously.
On 2014/01/09 17:39:15, David Benjamin wrote:
> On 2014/01/09 16:20:27, mmenke wrote:
> > On 2014/01/09 16:17:01, mmenke wrote:
> > > Now that we have the swap_observer, can't we just expect the swap to
> succeed? 
> > > In fact, without the swap observer, these tests could fail due to the
> > > asynchronous swap, no?
> > 
> > Oh, is the issue that OpenURL() returns the wrong WebContents?  If that's
the
> > case, I think that's very confusing, and wonder if we could do better, since
> > passing false for a swap that does in fact succeed before the function
returns
> > is really weird.
> > 
> > Maybe have the swap_observer get the right web_contents?
> 
> Yeah, it's really confusing. This isn't new from this CL though;
> NavigateToDestURLInNewTab also did the same thing. (It never even called
> DidDisplayPass.) My plan was to, when making all prerenders asynchronous for
> beforeunload purposes, remove that WebContents check and then this can be a
> normal NavigateToDestURL.
> 
> I can add a TODO to that effect or even just punt on the WebContents assert
> right now if you think that's preferable. Doing it in the observer would be a
> little screwy since it's not whether the swap happens or not. It's whether the
> swap happens before or after OpenURL returns.

The watcher would always be able to get the right WebContents, though, so you
could always just get it from that.

Regardless, I think it's fine to add a TODO, since this was pre-existing
behavior.

[davidben, 2014-01-09 21:51:48]

https://codereview.chromium.org/129383002/diff/1/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/129383002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:3454: // the swap does not
occur synchronously.
On 2014/01/09 21:48:35, mmenke wrote:
> On 2014/01/09 17:39:15, David Benjamin wrote:
> > On 2014/01/09 16:20:27, mmenke wrote:
> > > On 2014/01/09 16:17:01, mmenke wrote:
> > > > Now that we have the swap_observer, can't we just expect the swap to
> > succeed? 
> > > > In fact, without the swap observer, these tests could fail due to the
> > > > asynchronous swap, no?
> > > 
> > > Oh, is the issue that OpenURL() returns the wrong WebContents?  If that's
> the
> > > case, I think that's very confusing, and wonder if we could do better,
since
> > > passing false for a swap that does in fact succeed before the function
> returns
> > > is really weird.
> > > 
> > > Maybe have the swap_observer get the right web_contents?
> > 
> > Yeah, it's really confusing. This isn't new from this CL though;
> > NavigateToDestURLInNewTab also did the same thing. (It never even called
> > DidDisplayPass.) My plan was to, when making all prerenders asynchronous for
> > beforeunload purposes, remove that WebContents check and then this can be a
> > normal NavigateToDestURL.
> > 
> > I can add a TODO to that effect or even just punt on the WebContents assert
> > right now if you think that's preferable. Doing it in the observer would be
a
> > little screwy since it's not whether the swap happens or not. It's whether
the
> > swap happens before or after OpenURL returns.
> 
> The watcher would always be able to get the right WebContents, though, so you
> could always just get it from that.
> 
> Regardless, I think it's fine to add a TODO, since this was pre-existing
> behavior.

Added TODO.

[commit-bot: I haz the power, 2014-01-09 21:54:51]

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

[commit-bot: I haz the power, 2014-01-10 01:23:12]

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

[commit-bot: I haz the power, 2014-01-10 02:18:05]

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

[commit-bot: I haz the power, 2014-01-10 03:17:13]

Change committed as 244066