Prerendering: Add MatchComplete PPLT

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

[tburkard, 2011-11-11 23:58:06]

OK new attempt
added APP_TERMINATING as a condition when to not add the dummy.  will run
against trybots now.

[dominich, 2011-11-12 00:02:48]

LGTM

minor comments

http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_browsertest.cc:1843: //
http://crbug.com/103563
Any chance this version fixes these tests (on linux) too?

http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/preren...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/preren...
chrome/browser/prerender/prerender_manager.cc:695: //
NeedMatchCompleteDummyForFinalStatus, is not being updated.  Not sure
You could change NeedMatchCompleteDummyForFinalStatus to include every status
and a NOTREACHED() in an else.

[cbentzel, 2011-11-12 02:27:38]

Why did the old CL need to be reverted and what does this change to fix it?

On Fri, Nov 11, 2011 at 7:02 PM, <dominich@chromium.org> wrote:

> LGTM
>
> minor comments
>
>
> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>
browser/prerender/prerender_**browsertest.cc<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_browsertest.cc>
> File chrome/browser/prerender/**prerender_browsertest.cc (right):
>
> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>
browser/prerender/prerender_**browsertest.cc#newcode1843<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_browsertest.cc#newcode1843>
> chrome/browser/prerender/**prerender_browsertest.cc:1843: //
> http://crbug.com/103563
> Any chance this version fixes these tests (on linux) too?
>
> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>
browser/prerender/prerender_**manager.cc<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_manager.cc>
> File chrome/browser/prerender/**prerender_manager.cc (right):
>
> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>
browser/prerender/prerender_**manager.cc#newcode695<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_manager.cc#newcode695>
> chrome/browser/prerender/**prerender_manager.cc:695: //
> NeedMatchCompleteDummyForFinal**Status, is not being updated.  Not sure
> You could change NeedMatchCompleteDummyForFinal**Status to include every
> status and a NOTREACHED() in an else.
>
>
http://codereview.chromium.**org/8540025/<http://codereview.chromium.org/8540...
>

[tburkard, 2011-11-12 03:23:59]

It didn't fix it..  If I run trybots say 5 times on mac, about half the
time, PrerenderQuickQuit fails.  Here are two trybot samples with the
change:
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/2445
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/2444

As a sidenote, if i specify a filter to only run
browser_tests:PrerenderBrowserTest.Prerender*, almost all fail, in every
run.  Not sure why that is, but since this does not happen when running all
tests, and thats what trybots do, I am not worrying about it.  For
completeness, here are a few samples tho.  This is the identical change as
above.
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/2443
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/2442

If you have any ideas on the main issue (why PrerenderQuickQuit fails 1/2
the time), let me know!

Thanks.

Timo

On Fri, Nov 11, 2011 at 6:27 PM, Chris Bentzel <cbentzel@chromium.org>wrote:

> Why did the old CL need to be reverted and what does this change to fix it?
>
>
> On Fri, Nov 11, 2011 at 7:02 PM, <dominich@chromium.org> wrote:
>
>> LGTM
>>
>> minor comments
>>
>>
>> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>>
browser/prerender/prerender_**browsertest.cc<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_browsertest.cc>
>> File chrome/browser/prerender/**prerender_browsertest.cc (right):
>>
>> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>>
browser/prerender/prerender_**browsertest.cc#newcode1843<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_browsertest.cc#newcode1843>
>> chrome/browser/prerender/**prerender_browsertest.cc:1843: //
>> http://crbug.com/103563
>> Any chance this version fixes these tests (on linux) too?
>>
>> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>>
browser/prerender/prerender_**manager.cc<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_manager.cc>
>> File chrome/browser/prerender/**prerender_manager.cc (right):
>>
>> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>>
browser/prerender/prerender_**manager.cc#newcode695<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_manager.cc#newcode695>
>> chrome/browser/prerender/**prerender_manager.cc:695: //
>> NeedMatchCompleteDummyForFinal**Status, is not being updated.  Not sure
>> You could change NeedMatchCompleteDummyForFinal**Status to include every
>> status and a NOTREACHED() in an else.
>>
>>
http://codereview.chromium.**org/8540025/<http://codereview.chromium.org/8540...
>>
>
>

[tburkard, 2011-11-12 03:28:55]

Addendum: Chris, as for the change, it makes sense to add that
APP_TERMINATING condition, because when the app is terminating, there will
certainly be no more finished pageloads that we could recor dany meaningful
match complete pageloadtimes for, so it is ok to skip that case.

Timo

On Fri, Nov 11, 2011 at 7:23 PM, Timo Burkard <tburkard@chromium.org> wrote:

> It didn't fix it..  If I run trybots say 5 times on mac, about half the
> time, PrerenderQuickQuit fails.  Here are two trybot samples with the
> change:
> http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/2445
> http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/2444
>
> As a sidenote, if i specify a filter to only run
> browser_tests:PrerenderBrowserTest.Prerender*, almost all fail, in every
> run.  Not sure why that is, but since this does not happen when running all
> tests, and thats what trybots do, I am not worrying about it.  For
> completeness, here are a few samples tho.  This is the identical change as
> above.
> http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/2443
> http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/2442
>
> If you have any ideas on the main issue (why PrerenderQuickQuit fails 1/2
> the time), let me know!
>
> Thanks.
>
> Timo
>
> On Fri, Nov 11, 2011 at 6:27 PM, Chris Bentzel <cbentzel@chromium.org>wrote:
>
>> Why did the old CL need to be reverted and what does this change to fix
>> it?
>>
>>
>> On Fri, Nov 11, 2011 at 7:02 PM, <dominich@chromium.org> wrote:
>>
>>> LGTM
>>>
>>> minor comments
>>>
>>>
>>> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>>>
browser/prerender/prerender_**browsertest.cc<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_browsertest.cc>
>>> File chrome/browser/prerender/**prerender_browsertest.cc (right):
>>>
>>> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>>>
browser/prerender/prerender_**browsertest.cc#newcode1843<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_browsertest.cc#newcode1843>
>>> chrome/browser/prerender/**prerender_browsertest.cc:1843: //
>>> http://crbug.com/103563
>>> Any chance this version fixes these tests (on linux) too?
>>>
>>> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>>>
browser/prerender/prerender_**manager.cc<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_manager.cc>
>>> File chrome/browser/prerender/**prerender_manager.cc (right):
>>>
>>> http://codereview.chromium.**org/8540025/diff/1/chrome/**
>>>
browser/prerender/prerender_**manager.cc#newcode695<http://codereview.chromium.org/8540025/diff/1/chrome/browser/prerender/prerender_manager.cc#newcode695>
>>> chrome/browser/prerender/**prerender_manager.cc:695: //
>>> NeedMatchCompleteDummyForFinal**Status, is not being updated.  Not sure
>>> You could change NeedMatchCompleteDummyForFinal**Status to include every
>>> status and a NOTREACHED() in an else.
>>>
>>>
http://codereview.chromium.**org/8540025/<http://codereview.chromium.org/8540...
>>>
>>
>>
>

[cbentzel, 2011-11-15 12:05:29]

http://codereview.chromium.org/8540025/diff/11011/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (left):

http://codereview.chromium.org/8540025/diff/11011/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:306:
CHECK(!expected_final_status_queue_.empty()) <<
This has been useful in the past for catching problems - now it seems like we
will always create one with MATCH_COMPLETE_DUMMY. It seems like it would be
better to queue up a dummy expected final status in the places that use it.

http://codereview.chromium.org/8540025/diff/11011/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/8540025/diff/11011/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:1843: //
http://crbug.com/103563
Why is this disabled? As mentioned in the bug, we aren't seeing problems on the
flakiness dashboard after my fix. I'm worried that there was something wrong
with your local repo.