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

net/url_request/url_request.h

 // Copyright (c) 2012 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.
 
 #ifndef NET_URL_REQUEST_URL_REQUEST_H_
 #define NET_URL_REQUEST_URL_REQUEST_H_
 
 #include <stdint.h>
 
 #include <memory>
@@ skipping 178 matching lines @@
     // indicating what's wrong with the certificate.
     // If |fatal| is true then the host in question demands a higher level
     // of security (due e.g. to HTTP Strict Transport Security, user
     // preference, or built-in policy). In this case, errors must not be
     // bypassable by the user.
     virtual void OnSSLCertificateError(URLRequest* request,
                                        const SSLInfo& ssl_info,
                                        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,
   // then the request will be effectively canceled and the delegate
   // will not have any more of its methods called.
   ~URLRequest() override;
 
   // Changes the default cookie policy from allowing all cookies to blocking all
   // cookies. Embedders that want to implement a more flexible policy should
   // change the default to blocking all cookies, and provide a NetworkDelegate
@@ skipping 290 matching lines @@
   void SetLoadFlags(int flags);
 
   // Returns true if the request is "pending" (i.e., if Start() has been called,
   // and the response has not yet been called).
   bool is_pending() const { return is_pending_; }
 
   // Returns true if the request is in the process of redirecting to a new
   // 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_; }
 
   // This method is called to start the request.  The delegate will receive
   // a OnResponseStarted callback when the request is started.  The request
   // must have a delegate set before this method is called.
   void Start();
 
   // This method may be called at any time after Start() has been called to
   // cancel the request.  This method may be called many times, and it has
   // 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
   // is useful to attach a certificate and certificate error to a canceled
   // request.
   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
   // until OnReadComplete is called.  The buffer must be at least max_bytes in
   // 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.
   // Note that this method has no effect on other (simultaneous or not) requests
   // for the same resource. The typical example is a request that results in
   // the data being stored to disk (downloaded instead of rendered) so we don't
   // want to store it twice.
   void StopCaching();
 
   // This method may be called to follow a redirect that was deferred in
@@ skipping 53 matching lines @@
   // the more general response_info() is available, even though it is a subset.
   const HostPortPair& proxy_server() const {
     return proxy_server_;
   }
 
   // Gets the connection attempts made in the process of servicing this
   // URLRequest. Only guaranteed to be valid if called after the request fails
   // 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; }
 
   // Allow the URLRequestJob class to set our status too.
   void set_status(URLRequestStatus status);
 
   // Allow the URLRequestJob to redirect this request.  Returns OK if
   // successful, otherwise an error code is returned.
   int Redirect(const RedirectInfo& redirect_info);
 
   // Called by URLRequestJob to allow interception when a redirect occurs.
   void NotifyReceivedRedirect(const RedirectInfo& redirect_info,
                               bool* defer_redirect);
 
   // Allow an interceptor's URLRequestJob to restart this request.
   // Should only be called if the original job has not started a response.
   void Restart();
 
  private:
   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
   // URLRequestContext.
   URLRequest(const GURL& url,
              RequestPriority priority,
              Delegate* delegate,
              const URLRequestContext* context,
              NetworkDelegate* network_delegate);
 
@@ skipping 11 matching lines @@
   // happens when following a HTTP redirect.
   void RestartWithJob(URLRequestJob* job);
   void PrepareToRestart();
 
   // Detaches the job from this request in preparation for this object going
   // away or the job being replaced. The job will not call us back when it has
   // been orphaned.
   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.
   void OnHeadersComplete();
 
   // Notifies the network delegate that the request has been completed.
   // This does not imply a successful completion. Also a canceled request is
   // considered completed.
   void NotifyRequestCompleted();
 
@@ skipping 130 matching lines @@
 
   // The proxy server used for this request, if any.
   HostPortPair proxy_server_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequest);
 };
 
 }  // namespace net
 
 #endif  // NET_URL_REQUEST_URL_REQUEST_H_