[refactoring] Extract WebLocalFrame::CommitDataNavigationWithRequest.

This CL extracts the second half of CommitDataNavigation method into a
new CommitDataNavigationWithRequest method.  The new method is called by
the old WebLocalFrameImpl::CommitDataNavigation method and by
RenderFrameImpl::LoadNavigationErrorPageInternal.

The CL also consolidates and deduplicates RenderFrameImpl code related
to committing of error pages:

  - It avoids duplication of the code that calculates
    blink::WebFrameLoadType and blink::WebHistoryItem based
    on content::HistoryEntry*

  - It removes the need to pass arguments that either 1) do not differ
    among callers or 2) can be deduced from other arguments.  For example:
    - GURL(kUnreachableWebDataURL)
    - is_client_redirect

This CL should be a pure refactoring, with no change in behavior.

Bug: 874544
Change-Id: I0088ad6367407ba70aa28c550ca46d7121dd78eb
Reviewed-on: https://chromium-review.googlesource.com/1176230
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587208}

content/renderer/dom_serializer_browsertest.cc

@@ -142,8 +142,7 @@ class MAYBE_DomSerializerTests : public ContentBrowserTest,
           data, "text/html", encoding_info, base_url, WebURL(),
           false /* replace */, blink::WebFrameLoadType::kStandard,
           blink::WebHistoryItem(), false /* is_client_redirect */,
-          nullptr /* navigation_params */, nullptr /* navigation_data */,
-          nullptr /* original_request_to_replace */);
+          nullptr /* navigation_params */, nullptr /* navigation_data */);
     }
     base::MessageLoopCurrent::ScopedNestableTaskAllower allow;
     waiter.Wait();

content/renderer/render_frame_impl.cc

