Make URLRequest::Read to return net errors or bytes read instead of a bool

net/url_request/url_request.h

Index: net/url_request/url_request.h
diff --git a/net/url_request/url_request.h b/net/url_request/url_request.h
index 73e02d99c8710171d0c83c780eafabdcd1097645..dbc2c0a06d61120069c22ce6c0dcb4c1d15d5ed0 100644
--- a/net/url_request/url_request.h
+++ b/net/url_request/url_request.h
@@ -196,24 +196,37 @@ class NET_EXPORT URLRequest : NON_EXPORTED_BASE(public base::NonThreadSafe),
                                        bool fatal);
 
     // After calling Start(), the delegate will receive an OnResponseStarted
-    // callback when the request has completed.  If an error occurred, the
-    // request->status() will be set.  On success, all redirects have been
+    // callback when the request has completed. |net_error| will be set to OK
+    // or an actual net error.  On success, all redirects have been
     // followed and the final response is beginning to arrive.  At this point,
     // meta data about the response is available, including for example HTTP
     // response headers if this is a request for a HTTP resource.
-    virtual void OnResponseStarted(URLRequest* request) = 0;
+    virtual void OnResponseStarted(URLRequest* request, int net_error);
+    // Deprecated.
+    virtual void OnResponseStarted(URLRequest* request);
 
     // Called when the a Read of the response body is completed after an
     // IO_PENDING status from a Read() call.
     // The data read is filled into the buffer which the caller passed
     // to Read() previously.
     //
-    // If an error occurred, request->status() will contain the error,
-    // and bytes read will be -1.
+    // If an error occurred, |bytes_read| will be set to the error.
     virtual void OnReadCompleted(URLRequest* request, int bytes_read) = 0;
 
+    // Used to call OnResponseStarted(). There are two the same methods. The one
+    // that has |net_error| in arguments is a new one. This method is used to
+    // distinguish whether clients have already overridden new OnResponseStarted
+    // and call that one instead of old one without |net_error|.
+    //
+    // The method will be removed as soon as all clients are modified.
+    void NotifyOnResponseStarted(URLRequest* request, int net_error);
+
    protected:
     virtual ~Delegate() {}
+
+   private:
+    // Used to distinguish whether modified a method is overridden by clients.
+    bool implemented_;
   };
 
   // If destroyed after Start() has been called but while IO is pending,
@@ -524,9 +537,6 @@ class NET_EXPORT URLRequest : NON_EXPORTED_BASE(public base::NonThreadSafe),
   // URL but has not yet initiated the new request.
   bool is_redirecting() const { return is_redirecting_; }
 
-  // Returns the error status of the request.
-  const URLRequestStatus& status() const { return status_; }
-
   // Returns a globally unique identifier for this request.
   uint64_t identifier() const { return identifier_; }
 
@@ -540,12 +550,13 @@ class NET_EXPORT URLRequest : NON_EXPORTED_BASE(public base::NonThreadSafe),
   // no effect once the response has completed.  It is guaranteed that no
   // methods of the delegate will be called after the request has been
   // cancelled, except that this may call the delegate's OnReadCompleted()
-  // during the call to Cancel itself.
-  void Cancel();
+  // during the call to Cancel itself. Returns |ERR_ABORTED| or other net error
+  // if there was one.
+  int Cancel();
 
   // Cancels the request and sets the error to |error| (see net_error_list.h
-  // for values).
-  void CancelWithError(int error);
+  // for values). Returns set error.
+  int CancelWithError(int error);

[mmenke, 2016/08/26 19:36:48]

Can we just make these void?  Some things that cancel will not know the final
error code, in that case, but does that cause any big problems?

[maksims (do not use this acc), 2016/08/29 12:30:35]

Well, It might, but I'm not 100% sure.. I just want to be sure that a request is
really canceled with an error which is provided by a client. But it's not really
necessary, I guess. From the other hand, clients used to call Cancel() and then
check the status with request_->Status().

