Refactor prerender pending swap logic.

Refactor prerender pending swap logic.

Convert the timeout to a OneShotTimer. Also rearrange MaybeUsePrerenderedPage
and SwapInternal so the latter only does the swap while the former looks up the
PrerenderData. Move the merge and timeout callbacks into PendingData so we
don't need to pass in callbacks solely for the purpose of cancelling them.

This removes events PRERENDER_EVENT_MERGE_RESULT_TIMEOUT_CB and
RERENDER_EVENT_MERGE_RESULT_RESULT_CB as they were redundant with
TIMED_OUT and MERGE_DONE.

BUG=none
TEST=no behavior change; PrerenderBrowserTests.*

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

[davidben, 2013-12-06 19:36:34]

Matt asked me to clean up some of the pending swap code separately before doing
any more invasive changes to how prerenders work.

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (left):

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:854: (*it)->ClearPendingSwap();
Removing this line avoids nuisances where SwapInternal may or may not delete the
pending swap on failure if it calls Destroy. Instead, leave the pending swap
there; it will be cleaned up in the cleanup task. If the merge happens to
complete in the meantime, SwapInternal will refuse to swap anyway because the
final status is set.

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:1346: if (validated_url != url_)
This check is unnecessary; the throttled load can't commit.

[tburkard, 2013-12-06 20:36:24]

Nice cleanup. Looks very elegant now.

lgtm

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2013-12-06 20:50:32]

Won't get to this today, unfortunately.  I'll get to it on Monday,

[mmenke, 2013-12-09 17:06:35]

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (left):

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:597: if (prerender_data !=
swap_candidate && prerender_data->pending_swap()) {
I'm confused....  In calls from MaybeUsePrerenderedPage, swap_candidate used to
be NULL, so wouldn't this always fail here before in that case?

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:1346: if (validated_url != url_)
On 2013/12/06 19:36:34, David Benjamin wrote:
> This check is unnecessary; the throttled load can't commit.

Random comment not for this CL:  Wonder if we should cancel earlier, like if a
new load starts, rather than waiting until commit.

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

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:471: // finish and swap into the
intended target webcontents.
"Or this webcontents"...Where do we check if the web contents has a pending
swap?

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:516: DeleteOldEntries();
Couldn't this delete prerender_data?

[mmenke, 2013-12-09 17:13:34]

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (left):

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:597: if (prerender_data !=
swap_candidate && prerender_data->pending_swap()) {
On 2013/12/09 17:06:36, mmenke wrote:
> I'm confused....  In calls from MaybeUsePrerenderedPage, swap_candidate used
to
> be NULL, so wouldn't this always fail here before in that case?

Oh, this is an "and", rather than an "or"....that makes more sense.

[davidben, 2013-12-09 20:00:31]

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (left):

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:1346: if (validated_url != url_)
On 2013/12/09 17:06:36, mmenke wrote:
> On 2013/12/06 19:36:34, David Benjamin wrote:
> > This check is unnecessary; the throttled load can't commit.
> 
> Random comment not for this CL:  Wonder if we should cancel earlier, like if a
> new load starts, rather than waiting until commit.

We do. I believe the main interesting cancel hook here is
DidFailProvisionalLoad. The others mostly act as sanity checks I think?

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

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:471: // finish and swap into the
intended target webcontents.
On 2013/12/09 17:06:36, mmenke wrote:
> "Or this webcontents"...Where do we check if the web contents has a pending
> swap?

Hrmf. I think that got lost in the original CL's rewrites and the comment never
got updated. I think there may be a handful of races here, including this one,
with browser-initiated navigations. All the cancel hooks come from the renderer,
which is asynchronous with the browser initiating a navigation. Those already
exist before this cleanup CL, so I'll deal with them in a follow-up I guess.

https://codereview.chromium.org/98373010/diff/20001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:516: DeleteOldEntries();
On 2013/12/09 17:06:36, mmenke wrote:
> Couldn't this delete prerender_data?

Hrm. So it can. I guess that's what the ClearPendingSwap call and second
FindPrerenderData lookup in MoveEntryToPendingDelete prevented. This is a little
annoyingly precarious... I've moved the cleanup code back to the top of
MaybeUsePrerenderedPage which is probably where it should be anyway. Though it
does still mean that SwapInternal may be called on a PrerenderData in
to_delete_prerenders_.

Also moved the TODO(ajwong) to the line it was presumably attached to. Got
attached to the wrong block somewhere.

[mmenke, 2013-12-09 21:04:44]

LGTM (Though I haven't dug into all of the old swap logic, just looked at the
new code)

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

https://codereview.chromium.org/98373010/diff/60001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:477: return false;
Random comment:  We could still theoretically have another usable prerender with
a pending swap in this case.  I don't think it's worth worrying about, but I do
think it's worth being aware of funky cases like that.

https://codereview.chromium.org/98373010/diff/60001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:499: return false;
optional:  Suggest a comment that we'll defer the main frame load until the
pending swap completes, despite returning false.

https://codereview.chromium.org/98373010/diff/60001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:520: // Only if this WebContents
is used in a tabstrip may be swap.
nit:  Know this was an old comment, but could you fix the grammar here?

https://codereview.chromium.org/98373010/diff/60001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:1248: void
PrerenderManager::PendingSwap::SwapSuccessful() {
Can we just delete the swap object and get rid of this function?

[davidben, 2013-12-09 23:08:55]

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

https://codereview.chromium.org/98373010/diff/60001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:499: return false;
On 2013/12/09 21:04:44, mmenke wrote:
> optional:  Suggest a comment that we'll defer the main frame load until the
> pending swap completes, despite returning false.

Done.

https://codereview.chromium.org/98373010/diff/60001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:520: // Only if this WebContents
is used in a tabstrip may be swap.
On 2013/12/09 21:04:44, mmenke wrote:
> nit:  Know this was an old comment, but could you fix the grammar here?

Done.

https://codereview.chromium.org/98373010/diff/60001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:1248: void
PrerenderManager::PendingSwap::SwapSuccessful() {
On 2013/12/09 21:04:44, mmenke wrote:
> Can we just delete the swap object and get rid of this function?

There's the parameter for whether the swap succeeded or not. I replaced it with
a swap_successful parameter.

[mmenke, 2013-12-09 23:27:46]

LGTM, just one nit.

https://codereview.chromium.org/98373010/diff/80001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/98373010/diff/80001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:524: // out of it. For normal
WebContentses, this is if it is in a TabStripModel.
We'll show the WebContentses our precious....

It's the right plural, actually, but there's a pronoun/antecedent agreement
issue here (WebContentses/it).  Maybe "For a normal WebContents"?

[davidben, 2013-12-09 23:30:54]

https://codereview.chromium.org/98373010/diff/80001/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_manager.cc (right):

https://codereview.chromium.org/98373010/diff/80001/chrome/browser/prerender/...
chrome/browser/prerender/prerender_manager.cc:524: // out of it. For normal
WebContentses, this is if it is in a TabStripModel.
On 2013/12/09 23:27:46, mmenke wrote:
> We'll show the WebContentses our precious....
> 
> It's the right plural, actually, but there's a pronoun/antecedent agreement
> issue here (WebContentses/it).  Maybe "For a normal WebContents"?

Done.

[commit-bot: I haz the power, 2013-12-09 23:40:57]

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

[commit-bot: I haz the power, 2013-12-10 05:04:31]

Change committed as 239658