Prerender: Test that prefetch flags are set correctly on redirect.

Prerender: Test that prefetch flags are set correctly on redirect.

BUG=

Committed: https://crrev.com/e8ff1bfcc6ffda96ddbdfea344c0adf7959883ce
Cr-Commit-Position: refs/heads/master@{#434634}

[mattcary, 2016-11-23 14:21:53]

mattcary@chromium.org changed reviewers:
+ droger@chromium.org, pasko@chromium.org

[mattcary, 2016-11-23 14:21:54]

Is this what we had in mind for the prefetch task "Make sure redirects are
always cached with proper SetIsPrerendering"?

[droger, 2016-11-23 15:20:10]

Yes, I think that's what we need to test.

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

https://codereview.chromium.org/2529653002/diff/20001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:86:
base::Bind(&CreateCountingInterceptorOnIO, url, url_file,
Could we replace this by CreatePrefetchOnlyInterceptorOnIO, or would it make
some test fail?

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

https://codereview.chromium.org/2529653002/diff/20001/chrome/browser/prerende...
chrome/browser/prerender/prerender_test_utils.cc:123: return NULL;
nullptr

[droger, 2016-11-23 15:20:18]

lgtm

[mattcary, 2016-11-23 15:28:32]

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

https://codereview.chromium.org/2529653002/diff/20001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:86:
base::Bind(&CreateCountingInterceptorOnIO, url, url_file,
On 2016/11/23 15:20:10, droger wrote:
> Could we replace this by CreatePrefetchOnlyInterceptorOnIO, or would it make
> some test fail?

It will change slightly what's tested: the CountingInterceptor pings the request
counter when the HTTPJob starts; this interceptor pings the request counter
somewhat earlier, I presume.

Note that the CountingInterceptor is used by the prerender browser test, which
does not set LOAD_PREFETCH, as well as  I suppose I could change this to match
the HTTPJob start semantics of the counting interceptor, and replace
appropriately in the nostate tests. WDYT?

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

https://codereview.chromium.org/2529653002/diff/20001/chrome/browser/prerende...
chrome/browser/prerender/prerender_test_utils.cc:123: return NULL;
On 2016/11/23 15:20:10, droger wrote:
> nullptr

Yikes, how 2010 of me.

[droger, 2016-11-23 15:37:34]

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

https://codereview.chromium.org/2529653002/diff/20001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:86:
base::Bind(&CreateCountingInterceptorOnIO, url, url_file,
On 2016/11/23 15:28:32, mattcary wrote:
> On 2016/11/23 15:20:10, droger wrote:
> > Could we replace this by CreatePrefetchOnlyInterceptorOnIO, or would it make
> > some test fail?
> 
> It will change slightly what's tested: the CountingInterceptor pings the
request
> counter when the HTTPJob starts; this interceptor pings the request counter
> somewhat earlier, I presume.
> 
> Note that the CountingInterceptor is used by the prerender browser test, which
> does not set LOAD_PREFETCH, as well as  I suppose I could change this to match
> the HTTPJob start semantics of the counting interceptor, and replace
> appropriately in the nostate tests. WDYT?

Resolved offline. I did not notice that the two interceptors where behaving
differently and it's not trivial to just replace one by the other.

[pasko, 2016-11-23 16:47:29]

lgtm

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

https://codereview.chromium.org/2529653002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:285:
redirect_counter.AsWeakPtr()));
nit:

since we are waiting on the counter anyway, a weak pointer is not necessary,
wrapping with base::Unretained(&redirect_counter) should be sufficient.

It does not add much code/complexity to use a weak ptr, and there is some
flexibility in it, so I am ok either way, just fyi that this simpler way exists.

https://codereview.chromium.org/2529653002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_test_utils.h (right):

https://codereview.chromium.org/2529653002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_test_utils.h:378: // Checks that |url| has
been requested with prefetch load flags. Pings
nit: s/prefetch load flags/LOAD_PREFETCH/

[mattcary, 2016-11-24 08:30:22]

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

https://codereview.chromium.org/2529653002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:285:
redirect_counter.AsWeakPtr()));
On 2016/11/23 16:47:29, pasko wrote:
> nit:
> 
> since we are waiting on the counter anyway, a weak pointer is not necessary,
> wrapping with base::Unretained(&redirect_counter) should be sufficient.
> 
> It does not add much code/complexity to use a weak ptr, and there is some
> flexibility in it, so I am ok either way, just fyi that this simpler way
exists.

I did it this way to be consistent with the other interceptors.

https://codereview.chromium.org/2529653002/diff/40001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_test_utils.h (right):

https://codereview.chromium.org/2529653002/diff/40001/chrome/browser/prerende...
chrome/browser/prerender/prerender_test_utils.h:378: // Checks that |url| has
been requested with prefetch load flags. Pings
On 2016/11/23 16:47:29, pasko wrote:
> nit: s/prefetch load flags/LOAD_PREFETCH/

Sure, although in the future if the details of the load flags change, this
comment will have to be changed as well.

[mattcary, 2016-11-24 08:31:09]

The CQ bit was checked by mattcary@chromium.org

[mattcary, 2016-11-24 08:31:09]

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

