Move pending prerender logic into PrerenderLinkManager.

Move pending prerender logic into PrerenderLinkManager.

Pending prerenders should not count towards max_link_concurrency. Fix and
re-enable the tests accordingly. This also removes the last instances of
unpaired content::RunMessageLoop() / base::MessageLoop::current()->Quit() calls
in the browser tests. All waiting should now use the new base::RunLoop-based
code.

Also add a new test to check pending prerenders are canceled when their
launching prerenders are.

BUG=333276, 167792, 145248, 307592
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248646

[davidben, 2014-01-24 17:50:43]

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

https://codereview.chromium.org/146983002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_browsertest.cc:2171: // the page load one.
Not entirely thrilled by these "this relies on the fact that [...]" bits, but I
believe it is not racy right now. We can perhaps add some test-only observers to
PrerenderLinkManager if this ends up changing. (x5 or so)

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

https://codereview.chromium.org/146983002/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_link_manager.cc:77:
link_manager_->StartPendingPrerendersForLauncher(launcher);
(This does result in AddPrerender being called while a prerender swap is on the
stack. But the old code did that too.)

[davidben, 2014-01-24 21:47:06]

Alright, hopefully that fixes prerender_unittest.cc. I'm not very happy about
adding another virtual hook to PrerenderManager, but I'm not sure how to avoid
that short of reworking those unit tests to actually create WebContentses and
the like. These tests are rather annoyingly fragile.

[gavinp, 2014-01-30 19:58:48]

Overall, great. Only style nits, as you can see. I'm glad to see
prerender_manager.cc get shorter; it's too large so this change is very good for
that reason.

In the issue description, you are using TEST= wrong. That field isn't for
listing tests you edited or that confirm your functionality, its intended for
letting our QA department know what validation they should do. It's OK to omit
it, and that's what I'd suggest in this case.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (left):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:513: size_t
PrerenderContents::pending_prerender_count() const {
Nice to get rid of this style violation.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:198: void
PrerenderContents::PrepareForUse() {
Methods in .cc files should be in the same order as in the .h file.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.cc:53: }
This can move up a line too.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.cc:72: }
Nit: Move this to the previous line.

