Set user_gesture bit at NavigationHandle creation time.

content/browser/frame_host/navigation_handle_impl.cc

Index: content/browser/frame_host/navigation_handle_impl.cc
diff --git a/content/browser/frame_host/navigation_handle_impl.cc b/content/browser/frame_host/navigation_handle_impl.cc
index c58618c022319fce402850e6248941d9f8fae473..428346147a2c3ca40e388d4300376e46ad57342a 100644
--- a/content/browser/frame_host/navigation_handle_impl.cc
+++ b/content/browser/frame_host/navigation_handle_impl.cc
@@ -51,10 +51,12 @@ std::unique_ptr<NavigationHandleImpl> NavigationHandleImpl::Create(
     bool is_srcdoc,
     const base::TimeTicks& navigation_start,
     int pending_nav_entry_id,
+    bool has_user_gesture,
     bool started_from_context_menu) {
   return std::unique_ptr<NavigationHandleImpl>(new NavigationHandleImpl(
       url, frame_tree_node, is_renderer_initiated, is_same_page, is_srcdoc,
-      navigation_start, pending_nav_entry_id, started_from_context_menu));
+      navigation_start, pending_nav_entry_id, has_user_gesture,
+      started_from_context_menu));
 }
 
 NavigationHandleImpl::NavigationHandleImpl(
@@ -65,9 +67,10 @@ NavigationHandleImpl::NavigationHandleImpl(
     bool is_srcdoc,
     const base::TimeTicks& navigation_start,
     int pending_nav_entry_id,
+    bool has_user_gesture,
     bool started_from_context_menu)
     : url_(url),
-      has_user_gesture_(false),
+      has_user_gesture_(has_user_gesture),
       transition_(ui::PAGE_TRANSITION_LINK),
       is_external_protocol_(false),
       net_error_code_(net::OK),
@@ -190,8 +193,6 @@ const Referrer& NavigationHandleImpl::GetReferrer() {
 }
 
 bool NavigationHandleImpl::HasUserGesture() {
-  CHECK_NE(INITIAL, state_)
-      << "This accessor should not be called before the request is started.";
   return has_user_gesture_;
 }
 
@@ -288,7 +289,6 @@ NavigationThrottle::ThrottleCheckResult
 NavigationHandleImpl::CallWillStartRequestForTesting(
     bool is_post,
     const Referrer& sanitized_referrer,
-    bool has_user_gesture,
     ui::PageTransition transition,
     bool is_external_protocol) {
   NavigationThrottle::ThrottleCheckResult result = NavigationThrottle::DEFER;
@@ -304,7 +304,7 @@ NavigationHandleImpl::CallWillStartRequestForTesting(
   }
 
   WillStartRequest(method, resource_request_body, sanitized_referrer,
-                   has_user_gesture, transition, is_external_protocol,
+                   transition, is_external_protocol,
                    REQUEST_CONTEXT_TYPE_LOCATION,
                    base::Bind(&UpdateThrottleCheckResult, &result));
 
@@ -397,7 +397,6 @@ void NavigationHandleImpl::WillStartRequest(
     const std::string& method,
     scoped_refptr<content::ResourceRequestBodyImpl> resource_request_body,
     const Referrer& sanitized_referrer,
-    bool has_user_gesture,
     ui::PageTransition transition,
     bool is_external_protocol,
     RequestContextType request_context_type,
@@ -413,7 +412,6 @@ void NavigationHandleImpl::WillStartRequest(
   if (method_ == "POST")
     resource_request_body_ = resource_request_body;
   sanitized_referrer_ = sanitized_referrer;
-  has_user_gesture_ = has_user_gesture;
   transition_ = transition;
   is_external_protocol_ = is_external_protocol;
   request_context_type_ = request_context_type;
@@ -524,7 +522,6 @@ void NavigationHandleImpl::DidCommitNavigation(
   CHECK_EQ(url_, params.url);
 
   method_ = params.method;

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

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.

[Bryan McQuade, 2016/11/19 20:21:38]

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.

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

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

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

Done.

-  has_user_gesture_ = (params.gesture == NavigationGestureUser);
   transition_ = params.transition;
   render_frame_host_ = render_frame_host;