Add net::SourceStream and net::FilterSourceStream

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 <string>
+
+#include "base/macros.h"
+#include "base/memory/ref_counted.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;

[Randy Smith (Not in Mondays), 2016/08/22 22:37:16]

Why do we need both this and the #include "net/base/io_buffer.h"?

[xunjieli, 2016/08/29 16:25:28]

Done.

+
+// FilterSourceStream represents SourceStreams that always have an upstream
+// from which undecoded input can be read. Except the ultimate upstream in
+// the filter chain, all other streams should implement FilterSourceStream
+// instead of SourceStream.
+class NET_EXPORT_PRIVATE FilterSourceStream : public SourceStream {
+ public:
+  // |upstream| is the SourceStream from which |this| will read data.
+  // |upstream| cannot be null.
+  FilterSourceStream(SourceType type, std::unique_ptr<SourceStream> upstream);
+
+  ~FilterSourceStream() override;
+
+  int Read(IOBuffer* read_buffer,
+           int read_buffer_size,
+           const CompletionCallback& callback) override;
+
+  std::string OrderedTypeStringList() const override;
+
+ private:
+  enum State {
+    STATE_NONE,
+    // Reading data from |next|.

[Randy Smith (Not in Mondays), 2016/08/22 22:37:16]

nit, suggestion: From |upstream_| into |input_buffer_|, just so the enum list is
cohesive?

[Randy Smith (Not in Mondays), 2016/08/22 22:37:16]

nit: Don't you mean |upstream_|?

[xunjieli, 2016/08/29 16:25:28]

Done. Sorry about that. It's a leftover from the renaming.

[xunjieli, 2016/08/29 16:25:28]

Done.

+    STATE_READ_DATA,
+    // Reading data from |next| completed.
+    STATE_READ_DATA_COMPLETE,
+    // Filtering data contained in |buffer_|.

[Randy Smith (Not in Mondays), 2016/08/22 22:37:16]

nit: |input_buffer_|?

[xunjieli, 2016/08/29 16:25:28]

Done.

+    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();
+
+  // Helper method used as a callback in |upstream_->Read()|.

[Randy Smith (Not in Mondays), 2016/08/22 22:37:16]

nit, suggestion: "... used as a callback *argument passed to*
|upstream_->Read()|".  This phrasing makes it sound a bit like upstream_->Read()
accesses this directly, which isn't possible but it still a bit confusing.

[xunjieli, 2016/08/29 16:25:28]

Done.

+  void OnIOComplete(int result);
+
+  // 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). If an unrecoverable error occurred, this should return
+  // ERR_CONTENT_DECODING_FAILED or a more specific error code.
+  //
+  // FilterData() will be repeatedly invoked with the same |input_buffer| until
+  // FilterData() return 0, at which point, FilterSourceStream will read more
+  // input data from |upstream|.

[Randy Smith (Not in Mondays), 2016/08/22 22:37:16]

Confirming: This means that filters have no way to signal EOF until all data has
been read from the upstream source?

I'm not sure whether that's the semantics we want; I'm specifically thinking
that if SDCH bails out (and does the ugly meta-refresh thing) we don't really
want to have to drain the incoming data.  OTOH, I'm not sure that we want
meta-refresh driving requirements for this class.  WDYT?

[xunjieli, 2016/08/29 16:25:28]

Yes, that is right. Filters must consume all data from upstream source or return
an error code.
You are absolutely right. The filter won't be able to do the meta-refresh
without reading EOF from |upstream|. I am not sure if we can workaround this
with the current design. The interface doesn't have a way for the filter to
specify that it doesn't need more data. We can certainly add it, but it will
look like a hack specifically tailored to SDCH. How about I add a comment here
and re-visit this when we do SDCH refactoring?

[mmenke, 2016/08/29 16:41:03]

If this is a problem, seems like there are a couple API changes we could make:

1)  Add a "bool Done()" or "bool NeedMoreData()" method.
2)  Have subclasses implement Read(), and invoke a pull data method from the
parent class as needed.  (This is a lot like 1, actually, we'd just need to
duplicate the loop around the Done()/NeedMoreData() calls, and could inline the
logic).
3)  Add a magic net error code that means a filter needs more data.

Could also get rid of this class, but don't think we want to do that.  Open to
other ideas.

[xunjieli, 2016/08/30 17:27:21]

Done. Thanks for the suggestions! I chose (1). PTAL?

+  virtual int FilterData(IOBuffer* output_buffer,
+                         int 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;
+
+  // The SourceStream from which |this| will read data from. Data flows from
+  // |upstream_| to |this_|.
+  std::unique_ptr<SourceStream> upstream_;
+
+  State next_state_;
+
+  // Buffer for reading data out of |upstream_| and then for use by |this|
+  // before the filtered data is returned through Read().
+  scoped_refptr<IOBuffer> input_buffer_;
+
+  // Keep this as a member because subclass might not drain everything in a
+  // single FilterData().

[Randy Smith (Not in Mondays), 2016/08/22 22:37:15]

nit, suggestion: This comment describes why this is a data member, but not what
it does.  Maybe "Wrapper around input_buffer_ that makes visible only unread
data.  Kept as a data member because the subclass ..."?

[xunjieli, 2016/08/29 16:25:29]

Done.

+  scoped_refptr<DrainableIOBuffer> drainable_input_buffer_;
+
+  // Not null if there is a pending Read.
+  scoped_refptr<IOBuffer> output_buffer_;
+  int output_buffer_size_;
+  CompletionCallback callback_;
+
+  // Reading from |upstream_| has returned 0 byte or an error code.
+  bool upstream_end_reached_;

[Randy Smith (Not in Mondays), 2016/08/22 22:37:16]

nit, suggestion: |upstream_eof_reached_|?

[xunjieli, 2016/08/29 16:25:28]

I changed from |eof_reached_| to |end_reached_| in response to an earlier
comment. The reason is that eof only indicates that 0 byte is read. However, the
boolean is also set when an error code is encountered.

+
+  DISALLOW_COPY_AND_ASSIGN(FilterSourceStream);
+};
+
+}  // namespace net
+
+#endif  // NET_FILTER_FILTER_SOURCE_STREAM_H_