Unify multiple ways to construct DocumentLoader

This patch moves all logic related to constructing
DocumentLoader to it's constructor. It also removes
the methods which we only need when creating
a DocumentLoader.

Bug: 855189
Change-Id: I32521ab453b78c80e3f8266d07eb5a1268df0786
Reviewed-on: https://chromium-review.googlesource.com/c/1274836
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601842}

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

@@ -159,29 +159,6 @@ void ResetWheelAndTouchEventHandlerProperties(LocalFrame& frame) {
       cc::EventListenerProperties::kNone);
 }
 
-void SetupDocumentLoader(
-    DocumentLoader* document_loader,
-    std::unique_ptr<WebNavigationParams> navigation_params) {
-  if (!navigation_params) {
-    document_loader->GetTiming().SetNavigationStart(CurrentTimeTicks());
-    return;
-  }
-
-  const WebNavigationTimings& navigation_timings =
-      navigation_params->navigation_timings;
-  document_loader->UpdateNavigationTimings(
-      navigation_timings.navigation_start, navigation_timings.redirect_start,
-      navigation_timings.redirect_end, navigation_timings.fetch_start,
-      navigation_timings.input_start);
-
-  document_loader->SetSourceLocation(navigation_params->source_location);
-  if (navigation_params->is_user_activated)
-    document_loader->SetUserActivated();
-
-  document_loader->SetServiceWorkerNetworkProvider(
-      std::move(navigation_params->service_worker_network_provider));
-}
-
 }  // namespace
 
 LocalFrameClientImpl::LocalFrameClientImpl(WebLocalFrameImpl* frame)
@@ -761,13 +738,14 @@ DocumentLoader* LocalFrameClientImpl::CreateDocumentLoader(
     const SubstituteData& data,
     ClientRedirectPolicy client_redirect_policy,
     const base::UnguessableToken& devtools_navigation_token,
+    WebFrameLoadType load_type,
+    WebNavigationType navigation_type,
     std::unique_ptr<WebNavigationParams> navigation_params,
     std::unique_ptr<WebDocumentLoader::ExtraData> extra_data) {
   DCHECK(frame);
-
-  WebDocumentLoaderImpl* document_loader = WebDocumentLoaderImpl::Create(
-      frame, request, data, client_redirect_policy, devtools_navigation_token);
-  SetupDocumentLoader(document_loader, std::move(navigation_params));
+  WebDocumentLoaderImpl* document_loader = new WebDocumentLoaderImpl(
+      frame, request, data, client_redirect_policy, devtools_navigation_token,
+      load_type, navigation_type, std::move(navigation_params));
   document_loader->SetExtraData(std::move(extra_data));
   if (web_frame_->Client())
     web_frame_->Client()->DidCreateDocumentLoader(document_loader);

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

@@ -163,6 +163,8 @@ class LocalFrameClientImpl final : public LocalFrameClient {
       const SubstituteData&,
       ClientRedirectPolicy,
       const base::UnguessableToken& devtools_navigation_token,
+      WebFrameLoadType,
+      WebNavigationType,
       std::unique_ptr<WebNavigationParams> navigation_params,
       std::unique_ptr<WebDocumentLoader::ExtraData> extra_data) override;
 

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

@@ -48,18 +48,6 @@
 
 namespace blink {
 
-WebDocumentLoaderImpl* WebDocumentLoaderImpl::Create(
-    LocalFrame* frame,
-    const ResourceRequest& request,
-    const SubstituteData& data,
-    ClientRedirectPolicy client_redirect_policy,
-    const base::UnguessableToken& devtools_navigation_token) {
-  DCHECK(frame);
-
-  return new WebDocumentLoaderImpl(frame, request, data, client_redirect_policy,
-                                   devtools_navigation_token);
-}
-
 const WebURLRequest& WebDocumentLoaderImpl::OriginalRequest() const {
   return original_request_wrapper_;
 }
@@ -114,12 +102,18 @@ WebDocumentLoaderImpl::WebDocumentLoaderImpl(
     const ResourceRequest& request,
     const SubstituteData& data,
     ClientRedirectPolicy client_redirect_policy,
-    const base::UnguessableToken& devtools_navigation_token)
+    const base::UnguessableToken& devtools_navigation_token,
+    WebFrameLoadType load_type,
+    WebNavigationType navigation_type,
+    std::unique_ptr<WebNavigationParams> navigation_params)
     : DocumentLoader(frame,
                      request,
                      data,
                      client_redirect_policy,
-                     devtools_navigation_token),
+                     devtools_navigation_token,
+                     load_type,
+                     navigation_type,
+                     std::move(navigation_params)),
       original_request_wrapper_(DocumentLoader::OriginalRequest()),
       request_wrapper_(DocumentLoader::GetRequest()),
       response_wrapper_(DocumentLoader::GetResponse()) {}

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

