Set user_gesture bit at NavigationHandle creation time.

Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a gesture param to NavigationHandleImpl::Create
- adds a gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
  which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest

For the time being, we continue to update the gesture_ value at commit time.
Once crbug.com/667572 is addressed, gesture_ can be made a const member and
we can stop updating it at commit time.

BUG=665952
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/318ecffe833701cfd6db2ac43491ace0a68e18af
Cr-Commit-Position: refs/heads/master@{#435354}

[Bryan McQuade, 2016-11-15 20:15:30]

Description was changed from

==========
Set user gesture at nav handle creation time.

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

to

==========
Set user gesture at nav handle creation time.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Bryan McQuade, 2016-11-15 20:15:46]

Description was changed from

==========
Set user gesture at nav handle creation time.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set user gesture at nav handle creation time.

This is an initial prototype implementation and not yet ready for review.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Bryan McQuade, 2016-11-15 20:15:54]

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

[commit-bot: I haz the power, 2016-11-15 20:16: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-11-15 20:33:29]

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

[commit-bot: I haz the power, 2016-11-15 20:33:29]

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...)

[Bryan McQuade, 2016-11-15 21:18:04]

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

[commit-bot: I haz the power, 2016-11-15 21:18:35]

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-11-15 21:32:25]

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

[commit-bot: I haz the power, 2016-11-15 21:32:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Bryan McQuade, 2016-11-15 23:46:29]

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

[commit-bot: I haz the power, 2016-11-15 23:47:14]

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-11-16 00:38:02]

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

[commit-bot: I haz the power, 2016-11-16 00:38:03]

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_...)

[Bryan McQuade, 2016-11-16 01:04:00]

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

[commit-bot: I haz the power, 2016-11-16 01:04:26]

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-11-16 01:18:12]

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

[commit-bot: I haz the power, 2016-11-16 01:18:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Bryan McQuade, 2016-11-16 02:30:13]

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

[commit-bot: I haz the power, 2016-11-16 02:31:01]

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

[Bryan McQuade, 2016-11-16 02:46:33]

Description was changed from

==========
Set user gesture at nav handle creation time.

This is an initial prototype implementation and not yet ready for review.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set user_gesture bit at NavigationHandle creation time.

This is an initial prototype implementation and not yet ready for review.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-16 04:49:59]

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

[commit-bot: I haz the power, 2016-11-16 04:50:00]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[Bryan McQuade, 2016-11-16 17:43:54]

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

[commit-bot: I haz the power, 2016-11-16 17:44:17]

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

[Bryan McQuade, 2016-11-16 17:58:38]

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

[commit-bot: I haz the power, 2016-11-16 17:58:55]

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

[Bryan McQuade, 2016-11-16 18:10:49]

Description was changed from

==========
Set user_gesture bit at NavigationHandle creation time.

This is an initial prototype implementation and not yet ready for review.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time in all places.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following cases:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a has_user_gesture boolean param to NavigationHandleImpl::Create
  and makes has_user_gesture_ a const bool member, settable only in the
  constructor
- adds a has_user_gesture boolean param to FrameHostMsg_DidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Bryan McQuade, 2016-11-16 18:11:07]

Description was changed from

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time in all places.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following cases:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a has_user_gesture boolean param to NavigationHandleImpl::Create
  and makes has_user_gesture_ a const bool member, settable only in the
  constructor
- adds a has_user_gesture boolean param to FrameHostMsg_DidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time in all places.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a has_user_gesture boolean param to NavigationHandleImpl::Create
  and makes has_user_gesture_ a const bool member, settable only in the
  constructor
- adds a has_user_gesture boolean param to FrameHostMsg_DidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Bryan McQuade, 2016-11-16 18:12:26]

Description was changed from

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time in all places.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a has_user_gesture boolean param to NavigationHandleImpl::Create
  and makes has_user_gesture_ a const bool member, settable only in the
  constructor
- adds a has_user_gesture boolean param to FrameHostMsg_DidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time in all places.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a has_user_gesture boolean param to NavigationHandleImpl::Create
  and makes has_user_gesture_ a const bool member, settable only in the
  constructor
- adds a has_user_gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
  which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Bryan McQuade, 2016-11-16 18:20:56]

Description was changed from

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time in all places.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a has_user_gesture boolean param to NavigationHandleImpl::Create
  and makes has_user_gesture_ a const bool member, settable only in the
  constructor
- adds a has_user_gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
  which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest


BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time in all places.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a has_user_gesture boolean param to NavigationHandleImpl::Create
  and makes has_user_gesture_ a const bool member, settable only in the
  constructor
- adds a has_user_gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
  which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest


BUG=665952
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Bryan McQuade, 2016-11-16 18:33:02]

Description was changed from

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time in all places.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a has_user_gesture boolean param to NavigationHandleImpl::Create
  and makes has_user_gesture_ a const bool member, settable only in the
  constructor
- adds a has_user_gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
  which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest


BUG=665952
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a has_user_gesture boolean param to NavigationHandleImpl::Create
  and makes has_user_gesture_ a const bool member, settable only in the
  constructor
- adds a has_user_gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
  which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest


BUG=665952
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-16 19:09:22]

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

[commit-bot: I haz the power, 2016-11-16 19:09:23]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-11-16 19:21:42]

bmcquade@chromium.org changed reviewers:
+ clamy@chromium.org

[Bryan McQuade, 2016-11-16 19:22:19]

clamy, here's the change we discussed in email that sets the user gesture bit
when NavigationHandle is created. PTAL, thanks!

[clamy, 2016-11-17 14:30:04]

Thanks! This looks mostly good, a few comments below.

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:524: method_ =
params.method;
I think it'd be nice to DCHECK here that the gesture we got from the commit
message is the same that we set at creation time. This way we can catch
unexpected bugs.

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1170: has_user_gesture);
We have the info about user gesture in the BeginNavigationParams inside
NavigationRequest. I'd rather we use it directly instead of passing it as a
param here.

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3227: pending_nav_entry_id,
params.gesture == NavigationGestureUser,
While we're at it, why not replace the has_user_gesture boolean in
NavigationHandle by an actual NavigationGesture? This would also require to
modify the DidStartProvisionalLoad & BeginNavigation IPCs.

[Bryan McQuade, 2016-11-17 17:54:15]

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

[commit-bot: I haz the power, 2016-11-17 17:55:08]

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-11-17 18:17:13]

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

[commit-bot: I haz the power, 2016-11-17 18:17:14]

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...)

[Bryan McQuade, 2016-11-17 19:26:15]

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

[commit-bot: I haz the power, 2016-11-17 19:27:29]

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-11-17 19:52:39]

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

[commit-bot: I haz the power, 2016-11-17 19:52:41]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Bryan McQuade, 2016-11-18 01:51:29]

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

[commit-bot: I haz the power, 2016-11-18 01:52:55]

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-11-18 02:11:40]

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

[commit-bot: I haz the power, 2016-11-18 02:11:41]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Bryan McQuade, 2016-11-18 02:56:39]

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

[commit-bot: I haz the power, 2016-11-18 02:57:47]

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-11-18 05:32:24]

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

[commit-bot: I haz the power, 2016-11-18 05:32:25]

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

[Bryan McQuade, 2016-11-18 14:28:31]

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

[commit-bot: I haz the power, 2016-11-18 14:28: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-11-18 15:07:25]

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

[commit-bot: I haz the power, 2016-11-18 15:07:27]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bryan McQuade, 2016-11-18 16:20:48]

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

[commit-bot: I haz the power, 2016-11-18 16:21:39]

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

[Bryan McQuade, 2016-11-18 16:33:02]

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

[commit-bot: I haz the power, 2016-11-18 16:33:43]

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-11-18 17:20:10]

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

[commit-bot: I haz the power, 2016-11-18 17:20:11]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Bryan McQuade, 2016-11-18 19:02:16]

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

[commit-bot: I haz the power, 2016-11-18 19:03:33]

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-11-18 22:11:44]

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

[commit-bot: I haz the power, 2016-11-18 22:11:45]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-11-19 20:21:39]

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:524: method_ =
params.method;
On 2016/11/17 at 14:30:04, clamy (ooo 11-18) wrote:
> I think it'd be nice to DCHECK here that the gesture we got from the commit
message is the same that we set at creation time. This way we can catch
unexpected bugs.

I agree - this seems like something we should do. I added this, but the DCHECK
unfortunately fires in many tests, as well as at runtime. It seems that the
gesture tracking logic is already a bit buggy. I debugged one such issue in bug
667070. I encountered other cases as well but did not debug those.

I created a minimal patch that just adds this DCHECK for same page navs for the
existing gesture logic (without the rest of this patch) and you can see that
many tests fail as a result: https://codereview.chromium.org/2512043003.

It seems like we may need to invest a bit more time in fixing the existing
gesture tracking logic across blink and content first. I unfortunately don't
have cycles to do that - it looks like a larger project.

