Various Prerender Service / Prerender LocalPredictor related changes.

Various Prerender Service / Prerender LocalPredictor related changes.
- Permit multiple concurrent LocalPredictor prerenders
- Permit local predictor prerenders to trigger prefetches (and only prerender as a control gruop)
- Force enable prefetch if local predictor prefetching is enabled

R=davidben@chromium.org, jkarlin@chromium.org

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

[tburkard, 2014-06-19 13:28:49]

For testing Smart Prefetch, allowing prerender service prerenders to launch
multiple prerenders simultaneously, and add a mode which prefetches rather than
actually prerenders.

This is only for experimentation in Clank Beta. It'd be great if you could
review this today, so that I can commit it today.

Thanks.

[davidben, 2014-06-19 19:33:35]

Some nits and one comment below about the URL check.

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

https://codereview.chromium.org/330063004/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:100: int render_frame_id_;
Nit: add a comment that this is the route of the source, not the prerender
itself. At first read I thought it was the latter and was a little confused.

https://codereview.chromium.org/330063004/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:106: render_frame_id_ =
kInvalidFrameId;
Nit: these can be in an initializer list.

https://codereview.chromium.org/330063004/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:1080: if
(p->prerender_handle->Matches(url, NULL))
Does this check actually do what you want? If there's an empty slot before a
match URL, it'll still duplicate.

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

https://codereview.chromium.org/330063004/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:1536: if (origin ==
ORIGIN_LOCAL_PREDICTOR)
Nit: Add a comment that ORIGIN_LOCAL_PREDICTOR may try to keep a set of
prerenders alive at once and so it shouldn't be throttled here. At first I
thought this was because you didn't want to throttle prefetches or something.

[tburkard, 2014-06-19 20:01:49]

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

https://codereview.chromium.org/330063004/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:100: int render_frame_id_;
On 2014/06/19 19:33:34, David Benjamin wrote:
> Nit: add a comment that this is the route of the source, not the prerender
> itself. At first read I thought it was the latter and was a little confused.

Done.

https://codereview.chromium.org/330063004/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:106: render_frame_id_ =
kInvalidFrameId;
On 2014/06/19 19:33:34, David Benjamin wrote:
> Nit: these can be in an initializer list.

Done.

https://codereview.chromium.org/330063004/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_local_predictor.cc:1080: if
(p->prerender_handle->Matches(url, NULL))
On 2014/06/19 19:33:34, David Benjamin wrote:
> Does this check actually do what you want? If there's an empty slot before a
> match URL, it'll still duplicate.

Done.

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

https://codereview.chromium.org/330063004/diff/1/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:1536: if (origin ==
ORIGIN_LOCAL_PREDICTOR)
On 2014/06/19 19:33:34, David Benjamin wrote:
> Nit: Add a comment that ORIGIN_LOCAL_PREDICTOR may try to keep a set of
> prerenders alive at once and so it shouldn't be throttled here. At first I
> thought this was because you didn't want to throttle prefetches or something.

Done.

[davidben, 2014-06-19 20:08:23]

https://codereview.chromium.org/330063004/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/330063004/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_local_predictor.cc:1083: if
(p->prerender_handle->Matches(url, NULL))
This should check p->prerender_handle and also
p->prerender_handle->IsPrerendering(). Especially that first one since you might
have NULL pointer.

[tburkard, 2014-06-19 20:12:16]

Great catch!

Should have tested this before sending out the update, and should really add
actual automatic testing soon so that we can catch stuff like this
automatically.

For now, I need to get this in though so that we can gather numbers.

Thanks.

https://codereview.chromium.org/330063004/diff/20001/chrome/browser/prerender...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/330063004/diff/20001/chrome/browser/prerender...
chrome/browser/prerender/prerender_local_predictor.cc:1083: if
(p->prerender_handle->Matches(url, NULL))
On 2014/06/19 20:08:23, David Benjamin wrote:
> This should check p->prerender_handle and also
> p->prerender_handle->IsPrerendering(). Especially that first one since you
might
> have NULL pointer.

Done.

[davidben, 2014-06-19 20:27:25]

lgtm

[tburkard, 2014-06-19 20:28:24]

The CQ bit was checked by tburkard@chromium.org

[commit-bot: I haz the power, 2014-06-19 20:30:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tburkard@chromium.org/330063004/40001

[tburkard, 2014-06-19 21:41:17]

Committed patchset #3 manually as r278496 (presubmit successful).