Rewrite MHTML download logic using ContentBrowserClient

The current logic uses ResourceDispatcherHostDelegate.
This doesn't work when network service is enabled.
This CL rewrites the logic using ContentBrowserClient,
so test can work with and without network service enabled

BUG=715630

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I109a7f3fafe6a7d81a98ef39f2b828e7e0551cf0
Reviewed-on: https://chromium-review.googlesource.com/801370Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521047}

chrome/browser/chrome_content_browser_client.cc

@@ -3719,6 +3719,22 @@ void ChromeContentBrowserClient::
 #endif
 }
 
+bool ChromeContentBrowserClient::AllowRenderingMhtmlOverHttp(
+    content::NavigationUIData* navigation_ui_data) const {
+#if BUILDFLAG(ENABLE_OFFLINE_PAGES)
+  // It is OK to load the saved offline copy, in MHTML format.
+  ChromeNavigationUIData* chrome_navigation_ui_data =
+      static_cast<ChromeNavigationUIData*>(navigation_ui_data);
+  if (!chrome_navigation_ui_data)
+    return false;
+  offline_pages::OfflinePageNavigationUIData* offline_page_data =
+      chrome_navigation_ui_data->GetOfflinePageNavigationUIData();
+  return offline_page_data && offline_page_data->is_offline_page();
+#else
+  return false;
+#endif
+}
+
 // Static; handles rewriting Web UI URLs.
 bool ChromeContentBrowserClient::HandleWebUI(
     GURL* url,

chrome/browser/chrome_content_browser_client.h

@@ -387,6 +387,8 @@ class ChromeContentBrowserClient : public content::ContentBrowserClient {
       content::RenderFrameHost* frame_host,
       const GURL& frame_url,
       NonNetworkURLLoaderFactoryMap* factories) override;
+  bool AllowRenderingMhtmlOverHttp(
+      content::NavigationUIData* navigation_ui_data) const override;
 
  protected:
   static bool HandleWebUI(GURL* url, content::BrowserContext* browser_context);

chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc

@@ -992,20 +992,3 @@ ChromeResourceDispatcherHostDelegate::CreateClientCertStore(
   return ProfileIOData::FromResourceContext(resource_context)->
       CreateClientCertStore();
 }
-
-bool ChromeResourceDispatcherHostDelegate::AllowRenderingMhtmlOverHttp(
-    net::URLRequest* request) const {
-#if BUILDFLAG(ENABLE_OFFLINE_PAGES)
-  // It is OK to load the saved offline copy, in MHTML format.
-  const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request);
-  ChromeNavigationUIData* navigation_data =
-      static_cast<ChromeNavigationUIData*>(info->GetNavigationUIData());
-  if (!navigation_data)
-    return false;
-  offline_pages::OfflinePageNavigationUIData* offline_page_data =
-      navigation_data->GetOfflinePageNavigationUIData();
-  return offline_page_data && offline_page_data->is_offline_page();
-#else
-  return false;
-#endif
-}

chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h

@@ -93,7 +93,6 @@ class ChromeResourceDispatcherHostDelegate
       net::URLRequest* request) const override;
   std::unique_ptr<net::ClientCertStore> CreateClientCertStore(
       content::ResourceContext* resource_context) override;
-  bool AllowRenderingMhtmlOverHttp(net::URLRequest* request) const override;
 
   // Called on the UI thread. Allows switching out the
   // ExternalProtocolHandler::Delegate for testing code.

content/browser/download/download_browsertest.cc

@@ -40,10 +40,8 @@
 #include "content/browser/download/download_resource_handler.h"
 #include "content/browser/download/download_task_runner.h"
 #include "content/browser/download/parallel_download_utils.h"
-#include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/download_danger_type.h"
-#include "content/public/browser/resource_dispatcher_host_delegate.h"
 #include "content/public/browser/resource_throttle.h"
 #include "content/public/common/content_features.h"
 #include "content/public/common/content_paths.h"
@@ -99,17 +97,17 @@ const std::string kOriginOne = "one.example";
 const std::string kOriginTwo = "two.example";
 const std::string kOriginThree = "example.com";
 
