Change ProxyResolver::GetProxyForURL() to take a scoped_ptr<Request>* rather than a RequestHandle*

net/proxy/mojo_proxy_resolver_impl_unittest.cc

 // Copyright 2015 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 "net/proxy/mojo_proxy_resolver_impl.h"
 
 #include <string>
 #include <vector>
 
 #include "base/run_loop.h"
@@ skipping 83 matching lines @@
     interfaces::HostResolverRequestInfoPtr request_info,
     interfaces::HostResolverRequestClientPtr client) {
 }
 
 void TestRequestClient::OnConnectionError() {
   event_waiter_.NotifyEvent(CONNECTION_ERROR);
 }
 
 class MockProxyResolverV8Tracing : public ProxyResolverV8Tracing {
  public:
-  struct Request {
+  struct Job {
     GURL url;
     ProxyInfo* results;
     CompletionCallback callback;
     bool cancelled = false;
   };
+
+  class RequestImpl : public ProxyResolver::Request {
+   public:
+    RequestImpl(Job* job, MockProxyResolverV8Tracing* resolver)
+        : job_(job), resolver_(resolver) {}
+
+    ~RequestImpl() override {
+      job_->cancelled = true;
+      if (!resolver_->cancel_callback_.is_null()) {

[eroman, 2015/11/24 01:20:59]

I don't believe this is the right trigger anymore. RequestImpl will always reach
the destructor, but this only counts as "cancellation" if the request has not
yet completed.

If the request was already completed, should not call the cancellation callback.

+        resolver_->cancel_callback_.Run();
+        resolver_->cancel_callback_.Reset();
+      }
+    }
+
+    LoadState GetLoadState() override {
+      return LOAD_STATE_RESOLVING_PROXY_FOR_URL;
+    }
+
+   private:
+    Job* job_;
+    MockProxyResolverV8Tracing* resolver_;
+  };
+
   MockProxyResolverV8Tracing() {}
 
   // ProxyResolverV8Tracing overrides.
   void GetProxyForURL(const GURL& url,
                       ProxyInfo* results,
                       const CompletionCallback& callback,
-                      ProxyResolver::RequestHandle* request,
+                      scoped_ptr<ProxyResolver::Request>* request,
                       scoped_ptr<Bindings> bindings) override;
-  void CancelRequest(ProxyResolver::RequestHandle request_handle) override;
-  LoadState GetLoadState(ProxyResolver::RequestHandle request) const override;
 
   // Wait until the mock resolver has received a CancelRequest call.
   void WaitForCancel();
 
-  const std::vector<Request>& pending_requests() { return pending_requests_; }
+  const std::vector<Job>& pending_jobs() { return pending_jobs_; }
 
  private:
   base::Closure cancel_callback_;
-  std::vector<Request> pending_requests_;
+  std::vector<Job> pending_jobs_;
 };
 
 void MockProxyResolverV8Tracing::GetProxyForURL(
     const GURL& url,
     ProxyInfo* results,
     const CompletionCallback& callback,
-    ProxyResolver::RequestHandle* request,
+    scoped_ptr<ProxyResolver::Request>* request,
     scoped_ptr<Bindings> bindings) {
-  pending_requests_.push_back(Request());
-  auto& pending_request = pending_requests_.back();
-  pending_request.url = url;
-  pending_request.results = results;
-  pending_request.callback = callback;
-  *request =
-      reinterpret_cast<ProxyResolver::RequestHandle>(pending_requests_.size());
+  pending_jobs_.push_back(Job());
+  auto& pending_job = pending_jobs_.back();
+  pending_job.url = url;
+  pending_job.results = results;
+  pending_job.callback = callback;
+  request->reset(new RequestImpl(&pending_job, this));
 }
 
-void MockProxyResolverV8Tracing::CancelRequest(
-    ProxyResolver::RequestHandle request_handle) {
-  size_t id = reinterpret_cast<size_t>(request_handle) - 1;
-  pending_requests_[id].cancelled = true;
-  if (!cancel_callback_.is_null()) {
-    cancel_callback_.Run();
-    cancel_callback_.Reset();
-  }
-}
-
-LoadState MockProxyResolverV8Tracing::GetLoadState(
-    ProxyResolver::RequestHandle request) const {
-  return LOAD_STATE_RESOLVING_PROXY_FOR_URL;
-}
 
 void MockProxyResolverV8Tracing::WaitForCancel() {
-  while (std::find_if(pending_requests_.begin(), pending_requests_.end(),
-                      [](const Request& request) {
-                        return request.cancelled;
-                      }) != pending_requests_.end()) {
+  while (std::find_if(pending_jobs_.begin(), pending_jobs_.end(),
+                      [](const Job& job) { return job.cancelled; }) !=
+         pending_jobs_.end()) {
     base::RunLoop run_loop;
     cancel_callback_ = run_loop.QuitClosure();
     run_loop.Run();
   }
 }
 
 }  // namespace
 
 class MojoProxyResolverImplTest : public testing::Test {
  protected:
   void SetUp() override {
     scoped_ptr<MockProxyResolverV8Tracing> mock_resolver(
         new MockProxyResolverV8Tracing);
     mock_proxy_resolver_ = mock_resolver.get();
     resolver_impl_.reset(new MojoProxyResolverImpl(mock_resolver.Pass()));
     resolver_ = resolver_impl_.get();
   }
 
   MockProxyResolverV8Tracing* mock_proxy_resolver_;
 
   scoped_ptr<MojoProxyResolverImpl> resolver_impl_;
   interfaces::ProxyResolver* resolver_;
 };
 
 TEST_F(MojoProxyResolverImplTest, GetProxyForUrl) {
   interfaces::ProxyResolverRequestClientPtr client_ptr;
   TestRequestClient client(mojo::GetProxy(&client_ptr));
 
   resolver_->GetProxyForUrl("http://example.com", client_ptr.Pass());
-  ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size());
-  const MockProxyResolverV8Tracing::Request& request =
-      mock_proxy_resolver_->pending_requests()[0];
-  EXPECT_EQ(GURL("http://example.com"), request.url);
+  ASSERT_EQ(1u, mock_proxy_resolver_->pending_jobs().size());
+  const MockProxyResolverV8Tracing::Job& job =
+      mock_proxy_resolver_->pending_jobs()[0];
+  EXPECT_EQ(GURL("http://example.com"), job.url);
 
-  request.results->UsePacString(
+  job.results->UsePacString(
       "PROXY proxy.example.com:1; "
       "SOCKS4 socks4.example.com:2; "
       "SOCKS5 socks5.example.com:3; "
       "HTTPS https.example.com:4; "
       "QUIC quic.example.com:65000; "
       "DIRECT");
-  request.callback.Run(OK);
+  job.callback.Run(OK);
   client.WaitForResult();
 
   EXPECT_EQ(OK, client.error());
   std::vector<ProxyServer> servers =
       client.results().To<std::vector<ProxyServer>>();
   ASSERT_EQ(6u, servers.size());
   EXPECT_EQ(ProxyServer::SCHEME_HTTP, servers[0].scheme());
   EXPECT_EQ("proxy.example.com", servers[0].host_port_pair().host());
   EXPECT_EQ(1, servers[0].host_port_pair().port());
 
@@ skipping 14 matching lines @@
   EXPECT_EQ(65000, servers[4].host_port_pair().port());
 
   EXPECT_EQ(ProxyServer::SCHEME_DIRECT, servers[5].scheme());
 }
 
 TEST_F(MojoProxyResolverImplTest, GetProxyForUrlFailure) {
   interfaces::ProxyResolverRequestClientPtr client_ptr;
   TestRequestClient client(mojo::GetProxy(&client_ptr));
 
   resolver_->GetProxyForUrl("http://example.com", client_ptr.Pass());
-  ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size());
-  const MockProxyResolverV8Tracing::Request& request =
-      mock_proxy_resolver_->pending_requests()[0];
-  EXPECT_EQ(GURL("http://example.com"), request.url);
-  request.callback.Run(ERR_FAILED);
+  ASSERT_EQ(1u, mock_proxy_resolver_->pending_jobs().size());
+  const MockProxyResolverV8Tracing::Job& job =
+      mock_proxy_resolver_->pending_jobs()[0];
+  EXPECT_EQ(GURL("http://example.com"), job.url);
+  job.callback.Run(ERR_FAILED);
   client.WaitForResult();
 
   EXPECT_EQ(ERR_FAILED, client.error());
   std::vector<ProxyServer> proxy_servers =
       client.results().To<std::vector<ProxyServer>>();
   EXPECT_TRUE(proxy_servers.empty());
 }
 
 TEST_F(MojoProxyResolverImplTest, GetProxyForUrlMultiple) {
   interfaces::ProxyResolverRequestClientPtr client_ptr1;
   TestRequestClient client1(mojo::GetProxy(&client_ptr1));
   interfaces::ProxyResolverRequestClientPtr client_ptr2;
   TestRequestClient client2(mojo::GetProxy(&client_ptr2));
 
   resolver_->GetProxyForUrl("http://example.com", client_ptr1.Pass());
   resolver_->GetProxyForUrl("https://example.com", client_ptr2.Pass());
-  ASSERT_EQ(2u, mock_proxy_resolver_->pending_requests().size());
-  const MockProxyResolverV8Tracing::Request& request1 =
-      mock_proxy_resolver_->pending_requests()[0];
-  EXPECT_EQ(GURL("http://example.com"), request1.url);
-  const MockProxyResolverV8Tracing::Request& request2 =
-      mock_proxy_resolver_->pending_requests()[1];
-  EXPECT_EQ(GURL("https://example.com"), request2.url);
-  request1.results->UsePacString("HTTPS proxy.example.com:12345");
-  request1.callback.Run(OK);
-  request2.results->UsePacString("SOCKS5 another-proxy.example.com:6789");
-  request2.callback.Run(OK);
+  ASSERT_EQ(2u, mock_proxy_resolver_->pending_jobs().size());
+  const MockProxyResolverV8Tracing::Job& job1 =
+      mock_proxy_resolver_->pending_jobs()[0];
+  EXPECT_EQ(GURL("http://example.com"), job1.url);
+  const MockProxyResolverV8Tracing::Job& job2 =
+      mock_proxy_resolver_->pending_jobs()[1];
+  EXPECT_EQ(GURL("https://example.com"), job2.url);
+  job1.results->UsePacString("HTTPS proxy.example.com:12345");
+  job1.callback.Run(OK);
+  job2.results->UsePacString("SOCKS5 another-proxy.example.com:6789");
+  job2.callback.Run(OK);
   client1.WaitForResult();
   client2.WaitForResult();
 
   EXPECT_EQ(OK, client1.error());
   std::vector<ProxyServer> proxy_servers1 =
       client1.results().To<std::vector<ProxyServer>>();
   ASSERT_EQ(1u, proxy_servers1.size());
   ProxyServer& server1 = proxy_servers1[0];
   EXPECT_EQ(ProxyServer::SCHEME_HTTPS, server1.scheme());
   EXPECT_EQ("proxy.example.com", server1.host_port_pair().host());
   EXPECT_EQ(12345, server1.host_port_pair().port());
 
   EXPECT_EQ(OK, client2.error());
   std::vector<ProxyServer> proxy_servers2 =
       client2.results().To<std::vector<ProxyServer>>();
   ASSERT_EQ(1u, proxy_servers1.size());
   ProxyServer& server2 = proxy_servers2[0];
   EXPECT_EQ(ProxyServer::SCHEME_SOCKS5, server2.scheme());
   EXPECT_EQ("another-proxy.example.com", server2.host_port_pair().host());
   EXPECT_EQ(6789, server2.host_port_pair().port());
 }
 
 TEST_F(MojoProxyResolverImplTest, DestroyClient) {
   interfaces::ProxyResolverRequestClientPtr client_ptr;
   scoped_ptr<TestRequestClient> client(
       new TestRequestClient(mojo::GetProxy(&client_ptr)));
 
   resolver_->GetProxyForUrl("http://example.com", client_ptr.Pass());
-  ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size());
-  const MockProxyResolverV8Tracing::Request& request =
-      mock_proxy_resolver_->pending_requests()[0];
-  EXPECT_EQ(GURL("http://example.com"), request.url);
-  request.results->UsePacString("PROXY proxy.example.com:8080");
+  ASSERT_EQ(1u, mock_proxy_resolver_->pending_jobs().size());
+  const MockProxyResolverV8Tracing::Job& job =
+      mock_proxy_resolver_->pending_jobs()[0];
+  EXPECT_EQ(GURL("http://example.com"), job.url);
+  job.results->UsePacString("PROXY proxy.example.com:8080");
   client.reset();
   mock_proxy_resolver_->WaitForCancel();
 }
 
 TEST_F(MojoProxyResolverImplTest, DestroyService) {
   interfaces::ProxyResolverRequestClientPtr client_ptr;
   TestRequestClient client(mojo::GetProxy(&client_ptr));
 
   resolver_->GetProxyForUrl("http://example.com", client_ptr.Pass());
-  ASSERT_EQ(1u, mock_proxy_resolver_->pending_requests().size());
+  ASSERT_EQ(1u, mock_proxy_resolver_->pending_jobs().size());
   resolver_impl_.reset();
   client.event_waiter().WaitForEvent(TestRequestClient::CONNECTION_ERROR);
 }
 
 }  // namespace net