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

net/filter/stream_source.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_STREAM_SOURCE_H
+#define NET_FILTER_STREAM_SOURCE_H
+
+#include "base/callback.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+
+namespace net {
+
+class BlockBuffer;
+class SdchStreamSourceDelegate;
+
+// The StreamSource class implements a producer of bytes. Sources often
+// incorporate a previous source from which they read undecoded input. Those
+// which incorporate a previous source take ownership of the previous source
+// when they are created.
+class StreamSource {
+ public:
+  enum SourceType {
+    SOURCE_BROTLI,
+    SOURCE_DEFLATE,
+    SOURCE_GZIP,
+    SOURCE_SDCH,
+    SOURCE_GZIP_FALLBACK,
+    SOURCE_INVALID,
+    SOURCE_NONE,
+  };
+
+  // |type| is the type of the StreamSource. |previous| is the previous
+  // StreamSource in the chain. If |previous| is null, |this| is the start of
+  // the StreamSource chain.
+  StreamSource(SourceType type, scoped_ptr<StreamSource> previous);
+
+  virtual ~StreamSource();

[mmenke, 2016/03/04 21:15:57]

nit:  Blank line after destructor

[xunjieli, 2016/04/20 19:16:10]

Done.

+  typedef base::Callback<void(net::Error, size_t)> OnReadCompleteCallback;
+
+  // Loop on reading until either:
+  // * ReadInternal() returns some data, in which case this method completes
+  //   synchronously, or
+  // * previous_->Read() does not complete synchronously, in which case
+  //   OnReadComplete() is responsible for finishing the decompression.
+  // * If there is no |previous_|, subclass should invoke |callback_|
+  //   when done.

[Randy Smith (Not in Mondays), 2016/03/09 23:03:56]

This comment reads to me like an implementation comment, not an interface
comment.  If you agree with that statement, let's move this comment to the .cc
file.  The comment here should be everything that a consumer of this class needs
to know, and no more; I think that's more on the order of "This call initiates a
read from the filter chain starting at this filter."  followed with the below
information.

[xunjieli, 2016/04/20 19:16:10]

Done.

+  //
+  // If it completes synchronously, it:
+  //   - Returns an Error other than ERR_IO_PENDING
+  //   - Writes |*bytes_read| with the count of bytes read
+  //   - Writes bytes into |*dest_buffer|
+  //   - Does not call |callback|
+  // If it completes asynchronously, it:
+  //   - Returns ERR_IO_PENDING
+  //   - Does not write |*bytes_read|
+  //   - Calls |callback| when it does complete, with an error code and a count
+  //     of bytes read, which are them placed into |*dest_buffer|.
+  // This method takes a reference to |*dest_buffer| if it completes
+  // asynchronously to ensure it does not get freed mid-read.
+  net::Error Read(IOBuffer* dest_buffer,
+                  size_t buffer_size,
+                  size_t* bytes_read,
+                  const OnReadCompleteCallback& callback);
+
+  // Must be called by subclasses when ReadInternal complete asynchronously.
+  void OnReadComplete(IOBuffer* dest_buffer,

[Randy Smith (Not in Mondays), 2016/03/09 23:03:56]

I'm fairly disturbed (read: Please don't do this :-}) by the common name of
OnReadComplete() for routines in both the base and derived classes without an
inheritance relationship; that seems like it's inviting code readers to confuse
which method is actually being called.  

+                      size_t dest_buffer_size,
+                      Error error,
+                      size_t bytes_read);

[Randy Smith (Not in Mondays), 2016/03/09 23:03:56]

Why isn't this protected:?

I'm not sure which interface I prefer, but I'll call out for your consideration
that, since this feels to me like part of the interface to ReadInternal, it
might be a cleaner interface to just pass ReadInternal a callback rather than
make subclasses know about a special entry point.

ETA: Actually, it doesn't look to me like subclasses ever call this, other than
through the callback passed to Read(), which is a fine interface.  Maybe the
comment above is just wrong?  (And this should be private?)  Apologies if
there's something about the intricacies of how this is used I'm missing.

+
+  // This method constructs a chain of StreamSources from a vector of
+  // Content-Type values and an SdchStreamSourceDelegate. If any of the
+  // constructed StreamSources are SdchStreamSources, they will be constructed
+  // with |delegate|.
+  static scoped_ptr<StreamSource> BuildSourceChain(
+      scoped_ptr<StreamSource> current,
+      const std::vector<std::string>& type,
+      SdchStreamSourceDelegate* delegate);
+
+  // Returns a string describing the SourceTypes implemented by this filter.
+  std::string OrderedStreamSourceList() const;
+
+  SourceType type() const { return type_; }
+
+ protected:
+  SourceType type_;
+  // Previous StreamSource in the chain.
+  scoped_ptr<StreamSource> previous_;
+  scoped_refptr<IOBuffer> pending_read_buffer_;
+  OnReadCompleteCallback callback_;

[Randy Smith (Not in Mondays), 2016/03/09 23:03:56]

nit: Comment about when non-null?  (Presumably when there's a read outstanding.)

[xunjieli, 2016/04/20 19:16:10]

Done.

+
+  scoped_ptr<BlockBuffer> buffer_;

[Randy Smith (Not in Mondays), 2016/03/09 23:03:56]

nit: Comment about how used?

[Randy Smith (Not in Mondays), 2016/03/09 23:03:56]

Did you consider making this an explicit part of the ReadInternal interface (and
hence a private data member)?  I think I'd prefer that unless it's a real PITA,
since it makes the base/derived class interface more explicit (it's just
ReadInternal).

[xunjieli, 2016/04/20 19:16:10]

|buffer_| is used for reading from |previous_|, but sub classes also consume 
bytes from it in anyway they like. For instance, gzip stream source drains
header bytes from |buffer_|, so it can't be just a part of ReadInternal
interface and a private data member.

[xunjieli, 2016/04/20 19:16:10]

Done.

+
+ private:
+  // Also see Read(). Subclasses should implement this method to read data into
+  // |dest_buffer| until more input is needed from |previous_|. If |previous_|
+  // is null, the subclass should also take care of invoking |callback_| upon
+  // finishing asynchronously.

[Randy Smith (Not in Mondays), 2016/03/09 23:03:56]

This comment contradicts the one above on OnReadComplete (though I think this
one is accurate).  Having said that, would it be reasonable to make an explicit
callback argument to this interface and remove the magic of reaching into the
base class to access callback_?  If we can make the base/derived class interface
very explicit and in one place I think that's architectural goodness.

+  virtual net::Error ReadInternal(IOBuffer* dest_buffer,
+                                  size_t buffer_size,
+                                  size_t* bytes_read) = 0;
+};
+
+}  // namespace net
+
+#endif  // NET_FILTER_STREAM_SOURCE_H