Use mojo for Chrome Loading, Part 1

content/browser/loader/url_loader_factory_impl_unittest.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/loader/url_loader_factory_impl.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/browser/loader/resource_message_filter.h"
+#include "content/common/url_loader.mojom.h"
+#include "content/common/url_loader_factory.mojom.h"
+#include "content/public/browser/resource_context.h"
+#include "content/public/test/test_browser_context.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "net/base/net_errors.h"
+#include "net/test/url_request/url_request_failed_job.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_test_job.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace content {
+
+namespace {
+
+class URLLoaderClientImpl final : public mojom::URLLoaderClient {

[mmenke, 2016/05/26 16:28:29]

I'd suggest giving this at least a marginally more distinct name, like
TestURLLoaderClient.

[yhirano, 2016/06/01 14:40:52]

Done.

+ public:
+  URLLoaderClientImpl() : binding_(this) {}
+  void OnReceiveResponse(const ResourceResponseHead& response_head) override {
+    has_received_response_ = true;
+    response_head_ = response_head;
+  }
+  void OnStartLoadingResponseBody(
+      mojo::ScopedDataPipeConsumerHandle body) override {
+    response_body_ = std::move(body);
+  }
+  void OnComplete(const ResourceRequestCompletionStatus& status) override {
+    has_received_completion_ = true;
+    completion_status_ = status;
+  }
+
+  bool has_received_response() const { return has_received_response_; }
+  bool has_received_completion() const { return has_received_completion_; }
+  const ResourceResponseHead& response_head() const { return response_head_; }
+  mojo::Handle response_body() { return response_body_.get(); }
+  const ResourceRequestCompletionStatus& completion_status() const {
+    return completion_status_;
+  }
+
+  mojom::URLLoaderClientPtr CreateInterfacePtrAndBind() {
+    return binding_.CreateInterfacePtrAndBind();
+  }
+
+ private:
+  mojo::Binding<mojom::URLLoaderClient> binding_;
+  ResourceResponseHead response_head_;
+  mojo::ScopedDataPipeConsumerHandle response_body_;
+  ResourceRequestCompletionStatus completion_status_;
+  bool has_received_response_ = false;
+  bool has_received_completion_ = false;

[mmenke, 2016/05/26 16:28:29]

DISALLOW_COPY_AND_ASSIGN

[yhirano, 2016/06/01 14:40:52]

Done.

+};
+
+class TestURLRequestJobFactory : public net::URLRequestJobFactory {
+ public:
+  using Callback = base::Callback<net::URLRequestJob*(
+      const std::string& scheme,
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate)>;
+
+  TestURLRequestJobFactory() : callback_(base::Bind(&ConstNull)) {}
+  void SetCallback(Callback callback) { callback_ = callback; }

[mmenke, 2016/05/26 16:28:29]

nit:  const Callback&

Note that I made two suggestions about alternatives to this class further down,
so may want to read through all of my comments before responding to my ones up
here.

[yhirano, 2016/06/01 14:40:52]

Deleted

+
+  net::URLRequestJob* MaybeCreateJobWithProtocolHandler(
+      const std::string& scheme,
+      net::URLRequest* request,
+      net::NetworkDelegate* delegate) const override {
+    return callback_.Run(scheme, request, delegate);
+  }
+
+  net::URLRequestJob* MaybeInterceptRedirect(
+      net::URLRequest* request,
+      net::NetworkDelegate* delegate,
+      const GURL& location) const override {
+    return nullptr;
+  }
+
+  net::URLRequestJob* MaybeInterceptResponse(
+      net::URLRequest* request,
+      net::NetworkDelegate* delegate) const override {
+    return nullptr;
+  }
+
+  bool IsHandledProtocol(const std::string& scheme) const override {
+    return true;
+  }
+
+  bool IsHandledURL(const GURL& url) const override { return true; }
+
+  bool IsSafeRedirectTarget(const GURL& location) const override {
+    return false;
+  }
+
+ private:
+  static net::URLRequestJob* ConstNull(const std::string& scheme,
+                                       net::URLRequest* request,
+                                       net::NetworkDelegate* delegate) {
+    return nullptr;
+  }

[mmenke, 2016/05/26 16:28:29]

Suggest just checking if the callback_ is_null (is_empty?) in
MaybeCreateJobWithProtocolHandler instead of using this.  Think it makes the
method clearer.  And it saves a few lines of code!

[yhirano, 2016/06/01 14:40:52]

Deleted

+
+  Callback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestURLRequestJobFactory);
+};
+
+class URLLoaderFactoryImplTest : public ::testing::Test {
+ public:
+  URLLoaderFactoryImplTest()
+      : thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP),
+        browser_context_(new TestBrowserContext()),
+        resource_message_filter_(new ResourceMessageFilter(
+            0,
+            0,
+            nullptr,
+            nullptr,
+            nullptr,
+            nullptr,
+            nullptr,
+            base::Bind(&URLLoaderFactoryImplTest::GetContexts,
+                       base::Unretained(this)))) {
+    net::URLRequestContext* request_context =
+        browser_context_->GetResourceContext()->GetRequestContext();
+    job_factory_.reset(new TestURLRequestJobFactory());
+    request_context->set_job_factory(job_factory_.get());

[mmenke, 2016/05/26 16:28:29]

Modifying URLRequestContexts you didn't just create yourself is generally not a
good idea.  Other classes internal to the context may have pointers to the old
NetworkDelegate, for instance.

Suggest just using URLRequestFilter.

[yhirano, 2016/06/01 14:40:52]

Done.

+    request_context->set_network_delegate(&network_delegate_);

[mmenke, 2016/05/26 16:28:29]

Why do you need to change the NetworkDelegate?

[yhirano, 2016/06/01 14:40:51]

Deleted

+
+    factory_impl_retainer_.reset(new URLLoaderFactoryImpl(
+        resource_message_filter_, mojo::GetProxy(&factory_)));
+    base::RunLoop().RunUntilIdle();
+  }
+
+  ~URLLoaderFactoryImplTest() override {
+    rdh_.CancelRequestsForProcess(resource_message_filter_->child_id());
+    base::RunLoop().RunUntilIdle();
+  }
+
+  void GetContexts(ResourceType resource_type,
+                   int origin_pid,
+                   ResourceContext** resource_context,
+                   net::URLRequestContext** request_context) {
+    *resource_context = browser_context_->GetResourceContext();
+    *request_context =
+        browser_context_->GetResourceContext()->GetRequestContext();
+  }
+
+  net::URLRequestJob* RecordURLRequest(const std::string& scheme,
+                                       net::URLRequest* request,
+                                       net::NetworkDelegate* delegate) {
+    url_request_job_factory_url_ = request->url();
+    url_request_job_factory_method_ = request->method();

[mmenke, 2016/05/26 16:28:29]

Any reason to not just record these on every request in the Factory, and call
this method "HangingURLRequest" - it's the hanging that seems to make the
unique.

[mmenke, 2016/05/26 16:28:29]

Also, perhaps count URLRequests, to see if we see any unexpected ones?

[yhirano, 2016/06/01 14:40:52]

Deleted. Updating variables in const functions is complicated and I don't think
it's worth doing any more.

+    return new net::URLRequestFailedJob(request, delegate, net::ERR_IO_PENDING);
+  }
+
+  static net::URLRequestJob* CreateSuccessfulJob(
+      const std::string& scheme,
+      net::URLRequest* request,
+      net::NetworkDelegate* delegate) {
+    return new net::URLRequestTestJob(request, delegate,
+                                      net::URLRequestTestJob::test_headers(),
+                                      "hello", true);

[mmenke, 2016/05/26 16:28:29]

I'd recommend against using URLRequestTestJob - the process one pending message
thing is weird and bad - I'd really like to get rid of the class, actually. 
Instead, I'd just make a file on disk (Or use an already existing one) and use
URLRequestMockHTTPJob.

You could even use the EmbeddedTestServer, if you want.  You could even give it
callbacks instead of the URLRequestJobFactory, or the URLRequestFilter (Which I
suggested above).

[yhirano, 2016/06/01 14:40:52]

Done.

+  }
+
+  static net::URLRequestJob* CreateFailedJob(const std::string& scheme,
+                                             net::URLRequest* request,
+                                             net::NetworkDelegate* delegate) {
+    return new net::URLRequestFailedJob(request, delegate, net::ERR_FAILED);
+  }
+
+  TestBrowserThreadBundle thread_bundle_;
+  ResourceDispatcherHostImpl rdh_;
+  std::unique_ptr<TestBrowserContext> browser_context_;
+  std::unique_ptr<TestURLRequestJobFactory> job_factory_;
+  net::TestNetworkDelegate network_delegate_;
+  scoped_refptr<ResourceMessageFilter> resource_message_filter_;
+
+  mojom::URLLoaderFactoryPtr factory_;
+  std::unique_ptr<URLLoaderFactoryImpl> factory_impl_retainer_;
+
+  GURL url_request_job_factory_url_;
+  std::string url_request_job_factory_method_;
+};
+
+TEST_F(URLLoaderFactoryImplTest, Request) {
+  mojom::URLLoaderPtr loader;
+  factory_->CreateURLLoader(mojo::GetProxy(&loader));
+
+  job_factory_->SetCallback(base::Bind(
+      &URLLoaderFactoryImplTest::RecordURLRequest, base::Unretained(this)));
+
+  ResourceRequest request;
+  URLLoaderClientImpl client;
+  request.url = GURL("http://www.example.com/foo/bar");
+  request.method = "GET";
+  loader->Start(1, request, client.CreateInterfacePtrAndBind());
+
+  base::RunLoop().RunUntilIdle();
+  while (net::URLRequestTestJob::ProcessOnePendingMessage()) {
+  }
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_FALSE(client.has_received_response());
+  EXPECT_FALSE(client.has_received_completion());
+  EXPECT_FALSE(client.response_body().is_valid());
+  EXPECT_EQ(request.url, url_request_job_factory_url_);
+  EXPECT_EQ(request.method, url_request_job_factory_method_);
+}
+
+TEST_F(URLLoaderFactoryImplTest, GetResponse) {
+  mojom::URLLoaderPtr loader;
+  factory_->CreateURLLoader(mojo::GetProxy(&loader));
+
+  job_factory_->SetCallback(base::Bind(CreateSuccessfulJob));
+
+  ResourceRequest request;
+  URLLoaderClientImpl client;
+  request.url = GURL("http://www.example.com/");
+  request.method = "GET";
+  loader->Start(1, request, client.CreateInterfacePtrAndBind());
+
+  EXPECT_FALSE(client.has_received_response());
+  EXPECT_FALSE(client.has_received_completion());
+  base::RunLoop().RunUntilIdle();
+  while (net::URLRequestTestJob::ProcessOnePendingMessage()) {
+  }
+  base::RunLoop().RunUntilIdle();

[mmenke, 2016/05/26 16:28:29]

All this spinning of the RunLoop is a bit of an anti-pattern.  Better to use a
RunLoop, and call a quit closure when you get the completion message.

[yhirano, 2016/06/01 14:40:52]

Done.

+
+  ASSERT_TRUE(client.has_received_response());
+  EXPECT_EQ(200, client.response_head().headers->response_code());
+  std::string content_type;
+  client.response_head().headers->GetNormalizedHeader("content-type",
+                                                      &content_type);
+  EXPECT_EQ("text/html", content_type);
+
+  ASSERT_TRUE(client.has_received_completion());
+  EXPECT_TRUE(client.response_body().is_valid());

[mmenke, 2016/05/26 16:28:29]

Shouldn't we actually read the response body in one of these?

[yhirano, 2016/06/01 14:40:52]

Done.

+  EXPECT_EQ(0, client.completion_status().error_code);
+}
+
+TEST_F(URLLoaderFactoryImplTest, GetFailedResponse) {

[mmenke, 2016/05/26 16:28:29]

Other test suggestions:
* One that goes through AbortRequestBeforeItStarts (We should have tests for all
those paths, but I assume we have tests for them at another level, that we'll
get when switching to mojo completely here).
* One that runs into an error only after reading the headers (Know one of the
mocks in net can do this).
* One that runs into a failure only after reading some of the body (Not sure if
we have a mock that can do this, though I'd guess we do).

[yhirano, 2016/06/01 14:40:52]

Done.

+  mojom::URLLoaderPtr loader;
+  factory_->CreateURLLoader(mojo::GetProxy(&loader));
+
+  job_factory_->SetCallback(base::Bind(CreateFailedJob));
+
+  ResourceRequest request;
+  URLLoaderClientImpl client;
+  request.url = GURL("http://www.example.com/");
+  request.method = "GET";
+  loader->Start(1, request, client.CreateInterfacePtrAndBind());
+
+  EXPECT_FALSE(client.has_received_response());
+  EXPECT_FALSE(client.has_received_completion());
+  base::RunLoop().RunUntilIdle();
+  while (net::URLRequestTestJob::ProcessOnePendingMessage()) {
+  }
+  base::RunLoop().RunUntilIdle();
+
+  ASSERT_FALSE(client.has_received_response());
+  ASSERT_TRUE(client.has_received_completion());
+  EXPECT_FALSE(client.response_body().is_valid());
+  EXPECT_EQ(net::ERR_FAILED, client.completion_status().error_code);
+}
+
+}  // namespace
+
+}  // namespace content