Also, add a blank line between methods.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.cc:94:
pending_prerender_manager_(new PendingPrerenderManager(this)) {
Ditto.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.cc:143: if (prerender_contents)
{
Nit: the {} on this are not necessary. I wouldn't use them.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.cc:408:
std::list<LinkPrerender>::iterator current = iter++;
This looks correct.

However, just for absolute safety when iterating and mutating a data structure,
I'm an advocate of just constructing a vector of iterators to_erase, and then
calling:

std::for_each(to_erase.begin(), to_erase.end(),
                     std::bind1st(std::mem_fun(&std::list<LinkPrerender>::erase,
prerenders_));

at the end. It's safer and a bit less scary.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_link_manager.h (right):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.h:163: class
PendingPrerenderManager;
Our declaration order rule doesn't indicate where inner classes should be
declared:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
, but I'm more used to seeing declarations (even forward declarations) up at the
top of the section with typedefs and enums.

However, looks like this isn't actually a style violation, which I found
interesting.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.h:233: // Same as above, but returns
it for a given child_id, route_id pair.
Nit: don't reference another comment. Duplicating the text or omitting the
comment is more future proof against comments that lie.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:762:
prerender_link_manager()->OnAddPrerender(
Nice. I like that these tests are going through the prerender_link_manager()
more.

[davidben, 2014-01-30 21:01:51]

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:198: void
PrerenderContents::PrepareForUse() {
On 2014/01/30 19:58:49, gavinp wrote:
> Methods in .cc files should be in the same order as in the .h file.

Yeah, a lot of this file is rather horrifically out of order. (I just removed a
function and added a line to PrepareForUse; I think the diff is confused.)

Moved it somewhere more accurate as far as the header goes.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_link_manager.cc (right):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.cc:53: }
On 2014/01/30 19:58:49, gavinp wrote:
> This can move up a line too.

Done.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.cc:72: }
On 2014/01/30 19:58:49, gavinp wrote:
> Nit: Move this to the previous line.
> 
> Also, add a blank line between methods.

Done.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.cc:94:
pending_prerender_manager_(new PendingPrerenderManager(this)) {
On 2014/01/30 19:58:49, gavinp wrote:
> Ditto.

Done.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.cc:143: if (prerender_contents)
{
On 2014/01/30 19:58:49, gavinp wrote:
> Nit: the {} on this are not necessary. I wouldn't use them.

Done.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.cc:408:
std::list<LinkPrerender>::iterator current = iter++;
On 2014/01/30 19:58:49, gavinp wrote:
> This looks correct.
> 
> However, just for absolute safety when iterating and mutating a data
structure,
> I'm an advocate of just constructing a vector of iterators to_erase, and then
> calling:
> 
> std::for_each(to_erase.begin(), to_erase.end(),
>                     
std::bind1st(std::mem_fun(&std::list<LinkPrerender>::erase,
> prerenders_));
> 
> at the end. It's safer and a bit less scary.

Done.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_link_manager.h (right):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_link_manager.h:163: class
PendingPrerenderManager;
On 2014/01/30 19:58:49, gavinp wrote:
> Our declaration order rule doesn't indicate where inner classes should be
> declared:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...
> , but I'm more used to seeing declarations (even forward declarations) up at
the
> top of the section with typedefs and enums.
> 
> However, looks like this isn't actually a style violation, which I found
> interesting.

Done.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.h:233: // Same as above, but returns
it for a given child_id, route_id pair.
On 2014/01/30 19:58:49, gavinp wrote:
> Nit: don't reference another comment. Duplicating the text or omitting the
> comment is more future proof against comments that lie.

Done.

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/146983002/diff/160001/chrome/browser/prerende...
chrome/browser/prerender/prerender_unittest.cc:762:
prerender_link_manager()->OnAddPrerender(
On 2014/01/30 19:58:49, gavinp wrote:
> Nice. I like that these tests are going through the prerender_link_manager()
> more.

They have to anyway; the logic has moved to PrerenderLinkManager.

[gavinp, 2014-01-31 22:01:43]

lgtm

[davidben, 2014-01-31 22:03:43]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-01-31 22:06:08]

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

[commit-bot: I haz the power, 2014-01-31 22:29:04]

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

[commit-bot: I haz the power, 2014-01-31 22:29:04]

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

[commit-bot: I haz the power, 2014-01-31 22:29:08]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 22:29:17]

CQ bit was unchecked on CL. Ignoring.

[davidben, 2014-01-31 22:56:32]

On 2014/01/31 22:29:17, I haz the power (commit-bot) wrote:
> CQ bit was unchecked on CL. Ignoring.

Hrm. I'm very confused about the mention of browser_tests. The link looks more
like goma falling over? Trying this again.

[davidben, 2014-01-31 22:56:38]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-01-31 22:57:14]

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

[commit-bot: I haz the power, 2014-01-31 23:42:34]

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

[commit-bot: I haz the power, 2014-01-31 23:42:34]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-31 23:42:42]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-31 23:42:42]

CQ bit was unchecked on CL. Ignoring.

[davidben, 2014-02-03 17:06:59]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-02-03 17:07:17]

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

[commit-bot: I haz the power, 2014-02-03 17:41:21]

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

[commit-bot: I haz the power, 2014-02-03 17:41:21]

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

[commit-bot: I haz the power, 2014-02-03 17:41:26]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 17:41:29]

CQ bit was unchecked on CL. Ignoring.

[davidben, 2014-02-03 23:52:17]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-02-03 23:56:55]

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

[commit-bot: I haz the power, 2014-02-04 01:13:05]

Change committed as 248646