PlzNavigate: make NavigationResourceHandler a LayeredResourceHandler

content/browser/loader/resource_dispatcher_host_unittest.cc

Index: content/browser/loader/resource_dispatcher_host_unittest.cc
diff --git a/content/browser/loader/resource_dispatcher_host_unittest.cc b/content/browser/loader/resource_dispatcher_host_unittest.cc
index a41dfff2e95006f8f4f22b965794302f726692a0..06f8841b02187eb1a099bd2c0fc9feee42014c00 100644
--- a/content/browser/loader/resource_dispatcher_host_unittest.cc
+++ b/content/browser/loader/resource_dispatcher_host_unittest.cc
@@ -2559,6 +2559,9 @@ TEST_F(ResourceDispatcherHostTest, IgnoreCancelForDownloads) {
 }
 
 TEST_F(ResourceDispatcherHostTest, CancelRequestsForContext) {
+  if (IsBrowserSideNavigationEnabled())

[clamy, 2017/04/27 13:46:11]

I've had to disable this test with PlzNavigate. The reason is the following:
with the change, we properly wait on the UI thread before switching the last
ResourceHandler and make the request a proper download request. This is the
state the test expects the request to be in when it calls
CancelRequestsForContext. But when we initialize the DownloadResourceHandler, it
also posts a task to initialize the PowerSaveBlocker, which does not expect to
be intialized in this unit test. Since the task to intialize the
PowerSaveBlocker happens before notifying the UI thread of the response, when we
add the additional wait here we end up processing the PowerSaveBlocker
initialization.

To solve the issue, I'd have to either modify the download code so that it does
not create a PowerSaveBlocker, or just simulate the IO thread part of
navigation. Doing that would require making NavigationURLLoaderImplCore a
virtual class, with its Impl and Test version (along with the factories to make
them) which seems a bit heavy handed. Considering that in PlzNavigate we only
test about a third of what the test is testing anyway (because the rest doesn;t
make sense), I went with disabling it.

[mmenke, 2017/05/02 15:55:39]

This is fine for now, but what about when we actually enable PlzNavigate by
default, and remove IsBrowserSideNavigationEnabled?  How will we test this?

+    return;
+
   EXPECT_EQ(0, host_.pending_requests());
   NavigationResourceThrottle::set_ui_checks_always_succeed_for_testing(true);
 
@@ -2577,76 +2580,34 @@ TEST_F(ResourceDispatcherHostTest, CancelRequestsForContext) {
 
   const GURL download_url = GURL("http://example.com/blah");
 
-  if (IsBrowserSideNavigationEnabled()) {
-    // Create a NavigationRequest.
-    TestNavigationURLLoaderDelegate delegate;
-    BeginNavigationParams begin_params(
-        std::string(), net::LOAD_NORMAL, false, false,
-        REQUEST_CONTEXT_TYPE_LOCATION,
-        blink::WebMixedContentContextType::kBlockable,
-        false,  // is_form_submission
-        url::Origin(download_url));
-    CommonNavigationParams common_params;
-    common_params.url = download_url;
-    std::unique_ptr<NavigationRequestInfo> request_info(
-        new NavigationRequestInfo(common_params, begin_params, download_url,
-                                  true, false, false, -1, false, false,
-                                  blink::kWebPageVisibilityStateVisible));
-    std::unique_ptr<NavigationURLLoader> loader = NavigationURLLoader::Create(
-        browser_context_->GetResourceContext(),
-        BrowserContext::GetDefaultStoragePartition(browser_context_.get()),
-        std::move(request_info), nullptr, nullptr, nullptr, &delegate);
-
-    // Wait until a response has been received and proceed with the response.
-    KickOffRequest();
-
-    // Return some data so that the request is identified as a download
-    // and the proper resource handlers are created.
-    EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
-    content::RunAllBlockingPoolTasksUntilIdle();
-
-    // The UI thread will be informed that the navigation failed with an error
-    // code of ERR_ABORTED because the navigation turns out to be a download.
-    // The navigation is aborted, but the request goes on as a download.
-    EXPECT_EQ(delegate.net_error(), net::ERR_ABORTED);
-    EXPECT_EQ(1, host_.pending_requests());
-
-    // In PlzNavigate, the renderer cannot cancel the request directly.
-    // However, cancelling by context should work.
-    host_.CancelRequestsForContext(browser_context_->GetResourceContext());
-    EXPECT_EQ(0, host_.pending_requests());
-
-    content::RunAllBlockingPoolTasksUntilIdle();
-  } else {
-    MakeTestRequestWithResourceType(filter_.get(), render_view_id, request_id,
-                                    download_url, RESOURCE_TYPE_MAIN_FRAME);
+  MakeTestRequestWithResourceType(filter_.get(), render_view_id, request_id,
+                                  download_url, RESOURCE_TYPE_MAIN_FRAME);
 
-    // Return some data so that the request is identified as a download
-    // and the proper resource handlers are created.
-    EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
-    content::RunAllBlockingPoolTasksUntilIdle();
+  // Return some data so that the request is identified as a download
+  // and the proper resource handlers are created.
+  EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
+  content::RunAllBlockingPoolTasksUntilIdle();
 
-    // And now simulate a cancellation coming from the renderer.
-    ResourceHostMsg_CancelRequest msg(request_id);
-    OnMessageReceived(msg, filter_.get());
+  // And now simulate a cancellation coming from the renderer.
+  ResourceHostMsg_CancelRequest msg(request_id);
+  OnMessageReceived(msg, filter_.get());
 
-    // Since the request had already started processing as a download,
-    // the cancellation above should have been ignored and the request
-    // should still be alive.
-    EXPECT_EQ(1, host_.pending_requests());
+  // Since the request had already started processing as a download,
+  // the cancellation above should have been ignored and the request
+  // should still be alive.
+  EXPECT_EQ(1, host_.pending_requests());
 
-    // Cancelling by other methods shouldn't work either.
-    host_.CancelRequestsForProcess(render_view_id);
-    EXPECT_EQ(1, host_.pending_requests());
+  // Cancelling by other methods shouldn't work either.
+  host_.CancelRequestsForProcess(render_view_id);
+  EXPECT_EQ(1, host_.pending_requests());
 
-    // Cancelling by context should work.
-    host_.CancelRequestsForContext(filter_->resource_context());
-    EXPECT_EQ(0, host_.pending_requests());
+  // Cancelling by context should work.
+  host_.CancelRequestsForContext(filter_->resource_context());
+  EXPECT_EQ(0, host_.pending_requests());
 
-    while (net::URLRequestTestJob::ProcessOnePendingMessage()) {
-    }
-    content::RunAllBlockingPoolTasksUntilIdle();
+  while (net::URLRequestTestJob::ProcessOnePendingMessage()) {
   }
+  content::RunAllBlockingPoolTasksUntilIdle();
 }
 
 TEST_F(ResourceDispatcherHostTest, CancelRequestsForContextDetached) {