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

net/proxy/mojo_proxy_resolver_impl_unittest.cc

Index: net/proxy/mojo_proxy_resolver_impl_unittest.cc
diff --git a/net/proxy/mojo_proxy_resolver_impl_unittest.cc b/net/proxy/mojo_proxy_resolver_impl_unittest.cc
index b045b27b3008507f88f72db220ed8d34bbcd045d..3e1e3b204e0a8a3c5e06b6e70f2b1615adb55f23 100644
--- a/net/proxy/mojo_proxy_resolver_impl_unittest.cc
+++ b/net/proxy/mojo_proxy_resolver_impl_unittest.cc
@@ -102,68 +102,74 @@ void TestRequestClient::OnConnectionError() {
 
 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;

[eroman, 2016/02/24 19:20:29]

This does change the meaning from before:

Previously cancelled was only set when the request was actually cancelled,
whereas now it is set every time the RequestImpl is destroyed.

This also means that the "cancel_callback_" is run always, even when the request
was not truly cancelled.

To address this, I suggest the following changes:

  (1) Rename Job::callback --> Job::callback_, and make it private (to avoid
code calling it directly).
  (2) Instead of test code calling job->callback.Run(XXX), replace those with
job->Complete(XXX), where the new method is resets the callback after running
it:

   void Job::Complete(int result) {
      DCHECK(!callback_.is_null())
      callback_.Run(result);
      callback_.Reset();
    }

This allows distinguishing between requests that were completed vs those that
did not:

   bool Job::WasCompleted() {
     return callback_.is_null();
   }

Now this code in ~RequestImpl() can distinguish between an actual cancellation,
versus destruction of an already completed request:

   if (job_->WasCompleted())
     return;
   
   job_->cancelled = true;
   if (!resolver_->cancel_callback_.is_null()) {
      ...
   }

+      if (!resolver_->cancel_callback_.is_null()) {
+        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.

[eroman, 2016/02/24 19:20:29]

This comment is no longer up to date.

   void WaitForCancel();
 
-  const std::vector<Request>& pending_requests() { return pending_requests_; }
+  const std::vector<scoped_ptr<Job>>& pending_jobs() { return pending_jobs_; }
 
  private:
   base::Closure cancel_callback_;
-  std::vector<Request> pending_requests_;
+  std::vector<scoped_ptr<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(make_scoped_ptr(new Job()));
+  auto pending_job = pending_jobs_.back().get();
+  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 scoped_ptr<Job>& job) {
+                        return job->cancelled;
+                      }) != pending_jobs_.end()) {
     base::RunLoop run_loop;
     cancel_callback_ = run_loop.QuitClosure();
     run_loop.Run();
@@ -193,19 +199,19 @@ TEST_F(MojoProxyResolverImplTest, GetProxyForUrl) {
   TestRequestClient client(mojo::GetProxy(&client_ptr));
 
   resolver_->GetProxyForUrl("http://example.com", std::move(client_ptr));
-  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].get();
+  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());
@@ -240,11 +246,11 @@ TEST_F(MojoProxyResolverImplTest, GetProxyForUrlFailure) {
   TestRequestClient client(mojo::GetProxy(&client_ptr));
 
   resolver_->GetProxyForUrl("http://example.com", std::move(client_ptr));
-  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].get();
+  EXPECT_EQ(GURL("http://example.com"), job->url);
+  job->callback.Run(ERR_FAILED);
   client.WaitForResult();
 
   EXPECT_EQ(ERR_FAILED, client.error());
@@ -261,17 +267,17 @@ TEST_F(MojoProxyResolverImplTest, GetProxyForUrlMultiple) {
 
   resolver_->GetProxyForUrl("http://example.com", std::move(client_ptr1));
   resolver_->GetProxyForUrl("https://example.com", std::move(client_ptr2));
-  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].get();
+  EXPECT_EQ(GURL("http://example.com"), job1->url);
+  const MockProxyResolverV8Tracing::Job* job2 =
+      mock_proxy_resolver_->pending_jobs()[1].get();
+  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();
 
@@ -300,11 +306,11 @@ TEST_F(MojoProxyResolverImplTest, DestroyClient) {
       new TestRequestClient(mojo::GetProxy(&client_ptr)));
 
   resolver_->GetProxyForUrl("http://example.com", std::move(client_ptr));
-  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].get();
+  EXPECT_EQ(GURL("http://example.com"), job->url);
+  job->results->UsePacString("PROXY proxy.example.com:8080");
   client.reset();
   mock_proxy_resolver_->WaitForCancel();
 }
@@ -314,7 +320,7 @@ TEST_F(MojoProxyResolverImplTest, DestroyService) {
   TestRequestClient client(mojo::GetProxy(&client_ptr));
 
   resolver_->GetProxyForUrl("http://example.com", std::move(client_ptr));
-  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);
 }