[PlzNavigate] Add load flags for prefetch navigations

[PlzNavigate] Add load flags for prefetch navigations

BUG=668714

[droger, 2016-12-12 15:48:34]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 15:48:58]

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

[droger, 2016-12-12 15:49:19]

CC clamy: FYI this is the patch that actually fixes the NoStatePrefetch browser
tests.

[commit-bot: I haz the power, 2016-12-12 16:42:35]

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

[commit-bot: I haz the power, 2016-12-12 16:42:36]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[droger, 2016-12-13 12:03:27]

It looks like I will need information from the UI thread after all, either using
the "defer" shenanigans or the ChromeUINavigationData.

[droger, 2016-12-13 12:34:38]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 12:34:49]

Dry run: 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-12-13 14:10:22]

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

[commit-bot: I haz the power, 2016-12-13 14:10:23]

Dry run: 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_...)

[droger, 2016-12-14 16:34:38]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 16:35:03]

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

[droger, 2016-12-14 16:59:25]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 16:59:46]

Dry run: 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-12-14 17:08:29]

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

[commit-bot: I haz the power, 2016-12-14 17:08:30]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[droger, 2016-12-15 10:53:03]

Patchset #1 (id:1) has been deleted

[droger, 2016-12-15 10:53:10]

Patchset #1 (id:20001) has been deleted

[droger, 2016-12-15 10:53:17]

Patchset #1 (id:40001) has been deleted

[droger, 2016-12-15 10:53:23]

Patchset #1 (id:60001) has been deleted

[droger, 2016-12-15 10:53:29]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[droger, 2016-12-15 10:53:32]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-12-15 10:53:42]

Dry run: 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-12-15 11:57:37]

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

[commit-bot: I haz the power, 2016-12-15 11:57:38]

Dry run: This issue passed the CQ dry run.

[droger, 2016-12-15 12:08:03]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 12:08:13]

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

[droger, 2016-12-15 12:16:59]

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

[droger, 2016-12-15 12:17:00]

This is a fix using the existing |defer| mechanism.

clamy@, how do you feel about this approach? What I like is that it keeps the
code together in the same file, whereas adding the ChromeUINavigationData thing
scatters the code more.

[commit-bot: I haz the power, 2016-12-15 12:56:19]

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

[commit-bot: I haz the power, 2016-12-15 12:56:20]

Dry run: This issue passed the CQ dry run.

[mattcary, 2016-12-15 15:58:41]

lgtm

https://codereview.chromium.org/2566163002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2566163002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:143: if (prerender_mode_
== PREFETCH_ONLY)
Is there any reason to defer setting the load flags until Resume() happens?

It seems like there would be less chance for future inconsistency if
set_prerender_mode fiddles the load flags appropriately rather than splitting it
into two steps (set throttle prerender mode, then somewhat later setting the
load flags).

It doesn't really change anything as all the same thread hopping has to occur.

[droger, 2016-12-15 16:32:27]

The CQ bit was checked by droger@chromium.org to run a CQ dry run

[droger, 2016-12-15 16:32:36]

Thanks.

https://codereview.chromium.org/2566163002/diff/120001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2566163002/diff/120001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:143: if (prerender_mode_
== PREFETCH_ONLY)
On 2016/12/15 15:58:41, mattcary wrote:
> Is there any reason to defer setting the load flags until Resume() happens?
> 
> It seems like there would be less chance for future inconsistency if
> set_prerender_mode fiddles the load flags appropriately rather than splitting
it
> into two steps (set throttle prerender mode, then somewhat later setting the
> load flags).
> 
> It doesn't really change anything as all the same thread hopping has to occur.

Done.

[commit-bot: I haz the power, 2016-12-15 16:33:00]

Dry run: 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-12-15 17:27:43]

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

[commit-bot: I haz the power, 2016-12-15 17:27:44]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-12-22 16:55:02]

pasko@chromium.org changed reviewers:
+ clamy@chromium.org, pasko@chromium.org

