[ON HOLD] Implement pull-based design for content decoding

net/url_request/url_request_job.h

Index: net/url_request/url_request_job.h
diff --git a/net/url_request/url_request_job.h b/net/url_request/url_request_job.h
index beae02111bde59587c9705d5f6799a821372c390..d4192ac321a42c0ce8cd6e1b2757272904e8215e 100644
--- a/net/url_request/url_request_job.h
+++ b/net/url_request/url_request_job.h
@@ -22,6 +22,7 @@
 #include "net/base/request_priority.h"
 #include "net/base/upload_progress.h"
 #include "net/cookies/canonical_cookie.h"
+#include "net/filter/stream_source.h"
 #include "net/socket/connection_attempts.h"
 #include "net/url_request/redirect_info.h"
 #include "net/url_request/url_request.h"
@@ -143,14 +144,6 @@ class NET_EXPORT URLRequestJob : public base::PowerObserver {
   // network stack makes the request to.
   virtual void PopulateNetErrorDetails(NetErrorDetails* details) const;
 
-  // Called to setup a stream filter for this request. An example of filter is
-  // content encoding/decoding.
-  // Subclasses should return the appropriate Filter, or NULL for no Filter.
-  // This class takes ownership of the returned Filter.
-  //
-  // The default implementation returns NULL.
-  virtual std::unique_ptr<Filter> SetupFilter() const;
-
   // Called to determine if this response is a redirect.  Only makes sense
   // for some types of requests.  This method returns true if the response
   // is a redirect, and fills in the location param with the URL of the
@@ -216,10 +209,6 @@ class NET_EXPORT URLRequestJob : public base::PowerObserver {
   // Whether we have processed the response for that request yet.
   bool has_response_started() const { return has_handled_response_; }
 
-  // The number of bytes read before passing to the filter. This value reflects
-  // bytes read even when there is no filter.
-  int64_t prefilter_bytes_read() const { return prefilter_bytes_read_; }
-
   // These methods are not applicable to all connections.
   virtual bool GetMimeType(std::string* mime_type) const;
   virtual int GetResponseCode() const;
@@ -248,8 +237,16 @@ class NET_EXPORT URLRequestJob : public base::PowerObserver {
   static GURL ComputeReferrerForRedirect(URLRequest::ReferrerPolicy policy,
                                          const std::string& referrer,
                                          const GURL& redirect_destination);

[mmenke, 2016/07/28 18:40:13]

nit:  Blank line between unrelated methods.

[xunjieli, 2016/08/01 16:46:24]

Done.

+  // The number of bytes read before passing to the filter. This value reflects
+  // bytes read even when there is no filter.
+  int64_t prefilter_bytes_read() const;
 
  protected:
+  // Helper method used to perform tasks after reading from |source_| is
+  // completed. |synchronous| true if the read completed synchronously.
+  // See the documentation for |Read| above for the contract of this method.
+  void SourceReadComplete(bool synchronous, int result);
+
   // Notifies the job that a certificate is requested.
   void NotifyCertificateRequested(SSLCertRequestInfo* cert_request_info);
 
@@ -307,24 +304,10 @@ class NET_EXPORT URLRequestJob : public base::PowerObserver {
   // bodies are never read.
   virtual void DoneReadingRedirectResponse();
 
-  // 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();
+  // Called to set up a StreamSource chain for this request.
+  // Subclasses should return the appropriate last StreamSource of the chain,
+  // or nullptr on error.
+  virtual std::unique_ptr<StreamSource> SetupSource();

[mmenke, 2016/07/28 18:40:13]

Can this be const?  Not a big deal if not, but I assume it has no side effects?

[mmenke, 2016/07/28 18:40:14]

nit:  SetUpSource()

(Setup is a noun, Set Up functions as a verb)

[mmenke, 2016/07/28 18:40:14]

Also, should this be SetUpStream or SetUpStreamSource?  "Stream" seems more
important the "Source" to me.  I know what a Stream is.  I'm not sure what a
"Source" is (It could be a connection, for instance).

If Stream is the more important term, could even rename StreamSource to
SourceStream?  Don't have strong opinions here, just thinking out loud.  On the
down side, there's at least one other SourceStream class in Chrome, though in a
different namespace.  There are also BlahStreamSource classes as well, though
there's no other StreamSource I can find.

[xunjieli, 2016/08/01 16:46:23]

It can't be const because when URLRequestHttpJob creates the StreamSource, it
might need to create a SdchSourceStreamDelegate and make it a member variable.
Other than that, it doesn't have any side effect.

[xunjieli, 2016/08/01 16:46:23]

Done.

[xunjieli, 2016/08/01 16:46:24]

Done. I agree with you. I like SourceStream better.

 
   // Provides derived classes with access to the request's network delegate.
   NetworkDelegate* network_delegate() { return network_delegate_; }
@@ -337,7 +320,7 @@ class NET_EXPORT URLRequestJob : public base::PowerObserver {
 
   // The number of bytes read after passing through the filter. This value
   // reflects bytes read even when there is no filter.
-  int64_t postfilter_bytes_read() const { return postfilter_bytes_read_; }
+  int64_t postfilter_bytes_read() const;
 
   // Turns an integer result code into an Error and a count of bytes read.
   // The semantics are:
@@ -354,21 +337,17 @@ class NET_EXPORT URLRequestJob : public base::PowerObserver {
   URLRequest* request_;
 
  private:
+  class URLRequestJobStreamSource;
+
   // Set the status of the associated URLRequest.
   // TODO(mmenke): Make the URLRequest manage its own status.
   void SetStatus(const URLRequestStatus& status);
 
-  // When data filtering is enabled, this function is used to read data
-  // for the filter. Returns a net error code to indicate if raw data was
-  // successfully read,  an error happened, or the IO is pending.
-  Error ReadRawDataForFilter(int* bytes_read);
-
-  // Informs the filter chain that data has been read into its buffer.
-  void PushInputToFilter(int bytes_read);
-
   // Invokes ReadRawData and records bytes read if the read completes
   // synchronously.
-  Error ReadRawDataHelper(IOBuffer* buf, int buf_size, int* bytes_read);
+  int ReadRawDataHelper(IOBuffer* buf,
+                        int buf_size,
+                        const CompletionCallback& callback);
 
   // 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
@@ -377,18 +356,14 @@ class NET_EXPORT URLRequestJob : public base::PowerObserver {
 
   // 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. |error|
-  // specifies whether an error occurred and no bytes were read.
-  void GatherRawReadStats(Error error, int bytes_read);
+  // is 0, this indicates that there is no additional data to read.
+  // If |bytes_read| is negative, no bytes were read.
+  void GatherRawReadStats(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 that request is done. 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.
@@ -418,22 +393,12 @@ class NET_EXPORT URLRequestJob : public base::PowerObserver {
   // NotifyDone so that it is kept in sync with the request.
   bool done_;
 
-  int64_t prefilter_bytes_read_;
-  int64_t postfilter_bytes_read_;
-
-  // The data stream filter which is enabled on demand.
-  std::unique_ptr<Filter> filter_;
+  // The first StreamSource of the StreamSource chain used.

[mmenke, 2016/07/28 18:40:14]

Using "first" here is inconsistent with using "previous_" to mean the next
filter closer to the HttpTransaction in FilterStreamSource.  source_->previous()
should be null, if |source_| is the "first", but in this case, it's the "second"
filter.

I'd replace "previous_" with "next_", because I think of things in terms of
URLRequest->next_layer->next_layer->.... (top/right/first -> ... ->
bottom/left/last).

Calling this one the "last" or the "bottom" source seems really weird to me.  I
suppose calling this top, and considering |previous_| to mean "lower" kinda
works, but seems less natural to me.

[xunjieli, 2016/08/01 16:46:23]

Done.

+  std::unique_ptr<StreamSource> source_;
 
-  // If the filter filled its output buffer, then there is a change that it
-  // still has internal data to emit, and this flag is set.
-  bool filter_needs_more_output_space_;
-
-  // When we filter data, we receive data into the filter buffers.  After
-  // processing the filtered data, we return the data in the caller's buffer.
-  // While the async IO is in progress, we save the user buffer here, and
-  // when the IO completes, we fill this in.
-  scoped_refptr<IOBuffer> filtered_read_buffer_;
-  int filtered_read_buffer_len_;
+  // Keep a reference to the buffer passed in via URLRequestJob::Read() so it
+  // doesn't get destroyed when the read has not completed.
+  scoped_refptr<IOBuffer> pending_read_buffer_;
 
   // We keep a pointer to the read buffer while asynchronous reads are
   // in progress, so we are able to pass those bytes to job observers.
@@ -462,6 +427,16 @@ class NET_EXPORT URLRequestJob : public base::PowerObserver {
   // notification.
   int64_t last_notified_total_sent_bytes_;
 
+  // Non-null if ReadRawData() returned ERR_IO_PENDING, and the read has not
+  // completed.
+  CompletionCallback read_raw_callback_;
+
+  // Number of raw network bytes read from job subclass.
+  size_t prefilter_bytes_read_;
+
+  // Number of bytes after applying filters.
+  size_t postfilter_bytes_read_;

[mmenke, 2016/07/28 18:40:13]

BUG:  These need to be int64_t's.  If we're on a 32-bit system, we can still end
up downloading 4+ GB files.  (Main reason not to use uint64_t is that all the
other sizes are int64_t's, and changing all that is beyond the scope of this
CL).

Suppose we could test that, if we didn't actually write anything to the IOBuffer
and claimed we did, though testing that case may be overkill.

[xunjieli, 2016/08/01 16:46:23]

Done. I've reverted these to the originals which are int64_t's. Thanks for
pointing that out!

+
   base::WeakPtrFactory<URLRequestJob> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestJob);