Refactor prerender_browsertests.

Refactor prerender_browsertests.

This cleans up the browser tests in a number of ways:
- Add some convenience functions.
- Implement title-waiting outside of TestPrerenderContents.
- Remove most uses of GetPrerenderContents() in preparation for tests which
  manage two prerenders.
- Remove half of PrerenderTestURL variants by moving the one-off variations to
  the tests themselves.
- Move some initialization logic out of PrerenderTestURLImpl.

BUG=307592
TEST=Prerender*.*

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

[davidben, 2014-01-17 18:43:22]

I can try to split this one up if you prefer. Some of the rearranging is
probably not especially necessary; probably the test I want with two prerenders
can just call PrerenderTestURL() and then add call JS to run AddPrerender later,
but meh. It makes PrerenderTestURLImpl less long.

[mmenke, 2014-01-17 19:44:14]

On 2014/01/17 18:43:22, David Benjamin wrote:
> I can try to split this one up if you prefer. Some of the rearranging is
> probably not especially necessary; probably the test I want with two
prerenders
> can just call PrerenderTestURL() and then add call JS to run AddPrerender
later,
> but meh. It makes PrerenderTestURLImpl less long.

I think one C is fine.  This really doesn't look too bad.

[mmenke, 2014-01-17 22:00:38]

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

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:380:
content::SessionStorageNamespace* session_storage_namespace) OVERRIDE {
PrerenderContents::CreateWebContents need no longer be virtual.

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1490: TestPrerenderContents*
prerender_contents = GetPrerenderContents();
Can we make GetPrerenderContents() private now?

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

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1024: }
nit:  +line break?

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1318: dest_url_ =
prerender_url;
Wonder if we should just get rid of this in another CL.  Seems kinda weird to
just ignore it in a bunch of tests.  Of course, we may have to get rid of the
PrerenderURL functions that take a path instead of a URL, since capturing those
is its main purpose.

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1331: src_server =
https_src_server_.get();
Good idea...scoping here does seem like it was a wee bit weird.

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1369:
prerenders[0]->contents()->prerender_contents()));
prerenders[0]->contents() is just prerender_contents, as set a couple lines
above.  Any particular reason for the switch?

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1629: }
optional:  Suggest moving this test and its fixture to the end of the file.  I
like to keep all tests that use the same test fixture grouped together, just to
make it clearer which tests use which fixture.

[davidben, 2014-01-17 22:32:04]

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

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:380:
content::SessionStorageNamespace* session_storage_namespace) OVERRIDE {
On 2014/01/17 22:00:38, mmenke wrote:
> PrerenderContents::CreateWebContents need no longer be virtual.

Ooh, forgot about that. Done.

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1490: TestPrerenderContents*
prerender_contents = GetPrerenderContents();
On 2014/01/17 22:00:38, mmenke wrote:
> Can we make GetPrerenderContents() private now?

Done.

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

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1024: }
On 2014/01/17 22:00:38, mmenke wrote:
> nit:  +line break?

Done.

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1318: dest_url_ =
prerender_url;
On 2014/01/17 22:00:38, mmenke wrote:
> Wonder if we should just get rid of this in another CL.  Seems kinda weird to
> just ignore it in a bunch of tests.  Of course, we may have to get rid of the
> PrerenderURL functions that take a path instead of a URL, since capturing
those
> is its main purpose.

'this' being the dest_url_ line? Well there's also all the NavigateToDestURL
bits.

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1369:
prerenders[0]->contents()->prerender_contents()));
On 2014/01/17 22:00:38, mmenke wrote:
> prerenders[0]->contents() is just prerender_contents, as set a couple lines
> above.  Any particular reason for the switch?

Oh, hrm, I must have just done that blindly without noticing it didn't come from
a GetPrerenderContents. Reverted.

(Can I just say: it is incredibly confusing how we have
TestPrerender/PrerenderData which hold onto a PrerenderContents via a 'contents'
member, but PrerenderContents is not itself a WebContents and you have to access
the 'prerender_contents' member on the PrerenderContents to get to the
prerender's WebContents?)

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1629: }
On 2014/01/17 22:00:38, mmenke wrote:
> optional:  Suggest moving this test and its fixture to the end of the file.  I
> like to keep all tests that use the same test fixture grouped together, just
to
> make it clearer which tests use which fixture.

Done.

[davidben, 2014-01-17 23:55:03]

Couple more changes: noticed we could use content::WaitForLoad() somewhere,
which is a bit shorter. Also fixed the net-internals test.

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

LGTM

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

https://codereview.chromium.org/136583007/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:1318: dest_url_ =
prerender_url;
On 2014/01/17 22:32:04, David Benjamin wrote:
> On 2014/01/17 22:00:38, mmenke wrote:
> > Wonder if we should just get rid of this in another CL.  Seems kinda weird
to
> > just ignore it in a bunch of tests.  Of course, we may have to get rid of
the
> > PrerenderURL functions that take a path instead of a URL, since capturing
> those
> > is its main purpose.
> 
> 'this' being the dest_url_ line? Well there's also all the NavigateToDestURL
> bits.

Yea, 'this' being get rid of dest_url_ completely.  Could then also get rid of
more PrerenderTestURL overloads.

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

https://codereview.chromium.org/136583007/diff/260001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1022:
CHECK(prerender_manager);
optional:  Think these could safely be ASSERTs, though if you prefer CHECKs, I
can certainly live with them.

[davidben, 2014-01-21 20:17:06]

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

https://codereview.chromium.org/136583007/diff/260001/chrome/browser/prerende...
chrome/browser/prerender/prerender_browsertest.cc:1022:
CHECK(prerender_manager);
On 2014/01/21 17:16:10, mmenke wrote:
> optional:  Think these could safely be ASSERTs, though if you prefer CHECKs, I
> can certainly live with them.

Done.

[commit-bot: I haz the power, 2014-01-21 20:18:27]

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

[commit-bot: I haz the power, 2014-01-21 21:48:41]

Change committed as 246128