For example, URLDownloader calls CancelWithError()
(https://cs.chromium.org/chromium/src/content/browser/download/url_downloader....)
and then calls ResponseCompleted() that checks the status with
request_->status(). If it is assumed, the error, which is sent by the client, is
always set or doesn't cause any troubles, then I'm ok with reverting back to
void. But I'm suspicious it will cause troubles and bugs in the future. As an
alternative, it is possible to set the error specified without checking if an
error has already been set.

 
   // Cancels the request and sets the error to |error| (see net_error_list.h
   // for values) and attaches |ssl_info| as the SSLInfo for that request.  This
@@ -554,13 +565,18 @@ class NET_EXPORT URLRequest : NON_EXPORTED_BASE(public base::NonThreadSafe),
   void CancelWithSSLError(int error, const SSLInfo& ssl_info);
 
   // Read initiates an asynchronous read from the response, and must only
-  // be called after the OnResponseStarted callback is received with a
-  // successful status.
-  // If data is available, Read will return true, and the data and length will
-  // be returned immediately.  If data is not available, Read returns false,
-  // and an asynchronous Read is initiated.  The Read is finished when
-  // the caller receives the OnReadComplete callback.  Unless the request was
+  // be called after the OnResponseStarted callback is received with a net::OK.
+  // If data is available, length and the data will be returned immediately.

[mmenke, 2016/08/26 19:36:48]

I'd mention net error on failure up here instead, and remove the comment about
an async failure.

[maksims (do not use this acc), 2016/08/29 12:30:35]

I didn't get it.

+  // If data is not available, Read returns net::ERR_IO_PENDING, and an
+  // asynchronous Read is initiated.The Read is finished when the caller

[mmenke, 2016/08/26 19:36:48]

Space after period.

[maksims (do not use this acc), 2016/08/29 12:30:36]

Done.

+  // receives the OnReadComplete callback. Unless the request was
   // cancelled, OnReadComplete will always be called, even if the read failed.
+  // A value of 0 indicates that there is no more data available to read from
+  // the stream.

[mmenke, 2016/08/26 19:36:48]

Suggest removing the last sentence, and just modifying the above the sentence to
be "Unless the request was cancelled, OnReadComplete will always be called on
async completion with the result of the read."

Then you don't need to duplicate the description of the return value for the
sync/async cases.

[maksims (do not use this acc), 2016/08/29 12:30:35]

Done.

+  //
+  // If an error occurs during synchronous read, net error is
+  // returned immediately, otherwise OnReadComplete callback will be called with

[mmenke, 2016/08/26 19:36:48]

otherwise -> If an error occurs asynchronously,

[maksims (do not use this acc), 2016/08/29 12:30:36]

Done.

+  // |net_error| set to an actual error.

[mmenke, 2016/08/26 19:36:48]

"|net_error| set to an actual error" -> "the net error code instead."

[maksims (do not use this acc), 2016/08/29 12:30:36]

Done.

   //
   // The buf parameter is a buffer to receive the data.  If the operation
   // completes asynchronously, the implementation will reference the buffer
@@ -568,13 +584,8 @@ class NET_EXPORT URLRequest : NON_EXPORTED_BASE(public base::NonThreadSafe),
   // length.
   //
   // The max_bytes parameter is the maximum number of bytes to read.
-  //
-  // The bytes_read parameter is an output parameter containing the
-  // the number of bytes read.  A value of 0 indicates that there is no
-  // more data available to read from the stream.
-  //
-  // If a read error occurs, Read returns false and the request->status
-  // will be set to an error.
+  int Read(IOBuffer* buf, int max_bytes);
+  // Deprecated.
   bool Read(IOBuffer* buf, int max_bytes, int* bytes_read);
 
   // If this request is being cached by the HTTP cache, stop subsequent caching.
@@ -648,6 +659,10 @@ class NET_EXPORT URLRequest : NON_EXPORTED_BASE(public base::NonThreadSafe),
   // or after the response headers are received.
   void GetConnectionAttempts(ConnectionAttempts* out) const;
 
+  // Returns the error status of the request.
+  // Do not use! Going to be protected!
+  const URLRequestStatus& status() const { return status_; }

[mmenke, 2016/08/26 19:36:48]

Note that we could make this private, and friend current consumers.  Since it
looks like you're going to fix callers pretty quickly, I don't think that's
necessary, but it is an option.

[maksims (do not use this acc), 2016/08/29 12:30:35]

I'll consider this as well.

+
  protected:
   // Allow the URLRequestJob class to control the is_pending() flag.
   void set_is_pending(bool value) { is_pending_ = value; }
@@ -671,6 +686,9 @@ class NET_EXPORT URLRequest : NON_EXPORTED_BASE(public base::NonThreadSafe),
   friend class URLRequestJob;
   friend class URLRequestContext;
 
+  // Temporarily for testing purposes.

[mmenke, 2016/08/26 19:36:48]

Suggest this:

// For testing purposes.
// TODO(maksim): Remove this.

[maksims (do not use this acc), 2016/08/29 12:30:35]

Done.

+  friend class TestNetworkDelegate;
+
   // URLRequests are always created by calling URLRequestContext::CreateRequest.
   //
   // If no network delegate is passed in, will use the ones from the
@@ -702,8 +720,8 @@ class NET_EXPORT URLRequest : NON_EXPORTED_BASE(public base::NonThreadSafe),
   void OrphanJob();
 
   // Cancels the request and set the error and ssl info for this request to the
-  // passed values.
-  void DoCancel(int error, const SSLInfo& ssl_info);
+  // passed values. Returns the error that was set.
+  int DoCancel(int error, const SSLInfo& ssl_info);
 
   // Called by the URLRequestJob when the headers are received, before any other
   // method, to allow caching of load timing information.