Remove kNavigationImmediateResponse flag

Currently, the NavigationImmediateResponse feature is enabled by
default through a flag kNavigationImmediateResponseBody.

As this feature has been landed for some time, this patch removes this
flag and the support for the old behavior.

Bug: 1017705
Change-Id: If02d58b28ccaadfa1ca94e080217321521a009fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1911148
Commit-Queue: Minggang Wang <minggang.wang@intel.com>
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#716024}

components/download/internal/common/resource_downloader.cc

@@ -205,10 +205,7 @@ void ResourceDownloader::InterceptResponse(
   // Simulate on the new URLLoaderClient calls that happened on the old client.
   response_head->head.cert_status = cert_status;
   url_loader_client_->OnReceiveResponse(response_head->head);
-
-  // Available when NavigationImmediateResponse is enabled.
-  if (response_body)
-    url_loader_client_->OnStartLoadingResponseBody(std::move(response_body));
+  url_loader_client_->OnStartLoadingResponseBody(std::move(response_body));
 
   // Bind the new client.
   url_loader_client_receiver_ =

content/browser/loader/navigation_url_loader_impl.cc

@@ -64,7 +64,6 @@
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
-#include "content/public/common/navigation_policy.h"
 #include "content/public/common/referrer.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/common/url_utils.h"
@@ -813,11 +812,9 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
  private:
   // network::mojom::URLLoaderClient implementation:
   void OnReceiveResponse(network::mojom::URLResponseHeadPtr head) override {
-    // When NavigationImmediateResponseBody is enabled, wait for
-    // OnStartLoadingResponseBody() before sending anything to the renderer
-    // process.
-    if (IsNavigationImmediateResponseBodyEnabled() &&
-        !response_body_.is_valid()) {
+    // Wait for OnStartLoadingResponseBody() before sending anything to the
+    // renderer process.
+    if (!response_body_.is_valid()) {
       head_ = head;
       return;
     }
@@ -971,24 +968,13 @@ class NavigationURLLoaderImpl::URLLoaderRequestController
   }
 
   void OnReceiveCachedMetadata(mojo_base::BigBuffer data) override {
-    // No cached metadata is ever sent for the main resource. The
-    // NavigationImmediateResponse feature does not support it.
-    CHECK(false);
+    NOTREACHED();
   }
 
   void OnTransferSizeUpdated(int32_t transfer_size_diff) override {}
 
   void OnStartLoadingResponseBody(
       mojo::ScopedDataPipeConsumerHandle response_body) override {
-    // When NavigationImmediateResponseBody is disabled, this is not reached.
-    // Instead, the loader and client endpoints must have been unbound and
-    // forwarded to the renderer.
-    CHECK(IsNavigationImmediateResponseBodyEnabled());
-
-    // When NavigationImmediateResponseBody is enabled, the NavigationURLLoader
-    // waits for OnStartLoadingResponseBody() instead of OnReceiveResponse()
-    // before delegating the load to an URLLoaderClientImpl in the renderer
-    // process.
     response_body_ = std::move(response_body);
     OnReceiveResponse(head_);
   }

content/browser/web_package/prefetched_signed_exchange_cache.cc

@@ -18,7 +18,6 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_features.h"
-#include "content/public/common/navigation_policy.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver_set.h"

content/browser/web_package/signed_exchange_loader.cc

@@ -97,7 +97,7 @@ SignedExchangeLoader::SignedExchangeLoader(
 
   url_loader_.Bind(std::move(endpoints->url_loader));
 
-  // Available when NavigationImmediateResponse is enabled.
+  // |outer_response_body| is valid, when it's a navigation request.
   if (outer_response_body)
     OnStartLoadingResponseBody(std::move(outer_response_body));
 

content/browser/web_package/signed_exchange_loader_unittest.cc

@@ -175,9 +175,15 @@ TEST_P(SignedExchangeLoaderTest, Simple) {
       net::SHA256HashValue({{0x00}}))});
 
   SignedExchangeLoader::SetSignedExchangeHandlerFactoryForTest(&factory);
+
+  mojo::ScopedDataPipeProducerHandle producer_handle;
+  mojo::ScopedDataPipeConsumerHandle consumer_handle;
+  MojoResult rv =
+      mojo::CreateDataPipe(nullptr, &producer_handle, &consumer_handle);
+  ASSERT_EQ(MOJO_RESULT_OK, rv);
   std::unique_ptr<SignedExchangeLoader> signed_exchange_loader =
       std::make_unique<SignedExchangeLoader>(
-          resource_request, response, mojo::ScopedDataPipeConsumerHandle(),
+          resource_request, response, std::move(consumer_handle),
           std::move(client), std::move(endpoints),
           network::mojom::kURLLoadOptionNone,
           false /* should_redirect_to_fallback */, nullptr /* devtools_proxy */,
@@ -201,25 +207,19 @@ TEST_P(SignedExchangeLoaderTest, Simple) {
   EXPECT_CALL(mock_client, OnReceiveRedirect(_, _));
   base::RunLoop().RunUntilIdle();
 
-  const std::string kTestString = "Hello, world!";
-  mojo::DataPipe data_pipe(static_cast<uint32_t>(kTestString.size()));
   std::unique_ptr<mojo::DataPipeProducer> producer =
-      std::make_unique<mojo::DataPipeProducer>(
-          std::move(data_pipe.producer_handle));
-
+      std::make_unique<mojo::DataPipeProducer>(std::move(producer_handle));
   mojo::DataPipeProducer* raw_producer = producer.get();
   base::RunLoop run_loop;
   raw_producer->Write(
       std::make_unique<mojo::StringDataSource>(
-          kTestString, mojo::StringDataSource::AsyncWritingMode::
-                           STRING_MAY_BE_INVALIDATED_BEFORE_COMPLETION),
+          "Hello, world!", mojo::StringDataSource::AsyncWritingMode::
+                               STRING_MAY_BE_INVALIDATED_BEFORE_COMPLETION),
       base::BindOnce([](std::unique_ptr<mojo::DataPipeProducer> producer,
                         base::OnceClosure quit_closure,
                         MojoResult result) { std::move(quit_closure).Run(); },
                      std::move(producer), run_loop.QuitClosure()));
 
-  loader_client->OnStartLoadingResponseBody(
-      std::move(data_pipe.consumer_handle));
   network::URLLoaderCompletionStatus status;
   loader_client->OnComplete(network::URLLoaderCompletionStatus(net::OK));
   base::RunLoop().RunUntilIdle();

content/browser/worker_host/shared_worker_host_unittest.cc

@@ -84,6 +84,12 @@ class SharedWorkerHostTest : public testing::Test {
         blink::mojom::WorkerMainScriptLoadParams::New();
     main_script_load_params->response_head =
         network::mojom::URLResponseHead::New();
+    mojo::ScopedDataPipeProducerHandle producer_handle;
+    mojo::ScopedDataPipeConsumerHandle consumer_handle;
+    MojoResult rv =
+        mojo::CreateDataPipe(nullptr, &producer_handle, &consumer_handle);
+    ASSERT_EQ(MOJO_RESULT_OK, rv);
+    main_script_load_params->response_body = std::move(consumer_handle);
     auto subresource_loader_factories =
         std::make_unique<blink::URLLoaderFactoryBundleInfo>();
 

content/common/frame.mojom

@@ -95,9 +95,6 @@ interface FrameNavigationControl {
   // Note: |url_loader_client_endpoints| will be empty iff the navigation URL
   // wasn't handled by the network stack (i.e. about:blank, ...)
   //
-  // Note: |response_body| is only used when NavigationImmediateResponseBody is
-  // enabled. It contains the datapipe used to read the response body.
-  //
   // When the Network Service is enabled, |subresource_loader_factories| may
   // also be provided by the browser as a a means for the renderer to load
   // subresources where applicable.

content/public/common/content_features.cc

@@ -308,11 +308,6 @@ const base::Feature kMojoVideoCaptureSecondary{
 const base::Feature kMouseSubframeNoImplicitCapture{
     "MouseSubframeNoImplicitCapture", base::FEATURE_DISABLED_BY_DEFAULT};
 
-// Transmit the response body datapipe to the renderer process in
-// CommitNavigation() so that it can start reading earlier.
-const base::Feature kNavigationImmediateResponseBody{
-    "NavigationImmediateResponseBody", base::FEATURE_ENABLED_BY_DEFAULT};
-
 // If the network service is enabled, runs it in process.
 const base::Feature kNetworkServiceInProcess {
   "NetworkServiceInProcess",

content/public/common/content_features.h

@@ -73,7 +73,6 @@ CONTENT_EXPORT extern const base::Feature kMimeHandlerViewInCrossProcessFrame;
 CONTENT_EXPORT extern const base::Feature kMojoVideoCapture;
 CONTENT_EXPORT extern const base::Feature kMojoVideoCaptureSecondary;
 CONTENT_EXPORT extern const base::Feature kMouseSubframeNoImplicitCapture;
-CONTENT_EXPORT extern const base::Feature kNavigationImmediateResponseBody;
 CONTENT_EXPORT extern const base::Feature kNetworkQualityEstimatorWebHoldback;
 CONTENT_EXPORT extern const base::Feature kNetworkServiceInProcess;
 CONTENT_EXPORT extern const base::Feature kNeverSlowMode;

content/public/common/navigation_policy.cc

@@ -36,11 +36,6 @@ bool IsProactivelySwapBrowsingInstanceEnabled() {
          IsBackForwardCacheEnabled();
 }
 
-bool IsNavigationImmediateResponseBodyEnabled() {
-  return base::FeatureList::IsEnabled(
-      features::kNavigationImmediateResponseBody);
-}
-
 NavigationDownloadPolicy::NavigationDownloadPolicy() = default;
 NavigationDownloadPolicy::~NavigationDownloadPolicy() = default;
 NavigationDownloadPolicy::NavigationDownloadPolicy(

content/public/common/navigation_policy.h

@@ -17,7 +17,6 @@ namespace content {
 
 CONTENT_EXPORT bool IsBackForwardCacheEnabled();
 CONTENT_EXPORT bool IsProactivelySwapBrowsingInstanceEnabled();
-CONTENT_EXPORT bool IsNavigationImmediateResponseBodyEnabled();
 
 // Navigation type that affects the download decision and relevant metrics to be
 // reported at download-discovery time.

content/renderer/loader/navigation_body_loader.cc

@@ -6,7 +6,6 @@
 
 #include "base/bind.h"
 #include "base/macros.h"
-#include "content/public/common/navigation_policy.h"
 #include "content/renderer/loader/code_cache_loader_impl.h"
 #include "content/renderer/loader/resource_load_stats.h"
 #include "content/renderer/loader/web_url_loader_impl.h"
@@ -326,20 +325,18 @@ void NavigationBodyLoader::NotifyCompletionIfAppropriate() {
 
 void NavigationBodyLoader::
     BindURLLoaderAndStartLoadingResponseBodyIfPossible() {
-  // Bind the mojo::URLLoaderClient interface in advance, because when
-  // NavigationImmediateResponse is enabled we will start to read from the data
-  // pipe immediately which may potentially postpone the method calls from the
-  // remote. That causes the flakiness of some layout tests.
+  // Bind the mojo::URLLoaderClient interface in advance, because we will start
+  // to read from the data pipe immediately which may potentially postpone the
+  // method calls from the remote. That causes the flakiness of some layout
+  // tests.
   // TODO(minggang): The binding was executed after OnStartLoadingResponseBody
-  // when NavigationImmediateResponse is enabled, we should try to
-  // put it back if all the webkit_layout_tests can pass in that way.
+  // originally (prior to the launch of NavigationImmediateResponse), we should
+  // try to put it back if all the webkit_layout_tests can pass in that way.
   BindURLLoaderAndContinue();
 
-  if (response_body_.is_valid()) {
-    DCHECK(IsNavigationImmediateResponseBodyEnabled());
-    OnStartLoadingResponseBody(std::move(response_body_));
-    // Don't use |this| here as it might have been destroyed.
-  }
+  DCHECK(response_body_.is_valid());
+  OnStartLoadingResponseBody(std::move(response_body_));
+  // Don't use |this| here as it might have been destroyed.
 }
 
 }  // namespace content

content/renderer/loader/navigation_body_loader_unittest.cc

@@ -50,8 +50,7 @@ class NavigationBodyLoaderTest : public ::testing::Test,
     NavigationBodyLoader::FillNavigationParamsResponseAndBodyLoader(
         std::move(common_params), std::move(commit_params), 1 /* request_id */,
         network::mojom::URLResponseHead::New(),
-        mojo::ScopedDataPipeConsumerHandle() /* response_body */,
-        std::move(endpoints),
+        std::move(data_pipe_->consumer_handle), std::move(endpoints),
         blink::scheduler::GetSingleThreadTaskRunnerForTesting(),
         2 /* render_frame_id */, true /* is_main_frame */, &navigation_params);
     loader_ = std::move(navigation_params.body_loader);
@@ -59,8 +58,6 @@ class NavigationBodyLoaderTest : public ::testing::Test,
 
   void StartLoading() {
     loader_->StartLoadingBody(this, false /* use_isolated_code_cache */);
-    client_ptr_->OnStartLoadingResponseBody(
-        std::move(data_pipe_->consumer_handle));
     base::RunLoop().RunUntilIdle();
   }
 
@@ -317,11 +314,14 @@ TEST_F(NavigationBodyLoaderTest, FillResponseWithSecurityDetails) {
 
   blink::WebNavigationParams navigation_params;
   auto endpoints = network::mojom::URLLoaderClientEndpoints::New();
+  mojo::ScopedDataPipeProducerHandle producer_handle;
+  mojo::ScopedDataPipeConsumerHandle consumer_handle;
+  MojoResult rv =
+      mojo::CreateDataPipe(nullptr, &producer_handle, &consumer_handle);
+  ASSERT_EQ(MOJO_RESULT_OK, rv);
   NavigationBodyLoader::FillNavigationParamsResponseAndBodyLoader(
       std::move(common_params), std::move(commit_params), 1 /* request_id */,
-      std::move(response),
-      mojo::ScopedDataPipeConsumerHandle() /* response_body */,
-      std::move(endpoints),
+      std::move(response), std::move(consumer_handle), std::move(endpoints),
       blink::scheduler::GetSingleThreadTaskRunnerForTesting(),
       2 /* render_frame_id */, true /* is_main_frame */, &navigation_params);
   EXPECT_TRUE(

content/renderer/loader/resource_dispatcher.cc

@@ -22,7 +22,6 @@
 #include "build/build_config.h"
 #include "content/common/inter_process_time_ticks_converter.h"
 #include "content/common/navigation_params.h"
-#include "content/public/common/navigation_policy.h"
 #include "content/public/common/origin_util.h"
 #include "content/public/common/resource_type.h"
 #include "content/public/common/url_utils.h"
@@ -641,15 +640,13 @@ void ResourceDispatcher::ContinueForNavigation(int request_id) {
   if (!GetPendingRequestInfo(request_id))
     return;
 
-  if (response_override->response_body.is_valid()) {
-    DCHECK(IsNavigationImmediateResponseBodyEnabled());
-    client_ptr->OnStartLoadingResponseBody(
-        std::move(response_override->response_body));
+  DCHECK(response_override->response_body.is_valid());
+  client_ptr->OnStartLoadingResponseBody(
+      std::move(response_override->response_body));
 
-    // Abort if the request is cancelled.
-    if (!GetPendingRequestInfo(request_id))
-      return;
-  }
+  // Abort if the request is cancelled.
+  if (!GetPendingRequestInfo(request_id))
+    return;
 
   DCHECK(response_override->url_loader_client_endpoints);
   client_ptr->Bind(std::move(response_override->url_loader_client_endpoints));

third_party/blink/public/mojom/worker/worker_main_script_load_params.mojom

@@ -14,9 +14,7 @@ struct WorkerMainScriptLoadParams {
   // Used for loading the pre-requested main script in the renderer process.
   network.mojom.URLResponseHead response_head;
 
-  // The |response_body| will be null when NavigationImmediateResponse is
-  // disabled.
-  handle<data_pipe_consumer>? response_body;
+  handle<data_pipe_consumer> response_body;
 
   network.mojom.URLLoaderClientEndpoints? url_loader_client_endpoints;