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..77a6286a30b35a2d69ead72e3439b24677b44bbf 100644
--- a/net/url_request/url_request.h
+++ b/net/url_request/url_request.h
@@ -196,24 +196,38 @@ 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.
+    // TODO(maksims): Remove this;
+    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);

[mmenke, 2016/08/30 22:09:12]

This method doesn't exist any more.

[maksims (do not use this acc), 2016/08/31 11:16:59]

Done.

+
    protected:
     virtual ~Delegate() {}
+
+   private:
+    // Used to distinguish whether modified a method is overridden by clients.
+    bool implemented_;

[mmenke, 2016/08/30 22:09:12]

Not used.

[maksims (do not use this acc), 2016/08/31 11:16:59]

Done.

   };
 
   // If destroyed after Start() has been called but while IO is pending,
@@ -524,9 +538,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 +551,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

[mmenke, 2016/08/30 22:09:12]

Let's improve this comment.  How about "Cancels the request and sets the error
to |error|, unless the request already failed with another error code (see
net_error_list.h)."

[maksims (do not use this acc), 2016/08/31 11:16:59]

Sounds reasonable!

-  // for values).
-  void CancelWithError(int error);
+  // for values). Returns set error.

[mmenke, 2016/08/30 22:09:12]

"Returns set error." -> "Returns final network error code."

[maksims (do not use this acc), 2016/08/31 11:16:59]

Done.

+  int CancelWithError(int error);
 
   // 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 +566,17 @@ 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
-  // cancelled, OnReadComplete will always be called, even if the read failed.
+  // be called after the OnResponseStarted callback is received with a net::OK.
+  // If data is available, length and the data will be returned immediately.
+  // If data is not available, Read returns net::ERR_IO_PENDING, and an
+  // asynchronous Read is initiated. The Read is finished when the caller
+  // receives the OnReadComplete callback. Unless the request was cancelled,
+  // OnReadComplete will always be called on async completion with the result of
+  // the read.
+  //
+  // If an error occurs during synchronous read, net error is
+  // returned immediately, If an error occurs asynchronously, OnReadComplete
+  // callback will be called with the net error code instead.

[mmenke, 2016/08/30 22:09:12]

Let's merge these two paragraphs.  Suggested text:

Read initiates an asynchronous read from the response, and must only be called
after the OnResponseStarted callback is received with a net::OK. If data is
available, length and the data will be returned immediately. If the request has
failed, an error code will be returned. If data is not yet available, Read
returns net::ERR_IO_PENDING, and the Delegate's OnReadComplete method will be
called asynchronously with the result of the read, unless the URLRequest is
canceled.

[maksims (do not use this acc), 2016/08/31 11:16:59]

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,9 @@ 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.
+  // TODO(maksims): Remove this.
   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 +660,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_; }
+
  protected:
   // Allow the URLRequestJob class to control the is_pending() flag.
   void set_is_pending(bool value) { is_pending_ = value; }
@@ -671,6 +687,10 @@ class NET_EXPORT URLRequest : NON_EXPORTED_BASE(public base::NonThreadSafe),
   friend class URLRequestJob;
   friend class URLRequestContext;
 
+  // For testing purposes.
+  // TODO(maksims): Remove this.
+  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 +722,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.