URLRequestJob: change ReadRawData contract

net/url_request/url_request_job.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_JOB_H_
 #define NET_URL_REQUEST_URL_REQUEST_JOB_H_
 
 #include <stdint.h>
 
 #include <string>
 #include <vector>
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/power_monitor/power_observer.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_states.h"
+#include "net/base/net_errors.h"
 #include "net/base/net_export.h"
 #include "net/base/request_priority.h"
 #include "net/base/upload_progress.h"
 #include "net/cookies/canonical_cookie.h"
 #include "net/socket/connection_attempts.h"
 #include "net/url_request/redirect_info.h"
 #include "net/url_request/url_request.h"
 #include "url/gurl.h"
 
 namespace net {
@@ skipping 239 matching lines @@
 
   // Delegates to URLRequest::Delegate.
   bool CanEnablePrivacyMode() const;
 
   // Notifies the job that the network is about to be used.
   void NotifyBeforeNetworkStart(bool* defer);
 
   // Notifies the job that headers have been received.
   void NotifyHeadersComplete();
 
-  // Notifies the request that the job has completed a Read operation.
-  void NotifyReadComplete(int bytes_read);
-
   // Notifies the request that a start error has occurred.
   void NotifyStartError(const URLRequestStatus& status);
 
-  // NotifyDone marks when we are done with a request.  It is really
-  // a glorified set_status, but also does internal state checking and
-  // job tracking.  It should be called once per request, when the job is
-  // finished doing all IO.
-  void NotifyDone(const URLRequestStatus& status);
-
-  // Some work performed by NotifyDone must be completed on a separate task
-  // so as to avoid re-entering the delegate.  This method exists to perform
-  // that work.
-  void CompleteNotifyDone();
-
   // Used as an asynchronous callback for Kill to notify the URLRequest
   // that we were canceled.
   void NotifyCanceled();
 
   // Notifies the job the request should be restarted.
   // Should only be called if the job has not started a response.
   void NotifyRestartRequired();
 
   // See corresponding functions in url_request.h.
   void OnCallToDelegate();
   void OnCallToDelegateComplete();
 
-  // Called to read raw (pre-filtered) data from this Job.
-  // If returning true, data was read from the job.  buf will contain
-  // the data, and bytes_read will receive the number of bytes read.
-  // If returning true, and bytes_read is returned as 0, there is no
-  // additional data to be read.
-  // If returning false, an error occurred or an async IO is now pending.
-  // If async IO is pending, the status of the request will be
-  // URLRequestStatus::IO_PENDING, and buf must remain available until the
-  // operation is completed.  See comments on URLRequest::Read for more
-  // info.
-  virtual bool ReadRawData(IOBuffer* buf, int buf_size, int *bytes_read);
+  // Called to read raw (pre-filtered) data from this Job. Reads at most
+  // |buf_size| bytes into |buf|.
+  // Possible return values:
+  //   >= 0: Read completed synchronously. Return value is the number of bytes
+  //         read.

[mmenke, 2015/10/22 18:58:11]

Maybe add "0 means eof"?

[xunjieli, 2015/10/23 13:43:08]

Done.

+  //   ERR_IO_PENDING: Read pending asynchronously.
+  //                   When the read completes, |ReadRawDataComplete| should be
+  //                   called.
+  //   Any other ERR_: Read failed synchronously.

[mmenke, 2015/10/22 18:58:11]

Maybe:  Something like:  "Any other negative number: Read failed syncrhonously.
Return value is a network error code."

[xunjieli, 2015/10/23 13:43:08]

Done.

+  // This method might hold onto a reference to |buf| until it completes or is
+  // cancelled using the refcount on |buf|.

[Randy Smith (Not in Mondays), 2015/10/22 20:38:45]

nit, suggestion: I find this sentence a bit confusing as written (because it
runs the behavior description into the mechanism).  How about: "This method
might hold onto a reference to |buf| (by incrementing the refcount) until the
method completes or is cancelled."

[xunjieli, 2015/10/23 13:43:08]

Done.

+  virtual int ReadRawData(IOBuffer* buf, int buf_size);
 
   // Called to tell the job that a filter has successfully reached the end of
   // the stream.
   virtual void DoneReading();
 
   // Called to tell the job that the body won't be read because it's a redirect.
   // This is needed so that redirect headers can be cached even though their
   // bodies are never read.
   virtual void DoneReadingRedirectResponse();
 
-  // Informs the filter that data has been read into its buffer
-  void FilteredDataRead(int bytes_read);
-
-  // Reads filtered data from the request.  Returns true if successful,
-  // false otherwise.  Note, if there is not enough data received to
-  // return data, this call can issue a new async IO request under
-  // the hood.
-  bool ReadFilteredData(int *bytes_read);
+  // Reads filtered data from the request. Returns OK if immediately successful,
+  // ERR_IO_PENDING if the request couldn't complete synchronously, and some
+  // other error code if the request failed synchronously. Note that this
+  // function can issue new asynchronous requests if needed, in which case it
+  // returns ERR_IO_PENDING. If this method completes synchronously,
+  // |*bytes_read| is the number of bytes output by the filter chain if this
+  // method returns OK, or zero if this method returns an error.
+  Error ReadFilteredData(int* bytes_read);
 
   // Whether the response is being filtered in this job.
   // Only valid after NotifyHeadersComplete() has been called.
   bool HasFilter() { return filter_ != NULL; }
 
   // At or near destruction time, a derived class may request that the filters
   // be destroyed so that statistics can be gathered while the derived class is
   // still present to assist in calculations.  This is used by URLRequestHttpJob
   // to get SDCH to emit stats.
   void DestroyFilters();
@@ skipping 11 matching lines @@
   void SetProxyServer(const HostPortPair& proxy_server);
 
   // The number of bytes read before passing to the filter. This value reflects
   // bytes read even when there is no filter.
   int64 prefilter_bytes_read() const { return prefilter_bytes_read_; }
 
   // The number of bytes read after passing through the filter. This value
   // reflects bytes read even when there is no filter.
   int64 postfilter_bytes_read() const { return postfilter_bytes_read_; }
 
+  // Turns an integer result code as from |ReadRawData| into an Error and a

[mmenke, 2015/10/22 18:58:11]

Suggest "Turns an integer result code as from" -> "Turns an integer result code,
such as one from |ReadRawData|" (Could even remove the reference to ReadRawData)

[xunjieli, 2015/10/23 13:43:08]

Done.

+  // count of bytes read. The semantics are:
+  //   |result| >= 0: |*error| == OK, |*count| == |result|
+  //   |result| < 0: |*error| = |result|, |*count| == 0
+  static void ConvertResultToError(int result, Error* error, int* count);
+
+  // Completion callback for raw reads. See |ReadRawData| for details.
+  // |bytes_read| is either >= 0 to indicate a successful read and count of
+  // bytes read, or < 0 to indicate an error.
+  void ReadRawDataComplete(int bytes_read);
+
   // The request that initiated this job. This value MAY BE NULL if the
   // request was released by DetachRequest().
   URLRequest* request_;
 
  private:
   // When data filtering is enabled, this function is used to read data
   // for the filter.  Returns true if raw data was read.  Returns false if
   // an error occurred (or we are waiting for IO to complete).

[mmenke, 2015/10/22 18:58:11]

This comment is no longer accurate

[xunjieli, 2015/10/23 13:43:08]

Done.

-  bool ReadRawDataForFilter(int *bytes_read);
+  Error ReadRawDataForFilter(int* bytes_read);
+
+  // Informs the filter that data has been read into its buffer.

[Randy Smith (Not in Mondays), 2015/10/22 20:38:45]

nit, suggestion: "Inform the filter chain" or "Inform the first filter".

[xunjieli, 2015/10/23 13:43:08]

Done.

+  void PushInputToFilter(int bytes_read);
 
   // Invokes ReadRawData and records bytes read if the read completes
   // synchronously.
-  bool ReadRawDataHelper(IOBuffer* buf, int buf_size, int* bytes_read);
+  Error ReadRawDataHelper(IOBuffer* buf, int buf_size, int* bytes_read);
 
   // Called in response to a redirect that was not canceled to follow the
   // redirect. The current job will be replaced with a new job loading the
   // given redirect destination.
   void FollowRedirect(const RedirectInfo& redirect_info);
 
   // Called after every raw read. If |bytes_read| is > 0, this indicates
   // a successful read of |bytes_read| unfiltered bytes. If |bytes_read|
   // is 0, this indicates that there is no additional data to read. If
   // |bytes_read| is < 0, an error occurred and no bytes were read.

[mmenke, 2015/10/22 18:58:11]

This comment needs to be updated.

[xunjieli, 2015/10/23 13:43:08]

Done.

-  void OnRawReadComplete(int bytes_read);
+  void GatherRawReadStats(Error error, int bytes_read);
 
   // Updates the profiling info and notifies observers that an additional
   // |bytes_read| unfiltered bytes have been read for this job.
   void RecordBytesRead(int bytes_read);
 
   // Called to query whether there is data available in the filter to be read
   // out.
   bool FilterHasData();
 
+  // NotifyDone marks when we are done with a request.  It is really
+  // a glorified set_status, but also does internal state checking and
+  // job tracking.  It should be called once per request, when the job is
+  // finished doing all IO.

[mmenke, 2015/10/22 18:58:11]

nit:  --"we"

[xunjieli, 2015/10/23 13:43:08]

Done.

+  void NotifyDone(const URLRequestStatus& status);
+
+  // Some work performed by NotifyDone must be completed on a separate task so

[mmenke, 2015/10/22 18:58:11]

suggest "on a separate task" -> "asynchronously"

[xunjieli, 2015/10/23 13:43:08]

Done.

+  // as to avoid re-entering URLRequest::Delegate.  This method exists to
+  // perform that work.

[mmenke, 2015/10/22 18:58:11]

"exists to perform" -> "performs"

[xunjieli, 2015/10/23 13:43:08]

Done.

+  void CompleteNotifyDone();
+
   // Subclasses may implement this method to record packet arrival times.
   // The default implementation does nothing.  Only invoked when bytes have been
   // read since the last invocation.
   virtual void UpdatePacketReadTimes();
 
   // Computes a new RedirectInfo based on receiving a redirect response of
   // |location| and |http_status_code|.
   RedirectInfo ComputeRedirectInfo(const GURL& location, int http_status_code);
 
   // Notify the network delegate that more bytes have been received or sent over
@@ skipping 51 matching lines @@
   int64_t last_notified_total_sent_bytes_;
 
   base::WeakPtrFactory<URLRequestJob> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestJob);
 };
 
 }  // namespace net
 
 #endif  // NET_URL_REQUEST_URL_REQUEST_JOB_H_