-class TestResourceDispatcherHostDelegate
-    : public ResourceDispatcherHostDelegate {
+// Implementation of TestContentBrowserClient that overrides
+// AllowRenderingMhtmlOverHttp() and allows consumers to set a value.
+class DownloadTestContentBrowserClient : public ContentBrowserClient {
  public:
-  TestResourceDispatcherHostDelegate() = default;
+  DownloadTestContentBrowserClient() = default;
 
-  bool AllowRenderingMhtmlOverHttp(net::URLRequest* request) const override {
+  bool AllowRenderingMhtmlOverHttp(
+      NavigationUIData* navigation_data) const override {
     return allowed_rendering_mhtml_over_http_;
   }
 
-  void SetDelegate() { ResourceDispatcherHost::Get()->SetDelegate(this); }
-
   void set_allowed_rendering_mhtml_over_http(bool allowed) {
     allowed_rendering_mhtml_over_http_ = allowed;
   }
@@ -117,7 +115,7 @@ class TestResourceDispatcherHostDelegate
  private:
   bool allowed_rendering_mhtml_over_http_ = false;
 
-  DISALLOW_COPY_AND_ASSIGN(TestResourceDispatcherHostDelegate);
+  DISALLOW_COPY_AND_ASSIGN(DownloadTestContentBrowserClient);
 };
 
 class MockDownloadItemObserver : public DownloadItem::Observer {
@@ -702,14 +700,6 @@ class DownloadContentTest : public ContentBrowserTest {
                              real_host);
     host_resolver()->AddRule(TestDownloadHttpResponse::kTestDownloadHostName,
                              real_host);
-
-    test_resource_dispatcher_host_delegate_ =
-        std::make_unique<TestResourceDispatcherHostDelegate>();
-    content::BrowserThread::PostTask(
-        content::BrowserThread::IO, FROM_HERE,
-        base::BindOnce(
-            &TestResourceDispatcherHostDelegate::SetDelegate,
-            base::Unretained(test_resource_dispatcher_host_delegate_.get())));
   }
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
@@ -873,18 +863,12 @@ class DownloadContentTest : public ContentBrowserTest {
     return inject_error_callback_;
   }
 
-  TestResourceDispatcherHostDelegate* test_resource_dispatcher_host_delegate() {
-    return test_resource_dispatcher_host_delegate_.get();
-  }
-
  private:
   // Location of the downloads directory for these tests
   base::ScopedTempDir downloads_directory_;
   std::unique_ptr<TestShellDownloadManagerDelegate> test_delegate_;
   TestDownloadResponseHandler test_response_handler_;
   TestDownloadHttpResponse::InjectErrorCallback inject_error_callback_;
-  std::unique_ptr<TestResourceDispatcherHostDelegate>
-      test_resource_dispatcher_host_delegate_;
 };
 
 // Test fixture for parallel downloading.
@@ -3120,18 +3104,21 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, FetchErrorResponseBodyResumption) {
 
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, ForceDownloadMhtml) {
   // Force downloading the MHTML.
-  test_resource_dispatcher_host_delegate()
-      ->set_allowed_rendering_mhtml_over_http(false);
+  DownloadTestContentBrowserClient new_client;
+  new_client.set_allowed_rendering_mhtml_over_http(false);
+  ContentBrowserClient* old_client = SetBrowserClientForTesting(&new_client);
 
   NavigateToURLAndWaitForDownload(
       shell(), embedded_test_server()->GetURL("/download/hello.mhtml"),
       DownloadItem::COMPLETE);
+  SetBrowserClientForTesting(old_client);
 }
 
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, AllowRenderMhtml) {
   // Allows loading the MHTML, instead of downloading it.
-  test_resource_dispatcher_host_delegate()
-      ->set_allowed_rendering_mhtml_over_http(true);
+  DownloadTestContentBrowserClient new_client;
+  new_client.set_allowed_rendering_mhtml_over_http(true);
+  ContentBrowserClient* old_client = SetBrowserClientForTesting(&new_client);
 
   GURL url = embedded_test_server()->GetURL("/download/hello.mhtml");
   auto observer = std::make_unique<content::TestNavigationObserver>(url);
@@ -3141,6 +3128,7 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, AllowRenderMhtml) {
   NavigateToURL(shell(), url);
 
   observer->WaitForNavigationFinished();
+  SetBrowserClientForTesting(old_client);
 }
 
 }  // namespace content

content/browser/loader/mime_sniffing_resource_handler.cc

