Remove the BeginSaveFile and BeginDownload methods from ResourceDispatcherHostImpl

content/browser/download/download_manager_impl_unittest.cc

Index: content/browser/download/download_manager_impl_unittest.cc
diff --git a/content/browser/download/download_manager_impl_unittest.cc b/content/browser/download/download_manager_impl_unittest.cc
index 52933cae98f519541ead80fdd16eaca27e349a28..52ae849946ec979255b04d2567d8631ec310fbdb 100644
--- a/content/browser/download/download_manager_impl_unittest.cc
+++ b/content/browser/download/download_manager_impl_unittest.cc
@@ -18,28 +18,42 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "content/browser/byte_stream.h"
+#include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file_factory.h"
 #include "content/browser/download/download_item_factory.h"
 #include "content/browser/download/download_item_impl.h"
 #include "content/browser/download/download_item_impl_delegate.h"
 #include "content/browser/download/download_request_handle.h"
+#include "content/browser/download/download_resource_handler.h"
 #include "content/browser/download/mock_download_file.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/download_interrupt_reasons.h"
 #include "content/public/browser/download_item.h"
 #include "content/public/browser/download_manager_delegate.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/resource_context.h"
+#include "content/public/browser/site_instance.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/zoom_level_delegate.h"
 #include "content/public/test/mock_download_item.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "net/log/net_log.h"
+#include "net/url_request/url_request_job_factory.h"
+#include "net/url_request/url_request_test_job.h"
+#include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gmock_mutant.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -793,4 +807,205 @@ TEST_F(DownloadManagerTest, RemoveDownloadsByURL) {
   EXPECT_EQ(remove_count, 1);
 }
 
