Prerender: Disable prefetch if there's an appcache (unflaked tests)

Prerender: Disable prefetch if there's an appcache (with nonflakey tests)

Stops the preload scanner if an appcache manifest is detected and the document
is prefetching.

This un-reverts cl/2790323003. That test introduced by that CL was flakey due to
a race between AppCache creation and prefetching. This CL fixes the race by
blocking until the appropriate AppCache can be verified as existing.

BUG=632368
Review-Url: https://codereview.chromium.org/2819523002
Cr-Commit-Position: refs/heads/master@{#464713}
Committed: https://chromium.googlesource.com/chromium/src/+/f0abffcea31f665a2b1cc1979a955cf308c4dcb4

[mattcary, 2017-04-13 08:24:34]

Description was changed from

==========
Prerender: Disable prefetch if there's an appcache.

Stops the preload scanner if an appcache manifest is detected and the document
is prefetching.

This un-reverts cl/2790323003. That test introduced by that CL was flakey due to
a race between AppCache creation and prefetching. This CL fixes the race by
blocking until the appropriate AppCache can be verified as existing.

BUG=632368
==========

to

==========
Prerender: Disable prefetch if there's an appcache (with nonflakey tests)

Stops the preload scanner if an appcache manifest is detected and the document
is prefetching.

This un-reverts cl/2790323003. That test introduced by that CL was flakey due to
a race between AppCache creation and prefetching. This CL fixes the race by
blocking until the appropriate AppCache can be verified as existing.

BUG=632368
==========

[mattcary, 2017-04-13 08:27:08]

mattcary@chromium.org changed reviewers:
+ csharrison@chromium.org, droger@chromium.org, michaeln@chromium.org

[mattcary, 2017-04-13 08:27:09]

Hi!

My first try at the appcache browsertests were flaky. This deflakes the test. It
is perhaps heavier than necessary, but the GetAllAppCacheInfo api is
conveniently in content/public.

I didn't have to change any of the prod code, it was only an issue with the
test.

[droger, 2017-04-13 08:49:22]

On 2017/04/13 08:27:09, mattcary wrote:
> Hi!
> 
> My first try at the appcache browsertests were flaky. This deflakes the test.
It
> is perhaps heavier than necessary, but the GetAllAppCacheInfo api is
> conveniently in content/public.
> 
> I didn't have to change any of the prod code, it was only an issue with the
> test.

FYI: there is a somewhat common practice that I find useful for relanding CL:
upload the unmodified original CL as patch set 1, and the modified version as
patch set 2. This way it is easy to see what changed between the original CL and
the reland, which makes reviewing easier.
You don't have to do it for this CL, but maybe consider doing this next time.

[mattcary, 2017-04-13 08:50:51]

On 2017/04/13 08:49:22, droger wrote:
> On 2017/04/13 08:27:09, mattcary wrote:
> > Hi!
> > 
> > My first try at the appcache browsertests were flaky. This deflakes the
test.
> It
> > is perhaps heavier than necessary, but the GetAllAppCacheInfo api is
> > conveniently in content/public.
> > 
> > I didn't have to change any of the prod code, it was only an issue with the
> > test.
> 
> FYI: there is a somewhat common practice that I find useful for relanding CL:
> upload the unmodified original CL as patch set 1, and the modified version as
> patch set 2. This way it is easy to see what changed between the original CL
and
> the reland, which makes reviewing easier.
> You don't have to do it for this CL, but maybe consider doing this next time.

Thanks, that's a good point.

I started working on unflaking before the revert CL landed, so things got
confused.

[mattcary, 2017-04-13 08:52:23]

On 2017/04/13 08:50:51, mattcary wrote:
> On 2017/04/13 08:49:22, droger wrote:
> > On 2017/04/13 08:27:09, mattcary wrote:
> > > Hi!
> > > 
> > > My first try at the appcache browsertests were flaky. This deflakes the
> test.
> > It
> > > is perhaps heavier than necessary, but the GetAllAppCacheInfo api is
> > > conveniently in content/public.
> > > 
> > > I didn't have to change any of the prod code, it was only an issue with
the
> > > test.
> > 
> > FYI: there is a somewhat common practice that I find useful for relanding
CL:
> > upload the unmodified original CL as patch set 1, and the modified version
as
> > patch set 2. This way it is easy to see what changed between the original CL
> and
> > the reland, which makes reviewing easier.
> > You don't have to do it for this CL, but maybe consider doing this next
time.
> 
> Thanks, that's a good point.
> 
> I started working on unflaking before the revert CL landed, so things got
> confused.

To compound the error, my fix got uploaded in the original CL. That does mean
that you can see the difference, though, if you compare the last patch set.

[droger, 2017-04-13 09:01:51]

lgtm

https://codereview.chromium.org/2819523002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2819523002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:185:
base::Bind([](base::RunLoop* loop) { loop->Quit(); }, loop));
Nit:
base::Bind(&base::RunLoop::Quit, base::Unretained(loop));