@@ -527,11 +527,13 @@ bool MimeSniffingResourceHandler::MustDownload() {
   } else if (request()->url().SchemeIsHTTPOrHTTPS() &&
              // The MHTML mime type should be same as the one we check in
              // Blink's DocumentLoader.
-             response_->head.mime_type == "multipart/related" &&
-             !host_->delegate()->AllowRenderingMhtmlOverHttp(request())) {
-    // Force to download the MHTML page from the remote server, instead of
-    // loading it.
-    must_download_ = true;
+             response_->head.mime_type == "multipart/related") {
+    // It is OK to load the saved offline copy, in MHTML format.
+    const ResourceRequestInfo* info =
+        ResourceRequestInfo::ForRequest(request());
+    must_download_ =
+        !GetContentClient()->browser()->AllowRenderingMhtmlOverHttp(
+            info->GetNavigationUIData());
   } else {
     must_download_ = false;
   }

content/browser/loader/navigation_url_loader_network_service.cc

@@ -538,6 +538,8 @@ class NavigationURLLoaderNetworkService::URLLoaderRequestController
   DISALLOW_COPY_AND_ASSIGN(URLLoaderRequestController);
 };
 
+// TODO(https://crbug.com/790734): pass |navigation_ui_data| along with the
+// request so that it could be modified.
 NavigationURLLoaderNetworkService::NavigationURLLoaderNetworkService(
     ResourceContext* resource_context,
     StoragePartition* storage_partition,
@@ -743,6 +745,11 @@ bool NavigationURLLoaderNetworkService::IsDownload() const {
         net::HttpContentDisposition(disposition, std::string())
             .is_attachment()) {
       return true;
+    } else if (response_->head.mime_type == "multipart/related") {
+      // TODO(https://crbug.com/790734): retrieve the new NavigationUIData from
+      // the request and and pass it to AllowRenderingMhtmlOverHttp().
+      return !GetContentClient()->browser()->AllowRenderingMhtmlOverHttp(
+          nullptr);
     }
     // TODO(qinmin): Check whether this is special-case user script that needs
     // to be downloaded.

content/public/browser/content_browser_client.cc

@@ -589,4 +589,9 @@ bool ContentBrowserClient::ShouldOverrideUrlLoading(
 }
 #endif
 
+bool ContentBrowserClient::AllowRenderingMhtmlOverHttp(
+    NavigationUIData* navigation_ui_data) const {
+  return false;
+}
+
 }  // namespace content

content/public/browser/content_browser_client.h

@@ -977,6 +977,11 @@ class CONTENT_EXPORT ContentBrowserClient {
                                         bool is_main_frame,
                                         ui::PageTransition transition);
 #endif
+
+  // Called on IO or UI thread to determine whether or not to allow load and
+  // render MHTML page from http/https URLs.
+  virtual bool AllowRenderingMhtmlOverHttp(
+      NavigationUIData* navigation_ui_data) const;
 };
 
 }  // namespace content

content/public/browser/resource_dispatcher_host_delegate.cc

@@ -104,11 +104,6 @@ ResourceDispatcherHostDelegate::CreateClientCertStore(
   return std::unique_ptr<net::ClientCertStore>();
 }
 
-bool ResourceDispatcherHostDelegate::AllowRenderingMhtmlOverHttp(
-    net::URLRequest* request) const {
-  return false;
-}
-
 ResourceDispatcherHostDelegate::~ResourceDispatcherHostDelegate() {
 }
 

content/public/browser/resource_dispatcher_host_delegate.h

@@ -142,9 +142,6 @@ class CONTENT_EXPORT ResourceDispatcherHostDelegate {
   virtual std::unique_ptr<net::ClientCertStore> CreateClientCertStore(
       ResourceContext* resource_context);
 
-  // Whether or not to allow load and render MHTML page from http/https URLs.
-  virtual bool AllowRenderingMhtmlOverHttp(net::URLRequest* request) const;
-
  protected:
   virtual ~ResourceDispatcherHostDelegate();
 };

testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

@@ -15,10 +15,6 @@
 # https://crbug.com/756312
 -ServiceWorkerVersionBrowserV8CacheTest.Restart
 
-# http://crbug.com/715630
-# This started failing when the test got added in r514724.
--DownloadContentTest.ForceDownloadMhtml
-
 # http://crbug.com/721398
 -ClearSiteDataThrottleBrowserTest.CookiesIntegrationTest
 -ClearSiteDataThrottleBrowserTest.Credentials