I'm also open to moving this patch forward w/o adding the DCHECK, but I am very
happy to put it on hold if you prefer to get the existing gesture logic sorted
out first.

I've removed the DCHECK from this patch for now. Let me know what you think
makes sense as a next step.

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:1170: has_user_gesture);
On 2016/11/17 at 14:30:04, clamy (ooo 11-18) wrote:
> We have the info about user gesture in the BeginNavigationParams inside
NavigationRequest. I'd rather we use it directly instead of passing it as a
param here.

Ah, I missed that. Fixed, thanks!

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:3227: pending_nav_entry_id,
params.gesture == NavigationGestureUser,
On 2016/11/17 at 14:30:04, clamy (ooo 11-18) wrote:
> While we're at it, why not replace the has_user_gesture boolean in
NavigationHandle by an actual NavigationGesture? This would also require to
modify the DidStartProvisionalLoad & BeginNavigation IPCs.

Sure, makes sense, thanks! I went ahead and did the same for
BeginNavigationParams and RequestNavigationParams, as well.

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
content/common/navigation_params.cc:84: gesture(NavigationGestureAuto),
should this default to NavigationGestureAuto or NavigationGestureUnknown?

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
content/common/navigation_params.cc:138: gesture(NavigationGestureAuto) {}
same - auto or unknown here?

[clamy, 2016-11-21 16:50:46]

Thanks!

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:524: method_ =
params.method;
On 2016/11/19 20:21:38, Bryan McQuade wrote:
> On 2016/11/17 at 14:30:04, clamy (ooo 11-18) wrote:
> > I think it'd be nice to DCHECK here that the gesture we got from the commit
> message is the same that we set at creation time. This way we can catch
> unexpected bugs.
> 
> I agree - this seems like something we should do. I added this, but the DCHECK
> unfortunately fires in many tests, as well as at runtime. It seems that the
> gesture tracking logic is already a bit buggy. I debugged one such issue in
bug
> 667070. I encountered other cases as well but did not debug those.
> 
> I created a minimal patch that just adds this DCHECK for same page navs for
the
> existing gesture logic (without the rest of this patch) and you can see that
> many tests fail as a result: https://codereview.chromium.org/2512043003.
> 
> It seems like we may need to invest a bit more time in fixing the existing
> gesture tracking logic across blink and content first. I unfortunately don't
> have cycles to do that - it looks like a larger project.
> 
> I'm also open to moving this patch forward w/o adding the DCHECK, but I am
very
> happy to put it on hold if you prefer to get the existing gesture logic sorted
> out first.
> 
> I've removed the DCHECK from this patch for now. Let me know what you think
> makes sense as a next step.

Acknowledged. Could you add a TODO(clamy) to check that it remains the same, and
link to a bug number? Thanks!

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
content/common/navigation_params.cc:84: gesture(NavigationGestureAuto),
On 2016/11/19 20:21:39, Bryan McQuade wrote:
> should this default to NavigationGestureAuto or NavigationGestureUnknown?

I'd say NavigationGestureUnknown.

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
content/common/navigation_params.cc:138: gesture(NavigationGestureAuto) {}
On 2016/11/19 20:21:39, Bryan McQuade wrote:
> same - auto or unknown here?

See comment below.

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
content/common/navigation_params.cc:156: NavigationGesture gesture)
Since it seems it is both used in RequestNavigationParams &
BeginNavigationParams, could we move it to CommonNavigationParams, which is sent
in all navigation IPCs, instead of having it in two different structs? Thanks!

https://codereview.chromium.org/2499313003/diff/270001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2499313003/diff/270001/content/public/browser...
content/public/browser/navigation_handle.h:200: bool has_user_gesture = false);
Should this take a NavigationGesture as well?

[Bryan McQuade, 2016-11-22 03:00:17]

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

[commit-bot: I haz the power, 2016-11-22 03:00:44]

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-11-22 04:32:29]

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

