FrameLoader: consume transient user activation earlier

The main reason is alignment between start and commit navigation paths,
so that we can later call CommitNavigation directly from StartNavigation.

As a side effect, this patch also ensures that we consume the
activation for any navigation attempt, even if is was ignored
due to frame detach or some other reason.

Bug: 855189
Change-Id: Ib705c0f20d567341d88e0fc3097ebc4246dcfeff
Reviewed-on: https://chromium-review.googlesource.com/1232200
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593029}

content/renderer/render_frame_impl.cc

@@ -6381,11 +6381,14 @@ void RenderFrameImpl::OpenURL(const NavigationPolicyInfo& info,
       CloneBlobURLToken(info.blob_url_token.get()).PassHandle().release();
   params.should_replace_current_entry =
       info.replaces_current_history_item && render_view_->history_list_length_;
-  params.user_gesture =
-      WebUserGestureIndicator::IsProcessingUserGesture(frame_);
+  params.user_gesture = info.has_user_gesture;
   if (GetContentClient()->renderer()->AllowPopup())
     params.user_gesture = true;
 
+  // TODO(csharrison,dgozman): FrameLoader::StartNavigation already consumes for
+  // all main frame navigations, except in the case where page A is navigating
+  // page B (e.g. using anchor targets). This edge case can go away when
+  // UserActivationV2 ships, which would make the conditional below redundant.
   if (is_main_frame_ || policy == blink::kWebNavigationPolicyNewBackgroundTab ||
       policy == blink::kWebNavigationPolicyNewForegroundTab ||
       policy == blink::kWebNavigationPolicyNewWindow ||

third_party/blink/public/web/web_local_frame_client.h

@@ -341,6 +341,7 @@ class BLINK_EXPORT WebLocalFrameClient {
     WebURLRequest& url_request;
     WebNavigationType navigation_type;
     WebNavigationPolicy default_policy;
+    bool has_user_gesture;
     bool replaces_current_history_item;
     bool is_history_navigation_in_new_child_frame;
     bool is_client_redirect;
@@ -362,6 +363,7 @@ class BLINK_EXPORT WebLocalFrameClient {
         : url_request(url_request),
           navigation_type(kWebNavigationTypeOther),
           default_policy(kWebNavigationPolicyIgnore),
+          has_user_gesture(false),
           replaces_current_history_item(false),
           is_history_navigation_in_new_child_frame(false),
           is_client_redirect(false),

third_party/blink/renderer/core/exported/local_frame_client_impl.cc

@@ -520,6 +520,7 @@ NavigationPolicy LocalFrameClientImpl::DecidePolicyForNavigation(
     DocumentLoader* document_loader,
     WebNavigationType type,
     NavigationPolicy policy,
+    bool has_transient_activation,
     bool replaces_current_history_item,
     bool is_client_redirect,
     WebTriggeringEventInfo triggering_event_info,
@@ -541,6 +542,7 @@ NavigationPolicy LocalFrameClientImpl::DecidePolicyForNavigation(
   navigation_info.default_policy = static_cast<WebNavigationPolicy>(policy);
   // TODO(dgozman): remove this after some Canary coverage.
   CHECK(!web_document_loader || !web_document_loader->GetExtraData());
+  navigation_info.has_user_gesture = has_transient_activation;
   navigation_info.replaces_current_history_item = replaces_current_history_item;
   navigation_info.is_client_redirect = is_client_redirect;
   navigation_info.triggering_event_info = triggering_event_info;

third_party/blink/renderer/core/exported/local_frame_client_impl.h

@@ -117,6 +117,7 @@ class LocalFrameClientImpl final : public LocalFrameClient {
       DocumentLoader*,
       WebNavigationType,
       NavigationPolicy,
+      bool has_transient_activation,
       bool should_replace_current_entry,
       bool is_client_redirect,
       WebTriggeringEventInfo,

third_party/blink/renderer/core/frame/local_frame_client.h

@@ -152,6 +152,7 @@ class CORE_EXPORT LocalFrameClient : public FrameClient {
       DocumentLoader*,
       WebNavigationType,
       NavigationPolicy,
+      bool has_transient_activation,
       bool should_replace_current_entry,
       bool is_client_redirect,
       WebTriggeringEventInfo,

third_party/blink/renderer/core/loader/document_loader.cc

@@ -138,7 +138,7 @@ DocumentLoader::DocumentLoader(
       data_buffer_(SharedBuffer::Create()),
       devtools_navigation_token_(devtools_navigation_token),
       had_sticky_activation_(false),
-      had_transient_activation_(Frame::HasTransientUserActivation(frame_)),
+      had_transient_activation_(false),
       use_counter_(frame_->GetChromeClient().IsSVGImageChromeClient()
                        ? UseCounter::kSVGImageContext
                        : UseCounter::kDefaultContext) {
@@ -466,6 +466,10 @@ void DocumentLoader::SetUserActivated() {
   had_sticky_activation_ = true;
 }
 
+void DocumentLoader::SetHadTransientUserActivation() {
+  had_transient_activation_ = true;
+}
+
 const AtomicString& DocumentLoader::RequiredCSP() {
   return GetFrameLoader().RequiredCSP();
 }

third_party/blink/renderer/core/loader/document_loader.h

@@ -234,6 +234,7 @@ class CORE_EXPORT DocumentLoader
   void LoadFailed(const ResourceError&);
 
   void SetUserActivated();
+  void SetHadTransientUserActivation();
 
   const AtomicString& RequiredCSP();
 

third_party/blink/renderer/core/loader/empty_clients.cc

@@ -103,6 +103,7 @@ NavigationPolicy EmptyLocalFrameClient::DecidePolicyForNavigation(
     NavigationPolicy,
     bool,
     bool,
+    bool,
     WebTriggeringEventInfo,
     HTMLFormElement*,
     ContentSecurityPolicyDisposition,

third_party/blink/renderer/core/loader/empty_clients.h

@@ -273,6 +273,7 @@ class CORE_EXPORT EmptyLocalFrameClient : public LocalFrameClient {
                                              NavigationPolicy,
                                              bool,
                                              bool,
+                                             bool,
                                              WebTriggeringEventInfo,
                                              HTMLFormElement*,
                                              ContentSecurityPolicyDisposition,

third_party/blink/renderer/core/loader/frame_loader.cc

@@ -226,6 +226,8 @@ void FrameLoader::Init() {
       ClientRedirectPolicy::kNotClientRedirect,
       base::UnguessableToken::Create(), nullptr /* navigation_params */,
       nullptr /* extra_data */);
+  if (Frame::HasTransientUserActivation(frame_))
+    provisional_document_loader_->SetHadTransientUserActivation();
   provisional_document_loader_->StartLoading();
 
   frame_->GetDocument()->CancelParsing();
@@ -894,9 +896,19 @@ void FrameLoader::StartNavigation(const FrameLoadRequest& passed_request,
     return;
   }
 
+  bool has_transient_activation = Frame::HasTransientUserActivation(frame_);
+  // TODO(csharrison): In M71 when UserActivation v2 should ship, we can remove
+  // the check that the pages are equal, because consumption should not be
+  // shared across pages. After that, we can also get rid of consumption call
+  // in RenderFrameImpl::OpenURL.
+  if (frame_->IsMainFrame() && origin_document &&
+      frame_->GetPage() == origin_document->GetPage()) {
+    Frame::ConsumeTransientUserActivation(frame_);
+  }
+
   policy = Client()->DecidePolicyForNavigation(
       resource_request, origin_document, nullptr /* document_loader */,
-      navigation_type, policy,
+      navigation_type, policy, has_transient_activation,
       frame_load_type == WebFrameLoadType::kReplaceCurrentItem,
       request.ClientRedirect() == ClientRedirectPolicy::kClientRedirect,
       request.TriggeringEventInfo(), request.Form(),
@@ -947,14 +959,6 @@ void FrameLoader::StartNavigation(const FrameLoadRequest& passed_request,
                                             resource_request);
   DCHECK(provisional_document_loader_);
 
-  // TODO(csharrison): In M70 when UserActivation v2 should ship, we can remove
-  // the check that the pages are equal, because consumption should not be
-  // shared across pages.
-  if (frame_->IsMainFrame() && origin_document &&
-      frame_->GetPage() == origin_document->GetPage()) {
-    Frame::ConsumeTransientUserActivation(frame_);
-  }
-
   // TODO(dgozman): there is still a possibility of
   // |kNavigationPolicyCurrentTab| when starting a navigation. Perhaps, we can
   // just call CommitNavigation in this case instead, maybe from client side?
@@ -1752,6 +1756,8 @@ DocumentLoader* FrameLoader::CreateDocumentLoader(
 
   loader->SetLoadType(load_type);
   loader->SetNavigationType(navigation_type);
+  if (request.HasUserGesture())
+    loader->SetHadTransientUserActivation();
   // TODO(japhet): This is needed because the browser process DCHECKs if the
   // first entry we commit in a new frame has replacement set. It's unclear
   // whether the DCHECK is right, investigate removing this special case.