+// For verifying notifications sent to the WebContents.
+class TestDownloadWebContentsObserver : public WebContentsObserver {
+ public:
+  explicit TestDownloadWebContentsObserver(WebContents* web_contents)
+      : WebContentsObserver(web_contents),
+        resource_request_redirect_count_(0),
+        resource_response_start_count_(0) {}
+
+  void DidGetRedirectForResourceRequest(
+      RenderFrameHost* render_frame_host,
+      const ResourceRedirectDetails& details) override {
+    ++resource_request_redirect_count_;
+  }
+
+  void DidGetResourceResponseStart(
+      const ResourceRequestDetails& details) override {
+    ++resource_response_start_count_;
+  }
+
+  int resource_request_redirect_count() {
+    return resource_request_redirect_count_;
+  }
+
+  int resource_response_start_count() { return resource_response_start_count_; }
+
+ private:
+  int resource_request_redirect_count_;
+  int resource_response_start_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestDownloadWebContentsObserver);
+};
+
+// Intercepts URLRequests and returns an instance of URLRequestTestJob for
+// handling these requests if the scheme is a supported scheme.
+class TestDownloadURLRequestFactory : public net::URLRequestJobFactory {
+ public:
+  explicit TestDownloadURLRequestFactory() {}
+
+  void HandleScheme(const std::string& scheme) {
+    supported_schemes_.insert(scheme);
+  }
+
+  net::URLRequestJob* MaybeCreateJobWithProtocolHandler(
+      const std::string& scheme,
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    if (!IsHandledProtocol(scheme))
+      return nullptr;
+    return new net::URLRequestTestJob(request, network_delegate, false);
+  }
+
+  net::URLRequestJob* MaybeInterceptRedirect(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate,
+      const GURL& location) const override {
+    return nullptr;
+  }
+
+  net::URLRequestJob* MaybeInterceptResponse(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    return nullptr;
+  }
+
+  bool IsHandledProtocol(const std::string& scheme) const override {
+    return supported_schemes_.count(scheme) > 0;
+  }
+
+  bool IsHandledURL(const GURL& url) const override {
+    return supported_schemes_.count(url.scheme()) > 0;
+  }
+
+  bool IsSafeRedirectTarget(const GURL& location) const override {
+    return false;
+  }
+
+ private:
+  std::set<std::string> supported_schemes_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestDownloadURLRequestFactory);
+};
+
+// This class provides functionality for testing downloads initiated via URL
+// navigations.
+class DownloadRequestTest : public testing::Test {
+ public:
+  DownloadRequestTest()
+      : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
+        host_(base::Bind(&DownloadResourceHandler::Create)) {
+    browser_context_.reset(new TestBrowserContext());
+    BrowserContext::EnsureResourceContextInitialized(browser_context_.get());
+    base::RunLoop().RunUntilIdle();
+  }
+
+  ~DownloadRequestTest() override {}
+
+  // testing::Test overrides.
+  void SetUp() override {
+    // Boiler plate code necessary to initiate end to end navigations.
+    ChildProcessSecurityPolicyImpl::GetInstance()->Add(0);
+    scoped_refptr<SiteInstance> site_instance =
+        SiteInstance::Create(browser_context_.get());
+    web_contents_.reset(
+        WebContents::Create(WebContents::CreateParams(browser_context_.get())));
+    web_contents_observer_.reset(
+        new TestDownloadWebContentsObserver(web_contents_.get()));
+    child_ids_.insert(web_contents_->GetRenderProcessHost()->GetID());
+    net::URLRequestContext* request_context =
+        browser_context_->GetResourceContext()->GetRequestContext();
+    job_factory_.reset(new TestDownloadURLRequestFactory());
+    request_context->set_job_factory(job_factory_.get());
+    request_context->set_network_delegate(&network_delegate_);
+    HandleScheme("test");
+  }
+
+  void TearDown() override {
+    web_contents_observer_.reset();
+    web_contents_.reset();
+
+    for (std::set<int>::iterator it = child_ids_.begin();
+         it != child_ids_.end(); ++it) {
+      host_.CancelRequestsForProcess(*it);
+    }
+
+    host_.Shutdown();
+
+    ChildProcessSecurityPolicyImpl::GetInstance()->Remove(0);
+
+    // Flush the message loop to make application verifiers happy.
+    if (ResourceDispatcherHostImpl::Get()) {
+      ResourceDispatcherHostImpl::Get()->CancelRequestsForContext(
+          browser_context_->GetResourceContext());
+    }
+
+    browser_context_.reset();
+    base::RunLoop().RunUntilIdle();
+  }
+
+  // Intercepts requests for the given protocol.
+  void HandleScheme(const std::string& scheme) {
+    job_factory_->HandleScheme(scheme);
+    ChildProcessSecurityPolicyImpl* policy =
+        ChildProcessSecurityPolicyImpl::GetInstance();
+    if (!policy->IsWebSafeScheme(scheme))
+      policy->RegisterWebSafeScheme(scheme);
+  }
+
+  void InitiateDownloadRequest(int request_id, const GURL& url) {
+    net::URLRequestContext* request_context =
+        browser_context_->GetResourceContext()->GetRequestContext();
+    std::unique_ptr<net::URLRequest> request(
+        request_context->CreateRequest(url, net::DEFAULT_PRIORITY, NULL));
+
+    DownloadManagerImpl::BeginDownloadRequest(
+        std::move(request), Referrer(), browser_context_->GetResourceContext(),
+        false,  // is_content_initiated
+        web_contents_->GetRenderProcessHost()->GetID(),
+        web_contents_->GetRoutingID(),
+        web_contents_->GetMainFrame()->GetRoutingID(), false);
+  }
+
+ protected:
+  // The thread_bundle_ needs to be the first in the list of members to
+  // ensure that the BrowserThread globals are initialized.
+  content::TestBrowserThreadBundle thread_bundle_;
+  ResourceDispatcherHostImpl host_;
+  std::unique_ptr<TestBrowserContext> browser_context_;
+  std::unique_ptr<TestDownloadURLRequestFactory> job_factory_;
+  net::TestNetworkDelegate network_delegate_;
+  std::unique_ptr<WebContents> web_contents_;
+  std::unique_ptr<TestDownloadWebContentsObserver> web_contents_observer_;
+  // List of child ids granted access via policy.
+  std::set<int> child_ids_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadRequestTest);
+};
+
+// Confirm that resource response started notifications for downloads are not
+// transmitted to the WebContents.
+TEST_F(DownloadRequestTest, TransferResponseStartedDownload) {
+  int initial_count(web_contents_observer_->resource_response_start_count());
+
+  InitiateDownloadRequest(1, net::URLRequestTestJob::test_url_1());
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(initial_count,
+            web_contents_observer_->resource_response_start_count());
+}
+
+// Confirm that request redirected notifications for downloads are not
+// transmitted to the WebContents.
+TEST_F(DownloadRequestTest, TransferRequestRedirectedDownload) {
+  int initial_count(web_contents_observer_->resource_request_redirect_count());
+
+  InitiateDownloadRequest(1,
+                          net::URLRequestTestJob::test_url_redirect_to_url_2());
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(initial_count,
+            web_contents_observer_->resource_request_redirect_count());
+}
+

[Randy Smith (Not in Mondays), 2016/08/25 20:58:50]

Wow, I'm sorry, I guess I'm doing a poor job of expressing what I'm looking for.
 

The invariant I feel strongly about that I'd like this CL to keep is that code
in x.cc is tested in x_unittest.cc (or browsertest or ...), rather than some
other class' test file.  I had thought based on what you said earlier in the
thread that there were unit tests for the functionality in BeginDownload.  But
I'm now guessing that we have no unittests for BeginDownload (i.e. answering the
question "Did we correctly setup a URLRequest based on the arguments to
BeginDownload()"?), which makes me sad but isn't relevant to this CL.  These
tests  aren't testing code in BeginDownload; they are testing functionality
that's still in ResourceDispatcherHostImpl, specifically at
https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...
and
https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch....
 So they should be in resource_dispatcher_host_unittest.cc.

In a perfect world, those tests would/should probably be written in terms of
BeginUrlRequest rather than going through "helper functions" in other
compilation units, but that's the kind of thing I don't feel that strongly
about.  But I do want to keep tests for code grouped with that code in the
corresponding ...tests.cc, and to my eye these tests are testing code in RDHI.

[ananta, 2016/08/25 22:20:50]

Based on our discussion, keeping the test in
resource_dispatcher_host_unittest.cc

 }  // namespace content