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

net/filter/filter_source_stream.h

+// Copyright 2016 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_FILTER_FILTER_SOURCE_STREAM_H_
+#define NET_FILTER_FILTER_SOURCE_STREAM_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "net/base/completion_callback.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/base/net_export.h"
+#include "net/filter/source_stream.h"
+
+namespace net {
+
+class IOBuffer;
+
+// FilterSourceStream represents SourceStreams that always have a next
+// stream from which undecoded input can be read. Except the last stream in

[Randy Smith (Not in Mondays), 2016/08/09 20:28:30]

Suggestion: I'm struggling around the nomenclature used to express stream
relationships/ordering.  Specifically, I'm not sure that "next"/"previous"
communicates well the actual relationship between the streams is (I don't
remember if you were involved in the design discussions around whether a sink
stream should own its source stream, or a source stream should own its sync
stream--we settled on a sink stream owning its source stream, but that was a
live design issue for a while).  So I'm tempted to suggest a change of
nomenclature in that wherever there are descriptive words referring to the
relationship between streams, instead of using next/previous use source/sink. 
That would involve a lot of changes of names, but I think would make the code
easier to understand for future readers.  If you do this, it might be reasonable
to refer to "the last stream" might be referred to as "the ultimate source
stream", or it might be left as "the last stream" with the context being enough
for readers to figure out that it's all the way to the source in the chain, as
opposed to all the way to the sink.

Up to you, but I wanted to at least raise the question.

[xunjieli, 2016/08/15 15:19:07]

Thanks for the suggestion. I have considered it, but I don't see a clear way to
change the nomenclature to source/sink. The classes have "source" in their
names. If we refer |this| as "sink", and |next| as "source", I find it
counter-intuitive. Furthermore, a "sink" should provide a drain method and not a
read method. Maybe I am missing something, since I wasn't involved in the
initial design discussion.
Let's talk about this in person. I've put it on your calendar, please feel free
to re-schedule.

+// the filter chain, all other streams should implement FilterSourceStream
+// instead of SourceStream.
+class NET_EXPORT_PRIVATE FilterSourceStream : public SourceStream {
+ public:
+  // |next| is the next SourceStream in the chain. |next| cannot be
+  // null.
+  FilterSourceStream(SourceType type, std::unique_ptr<SourceStream> next);
+
+  ~FilterSourceStream() override;
+
+  int Read(IOBuffer* read_buffer,
+           size_t read_buffer_size,
+           const CompletionCallback& callback) override;
+
+  std::string OrderedTypeStringList() const override;
+
+ private:
+  enum State {
+    STATE_NONE,
+    // Reading data from |next|.
+    STATE_READ_DATA,
+    // Reading data from |next| completed.
+    STATE_READ_DATA_COMPLETE,
+    // Filtering data contained in |buffer_|.
+    STATE_FILTER_DATA,
+    // Filtering data contained in |buffer_| completed.
+    STATE_FILTER_DATA_COMPLETE,
+    STATE_DONE,
+  };
+
+  int DoLoop(int result);
+  int DoReadData();
+  int DoReadDataComplete(int result);
+  int DoFilterData(int result);
+
+  void DoCallback(int result);
+
+  // Helper method used as a callback when |next_stream_| is to complete a read

[Randy Smith (Not in Mondays), 2016/08/09 20:28:30]

nit: Did you meant "... |next_stream_->Read()| is called to complete ..."?

[xunjieli, 2016/08/15 15:19:07]

Done. I rephrased. Please let me know if it's clearer.

+  // asynchronously.
+  void OnNextReadCompleted(int result);

[Randy Smith (Not in Mondays), 2016/08/09 20:28:30]

nit, suggestion: I find these classes easier to read when there's a vertical
whitespace between methods that have comments next to them.

[xunjieli, 2016/08/15 15:19:07]

Done.

+  // Subclasses should implement this method to filter data from
+  // |input_buffer| and write to |output_buffer|.
+  // This method must complete synchronously (i.e. It cannot return
+  // ERR_IO_PENDING).
+  virtual int FilterData(IOBuffer* output_buffer,
+                         size_t output_buffer_size,
+                         DrainableIOBuffer* input_buffer) = 0;
+
+  // Returns a string representation of the type of this FilterSourceStream.
+  // This is for UMA logging.
+  virtual std::string GetTypeAsString() const = 0;
+
+  // Next SourceStream in the chain that is lower in the stack (closer to the
+  // HttpTransaction).
+  std::unique_ptr<SourceStream> next_stream_;
+
+  State next_state_;
+
+  // Buffer for reading data out of |next_stream_| and then for use by |this|
+  // before the filtered data is returned through Read().
+  scoped_refptr<IOBuffer> input_buffer_;

[Randy Smith (Not in Mondays), 2016/08/09 20:28:30]

nit, suggestion: Same as above--a line of vertical whitespace here?

[xunjieli, 2016/08/15 15:19:07]

Done.

+  // Keep this as a member because subclass might not drain everything in a
+  // single FilterData().
+  scoped_refptr<DrainableIOBuffer> drainable_input_buffer_;
+
+  // Not null if there is a pending Read.
+  scoped_refptr<IOBuffer> output_buffer_;
+  size_t output_buffer_size_;
+  CompletionCallback callback_;
+
+  // Reading from |next_stream_| has returned 0 byte or an error code.
+  bool next_stream_end_reached_;
+
+  DISALLOW_COPY_AND_ASSIGN(FilterSourceStream);
+};
+
+}  // namespace net
+
+#endif  // NET_FILTER_FILTER_SOURCE_STREAM_H_