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.
+    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,
   // 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
-  // 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
   // 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
+  // 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
   // 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.
   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_; }

[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; }
 
   // 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;
 
+  // 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
   // 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_