[commit-bot: I haz the power, 2016-11-22 04:32:30]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-11-22 14:13:03]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2499313003/diff/120001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:524: method_ =
params.method;
On 2016/11/21 at 16:50:46, clamy (slow) wrote:
> On 2016/11/19 20:21:38, Bryan McQuade wrote:
> > On 2016/11/17 at 14:30:04, clamy (ooo 11-18) wrote:
> > > I think it'd be nice to DCHECK here that the gesture we got from the
commit
> > message is the same that we set at creation time. This way we can catch
> > unexpected bugs.
> > 
> > I agree - this seems like something we should do. I added this, but the
DCHECK
> > unfortunately fires in many tests, as well as at runtime. It seems that the
> > gesture tracking logic is already a bit buggy. I debugged one such issue in
bug
> > 667070. I encountered other cases as well but did not debug those.
> > 
> > I created a minimal patch that just adds this DCHECK for same page navs for
the
> > existing gesture logic (without the rest of this patch) and you can see that
> > many tests fail as a result: https://codereview.chromium.org/2512043003.
> > 
> > It seems like we may need to invest a bit more time in fixing the existing
> > gesture tracking logic across blink and content first. I unfortunately don't
> > have cycles to do that - it looks like a larger project.
> > 
> > I'm also open to moving this patch forward w/o adding the DCHECK, but I am
very
> > happy to put it on hold if you prefer to get the existing gesture logic
sorted
> > out first.
> > 
> > I've removed the DCHECK from this patch for now. Let me know what you think
> > makes sense as a next step.
> 
> Acknowledged. Could you add a TODO(clamy) to check that it remains the same,
and link to a bug number? Thanks!

Done.

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
File content/common/navigation_params.cc (right):

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
content/common/navigation_params.cc:84: gesture(NavigationGestureAuto),
On 2016/11/21 at 16:50:46, clamy (slow) wrote:
> On 2016/11/19 20:21:39, Bryan McQuade wrote:
> > should this default to NavigationGestureAuto or NavigationGestureUnknown?
> 
> I'd say NavigationGestureUnknown.

Done (in CommonNavParams)

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
content/common/navigation_params.cc:138: gesture(NavigationGestureAuto) {}
On 2016/11/21 at 16:50:46, clamy (slow) wrote:
> On 2016/11/19 20:21:39, Bryan McQuade wrote:
> > same - auto or unknown here?
> 
> See comment below.

Done

https://codereview.chromium.org/2499313003/diff/140001/content/common/navigat...
content/common/navigation_params.cc:156: NavigationGesture gesture)
On 2016/11/21 at 16:50:46, clamy (slow) wrote:
> Since it seems it is both used in RequestNavigationParams &
BeginNavigationParams, could we move it to CommonNavigationParams, which is sent
in all navigation IPCs, instead of having it in two different structs? Thanks!

Ah, sure, done.

https://codereview.chromium.org/2499313003/diff/270001/content/public/browser...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/2499313003/diff/270001/content/public/browser...
content/public/browser/navigation_handle.h:200: bool has_user_gesture = false);
On 2016/11/21 at 16:50:46, clamy (slow) wrote:
> Should this take a NavigationGesture as well?

NavigationGesture isn't currently part of the public content API, so I'm
inclined to defer that until its own subsequent change, since that'll involve
updating #includes of navigation_gesture.h. I did prototype moving
NavigationGesture to the public API in
https://codereview.chromium.org/2524763002 and we can go forward with that if
you like, but also happy to defer that decision to a separate change.

[clamy, 2016-11-24 12:20:02]

Thanks! Lgtm provided the comments below are addressed.

https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (left):

https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:547: has_user_gesture_ =
(params.gesture == NavigationGestureUser);
While we investigate why the gesture doesn't match, can we keep updating it
here?

https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:543: // TODO(clamy): DCHECK
that params.gesture matches
nit: empty line above.

[Bryan McQuade, 2016-11-27 16:07:32]

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

[commit-bot: I haz the power, 2016-11-27 16:07:41]

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-11-27 18:42:47]

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

[commit-bot: I haz the power, 2016-11-27 18:42:48]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-11-28 01:41:03]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (left):

https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:547: has_user_gesture_ =
(params.gesture == NavigationGestureUser);
On 2016/11/24 at 12:20:02, clamy (slow) wrote:
> While we investigate why the gesture doesn't match, can we keep updating it
here?

Sure, updated.

https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
File content/browser/frame_host/navigation_handle_impl.cc (right):

https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
content/browser/frame_host/navigation_handle_impl.cc:543: // TODO(clamy): DCHECK
that params.gesture matches
On 2016/11/24 at 12:20:02, clamy (slow) wrote:
> nit: empty line above.

Done

[Bryan McQuade, 2016-11-28 01:41:53]