[commit-bot: I haz the power, 2016-11-24 08:31:49]

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, 2016-11-24 10:32:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 10:32:59]

Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[mattcary, 2016-11-24 13:03:52]

The CQ bit was checked by mattcary@chromium.org

[commit-bot: I haz the power, 2016-11-24 13:04:21]

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, 2016-11-24 15:15:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 15:15:37]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pasko, 2016-11-24 19:35:53]

The CQ bit was checked by pasko@chromium.org

[commit-bot: I haz the power, 2016-11-24 19:36:03]

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, 2016-11-24 20:24:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 20:24:35]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mattcary, 2016-11-25 13:09:21]

I've added a new test that checks that load flags are set for a simple prefetch.
This passes... except on PlzNav. The same problem occurs with the redirect load
flags.

I'll start looking into why this is happening---presumably our SetIsPrefetch
isn't plumbed through correctly to the renderer for PlzNav, but any suggestions
are welcome that might speed me up.

[pasko, 2016-11-25 14:21:17]

On 2016/11/25 13:09:21, mattcary wrote:
> I've added a new test that checks that load flags are set for a simple
prefetch.
> This passes... except on PlzNav. The same problem occurs with the redirect
load
> flags.
> 
> I'll start looking into why this is happening---presumably our SetIsPrefetch
> isn't plumbed through correctly to the renderer for PlzNav, but any
suggestions
> are welcome that might speed me up.

my assumptions are basically on the same level of detail, sry :/

[pasko, 2016-11-25 14:30:33]

On 2016/11/25 14:21:17, pasko wrote:
> On 2016/11/25 13:09:21, mattcary wrote:
> > I've added a new test that checks that load flags are set for a simple
> prefetch.
> > This passes... except on PlzNav. The same problem occurs with the redirect
> load
> > flags.
> > 
> > I'll start looking into why this is happening---presumably our SetIsPrefetch
> > isn't plumbed through correctly to the renderer for PlzNav, but any
> suggestions
> > are welcome that might speed me up.
> 
> my assumptions are basically on the same level of detail, sry :/

talked to clamy@, this will need some plznav-side changes, creating a bug
shortly

[mattcary, 2016-11-25 14:37:38]

On 2016/11/25 14:30:33, pasko wrote:
> On 2016/11/25 14:21:17, pasko wrote:
> > On 2016/11/25 13:09:21, mattcary wrote:
> > > I've added a new test that checks that load flags are set for a simple
> > prefetch.
> > > This passes... except on PlzNav. The same problem occurs with the redirect
> > load
> > > flags.
> > > 
> > > I'll start looking into why this is happening---presumably our
SetIsPrefetch
> > > isn't plumbed through correctly to the renderer for PlzNav, but any
> > suggestions
> > > are welcome that might speed me up.
> > 
> > my assumptions are basically on the same level of detail, sry :/
> 
> talked to clamy@, this will need some plznav-side changes, creating a bug
> shortly

Groovy, thanks.

[pasko, 2016-11-25 14:48:18]

http://crbug.com/668714

[pasko, 2016-11-25 14:50:18]

In the meantime let's commit this change with an exclusion filter mentioning the
bug:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/rmJFpRZBEjw/b4Zto...

[mattcary, 2016-11-25 15:23:57]

On 2016/11/25 14:50:18, pasko wrote:
> In the meantime let's commit this change with an exclusion filter mentioning
the
> bug:
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/rmJFpRZBEjw/b4Zto...

Done (I hope, double-check my exclusion filter)

[mattcary, 2016-11-25 15:24:09]

The CQ bit was checked by mattcary@chromium.org

[mattcary, 2016-11-25 15:24:10]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org,
droger@chromium.org
Link to the patchset: https://codereview.chromium.org/2529653002/#ps100001
(title: "disable tests for plznav")

[commit-bot: I haz the power, 2016-11-25 15:24:26]

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

[pasko, 2016-11-25 15:38:15]

On 2016/11/25 15:23:57, mattcary wrote:
> Done (I hope, double-check my exclusion filter

filter lgtm

[commit-bot: I haz the power, 2016-11-25 15:59:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-25 15:59:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mattcary, 2016-11-28 08:38:46]

The CQ bit was checked by mattcary@chromium.org

[commit-bot: I haz the power, 2016-11-28 08:39:07]

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, 2016-11-28 11:39:59]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480322326873490,
"parent_rev": "89efc7467dd9e0dc7e1f3b4292e1a466af9ee12c", "commit_rev":
"7cda7ae3af0bba70d9337095af2445a7cef8d594"}

[commit-bot: I haz the power, 2016-11-28 11:40:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-28 11:42:23]

Description was changed from

==========
Prerender: Test that prefetch flags are set correctly on redirect.

BUG=
==========

to

==========
Prerender: Test that prefetch flags are set correctly on redirect.

BUG=

Committed: https://crrev.com/e8ff1bfcc6ffda96ddbdfea344c0adf7959883ce
Cr-Commit-Position: refs/heads/master@{#434634}
==========

[commit-bot: I haz the power, 2016-11-28 11:42:25]

Patchset 6 (id:??) landed as
https://crrev.com/e8ff1bfcc6ffda96ddbdfea344c0adf7959883ce
Cr-Commit-Position: refs/heads/master@{#434634}