@@ -49,12 +49,14 @@ namespace blink {
 class CORE_EXPORT WebDocumentLoaderImpl final : public DocumentLoader,
                                                 public WebDocumentLoader {
  public:
-  static WebDocumentLoaderImpl* Create(
-      LocalFrame*,
-      const ResourceRequest&,
-      const SubstituteData&,
-      ClientRedirectPolicy,
-      const base::UnguessableToken& devtools_navigation_token);
+  WebDocumentLoaderImpl(LocalFrame*,
+                        const ResourceRequest&,
+                        const SubstituteData&,
+                        ClientRedirectPolicy,
+                        const base::UnguessableToken& devtools_navigation_token,
+                        WebFrameLoadType load_type,
+                        WebNavigationType navigation_type,
+                        std::unique_ptr<WebNavigationParams> navigation_params);
 
   static WebDocumentLoaderImpl* FromDocumentLoader(DocumentLoader* loader) {
     return static_cast<WebDocumentLoaderImpl*>(loader);
@@ -87,12 +89,6 @@ class CORE_EXPORT WebDocumentLoaderImpl final : public DocumentLoader,
   void Trace(blink::Visitor*) override;
 
  private:
-  WebDocumentLoaderImpl(
-      LocalFrame*,
-      const ResourceRequest&,
-      const SubstituteData&,
-      ClientRedirectPolicy,
-      const base::UnguessableToken& devtools_navigation_token);
   ~WebDocumentLoaderImpl() override;
   void DetachFromFrame(bool flush_microtask_queue) override;
   String DebugName() const override { return "WebDocumentLoaderImpl"; }

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

@@ -243,6 +243,8 @@ class CORE_EXPORT LocalFrameClient : public FrameClient {
       const SubstituteData&,
       ClientRedirectPolicy,
       const base::UnguessableToken& devtools_navigation_token,
+      WebFrameLoadType,
+      WebNavigationType,
       std::unique_ptr<WebNavigationParams> navigation_params,
       std::unique_ptr<WebDocumentLoader::ExtraData> extra_data) = 0;
 

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

@@ -116,33 +116,78 @@ DocumentLoader::DocumentLoader(
     const ResourceRequest& req,
     const SubstituteData& substitute_data,
     ClientRedirectPolicy client_redirect_policy,
-    const base::UnguessableToken& devtools_navigation_token)
+    const base::UnguessableToken& devtools_navigation_token,
+    WebFrameLoadType load_type,
+    WebNavigationType navigation_type,
+    std::unique_ptr<WebNavigationParams> navigation_params)
     : frame_(frame),
       fetcher_(FrameFetchContext::CreateFetcherFromDocumentLoader(this)),
       original_request_(req),
       substitute_data_(substitute_data),
       request_(req),
-      load_type_(WebFrameLoadType::kStandard),
+      load_type_(load_type),
       is_client_redirect_(client_redirect_policy ==
                           ClientRedirectPolicy::kClientRedirect),
       replaces_current_history_item_(false),
       data_received_(false),
-      navigation_type_(kWebNavigationTypeOther),
+      navigation_type_(navigation_type),
       document_load_timing_(*this),
       application_cache_host_(ApplicationCacheHost::Create(this)),
+      service_worker_network_provider_(
+          navigation_params
+              ? std::move(navigation_params->service_worker_network_provider)
+              : nullptr),
       was_blocked_after_csp_(false),
       state_(kNotStarted),
       committed_data_buffer_(nullptr),
       in_data_received_(false),
       data_buffer_(SharedBuffer::Create()),
       devtools_navigation_token_(devtools_navigation_token),
-      had_sticky_activation_(false),
-      had_transient_activation_(false),
+      had_sticky_activation_(navigation_params &&
+                             navigation_params->is_user_activated),
+      had_transient_activation_(request_.HasUserGesture()),
       use_counter_(frame_->GetChromeClient().IsSVGImageChromeClient()
                        ? UseCounter::kSVGImageContext
                        : UseCounter::kDefaultContext) {
   DCHECK(frame_);
 
+  WebNavigationTimings timings;
+  if (navigation_params)
+    timings = navigation_params->navigation_timings;
+  if (!timings.input_start.is_null())
+    document_load_timing_.SetInputStart(timings.input_start);
+  if (timings.navigation_start.is_null()) {
+    // If we don't have any navigation timings yet, it starts now.
+    document_load_timing_.SetNavigationStart(CurrentTimeTicks());
+  } else {
+    document_load_timing_.SetNavigationStart(timings.navigation_start);
+    if (!timings.redirect_start.is_null()) {
+      document_load_timing_.SetRedirectStart(timings.redirect_start);
+      document_load_timing_.SetRedirectEnd(timings.redirect_end);
+    }
+    if (!timings.fetch_start.is_null()) {
+      // If we started fetching, we should have started the navigation.
+      DCHECK(!timings.navigation_start.is_null());
+      document_load_timing_.SetFetchStart(timings.fetch_start);
+    }
+  }
+
+  if (navigation_params) {
+    WebSourceLocation& location = navigation_params->source_location;
+    source_location_ = SourceLocation::Create(
+        location.url, location.line_number, location.column_number, nullptr);
+  }
+
+  // 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.
+  // TODO(dgozman): we should get rid of this boolean field, and make client
+  // responsible for it's own view of "replaces current item", based on the
+  // frame load type.
+  replaces_current_history_item_ =
+      load_type_ == WebFrameLoadType::kReplaceCurrentItem &&
+      (!frame_->Loader().Opener() || !request_.Url().IsEmpty());
+
   // The document URL needs to be added to the head of the list as that is
   // where the redirects originated.
   if (is_client_redirect_)
@@ -255,14 +300,6 @@ void DocumentLoader::SetServiceWorkerNetworkProvider(
   service_worker_network_provider_ = std::move(provider);
 }
 
-void DocumentLoader::SetSourceLocation(
-    const WebSourceLocation& source_location) {
-  std::unique_ptr<SourceLocation> location =
-      SourceLocation::Create(source_location.url, source_location.line_number,
-                             source_location.column_number, nullptr);
-  source_location_ = std::move(location);
-}
-
 void DocumentLoader::ResetSourceLocation() {
   source_location_ = nullptr;
 }
@@ -466,10 +503,6 @@ void DocumentLoader::SetUserActivated() {
   had_sticky_activation_ = true;
 }
 
-void DocumentLoader::SetHadTransientUserActivation() {
-  had_transient_activation_ = true;
-}
-
 const AtomicString& DocumentLoader::RequiredCSP() {
   return GetFrameLoader().RequiredCSP();
 }
@@ -1271,34 +1304,6 @@ void DocumentLoader::ResumeParser() {
   }
 }
 
-void DocumentLoader::UpdateNavigationTimings(
-    base::TimeTicks navigation_start_time,
-    base::TimeTicks redirect_start_time,
-    base::TimeTicks redirect_end_time,
-    base::TimeTicks fetch_start_time,
-    base::TimeTicks input_start_time) {
-  if (!input_start_time.is_null()) {
-    GetTiming().SetInputStart(input_start_time);
-  }
-
-  // If we don't have any navigation timings yet, just start the navigation.
-  if (navigation_start_time.is_null()) {
-    GetTiming().SetNavigationStart(CurrentTimeTicks());
-    return;
-  }
-
-  GetTiming().SetNavigationStart(navigation_start_time);
-  if (!redirect_start_time.is_null()) {
-    GetTiming().SetRedirectStart(redirect_start_time);
-    GetTiming().SetRedirectEnd(redirect_end_time);
-  }
-  if (!fetch_start_time.is_null()) {
-    // If we started fetching, we should have started the navigation.
-    DCHECK(!navigation_start_time.is_null());
-    GetTiming().SetFetchStart(fetch_start_time);
-  }
-}
-
 DEFINE_WEAK_IDENTIFIER_MAP(DocumentLoader);
 
 }  // namespace blink

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

@@ -36,6 +36,7 @@
 #include "third_party/blink/public/platform/web_loading_behavior_flag.h"
 #include "third_party/blink/public/web/web_frame_load_type.h"
 #include "third_party/blink/public/web/web_global_object_reuse_policy.h"
+#include "third_party/blink/public/web/web_navigation_params.h"
 #include "third_party/blink/public/web/web_navigation_type.h"
 #include "third_party/blink/renderer/bindings/core/v8/source_location.h"
 #include "third_party/blink/renderer/core/core_export.h"
@@ -84,17 +85,14 @@ class CORE_EXPORT DocumentLoader
   USING_GARBAGE_COLLECTED_MIXIN(DocumentLoader);
 
  public:
-  static DocumentLoader* Create(
-      LocalFrame* frame,
-      const ResourceRequest& request,
-      const SubstituteData& data,
-      ClientRedirectPolicy client_redirect_policy,
-      const base::UnguessableToken& devtools_navigation_token) {
-    DCHECK(frame);
-
-    return new DocumentLoader(frame, request, data, client_redirect_policy,
-                              devtools_navigation_token);
-  }
+  DocumentLoader(LocalFrame*,
+                 const ResourceRequest&,
+                 const SubstituteData&,
+                 ClientRedirectPolicy,
+                 const base::UnguessableToken& devtools_navigation_token,
+                 WebFrameLoadType load_type,
+                 WebNavigationType navigation_type,
+                 std::unique_ptr<WebNavigationParams> navigation_params);
   ~DocumentLoader() override;
 
   LocalFrame* GetFrame() const { return frame_; }
@@ -227,14 +225,12 @@ class CORE_EXPORT DocumentLoader
   }
 
   // Allows to specify the SourceLocation that triggered the navigation.