On 2016/11/28 at 01:41:03, Bryan McQuade wrote:
> Thanks! Addressed comments. PTAL.
> 
>
https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
> File content/browser/frame_host/navigation_handle_impl.cc (left):
> 
>
https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
> content/browser/frame_host/navigation_handle_impl.cc:547: has_user_gesture_ =
(params.gesture == NavigationGestureUser);
> On 2016/11/24 at 12:20:02, clamy (slow) wrote:
> > While we investigate why the gesture doesn't match, can we keep updating it
here?
> 
> Sure, updated.
> 
>
https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
> File content/browser/frame_host/navigation_handle_impl.cc (right):
> 
>
https://codereview.chromium.org/2499313003/diff/290001/content/browser/frame_...
> content/browser/frame_host/navigation_handle_impl.cc:543: // TODO(clamy):
DCHECK that params.gesture matches
> On 2016/11/24 at 12:20:02, clamy (slow) wrote:
> > nit: empty line above.
> 
> Done

Ah, I see your earlier LGTM, thanks! You can disregard my request to re-review,
thanks.

[Bryan McQuade, 2016-11-28 15:09:49]

Description was changed from

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a has_user_gesture boolean param to NavigationHandleImpl::Create
  and makes has_user_gesture_ a const bool member, settable only in the
  constructor
- adds a has_user_gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
  which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest


BUG=665952
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a gesture param to NavigationHandleImpl::Create
- adds a gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
  which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest

For the time being, we continue to update the gesture_ value at commit time.
Once crbug.com/667572 is addressed, gesture_ can be made a const member and
we can stop updating it at commit time.

BUG=665952
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Bryan McQuade, 2016-11-28 15:12:13]

bmcquade@chromium.org changed reviewers:
+ bauerb@chromium.org, michaelbai@chromium.org, rdevlin.cronin@chromium.org

[Bryan McQuade, 2016-11-28 15:13:19]

bauerb, PTAL for flash_download_interception_unittest.cc
michaelbai, PTAL for intercept_navigation_throttle_unittest.cc
rdevlin.cronin, PTAL for extension_navigation_throttle_unittest.cc

Thanks!

[Bryan McQuade, 2016-11-28 15:18:45]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2016-11-28 15:18:45]

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

[Bryan McQuade, 2016-11-28 15:18:49]

The CQ bit was unchecked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-11-28 15:18:56]

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

[Devlin, 2016-11-28 15:48:50]

extension_navigation_throttle_unittest.cc lgtm

[Bernhard Bauer, 2016-11-28 17:06:57]

LGTM

[Bryan McQuade, 2016-11-28 20:13:07]

bmcquade@chromium.org changed reviewers:
+ sgurun@chromium.org

[michaelbai, 2016-11-29 00:55:11]

components/navigation_interception/ LGTM

[Bryan McQuade, 2016-11-29 01:46:53]

bmcquade@chromium.org changed reviewers:
- sgurun@chromium.org

[Bryan McQuade, 2016-11-29 16:56:02]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-11-29 16:56:26]

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-29 17:07:31]

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

[commit-bot: I haz the power, 2016-11-29 17:07:33]

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

[Bryan McQuade, 2016-11-29 19:13:49]

bmcquade@chromium.org changed reviewers:
+ tsepez@chromium.org

[Bryan McQuade, 2016-11-29 19:14:09]

tsepez, PTAL for frame_messages.h, thanks!

[Tom Sepez, 2016-11-29 20:11:44]

lgtm

[Bryan McQuade, 2016-11-29 20:16:14]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-11-29 20:16:36]

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-29 20:36:07]

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

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

Failed to apply patch for
content/browser/loader/resource_dispatcher_host_impl.cc:
While running git apply --index -p1;
  error: patch failed:
content/browser/loader/resource_dispatcher_host_impl.cc:2208
  error: content/browser/loader/resource_dispatcher_host_impl.cc: patch does not
apply

