Prerendering: Add MatchComplete PPLT

Prerendering: Add MatchComplete PPLT
R=cbentzel
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109719

[dominich, 2011-11-09 20:48:15]

drive-by

http://codereview.chromium.org/8503040/diff/8009/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8503040/diff/8009/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:667: if (final_status !=
FINAL_STATUS_USED &&
Move this to a utility function IsCancellationStatus where FINAL_STATUS_* is
defined.

[cbentzel, 2011-11-09 22:17:29]

my mind is mush right now and this is non-trivial. I'll take a look in the AM.

[cbentzel, 2011-11-10 13:16:25]

Update the CL description to provide more details about what the MatchComplete
PPLT is. This will help when trying to figure it out later.

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:355: // Don't override
final_status_ if it's FINAL_STATUS_CONTROL_GROUP,
Update comment.

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:366:
DCHECK(prerendering_has_been_cancelled_ ||
Does this need to be updated with the MATCH_COMPLETE_DUMMY?

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_histograms.h (right):

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_histograms.h:34: bool was_prerender,
Rather than two booleans, could be easier to have an enum with PRERENDER_PARTIAL
+ PRERENDER_COMPLETE or similar values.

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:671: final_status !=
FINAL_STATUS_WINDOW_OPENER &&
Is the WINDOW_OPENER case one that would be caught in the control group?

[tburkard, 2011-11-10 18:36:55]

http://codereview.chromium.org/8503040/diff/8009/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8503040/diff/8009/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:667: if (final_status !=
FINAL_STATUS_USED &&
On 2011/11/09 20:48:16, dominich wrote:
> Move this to a utility function IsCancellationStatus where FINAL_STATUS_* is
> defined.

Done.

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:355: // Don't override
final_status_ if it's FINAL_STATUS_CONTROL_GROUP,
On 2011/11/10 13:16:25, cbentzel wrote:
> Update comment.

Done.

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:366:
DCHECK(prerendering_has_been_cancelled_ ||
On 2011/11/10 13:16:25, cbentzel wrote:
> Does this need to be updated with the MATCH_COMPLETE_DUMMY?

Yes.  Excellent catch.  Surprised neither my testing nor the unit tests /
browser tests exposed a bug here.

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_histograms.h (right):

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_histograms.h:34: bool was_prerender,
Let's keep it like this for now, especially since we will likely remove this
again soon.

On 2011/11/10 13:16:25, cbentzel wrote:
> Rather than two booleans, could be easier to have an enum with
PRERENDER_PARTIAL
> + PRERENDER_COMPLETE or similar values.

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:671: final_status !=
FINAL_STATUS_WINDOW_OPENER &&
Yes, see MaybeUsePrerenderedPage in the same file.

On 2011/11/10 13:16:25, cbentzel wrote:
> Is the WINDOW_OPENER case one that would be caught in the control group?

[cbentzel, 2011-11-10 19:45:28]

Oops - forgot to publish this. Once you understand why the DCHECK in
~PrerenderContents wasn't firing and whether it's indicative of a problem, I'll
be happy.

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/8503040/diff/12004/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:366:
DCHECK(prerendering_has_been_cancelled_ ||
On 2011/11/10 18:36:55, tburkard wrote:
> On 2011/11/10 13:16:25, cbentzel wrote:
> > Does this need to be updated with the MATCH_COMPLETE_DUMMY?
> 
> Yes.  Excellent catch.  Surprised neither my testing nor the unit tests /
> browser tests exposed a bug here.

Yeah - I'm actually a little worried about that. Do you understand why this
DCHECK wasn't firing?

[tburkard, 2011-11-10 21:00:09]

Hey Chris,

fixed & verified everything works now..  See update!

[cbentzel, 2011-11-11 12:13:20]

Looks like a number of prerender browsertests are failing on the bots.

[tburkard, 2011-11-11 19:39:25]

Ok, figured out the problem, this should fix it!

Thanks.

[dominich, 2011-11-11 20:02:09]

http://codereview.chromium.org/8503040/diff/30002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/8503040/diff/30002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:489: bool
PrerenderContents::AddAliasURL(const GURL& url, bool destroy_on_failure) {
How does the caller know what final status to give it when destroying it?

http://codereview.chromium.org/8503040/diff/30002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.h (right):

http://codereview.chromium.org/8503040/diff/30002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.h:164: virtual bool
AddAliasURL(const GURL& url, bool destroy_on_failure);
I've been through the CL but I don't understand why this new bool is needed. I'm
very nervous about any changes to who is responsible for deleting these.

[tburkard, 2011-11-11 20:44:05]

Agreed, that was not a good way of doing it.

I found a way to get rid of this infite call stack by changing the order in
which I do things.

Please re-review.

[dominich, 2011-11-11 20:53:46]

http://codereview.chromium.org/8503040/diff/24002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8503040/diff/24002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:655: bool
PrerenderManager::NeedMatchCompleteDummyForFinalStatus(
This doesn't need to be a member of a PrerenderManager instance. It could be
global in an anonymous namespace.

http://codereview.chromium.org/8503040/diff/24002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:693: CreatePrerenderContents(
Instead of exposing these internals (referrer, experiment_id) is it worth just
having a PrerenderContents::Clone method that returns a copy?

http://codereview.chromium.org/8503040/diff/24002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:699:
dummy_replacement_prerender_contents->Init()) {
If Init fails here you should delete dummy_replacement_prerender_contents.

[tburkard, 2011-11-11 21:50:06]

http://codereview.chromium.org/8503040/diff/24002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8503040/diff/24002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:655: bool
PrerenderManager::NeedMatchCompleteDummyForFinalStatus(
On 2011/11/11 20:53:46, dominich wrote:
> This doesn't need to be a member of a PrerenderManager instance. It could be
> global in an anonymous namespace.

Done.

http://codereview.chromium.org/8503040/diff/24002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:693: CreatePrerenderContents(
Since we will likely remove this again, I will leave it like tihs for now.
On 2011/11/11 20:53:46, dominich wrote:
> Instead of exposing these internals (referrer, experiment_id) is it worth just
> having a PrerenderContents::Clone method that returns a copy?

http://codereview.chromium.org/8503040/diff/24002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.cc:699:
dummy_replacement_prerender_contents->Init()) {
Didn't we talk about this in the previous iteration, how you wanted to have it
auto-delete itself rather than have the caller take care of it?  (see other call
to Init() in this file).
On 2011/11/11 20:53:46, dominich wrote:
> If Init fails here you should delete dummy_replacement_prerender_contents.

[dominich, 2011-11-11 22:01:40]

LGTM

I didn't see that you'd changed the Init semantics so never mind about the
deletion comment.