Change HostResolver::Resolve() to take an std::unique_ptr<Request>* rather than a RequestHandle*

net/dns/host_resolver.h

Index: net/dns/host_resolver.h
diff --git a/net/dns/host_resolver.h b/net/dns/host_resolver.h
index 5c371e43ad533e66ed0f366c36f5f17bb73e1bf7..3aa8445de267c5eea23bb0a6642748af3c65d196 100644
--- a/net/dns/host_resolver.h
+++ b/net/dns/host_resolver.h
@@ -42,6 +42,15 @@ class NetLog;
 // goes out of scope).
 class NET_EXPORT HostResolver {
  public:
+  class NET_EXPORT Request {

[mmenke, 2016/07/12 18:50:23]

Document this class.

(Owned by the consumer, deletion cancels the request)

[maksims (do not use this acc), 2016/07/19 15:00:26]

Done.

+   public:
+    virtual ~Request() {}
+
+    // Changes the priority of the request. ChangeRequestPriority must NOT be
+    // called after the request's completion callback has already run.
+    virtual void ChangeRequestPriority(RequestPriority priority) = 0;
+  };
+
   // |max_concurrent_resolves| is how many resolve requests will be allowed to
   // run in parallel. Pass HostResolver::kDefaultParallelism to choose a
   // default value.
@@ -115,9 +124,6 @@ class NET_EXPORT HostResolver {
     bool is_my_ip_address_;
   };
 
-  // Opaque type used to cancel a request.
-  typedef void* RequestHandle;
-
   // Set Options.max_concurrent_resolves to this to select a default level
   // of concurrency.
   static const size_t kDefaultParallelism = 0;
@@ -152,21 +158,9 @@ class NET_EXPORT HostResolver {
                       RequestPriority priority,
                       AddressList* addresses,
                       const CompletionCallback& callback,
-                      RequestHandle* out_req,
+                      std::unique_ptr<Request>* out_req,

[Devlin, 2016/07/12 15:04:51]

I'm not familiar with the implementation here at all, so more of a question than
a comment.

It looks like we now link the Job with the request - is there a lifetime
requirement related to the out_req?  e.g., if I pass in a stack-allocated
std::unique_ptr<Request> that's deleted right after the call to this, and the
Job finishes async, will that cause a crash?  We should document any lifetime
requirements here.

[mmenke, 2016/07/12 18:50:23]

No - it should be deleted the cancel the request.  IT was already implicitly
linked to the job before, this just makes cancellation less prone to bugginess.

This should be documented.

[maksims (do not use this acc), 2016/07/19 15:00:25]

Done.

                       const BoundNetLog& net_log) = 0;
 
-  // Changes the priority of the specified request. |req| is the handle returned
-  // by Resolve(). ChangeRequestPriority must NOT be called after the request's
-  // completion callback has already run or the request was canceled.
-  virtual void ChangeRequestPriority(RequestHandle req,
-                                     RequestPriority priority);
-
-  // Cancels the specified request. |req| is the handle returned by Resolve().
-  // After a request is canceled, its completion callback will not be called.
-  // CancelRequest must NOT be called after the request's completion callback
-  // has already run or the request was canceled.
-  virtual void CancelRequest(RequestHandle req) = 0;
-
   // Resolves the given hostname (or IP address literal) out of cache or HOSTS
   // file (if enabled) only. This is guaranteed to complete synchronously.
   // This acts like |Resolve()| if the hostname is IP literal, or cached value