-  void SetSourceLocation(const WebSourceLocation& source_location);
   void ResetSourceLocation();
   std::unique_ptr<SourceLocation> CopySourceLocation() const;
 
   void LoadFailed(const ResourceError&);
 
   void SetUserActivated();
-  void SetHadTransientUserActivation();
 
   const AtomicString& RequiredCSP();
 
@@ -267,25 +263,9 @@ class CORE_EXPORT DocumentLoader
     return content_security_policy_.Get();
   }
 
-  // Updates navigation timings with provided values. This
-  // should be called before WebLocalFrameClient::didCommitProvisionalLoad.
-  // Calling it later may confuse users, because JavaScript may have run and
-  // the user may have already recorded the original value.
-  // Note: if |redirect_start_time| is null, redirect timings are not updated.
-  void UpdateNavigationTimings(base::TimeTicks navigation_start_time,
-                               base::TimeTicks redirect_start_time,
-                               base::TimeTicks redirect_end_time,
-                               base::TimeTicks fetch_start_time,
-                               base::TimeTicks input_start_time);
   UseCounter& GetUseCounter() { return use_counter_; }
 
  protected:
-  DocumentLoader(LocalFrame*,
-                 const ResourceRequest&,
-                 const SubstituteData&,
-                 ClientRedirectPolicy,
-                 const base::UnguessableToken& devtools_navigation_token);
-
   static bool ShouldClearWindowName(
       const LocalFrame&,
       const SecurityOrigin* previous_security_origin,

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

@@ -120,13 +120,16 @@ DocumentLoader* EmptyLocalFrameClient::CreateDocumentLoader(
     const SubstituteData& substitute_data,
     ClientRedirectPolicy client_redirect_policy,
     const base::UnguessableToken& devtools_navigation_token,
+    WebFrameLoadType load_type,
+    WebNavigationType navigation_type,
     std::unique_ptr<WebNavigationParams> navigation_params,
     std::unique_ptr<WebDocumentLoader::ExtraData> extra_data) {
   DCHECK(frame);
 
-  return DocumentLoader::Create(frame, request, substitute_data,
-                                client_redirect_policy,
-                                devtools_navigation_token);
+  return new DocumentLoader(frame, request, substitute_data,
+                            client_redirect_policy, devtools_navigation_token,
+                            load_type, navigation_type,
+                            std::move(navigation_params));
 }
 
 LocalFrame* EmptyLocalFrameClient::CreateFrame(const AtomicString&,

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

@@ -301,6 +301,8 @@ class CORE_EXPORT EmptyLocalFrameClient : public LocalFrameClient {
       const SubstituteData&,
       ClientRedirectPolicy,
       const base::UnguessableToken& devtools_navigation_token,
+      WebFrameLoadType,
+      WebNavigationType,
       std::unique_ptr<WebNavigationParams> navigation_params,
       std::unique_ptr<WebDocumentLoader::ExtraData> extra_data) override;
   void UpdateDocumentLoader(

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

@@ -230,14 +230,15 @@ void FrameLoader::Init() {
   initial_request.SetFrameType(
       frame_->IsMainFrame() ? network::mojom::RequestContextFrameType::kTopLevel
                             : network::mojom::RequestContextFrameType::kNested);
+  initial_request.SetHasUserGesture(
+      LocalFrame::HasTransientUserActivation(frame_));
 
-  provisional_document_loader_ = Client()->CreateDocumentLoader(
-      frame_, initial_request, SubstituteData(),
+  provisional_document_loader_ = CreateDocumentLoader(
+      initial_request, SubstituteData(),
       ClientRedirectPolicy::kNotClientRedirect,
-      base::UnguessableToken::Create(), nullptr /* navigation_params */,
+      base::UnguessableToken::Create(), WebFrameLoadType::kStandard,
+      kWebNavigationTypeOther, nullptr /* navigation_params */,
       nullptr /* extra_data */);
-  if (LocalFrame::HasTransientUserActivation(frame_))
-    provisional_document_loader_->SetHadTransientUserActivation();
   provisional_document_loader_->StartLoading();
 
   frame_->GetDocument()->CancelParsing();
@@ -1791,24 +1792,8 @@ DocumentLoader* FrameLoader::CreateDocumentLoader(
       frame_, request,
       substitute_data.IsValid() ? substitute_data
                                 : DefaultSubstituteDataForURL(request.Url()),
-      client_redirect_policy, devtools_navigation_token,
-      std::move(navigation_params), std::move(extra_data));
-
-  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.
-  bool replace_current_item =
-      load_type == WebFrameLoadType::kReplaceCurrentItem &&
-      (!Opener() || !request.Url().IsEmpty());
-  // TODO(dgozman): we should get rid of this boolean field, and make client
-  // responsible for it's own view of "replaces current item", based on the
-  // frame load type.
-  loader->SetReplacesCurrentHistoryItem(replace_current_item);
-
+      client_redirect_policy, devtools_navigation_token, load_type,
+      navigation_type, std::move(navigation_params), std::move(extra_data));
   probe::lifecycleEvent(frame_, loader, "init", CurrentTimeTicksInSeconds());
   return loader;
 }