[pasko, 2016-12-22 16:55:02]

I think David intended to add clamy@

fwiw, lgtm

[jam, 2016-12-22 17:56:56]

jam@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2016-12-22 17:56:57]

lgtm, thanks!

[clamy, 2016-12-23 12:33:32]

Thanks! A few comments below.

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:155:
scoped_refptr<PrerenderThrottleInfo> prerender_throttle_info) {
nit: It'd be nice in the long run to have
DCHECK(CurrentlyOn(BrowserThread::{UI/IO}) in this code. This helps with
correctness and understanding what is happening.

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:164:
BrowserThread::PostTask(
Rather than having two different PostTasks, why not pass the prerender mode as
an argument to ResumeHandler?

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle.h (right):

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.h:87: // Sets the prerender
mode. Must be called befor ResumeHandler().
nit:s/befor/before

[pasko, 2016-12-23 13:28:51]

clamy: one way is to land this now and I'll address comments in a followup
change, does that SG?

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:155:
scoped_refptr<PrerenderThrottleInfo> prerender_throttle_info) {
On 2016/12/23 12:33:32, clamy wrote:
> nit: It'd be nice in the long run to have
> DCHECK(CurrentlyOn(BrowserThread::{UI/IO}) in this code. This helps with
> correctness and understanding what is happening.

With correctness - yes, probably should add these in the long run.

I am not sure how much it helps understanding in practice. We all know that
throttles live on IO thread. Static methods that have a suffix "OnUI" are
documenting this part, right?

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:164:
BrowserThread::PostTask(
On 2016/12/23 12:33:31, clamy wrote:
> Rather than having two different PostTasks, why not pass the prerender mode as
> an argument to ResumeHandler?

I had the same question, but then saw that ResumeHandler is also called in
ResumeThrottles() that runs on the IO thread without necessary context:

https://codesearch.chromium.org/chromium/src/chrome/browser/prerender/prerend...

[clamy, 2016-12-23 14:21:11]

Thanks! Lgtm.

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
File chrome/browser/prerender/prerender_resource_throttle.cc (right):

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:155:
scoped_refptr<PrerenderThrottleInfo> prerender_throttle_info) {
On 2016/12/23 13:28:51, pasko wrote:
> On 2016/12/23 12:33:32, clamy wrote:
> > nit: It'd be nice in the long run to have
> > DCHECK(CurrentlyOn(BrowserThread::{UI/IO}) in this code. This helps with
> > correctness and understanding what is happening.
> 
> With correctness - yes, probably should add these in the long run.
> 
> I am not sure how much it helps understanding in practice. We all know that
> throttles live on IO thread. Static methods that have a suffix "OnUI" are
> documenting this part, right?

That's why I'm marking this as a nit, it doesn't need to be done in this CL.

https://codereview.chromium.org/2566163002/diff/160001/chrome/browser/prerend...
chrome/browser/prerender/prerender_resource_throttle.cc:164:
BrowserThread::PostTask(
On 2016/12/23 13:28:51, pasko wrote:
> On 2016/12/23 12:33:31, clamy wrote:
> > Rather than having two different PostTasks, why not pass the prerender mode
as
> > an argument to ResumeHandler?
> 
> I had the same question, but then saw that ResumeHandler is also called in
> ResumeThrottles() that runs on the IO thread without necessary context:
> 
>
https://codesearch.chromium.org/chromium/src/chrome/browser/prerender/prerend...

Acknowledged.

[pasko, 2016-12-23 14:29:25]

Thanks, Camille. I am going to proceed with committing this and fixing the typo
plus thread checks in a separate change.

[pasko, 2016-12-23 14:29:39]

The CQ bit was checked by pasko@chromium.org

[pasko, 2016-12-23 14:29:39]

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

[commit-bot: I haz the power, 2016-12-23 14:29:46]

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-12-23 14:31:19]

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

[commit-bot: I haz the power, 2016-12-23 14:31:20]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)