@@ -2701,12 +2701,6 @@ void RenderFrameImpl::LoadNavigationErrorPage(
     const base::Optional<std::string>& error_page_content,
     std::unique_ptr<blink::WebNavigationParams> navigation_params,
     std::unique_ptr<blink::WebDocumentLoader::ExtraData> navigation_data) {
-  blink::WebFrameLoadType frame_load_type =
-      entry ? blink::WebFrameLoadType::kBackForward
-            : blink::WebFrameLoadType::kStandard;
-  const blink::WebHistoryItem& history_item =
-      entry ? entry->root() : blink::WebHistoryItem();
-
   std::string error_html;
   if (error_page_content.has_value()) {
     error_html = error_page_content.value();
@@ -2716,10 +2710,9 @@ void RenderFrameImpl::LoadNavigationErrorPage(
     GetContentClient()->renderer()->PrepareErrorPage(
         this, failed_request, error, &error_html, nullptr);
   }
-  LoadNavigationErrorPageInternal(error_html, GURL(kUnreachableWebDataURL),
-                                  error.url(), replace, frame_load_type,
-                                  history_item, std::move(navigation_params),
-                                  std::move(navigation_data), failed_request);
+  LoadNavigationErrorPageInternal(error_html, error.url(), replace, entry,
+                                  std::move(navigation_params),
+                                  std::move(navigation_data), &failed_request);
 }
 
 void RenderFrameImpl::LoadNavigationErrorPageForHttpStatusError(
@@ -2730,36 +2723,45 @@ void RenderFrameImpl::LoadNavigationErrorPageForHttpStatusError(
     HistoryEntry* entry,
     std::unique_ptr<blink::WebNavigationParams> navigation_params,
     std::unique_ptr<blink::WebDocumentLoader::ExtraData> navigation_data) {
-  blink::WebFrameLoadType frame_load_type =
-      entry ? blink::WebFrameLoadType::kBackForward
-            : blink::WebFrameLoadType::kStandard;
-  const blink::WebHistoryItem& history_item =
-      entry ? entry->root() : blink::WebHistoryItem();
-
   std::string error_html;
   GetContentClient()->renderer()->PrepareErrorPageForHttpStatusError(
       this, failed_request, unreachable_url, http_status, &error_html, nullptr);
-  LoadNavigationErrorPageInternal(error_html, GURL(kUnreachableWebDataURL),
-                                  unreachable_url, replace, frame_load_type,
-                                  history_item, std::move(navigation_params),
-                                  std::move(navigation_data), failed_request);
+  LoadNavigationErrorPageInternal(error_html, unreachable_url, replace, entry,
+                                  std::move(navigation_params),
+                                  std::move(navigation_data), &failed_request);
 }
 
 void RenderFrameImpl::LoadNavigationErrorPageInternal(
     const std::string& error_html,
-    const GURL& error_page_url,
     const GURL& error_url,
     bool replace,
-    blink::WebFrameLoadType frame_load_type,
-    const blink::WebHistoryItem& history_item,
+    HistoryEntry* history_entry,
     std::unique_ptr<blink::WebNavigationParams> navigation_params,
     std::unique_ptr<blink::WebDocumentLoader::ExtraData> navigation_data,
-    const WebURLRequest& failed_request) {
-  frame_->CommitDataNavigation(
-      error_html, WebString::FromUTF8("text/html"),
-      WebString::FromUTF8("UTF-8"), error_page_url, error_url, replace,
-      frame_load_type, history_item, false, std::move(navigation_params),
-      std::move(navigation_data), &failed_request);
+    const WebURLRequest* failed_request) {
+  blink::WebFrameLoadType frame_load_type =
+      history_entry ? blink::WebFrameLoadType::kBackForward
+                    : blink::WebFrameLoadType::kStandard;
+  const blink::WebHistoryItem& history_item =
+      history_entry ? history_entry->root() : blink::WebHistoryItem();
+
+  // Failed navigations will always provide a |failed_request|.  Error induced
+  // by the client/renderer side after a commit won't have a |failed_request|.
+  bool is_client_redirect = !failed_request;
+
+  WebURLRequest new_request;
+  if (failed_request)
+    new_request = *failed_request;
+  new_request.SetURL(GURL(kUnreachableWebDataURL));
+
+  // Locally generated error pages should not be cached (in particular they
+  // should not inherit the cache mode from |failed_request|).
+  new_request.SetCacheMode(blink::mojom::FetchCacheMode::kNoStore);
+
+  frame_->CommitDataNavigationWithRequest(
+      new_request, error_html, "text/html", "UTF-8", error_url, replace,
+      frame_load_type, history_item, is_client_redirect,
+      std::move(navigation_params), std::move(navigation_data));
 }
 
 void RenderFrameImpl::DidMeaningfulLayout(
@@ -2882,12 +2884,10 @@ void RenderFrameImpl::LoadErrorPage(int reason) {
       this, frame_->GetDocumentLoader()->GetRequest(), error, &error_html,
       nullptr);
 
-  frame_->CommitDataNavigation(
-      error_html, WebString::FromUTF8("text/html"),
-      WebString::FromUTF8("UTF-8"), GURL(kUnreachableWebDataURL), error.url(),
-      true, blink::WebFrameLoadType::kStandard, blink::WebHistoryItem(), true,
+  LoadNavigationErrorPageInternal(
+      error_html, error.url(), true /* replace */, nullptr /* history_entry */,
       nullptr /* navigation_params */, nullptr /* navigation_data */,
-      nullptr /* original_failed_request */);
+      nullptr /* failed_request */);
 }
 
 void RenderFrameImpl::ExecuteJavaScript(const base::string16& javascript) {
@@ -6913,8 +6913,7 @@ void RenderFrameImpl::LoadDataURL(
             common_params, request_params,
             BuildServiceWorkerNetworkProviderForNavigation(
                 &request_params, nullptr /* controller_service_worker_info */)),
-        std::move(navigation_data),
-        nullptr);  // original_failed_request
+        std::move(navigation_data));
   } else {
     CHECK(false) << "Invalid URL passed: "
                  << common_params.url.possibly_invalid_spec();

content/renderer/render_frame_impl.h

@@ -1146,14 +1146,12 @@ class CONTENT_EXPORT RenderFrameImpl
       std::unique_ptr<blink::WebDocumentLoader::ExtraData> navigation_data);
   void LoadNavigationErrorPageInternal(
       const std::string& error_html,
-      const GURL& error_page_url,
       const GURL& error_url,
       bool replace,
-      blink::WebFrameLoadType frame_load_type,
-      const blink::WebHistoryItem& history_item,
+      HistoryEntry* history_entry,
       std::unique_ptr<blink::WebNavigationParams> navigation_params,
       std::unique_ptr<blink::WebDocumentLoader::ExtraData> navigation_data,
-      const blink::WebURLRequest& failed_request);
+      const blink::WebURLRequest* failed_request);
 
   void HandleJavascriptExecutionResult(const base::string16& javascript,
                                        int id,

third_party/blink/public/web/web_local_frame.h

@@ -239,16 +239,15 @@ class WebLocalFrame : public WebFrame {
                               const WebURL& unreachable_url = WebURL(),
                               bool replace = false) = 0;
 
-  // Navigates to the given |data| with specified |mime_type| and optional
-  // |text_encoding|.  For HTML data, |base_url| indicates the security origin
-  // of the document and is used to resolve links.  If specified,
-  // |unreachable_url| is reported via WebDocumentLoader::UnreachableURL.  If
-  // |replace| is false, then this data will be loaded as a normal navigation.
-  // Otherwise, the current history item will be replaced.  The request to
-  // commit will be based either on 1) the previous, provisional request (if
-  // |replace| is true and |unreachable_url| is present) or 2)
-  // |original_failed_request| (if present) or 3) will be constructed from
-  // scratch otherwise.
+  // Calls CommitDataNavigationWithRequest with a WebURLRequest that is built
+  // either 1) based on the previous, provisional request (if |replace| is true
+  // and |unreachable_url| is present) or 2) constructed from scratch otherwise.
+  //
+  // |base_url| indicates the security origin and is used to resolve links in
+  // the committed document.
+  //
+  // Please see documentation of CommitDataNavigationWithRequest for description
+  // of other parameters.
   virtual void CommitDataNavigation(
       const WebData& data,
       const WebString& mime_type,
@@ -260,8 +259,32 @@ class WebLocalFrame : public WebFrame {
       const WebHistoryItem&,
       bool is_client_redirect,
       std::unique_ptr<WebNavigationParams> navigation_params,
-      std::unique_ptr<WebDocumentLoader::ExtraData> navigation_data,
-      const WebURLRequest* original_failed_request) = 0;
+      std::unique_ptr<WebDocumentLoader::ExtraData> navigation_data) = 0;
+
+  // Navigates to the given |data| with specified |mime_type| and optional
+  // |text_encoding|.
+  //
+  // If specified, |unreachable_url| is reported via
+  // WebDocumentLoader::UnreachableURL.
+  //
+  // If |replace| is false, then this data will be loaded as a normal
+  // navigation.  Otherwise, the current history item will be replaced.
+  //
+  // This method can be called instead of CommitDataNavigation when a
+  // ResourceRequest has already been precomputed (e.g. when trying to commit an
+  // error page, while mimicking the original failed request).
+  virtual void CommitDataNavigationWithRequest(
+      const WebURLRequest&,
+      const WebData&,
+      const WebString& mime_type,
+      const WebString& text_encoding,
+      const WebURL& unreachable_url,
+      bool replace,
+      WebFrameLoadType,
+      const WebHistoryItem&,
+      bool is_client_redirect,
+      std::unique_ptr<WebNavigationParams> navigation_params,
+      std::unique_ptr<WebDocumentLoader::ExtraData> navigation_data) = 0;
 
   // Returns the document loader that is currently loading.  May be null.
   virtual WebDocumentLoader* GetProvisionalDocumentLoader() const = 0;

third_party/blink/public/web/web_remote_frame_client.h

@@ -13,8 +13,6 @@
 #include "third_party/blink/public/web/web_frame.h"
 
 namespace blink {
-enum class ClientRedirectPolicy;
-enum class WebFrameLoadType;
 class WebURLRequest;
 struct WebRect;
 

third_party/blink/renderer/core/frame/web_local_frame_impl.cc

@@ -949,7 +949,7 @@ void WebLocalFrameImpl::LoadHTMLString(const WebData& data,
   CommitDataNavigation(data, WebString::FromUTF8("text/html"),
                        WebString::FromUTF8("UTF-8"), base_url, unreachable_url,
                        replace, WebFrameLoadType::kStandard, WebHistoryItem(),
-                       false, nullptr, nullptr, nullptr);
+                       false, nullptr, nullptr);
 }
 
 void WebLocalFrameImpl::StopLoading() {
@@ -2119,8 +2119,7 @@ void WebLocalFrameImpl::CommitDataNavigation(
     const WebHistoryItem& item,
     bool is_client_redirect,
     std::unique_ptr<WebNavigationParams> navigation_params,
-    std::unique_ptr<WebDocumentLoader::ExtraData> navigation_data,
-    const WebURLRequest* original_failed_request) {
+    std::unique_ptr<WebDocumentLoader::ExtraData> navigation_data) {
   DCHECK(GetFrame());
 
   // TODO(dgozman): this whole logic of rewriting the params is odd,
@@ -2151,22 +2150,30 @@ void WebLocalFrameImpl::CommitDataNavigation(
                previous_load_type == WebFrameLoadType::kReloadBypassingCache) {
       web_frame_load_type = previous_load_type;
     }
-  } else if (original_failed_request) {
-    // We should only come here when committing an error page.  In particular,
-    // outside of unit tests, unreachable_url should be non-empty.
-    //
-    // TODO(lukasza): Extract error handling to a separate method.  See a WIP CL
-    // @ https://crrev.com/c/1176230.
-    request = original_failed_request->ToResourceRequest();
-
-    // Locally generated error pages should not be cached (in particular they
-    // should not inherit the cache mode from |original_failed_request|).
-    request.SetCacheMode(mojom::FetchCacheMode::kNoStore);
   }
   request.SetURL(base_url);
 
+  CommitDataNavigationWithRequest(
+      WrappedResourceRequest(request), data, mime_type, text_encoding,
+      unreachable_url, replace, web_frame_load_type, history_item,
+      is_client_redirect, std::move(navigation_params),
+      std::move(navigation_data));
+}
+
+void WebLocalFrameImpl::CommitDataNavigationWithRequest(
+    const WebURLRequest& request,
+    const WebData& data,
+    const WebString& mime_type,
+    const WebString& text_encoding,
+    const WebURL& unreachable_url,
+    bool replace,
+    WebFrameLoadType web_frame_load_type,
+    const WebHistoryItem& history_item,
+    bool is_client_redirect,
+    std::unique_ptr<WebNavigationParams> navigation_params,
+    std::unique_ptr<WebDocumentLoader::ExtraData> navigation_data) {
   FrameLoadRequest frame_request(
-      nullptr, request,
+      nullptr, request.ToResourceRequest(),
       SubstituteData(data, mime_type, text_encoding, unreachable_url));
   DCHECK(frame_request.GetSubstituteData().IsValid());
   frame_request.SetReplacesCurrentItem(replace);

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

@@ -284,8 +284,19 @@ class CORE_EXPORT WebLocalFrameImpl final
       const WebHistoryItem&,
       bool is_client_redirect,
       std::unique_ptr<WebNavigationParams> navigation_params,
-      std::unique_ptr<WebDocumentLoader::ExtraData> navigation_data,
-      const WebURLRequest* original_request_to_replace) override;
+      std::unique_ptr<WebDocumentLoader::ExtraData> navigation_data) override;
+  void CommitDataNavigationWithRequest(
+      const WebURLRequest&,
+      const WebData&,
+      const WebString& mime_type,
+      const WebString& text_encoding,
+      const WebURL& unreachable_url,
+      bool replace,
+      WebFrameLoadType,
+      const WebHistoryItem&,
+      bool is_client_redirect,
+      std::unique_ptr<WebNavigationParams> navigation_params,
+      std::unique_ptr<WebDocumentLoader::ExtraData> navigation_data) override;
   FallbackContentResult MaybeRenderFallbackContent(
       const WebURLError&) const override;
   void ReportContentSecurityPolicyViolation(