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

net/proxy/proxy_resolver_v8_tracing.cc

 // Copyright (c) 2013 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/proxy_resolver_v8_tracing.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 293 matching lines @@
   ProxyResolverV8TracingImpl(scoped_ptr<base::Thread> thread,
                              scoped_ptr<ProxyResolverV8> resolver,
                              scoped_ptr<Job::Params> job_params);
 
   ~ProxyResolverV8TracingImpl() override;
 
   // 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) override;
-  LoadState GetLoadState(ProxyResolver::RequestHandle request) const override;
+
+  class RequestImpl : public ProxyResolverV8Tracing::Request {
+   public:
+    RequestImpl(scoped_refptr<Job> job);

[eroman, 2016/02/24 03:08:07]

explicit

+    ~RequestImpl() override;
+    LoadState GetLoadState() override;
+
+   private:
+    scoped_refptr<Job> job_;
+  };
 
  private:
   // The worker thread on which the ProxyResolverV8 will be run.
   scoped_ptr<base::Thread> thread_;
   scoped_ptr<ProxyResolverV8> v8_resolver_;
 
   scoped_ptr<Job::Params> job_params_;
 
   // The number of outstanding (non-cancelled) jobs.
   int num_outstanding_callbacks_;
@@ skipping 46 matching lines @@
   // (a) The job has been posted to the worker thread, however script execution
   //     has not yet started.
   // (b) The script is executing on the worker thread.
   // (c) The script is executing on the worker thread, however is blocked inside
   //     of dnsResolve() waiting for a response from the origin thread.
   // (d) Nothing is running on the worker thread, however the host resolver has
   //     a pending DNS request which upon completion will restart the script
   //     execution.
   // (e) The worker thread has a pending task to restart execution, which was
   //     posted after the DNS dependency was resolved and saved to local cache.
   // (f) The script execution completed entirely, and posted a task to the

[eroman, 2016/02/24 03:08:07]

There is another possibility - that the job already completed successfully. (see
later comment for suggestion).

   //     origin thread to notify the caller.
   //
   // |cancelled_| is read on both the origin thread and worker thread. The
   // code that runs on the worker thread is littered with checks on
   // |cancelled_| to break out early.
   cancelled_.Set();
 
   ReleaseCallback();
 
   if (pending_dns_) {
@@ skipping 32 matching lines @@
 }
 
 void Job::SetCallback(const CompletionCallback& callback) {
   CheckIsOnOriginThread();
   DCHECK(callback_.is_null());
   (*params_->num_outstanding_callbacks)++;
   callback_ = callback;
 }
 
 void Job::ReleaseCallback() {
+  if (callback_.is_null())

[eroman, 2016/02/24 03:08:07]

I don't think this change should go here.

(I presume this was added because Job::Cancel() might be called on a request
that has already completed, in which case it should not be completed a second
time.)

Instead I think we should do the check in Job::Cancel(), since cancellation is
already fairly complicated.

In Job::Cancel() at the top we can do:

  // If the job already completed, there is nothing to be cancelled.
  if (callback_.is_null())
    return;

And then remove this change to ReleaseCallback()

+    return;
   CheckIsOnOriginThread();
   DCHECK(!callback_.is_null());
   CHECK_GT(*params_->num_outstanding_callbacks, 0);
   (*params_->num_outstanding_callbacks)--;
   callback_.Reset();
 
   // For good measure, clear this other user-owned pointer.
   user_results_ = NULL;
 }
 
@@ skipping 480 matching lines @@
 
 ProxyResolverV8TracingImpl::~ProxyResolverV8TracingImpl() {
   // Note, all requests should have been cancelled.
   CHECK_EQ(0, num_outstanding_callbacks_);
 
   // Join the worker thread. See http://crbug.com/69710.
   base::ThreadRestrictions::ScopedAllowIO allow_io;
   thread_.reset();
 }
 
+ProxyResolverV8TracingImpl::RequestImpl::RequestImpl(scoped_refptr<Job> job)
+    : job_(job) {}

[eroman, 2016/02/24 03:08:07]

std::move()

+
+ProxyResolverV8TracingImpl::RequestImpl::~RequestImpl() {
+  job_->Cancel();
+}
+
+LoadState ProxyResolverV8TracingImpl::RequestImpl::GetLoadState() {
+  return job_->GetLoadState();
+}
+
 void ProxyResolverV8TracingImpl::GetProxyForURL(
     const GURL& url,
     ProxyInfo* results,
     const CompletionCallback& callback,
-    ProxyResolver::RequestHandle* request,
+    scoped_ptr<ProxyResolver::Request>* request,
     scoped_ptr<Bindings> bindings) {
   DCHECK(CalledOnValidThread());
   DCHECK(!callback.is_null());
 
   scoped_refptr<Job> job = new Job(job_params_.get(), std::move(bindings));
 
-  if (request)

[eroman, 2016/02/24 03:08:06]

Was this intentionally removed? If nothing depends on it and you did mean to
remove it, then please update the documentation for ProxyResolverV8Tracing which
I believe says this can be nullptr.

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

This comment was not addressed.

-    *request = job.get();
+  request->reset(new RequestImpl(job));
 
   job->StartGetProxyForURL(url, results, callback);
 }
 
-void ProxyResolverV8TracingImpl::CancelRequest(
-    ProxyResolver::RequestHandle request) {
-  Job* job = reinterpret_cast<Job*>(request);
-  job->Cancel();
-}
-
-LoadState ProxyResolverV8TracingImpl::GetLoadState(
-    ProxyResolver::RequestHandle request) const {
-  Job* job = reinterpret_cast<Job*>(request);
-  return job->GetLoadState();
-}
 
 class ProxyResolverV8TracingFactoryImpl : public ProxyResolverV8TracingFactory {
  public:
   ProxyResolverV8TracingFactoryImpl();
   ~ProxyResolverV8TracingFactoryImpl() override;
 
   void CreateProxyResolverV8Tracing(
       const scoped_refptr<ProxyResolverScriptData>& pac_script,
       scoped_ptr<ProxyResolverV8Tracing::Bindings> bindings,
       scoped_ptr<ProxyResolverV8Tracing>* resolver,
@@ skipping 118 matching lines @@
 
 }  // namespace
 
 // static
 scoped_ptr<ProxyResolverV8TracingFactory>
 ProxyResolverV8TracingFactory::Create() {
   return make_scoped_ptr(new ProxyResolverV8TracingFactoryImpl());
 }
 
 }  // namespace net