Patch:       content/browser/loader/resource_dispatcher_host_impl.cc
Index: content/browser/loader/resource_dispatcher_host_impl.cc
diff --git a/content/browser/loader/resource_dispatcher_host_impl.cc
b/content/browser/loader/resource_dispatcher_host_impl.cc
index
7bcf5e0bb86edf57181d3b36be9ccf3e989b08e8..2c68cc9fdf4d888c0c82219e27c9fc26d6dc89be
100644
--- a/content/browser/loader/resource_dispatcher_host_impl.cc
+++ b/content/browser/loader/resource_dispatcher_host_impl.cc
@@ -2197,7 +2197,8 @@ void ResourceDispatcherHostImpl::BeginNavigationRequest(
       false,
       false,  // is download
       false,  // is stream
-      info.common_params.allow_download, info.begin_params.has_user_gesture,
+      info.common_params.allow_download,
+      info.common_params.gesture == NavigationGestureUser,
       true,   // enable_load_timing
       false,  // enable_upload_progress
       false,  // do_not_prompt_for_login
@@ -2208,7 +2209,7 @@ void ResourceDispatcherHostImpl::BeginNavigationRequest(
       blink::WebPageVisibilityStateVisible, resource_context,
       base::WeakPtr<ResourceMessageFilter>(),  // filter
       info.report_raw_headers,
-      true,   // is_async
+      true,  // is_async
       IsUsingLoFi(info.common_params.lofi_state, delegate_, *new_request,
                   resource_context, info.is_main_frame),
       // The original_headers field is for stale-while-revalidate but the

[Bryan McQuade, 2016-11-29 21:54:03]

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

[commit-bot: I haz the power, 2016-11-29 21:54:54]

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-11-29 23:57:06]

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

[commit-bot: I haz the power, 2016-11-29 23:57:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_site_isolation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[Bryan McQuade, 2016-11-30 15:20:34]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2016-11-30 15:20:35]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
michaelbai@chromium.org, clamy@chromium.org, rdevlin.cronin@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2499313003/#ps330001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-30 15:20:53]

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-30 15:27:39]

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

[commit-bot: I haz the power, 2016-11-30 15:27:41]

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

[Bryan McQuade, 2016-11-30 16:46:22]

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

[commit-bot: I haz the power, 2016-11-30 16:46:54]

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-11-30 18:29:14]

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

[commit-bot: I haz the power, 2016-11-30 18:29:16]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-11-30 18:38:22]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2016-11-30 18:38:22]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
michaelbai@chromium.org, clamy@chromium.org, rdevlin.cronin@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2499313003/#ps350001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-30 18:39:02]

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-30 18:44:30]

CQ is committing da patch.

Bot data: {"patchset_id": 350001, "attempt_start_ts": 1480531102384670,
"parent_rev": "4a212d91185454feb5f4a8a0e2c468c8b8de10b7", "commit_rev":
"e53feba6b97e9bb6b13d5f72db1bc63140797e6c"}

[commit-bot: I haz the power, 2016-11-30 18:45:02]

Committed patchset #19 (id:350001)

[commit-bot: I haz the power, 2016-11-30 18:49:16]

Description was changed from

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a gesture param to NavigationHandleImpl::Create
- adds a gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
  which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest

For the time being, we continue to update the gesture_ value at commit time.
Once crbug.com/667572 is addressed, gesture_ can be made a const member and
we can stop updating it at commit time.

BUG=665952
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Set user_gesture bit at NavigationHandle creation time.

Previously, the user_gesture bit was set at different places and could be
updated during the lifetime of a navigation, such as in WillStartNavigation
and at commit time. In practice, whether a navigation was initiated by
user gesture is known at the time a NavigationHandle is created.
This change makes has_user_gesture_ a const member of NavigationHandleImpl,
and sets it once at construction time.

clamy suggested that we could set the user gesture bit at NavHandle
construction time in the following places:
- PlzNavigate: we have it when we create the handle.
- Regular navs: we add the parameter to DidStartProvisionalLoad.
- same-page navs: we create the NavigationHandle in DidCommitProvisionalLoad,
  where we have the information.

This patch makes the following changes:
- adds a gesture param to NavigationHandleImpl::Create
- adds a gesture boolean param to FrameHostMsg_DidStartProvisionalLoad,
  which is passed to RenderFrameHostImpl::OnDidStartProvisionalLoad
- removes the has_user_gesture boolean param from
  NavigationHandleImpl::WillStartRequest

For the time being, we continue to update the gesture_ value at commit time.
Once crbug.com/667572 is addressed, gesture_ can be made a const member and
we can stop updating it at commit time.

BUG=665952
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/318ecffe833701cfd6db2ac43491ace0a68e18af
Cr-Commit-Position: refs/heads/master@{#435354}
==========

[commit-bot: I haz the power, 2016-11-30 18:49:18]

Patchset 19 (id:??) landed as
https://crrev.com/318ecffe833701cfd6db2ac43491ace0a68e18af
Cr-Commit-Position: refs/heads/master@{#435354}

[Bryan McQuade, 2016-12-08 01:27:03]

A revert of this CL (patchset #19 id:350001) has been created in
https://codereview.chromium.org/2557233002/ by bmcquade@chromium.org.

The reason for reverting is: This patch broke
UrlOverridingTest#testOpenWindowFromUserGesture. See
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=....