[droger, 2017-04-13 11:29:04]

https://codereview.chromium.org/2819523002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2819523002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:185:
base::Bind([](base::RunLoop* loop) { loop->Quit(); }, loop));
On 2017/04/13 09:01:51, droger wrote:
> Nit:
> base::Bind(&base::RunLoop::Quit, base::Unretained(loop));

And you can actually use loop->QuitClosure().

[mattcary, 2017-04-13 12:00:21]

https://codereview.chromium.org/2819523002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2819523002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:185:
base::Bind([](base::RunLoop* loop) { loop->Quit(); }, loop));
On 2017/04/13 11:29:04, droger wrote:
> On 2017/04/13 09:01:51, droger wrote:
> > Nit:
> > base::Bind(&base::RunLoop::Quit, base::Unretained(loop));
> 
> And you can actually use loop->QuitClosure().

Done, including correctly plumbing the closure rather than the loop in from the
beginning.

[Charlie Harrison, 2017-04-13 12:04:24]

parser lgtm

[droger, 2017-04-13 13:40:29]

Thanks, looks good!

https://codereview.chromium.org/2819523002/diff/20001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2819523002/diff/20001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:123: } while
(!found_manifest);
Can we remove the while loop and just EXPECT_TRUE(found_manifest)?

[mattcary, 2017-04-13 14:05:54]

https://codereview.chromium.org/2819523002/diff/20001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right):

https://codereview.chromium.org/2819523002/diff/20001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:123: } while
(!found_manifest);
On 2017/04/13 13:40:29, droger wrote:
> Can we remove the while loop and just EXPECT_TRUE(found_manifest)?

No, it sometimes takes a while for the manifest to get read & created. I suppose
it depends on the timing of the manifest resource fetch which is racing with our
GetAllAppCacheInfo call.

[michaeln, 2017-04-13 21:06:53]

lgtm 2

[mattcary, 2017-04-14 09:13:03]

The CQ bit was checked by mattcary@chromium.org

[mattcary, 2017-04-14 09:13:04]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org
Link to the patchset: https://codereview.chromium.org/2819523002/#ps20001
(title: "comments")

[commit-bot: I haz the power, 2017-04-14 09:13:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 10:44:41]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492161183834300,
"parent_rev": "09bb93957f9fb4390de8dcca9ca40a6c467f9362", "commit_rev":
"f0abffcea31f665a2b1cc1979a955cf308c4dcb4"}

[commit-bot: I haz the power, 2017-04-14 10:45:35]

Description was changed from

==========
Prerender: Disable prefetch if there's an appcache (with nonflakey tests)

Stops the preload scanner if an appcache manifest is detected and the document
is prefetching.

This un-reverts cl/2790323003. That test introduced by that CL was flakey due to
a race between AppCache creation and prefetching. This CL fixes the race by
blocking until the appropriate AppCache can be verified as existing.

BUG=632368
==========

to

==========
Prerender: Disable prefetch if there's an appcache (with nonflakey tests)

Stops the preload scanner if an appcache manifest is detected and the document
is prefetching.

This un-reverts cl/2790323003. That test introduced by that CL was flakey due to
a race between AppCache creation and prefetching. This CL fixes the race by
blocking until the appropriate AppCache can be verified as existing.

BUG=632368

Review-Url: https://codereview.chromium.org/2819523002
Cr-Commit-Position: refs/heads/master@{#464713}
Committed:
https://chromium.googlesource.com/chromium/src/+/f0abffcea31f665a2b1cc1979a95...
==========

[commit-bot: I haz the power, 2017-04-14 10:45:36]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f0abffcea31f665a2b1cc1979a95...