Issue 2251853002: Add net::SourceStream and net::FilterSourceStream

xunjieli

Description was changed from ========== Add net::SourceStream and net::FilterSourceStream This CL adds two new classes ...

4 years, 4 months ago (2016-08-16 22:31:29 UTC) #1

xunjieli

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-16 22:34:42 UTC) #2

xunjieli

xunjieli@chromium.org changed reviewers: + mmenke@chromium.org, rdsmith@chromium.org

4 years, 4 months ago (2016-08-16 22:35:21 UTC) #3

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/40001

4 years, 4 months ago (2016-08-16 22:35:24 UTC) #5

xunjieli

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-16 22:48:30 UTC) #6

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/60001

4 years, 4 months ago (2016-08-16 22:48:40 UTC) #7

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-16 23:41:46 UTC) #8

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-16 23:41:46 UTC) #9

Randy Smith (Not in Mondays)

Which PS in the original CL does this CL correspond to? I want to figure ...

4 years, 4 months ago (2016-08-17 15:56:33 UTC) #10

xunjieli

On 2016/08/17 15:56:33, Randy Smith - Not in Fridays wrote: > Which PS in the ...

4 years, 4 months ago (2016-08-17 15:59:55 UTC) #11

Randy Smith (Not in Mondays)

On 2016/08/17 15:59:55, xunjieli wrote: > On 2016/08/17 15:56:33, Randy Smith - Not in Fridays ...

4 years, 4 months ago (2016-08-17 16:08:55 UTC) #12

xunjieli

On 2016/08/17 16:08:55, Randy Smith - Not in Fridays wrote: > On 2016/08/17 15:59:55, xunjieli ...

4 years, 4 months ago (2016-08-17 17:00:18 UTC) #16

mmenke

Just nits. Haven't looked at tests. https://codereview.chromium.org/2251853002/diff/60001/net/filter/filter_source_stream.cc File net/filter/filter_source_stream.cc (right): https://codereview.chromium.org/2251853002/diff/60001/net/filter/filter_source_stream.cc#newcode35 net/filter/filter_source_stream.cc:35: const CompletionCallback& callback) ...

4 years, 4 months ago (2016-08-17 19:01:56 UTC) #17

mmenke

https://codereview.chromium.org/2251853002/diff/60001/net/filter/mock_source_stream.cc File net/filter/mock_source_stream.cc (right): https://codereview.chromium.org/2251853002/diff/60001/net/filter/mock_source_stream.cc#newcode37 net/filter/mock_source_stream.cc:37: DCHECK_GE(buffer_size, r.len); On 2016/08/17 19:01:56, mmenke wrote: > May ...

4 years, 4 months ago (2016-08-17 19:05:42 UTC) #18

xunjieli

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-17 19:42:45 UTC) #20

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/80001

4 years, 4 months ago (2016-08-17 19:43:04 UTC) #21

xunjieli

Thanks for the review! very helpful. thank you. https://codereview.chromium.org/2251853002/diff/60001/net/filter/filter_source_stream.cc File net/filter/filter_source_stream.cc (right): https://codereview.chromium.org/2251853002/diff/60001/net/filter/filter_source_stream.cc#newcode35 net/filter/filter_source_stream.cc:35: const ...

4 years, 4 months ago (2016-08-17 20:00:20 UTC) #22

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 4 months ago (2016-08-17 20:06:14 UTC) #23

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/124454)

4 years, 4 months ago (2016-08-17 20:06:15 UTC) #24

xunjieli

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-17 20:13:33 UTC) #25

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/100001

4 years, 4 months ago (2016-08-17 20:13:59 UTC) #26

xunjieli

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-18 13:33:02 UTC) #27

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/120001

4 years, 4 months ago (2016-08-18 13:33:24 UTC) #28

mmenke

Trying to bisect a major perf regression that I can reproduce only after reviewing code ...

4 years, 4 months ago (2016-08-18 16:18:43 UTC) #29

mmenke

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_source_stream_unittest.cc File net/filter/filter_source_stream_unittest.cc (right): https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_source_stream_unittest.cc#newcode120 net/filter/filter_source_stream_unittest.cc:120: }; On 2016/08/18 16:18:42, mmenke wrote: > Suggestions: > ...

4 years, 4 months ago (2016-08-18 16:59:21 UTC) #30

xunjieli

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-08-19 14:07:54 UTC) #32

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/180001

4 years, 4 months ago (2016-08-19 14:08:14 UTC) #33

xunjieli

Thanks for being so incredibly patient with this very unpolished code!! I have uploaded a ...

4 years, 4 months ago (2016-08-19 14:31:40 UTC) #34

Thanks for being so incredibly patient with this very unpolished code!! I have
uploaded a new patch to address the comments.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
File net/filter/filter_source_stream.cc (right):

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:11: #include "base/strings/string_util.h"
On 2016/08/18 16:18:41, mmenke wrote:
> include net_errors.h

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:28: DCHECK(upstream_);
On 2016/08/18 16:18:41, mmenke wrote:
> include logging.h

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:44: if (drainable_input_buffer_ == nullptr) {
On 2016/08/18 16:18:41, mmenke wrote:
> This check is the same as checking if !input_buffer_, no?  They'll be true in
> the exact same cases.  Think we should just merge the if statements.

Done. Good catch!

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:108: next_state_ = STATE_READ_DATA_COMPLETE;
On 2016/08/18 16:18:41, mmenke wrote:
> The general way this is done is to unconditionally set next_state_
> (ERR_IO_PENDING will cause us to exit the DoLoop), and call
> "OnNextReadCompleted" the more generic "OnIOComplete".  This pattern isn't
> wrong, it's just that "OnNextReadComplete" vs "DoReadDataComplete" seems very
> confusing.

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:126: void
FilterSourceStream::OnNextReadCompleted(int result) {
On 2016/08/18 16:18:42, mmenke wrote:
> DCHECK_EQ on next_state_'s value?

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:129: if (rv != ERR_IO_PENDING) {
On 2016/08/18 16:18:42, mmenke wrote:
> nit:  Early is generally preferred.

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream.h:64: // ERR_IO_PENDING).
On 2016/08/18 16:18:42, mmenke wrote:
> Also, should we explicitly say what the filter should do if it gets extra
bytes
> it doesn't care about?  Ignore them vs return an error.  Presumably all
filters
> should handle that the same. 

I am not sure if we can enforce that for this interface. SDCH can recover from
extra bytes and pass through inputs unchanged, whereas Gzip filter probably
returns ERR_CONTENT_DECODING_FAILED. I've adjusted the  documentation. Let me
know if that is clearer.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream.h:64: // ERR_IO_PENDING).
On 2016/08/18 16:18:42, mmenke wrote:
> Mention explicitly that it will continue to be called with the same
input_buffer
> until it returns 0 (Regardless of whether it's consumed all bytes or not)?

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream.h:71: virtual std::string GetTypeAsString()
const = 0;
On 2016/08/18 16:18:42, mmenke wrote:
> include <string>

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream.h:81: scoped_refptr<IOBuffer> input_buffer_;
On 2016/08/18 16:18:42, mmenke wrote:
> include base/memory/ref_counted.h

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
File net/filter/filter_source_stream_unittest.cc (right):

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:46: EXPECT_EQ(0u,
expected_remaining_bytes_);
On 2016/08/18 16:18:42, mmenke wrote:
> This seems weird - maybe EXPECT_LE(expected_remaining_bytes_,
> input_buffer->BytesRemaining()) just before the subtraction, instead?

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:53: std::string GetTypeAsString()
const override { return ""; }
On 2016/08/18 16:18:42, mmenke wrote:
> It's a bit silly, but suggest a couple test cases for OrderedTypeStringList
> (Well, just testing it with one filter other than the last one, and with
> multiple filters).  Need to make these return more interesting strings for
that.

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:67:
PassThroughFilterSourceStream(std::unique_ptr<SourceStream> upstream)
On 2016/08/18 16:18:42, mmenke wrote:
> explicit

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:85: return bytes_flushed +
base::checked_cast<int>(bytes_to_filter);
On 2016/08/18 16:18:42, mmenke wrote:
> This seems much more complicated than it needs to be, unless I'm missing
> something.  Could just be:
> 
> buffer_.append(input_buffer->data(), input_buffer->BytesRemaining());
> input_buffer->DidConsume(input_buffer->BytesRemaining());
> 
> int bytes_to_read = std::min(buffer_.size(), output_buffer_size);
> memcpy(...);
> buffer_.erase(...);
> return bytes_to_read;

Done. You are right! I mush have been day dreaming when I wrote this convoluted
code! :)

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:107:
ErrorFilterSourceStream(std::unique_ptr<SourceStream> upstream)
On 2016/08/18 16:18:42, mmenke wrote:
> explicit

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:113: return
ERR_CONTENT_DECODING_FAILED;
On 2016/08/18 16:18:42, mmenke wrote:
> Should we check the case where the second FilterData call with the same
> input_buffer fails?  May be overkill.

I added a second read to FilterDataReturnError test. I am not sure if that's
exactly what you suggested though. Let me know if it's not.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:120: };
On 2016/08/18 16:59:20, mmenke wrote:
> On 2016/08/18 16:18:42, mmenke wrote:
> > Suggestions:  
> > 
> > 
> > * A test that fills both the drainable buffer (all 32k of it) and the buffer
> the
> > consumer reads to.  Largely to make sure things are sized as expected, and
the
> > limits are being passed around.  Not a great test for the 32k, but a
> reasonable
> > test for the 4k kDefaultBufferSize.
> 
> Oops, ignore this one - forgot to delete it, you're already checking this case
> (And moved my other suggestions to better places).

Acknowledged.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:128: TEST(FilterSourceStreamTest,
FilterDataReturnNoBytesExceptLast) {
On 2016/08/18 16:18:42, mmenke wrote:
> Suggest an async version of this test - mainly thinking testing the case where
> no data is returned by the filter in response to an OnNextReadCompleted call.

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:136:
source->AddReadResult(input.data() + input.length() - num_bytes_remaining,
On 2016/08/18 16:18:42, mmenke wrote:
> Maybe count up using |offset| or |position| or something, instead of counting
> backwards like this?  Doesn't really matter, but seems more intuitive.  Also,
> this seems like it use a for loop.  Same goes for other loops like this.

Great idea! Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:137: bytes_added, OK,
/*synchronous=*/true);
On 2016/08/18 16:18:42, mmenke wrote:
> optional:  May want to make the last parameter an enum class instead - it
> basically forces consumers to annotate it, and protects against regression if
> arguments are added.

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:146:
EXPECT_EQ(static_cast<int>(input.length()), rv);
On 2016/08/18 16:18:42, mmenke wrote:
> ASSERT_EQ (To prevent, or reduce the chances of, Bad Things on the next line)

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:147: EXPECT_EQ(0,
memcmp(input.data(), output_buffer->data(), rv));
On 2016/08/18 16:18:42, mmenke wrote:
> This provides better output on failure if you use this instead:
> 
> EXPECT_EQ(input, std::string(output_buffer->data(), rv));

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:152: TEST(FilterSourceStreamTest,
FilterDataReturnNoBytesSync) {
On 2016/08/18 16:18:42, mmenke wrote:
> Hrm...Should have a version of this test where the filter gets data, but
returns
> nothing?

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:207: do {
On 2016/08/18 16:18:42, mmenke wrote:
> I recommend against using do/while loops - they tend to be harder to read,
just
> because everyone is more used to while and for loops (x2).

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:306: }
On 2016/08/18 16:18:42, mmenke wrote:
> Suggestion:  Two tests where one read from the mock stream is returned in
> multiple reads.  In one one test due to a small output buffer, in one test due
> to a filter that only returns a few bytes at a time (Admittedly, well written
> filters probably shouldn't do that, but our API allows it).

Done. I've added

NeedsOutputSpaceForOneRead
ThrottleSourceStream

Thanks for the suggestions!

https://codereview.chromium.org/2251853002/diff/140001/net/filter/mock_source...
File net/filter/mock_source_stream.cc (right):

https://codereview.chromium.org/2251853002/diff/140001/net/filter/mock_source...
net/filter/mock_source_stream.cc:18: DCHECK(results_.empty());
On 2016/08/18 16:18:43, mmenke wrote:
> include base/logging.h

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/mock_source...
File net/filter/mock_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/140001/net/filter/mock_source...
net/filter/mock_source_stream.h:31: std::string OrderedTypeStringList() const
override;
On 2016/08/18 16:18:43, mmenke wrote:
> include <string>

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/mock_source...
net/filter/mock_source_stream.h:31: std::string OrderedTypeStringList() const
override;
On 2016/08/18 16:18:43, mmenke wrote:
> nit:  Remove blank line between override method.  That's just the common
> convention, not official rule about it.

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/mock_source...
net/filter/mock_source_stream.h:37: void AddReadResult(const char* data, int
len, Error error, bool sync);
On 2016/08/18 16:18:43, mmenke wrote:
> include net_error.h

Done.

https://codereview.chromium.org/2251853002/diff/140001/net/filter/mock_source...
net/filter/mock_source_stream.h:54: scoped_refptr<IOBuffer> dest_buffer_;
On 2016/08/18 16:18:43, mmenke wrote:
> include ref_counted.h

Done.

Randy Smith (Not in Mondays)

Despite the number of comments, this looks pretty good-most of my comments are nits. https://codereview.chromium.org/2251853002/diff/60001/net/docs/filter.md ...

4 years, 4 months ago (2016-08-22 22:37:17 UTC) #35

mmenke

https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source_stream.cc File net/filter/mock_source_stream.cc (right): https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source_stream.cc#newcode26 net/filter/mock_source_stream.cc:26: DCHECK(!results_.empty()); On 2016/08/22 22:37:16, Randy Smith - Not in ...

4 years, 4 months ago (2016-08-22 22:46:42 UTC) #36

Randy Smith (Not in Mondays)

On 2016/08/22 22:46:42, mmenke wrote: > https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source_stream.cc > File net/filter/mock_source_stream.cc (right): > > https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source_stream.cc#newcode26 > ...

4 years, 4 months ago (2016-08-23 17:43:15 UTC) #37

On 2016/08/22 22:46:42, mmenke wrote:
>
https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
> File net/filter/mock_source_stream.cc (right):
> 
>
https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
> net/filter/mock_source_stream.cc:26: DCHECK(!results_.empty());
> On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> > I'd vote gently against these DCHECKs.  My rule of thumb is that DCHECKs in
> > testing code are for testing to see if unexpected things are happening in
the
> > *testing* code.  If the code under test could conceivably cause something to
> > happen, even if that reflects a bug in the code under test, the result
should
> be
> > a test failure, not a crash.  As I understand this code, if a downstream
> filter
> > calls read more times than it should, or calls it without waiting for the
> > callback, this could happen.  So if MockSourceStream could be used to test
> real
> > downstream filters, I'd think it shouldn't have these DCHECKs but instead
> > surface these errors gracefully.  That should probably include setting an
> error
> > indicator on the MockSourceStream that test code can check, since you can't
> rely
> > on code under test to pass through errors.
> 
> I tend to prefer this, just because if something unexpected happens at
anything
> below the test fixture, tests can hang.  However, if we want to go that route,
> I'd use EXPECT/ASSERT here (Maybe along with returning an error code
> synchronously) and Test::HasFailure() in the test body...But that seems really
> flaky to me, to the point that checking for failures in the test body at every
> point that could have reached this code really isn't worth the effort.  Code
> clarity matters more than not hanging, for test code.

Ok, after thinking about this some, I agree with you.  {D,}CHECK it is.

I believe tests are always run in debug mode, so DCHECK rather than CHECK is ok?

mmenke

On 2016/08/23 17:43:15, Randy Smith - Not in Fridays wrote: > On 2016/08/22 22:46:42, mmenke ...

4 years, 4 months ago (2016-08-23 17:45:01 UTC) #38

On 2016/08/23 17:43:15, Randy Smith - Not in Fridays wrote:
> On 2016/08/22 22:46:42, mmenke wrote:
> >
>
https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
> > File net/filter/mock_source_stream.cc (right):
> > 
> >
>
https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
> > net/filter/mock_source_stream.cc:26: DCHECK(!results_.empty());
> > On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> > > I'd vote gently against these DCHECKs.  My rule of thumb is that DCHECKs
in
> > > testing code are for testing to see if unexpected things are happening in
> the
> > > *testing* code.  If the code under test could conceivably cause something
to
> > > happen, even if that reflects a bug in the code under test, the result
> should
> > be
> > > a test failure, not a crash.  As I understand this code, if a downstream
> > filter
> > > calls read more times than it should, or calls it without waiting for the
> > > callback, this could happen.  So if MockSourceStream could be used to test
> > real
> > > downstream filters, I'd think it shouldn't have these DCHECKs but instead
> > > surface these errors gracefully.  That should probably include setting an
> > error
> > > indicator on the MockSourceStream that test code can check, since you
can't
> > rely
> > > on code under test to pass through errors.
> > 
> > I tend to prefer this, just because if something unexpected happens at
> anything
> > below the test fixture, tests can hang.  However, if we want to go that
route,
> > I'd use EXPECT/ASSERT here (Maybe along with returning an error code
> > synchronously) and Test::HasFailure() in the test body...But that seems
really
> > flaky to me, to the point that checking for failures in the test body at
every
> > point that could have reached this code really isn't worth the effort.  Code
> > clarity matters more than not hanging, for test code.
> 
> Ok, after thinking about this some, I agree with you.  {D,}CHECK it is.
> 
> I believe tests are always run in debug mode, so DCHECK rather than CHECK is
ok?

Tests aren't always run in debug mode, but nethier are DCHECKs...Erm, DCHECKs
are enabled when building tests, rather.

mmenke

On 2016/08/23 17:45:01, mmenke wrote: > On 2016/08/23 17:43:15, Randy Smith - Not in Fridays ...

4 years, 4 months ago (2016-08-23 17:45:55 UTC) #39

On 2016/08/23 17:45:01, mmenke wrote:
> On 2016/08/23 17:43:15, Randy Smith - Not in Fridays wrote:
> > On 2016/08/22 22:46:42, mmenke wrote:
> > >
> >
>
https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
> > > File net/filter/mock_source_stream.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
> > > net/filter/mock_source_stream.cc:26: DCHECK(!results_.empty());
> > > On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> > > > I'd vote gently against these DCHECKs.  My rule of thumb is that DCHECKs
> in
> > > > testing code are for testing to see if unexpected things are happening
in
> > the
> > > > *testing* code.  If the code under test could conceivably cause
something
> to
> > > > happen, even if that reflects a bug in the code under test, the result
> > should
> > > be
> > > > a test failure, not a crash.  As I understand this code, if a downstream
> > > filter
> > > > calls read more times than it should, or calls it without waiting for
the
> > > > callback, this could happen.  So if MockSourceStream could be used to
test
> > > real
> > > > downstream filters, I'd think it shouldn't have these DCHECKs but
instead
> > > > surface these errors gracefully.  That should probably include setting
an
> > > error
> > > > indicator on the MockSourceStream that test code can check, since you
> can't
> > > rely
> > > > on code under test to pass through errors.
> > > 
> > > I tend to prefer this, just because if something unexpected happens at
> > anything
> > > below the test fixture, tests can hang.  However, if we want to go that
> route,
> > > I'd use EXPECT/ASSERT here (Maybe along with returning an error code
> > > synchronously) and Test::HasFailure() in the test body...But that seems
> really
> > > flaky to me, to the point that checking for failures in the test body at
> every
> > > point that could have reached this code really isn't worth the effort. 
Code
> > > clarity matters more than not hanging, for test code.
> > 
> > Ok, after thinking about this some, I agree with you.  {D,}CHECK it is.
> > 
> > I believe tests are always run in debug mode, so DCHECK rather than CHECK is
> ok?
> 
> Tests aren't always run in debug mode, but nethier are DCHECKs...Erm, DCHECKs
> are enabled when building tests, rather.

I'm actually fine with CHECKs, too.  I'd tend to prefer them, but there's been
debate over this before, and people have pushed on using DCHECKs, and it's not a
point I care to argue.  :)

xunjieli

Thanks a lot for the review! I uploaded a new patch. Additionally I've changed OrderedTypeStringList() ...

4 years, 3 months ago (2016-08-29 16:25:29 UTC) #40

Thanks a lot for the review! I uploaded a new patch. Additionally I've changed
OrderedTypeStringList() to Description() in response to the comment from CL #1.
I also added the revision number to the name of the PatchSet. PTAL.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
File net/filter/filter_source_stream.cc (right):

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:114: // If EOF is read (result == OK), also
pass that to the the filter.
On 2016/08/22 22:37:15, Randy Smith - Not in Fridays wrote:
> I don't understand how the comment relates to the code after it?  I'd think
the
> passing of EOF along to the filter would be through the upstream_end_reached_
> assignment below.

Done. Sorry about the outdated comment.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:151: // data, it is likely that the filter
needs more input.
On 2016/08/22 22:37:15, Randy Smith - Not in Fridays wrote:
> "it is likely": What other things can it mean?  

Done. A misbehaving filter? :) I got rid of "likely".

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.h:21: class IOBuffer;
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> Why do we need both this and the #include "net/base/io_buffer.h"?

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.h:44: // Reading data from |next|.
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> nit: Don't you mean |upstream_|?

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

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.h:44: // Reading data from |next|.
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> nit, suggestion: From |upstream_| into |input_buffer_|, just so the enum list
is
> cohesive?

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.h:48: // Filtering data contained in |buffer_|.
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> nit: |input_buffer_|?

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.h:60: // Helper method used as a callback in
|upstream_->Read()|.
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> 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.

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.h:71: // input data from |upstream|.
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> Confirming: This means that filters have no way to signal EOF until all data
has
> been read from the upstream source?

Yes, that is right. Filters must consume all data from upstream source or return
an error code.

> 
> 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?

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?

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.h:91: // single FilterData().
On 2016/08/22 22:37:15, Randy Smith - Not in Fridays wrote:
> 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 ..."?

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.h:100: bool upstream_end_reached_;
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> nit, suggestion: |upstream_eof_reached_|?

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.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
File net/filter/filter_source_stream_unittest.cc (right):

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:64: // Keep returning 0 byte read
until all |expected_input_bytes| have
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> nit: "0 bytes".

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:66: return OK;
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> nitty nit, suggestion: "return 0" (since we're returning a byte count, not an
> error code).

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:95: }
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> nit, suggestion: You don't need the curly braces here.

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:125: return bytes_to_read;
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> I'm tempted to suggest you take these last four lines and move them into the
> base class as a utility function--I think they're pretty much the same
> everywhere (at least so far).  Up to you.

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream_unittest.cc:238: }
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> Suggestion: Utility function?

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
File net/filter/mock_source_stream.cc (right):

https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
net/filter/mock_source_stream.cc:26: DCHECK(!results_.empty());
On 2016/08/22 22:46:42, mmenke wrote:
> On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> > I'd vote gently against these DCHECKs.  My rule of thumb is that DCHECKs in
> > testing code are for testing to see if unexpected things are happening in
the
> > *testing* code.  If the code under test could conceivably cause something to
> > happen, even if that reflects a bug in the code under test, the result
should
> be
> > a test failure, not a crash.  As I understand this code, if a downstream
> filter
> > calls read more times than it should, or calls it without waiting for the
> > callback, this could happen.  So if MockSourceStream could be used to test
> real
> > downstream filters, I'd think it shouldn't have these DCHECKs but instead
> > surface these errors gracefully.  That should probably include setting an
> error
> > indicator on the MockSourceStream that test code can check, since you can't
> rely
> > on code under test to pass through errors.
> 
> I tend to prefer this, just because if something unexpected happens at
anything
> below the test fixture, tests can hang.  However, if we want to go that route,
> I'd use EXPECT/ASSERT here (Maybe along with returning an error code
> synchronously) and Test::HasFailure() in the test body...But that seems really
> flaky to me, to the point that checking for failures in the test body at every
> point that could have reached this code really isn't worth the effort.  Code
> clarity matters more than not hanging, for test code.

Acknowledged. Thanks for the suggestions and the different opinions!  The parts
on test hanging and the need to check HasFailure() in the test are excellent
points. Based my experience with this code for the past few weeks, I agree that
test crash is much easier to diagnose compared to test hanging. I will keep the
DCHECK as it is.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
net/filter/mock_source_stream.cc:32: DCHECK_GE(buffer_size, r.len);
On 2016/08/22 22:37:17, Randy Smith - Not in Fridays wrote:
> Same comment here--this could be the result of an error in the code under test
> (I believe?) and hence should probably not be a DCHECK.

Acknowledged.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
net/filter/mock_source_stream.cc:65: DCHECK(awaiting_completion_);
On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> And here, unless its part of the contract that the testing code should always
> test to make sure that this object is awaiting completion before calling
> CompleteNextRead() (in which case that should be documented in the header
file).

Done. I added documentation in the header file.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
File net/filter/mock_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
net/filter/mock_source_stream.h:20: class IOBuffer;
On 2016/08/22 22:37:17, Randy Smith - Not in Fridays wrote:
> Why both this and the include file?

Done.

https://codereview.chromium.org/2251853002/diff/200001/net/filter/mock_source...
net/filter/mock_source_stream.h:40: // copy of |data|, so |data| must outlive
this object. If |sync| is true,
On 2016/08/22 22:37:17, Randy Smith - Not in Fridays wrote:
> nit: I think you mean |mode == SYNC|?

Done.

mmenke

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_source_stream.h File net/filter/filter_source_stream.h (right): https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_source_stream.h#newcode71 net/filter/filter_source_stream.h:71: // input data from |upstream|. On 2016/08/29 16:25:28, xunjieli(OOO-8.21-8.26) ...

4 years, 3 months ago (2016-08-29 16:41:03 UTC) #41

xunjieli

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-08-30 16:15:29 UTC) #42

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/220001

4 years, 3 months ago (2016-08-30 16:15:43 UTC) #43

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-08-30 17:20:39 UTC) #44

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-08-30 17:20:40 UTC) #45

xunjieli

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_source_stream.h File net/filter/filter_source_stream.h (right): https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_source_stream.h#newcode71 net/filter/filter_source_stream.h:71: // input data from |upstream|. On 2016/08/29 16:41:03, mmenke ...

4 years, 3 months ago (2016-08-30 17:27:21 UTC) #46

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/200001/net/filter/filter_sour...
net/filter/filter_source_stream.h:71: // input data from |upstream|.
On 2016/08/29 16:41:03, mmenke wrote:
> On 2016/08/29 16:25:28, xunjieli(OOO-8.21-8.26) wrote:
> > On 2016/08/22 22:37:16, Randy Smith - Not in Fridays wrote:
> > > Confirming: This means that filters have no way to signal EOF until all
data
> > has
> > > been read from the upstream source?
> > 
> > Yes, that is right. Filters must consume all data from upstream source or
> return
> > an error code.
> > 
> > > 
> > > 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?
> > 
> > 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?
> 
> 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.

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

Randy Smith (Not in Mondays)

LGTM; everything below are suggestions. https://codereview.chromium.org/2251853002/diff/240001/net/docs/filter.md File net/docs/filter.md (right): https://codereview.chromium.org/2251853002/diff/240001/net/docs/filter.md#newcode14 net/docs/filter.md:14: always flows from upstream ...

4 years, 3 months ago (2016-08-31 19:22:45 UTC) #47

LGTM; everything below are suggestions.

https://codereview.chromium.org/2251853002/diff/240001/net/docs/filter.md
File net/docs/filter.md (right):

https://codereview.chromium.org/2251853002/diff/240001/net/docs/filter.md#new...
net/docs/filter.md:14: always flows from upstream to downstream:
nit, suggestion: I find the last sentence redundant and a little confusing; if
the upstream/downstream metaphor is doing its job, it should be clear that data
is flowing upstream->downstream.   I think introducing the upstream/downstream
terminology in this paragraph is a good idea, but I'd instead just add a third
option to the previous sentence, something like |, or "X is downstream from Y"|.

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
File net/filter/filter_source_stream.cc (right):

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:158: return !upstream_end_reached_;
I like this architecture, but I'd like to avoid the duplication of the
|!upstream_end_reached_| test between here and the if statement above.   I'd
suggest either a) documenting that if the subclass overrides this it must also
track if the upstream data end has been reached and removing the
upstream_end_reached_ test in the conditional, or b) just having the default
implementation here return true and document that a subclass overriding only
needs to track filter-specific information and the framework will track EOF from
the upstream.  Having written out the two alternatives, I think I prefer (b),
but up to you.

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
net/filter/filter_source_stream.h:82: // or EOF. Subclass can override this to
skip reading all the input from
nit, suggestion: "... can override this method to return false to skip ..."

mmenke

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_source_stream.cc File net/filter/filter_source_stream.cc (right): https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_source_stream.cc#newcode140 net/filter/filter_source_stream.cc:140: if (bytes_output == OK && !upstream_end_reached_ && NeedMoreData()) I ...

4 years, 3 months ago (2016-08-31 20:52:29 UTC) #48

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
File net/filter/filter_source_stream.cc (right):

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:140: if (bytes_output == OK &&
!upstream_end_reached_ && NeedMoreData())
I think this may be clearer as:

// Received data or encountered an error.
if (bytes_output != 0)
  return bytes_output;

// Useful comment...
if (!upstream_end_reached_ && NeedMoreData())
  next_state_ = STATE_READ_DATA;
return OK;

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:158: return !upstream_end_reached_;
On 2016/08/31 19:22:44, Randy Smith - Not in Fridays wrote:
> I like this architecture, but I'd like to avoid the duplication of the
> |!upstream_end_reached_| test between here and the if statement above.   I'd
> suggest either a) documenting that if the subclass overrides this it must also
> track if the upstream data end has been reached and removing the
> upstream_end_reached_ test in the conditional, or b) just having the default
> implementation here return true and document that a subclass overriding only
> needs to track filter-specific information and the framework will track EOF
from
> the upstream.  Having written out the two alternatives, I think I prefer (b),
> but up to you.

I agree with Randy here, though I weakly prefer (a).

One concern I have is that this API makes it very difficult for the embedder to
demand more data, and to consider it an error if they don't have more data. 
There's basically no reasonable way to do that, with this API.  We could say
that's a feature, and not a bug, as that's how current filters work, but we may
want to change that, to better handle truncated streams.

There are some simple ways to work around that:
1) Pass in nullptr on repeat calls to FilterData, and have subclasses cache the
buffer if needed - could even separate out the method to pass in data, and call
it with an empty string when done which I think is much cleaner, though we'd
probably need to keep NeedMoreData, or have a bool*/net error code that
indicates we need more data.
2) Pass in nullptr after the final read that received no data, or add another
method for that specific case).
3) If NeedMoreData() is true, but there is no more data, return an error.  This
would require all filters to know when their input stream is done, but they
might not all know that.

Certainly other, possibly better, workaround, though they're require a bit more
reworking of the API.

xunjieli

https://codereview.chromium.org/2251853002/diff/240001/net/docs/filter.md File net/docs/filter.md (right): https://codereview.chromium.org/2251853002/diff/240001/net/docs/filter.md#newcode14 net/docs/filter.md:14: always flows from upstream to downstream: On 2016/08/31 19:22:44, ...

4 years, 3 months ago (2016-09-01 21:26:59 UTC) #50

https://codereview.chromium.org/2251853002/diff/240001/net/docs/filter.md
File net/docs/filter.md (right):

https://codereview.chromium.org/2251853002/diff/240001/net/docs/filter.md#new...
net/docs/filter.md:14: always flows from upstream to downstream:
On 2016/08/31 19:22:44, Randy Smith - Not in Fridays wrote:
> nit, suggestion: I find the last sentence redundant and a little confusing; if
> the upstream/downstream metaphor is doing its job, it should be clear that
data
> is flowing upstream->downstream.   I think introducing the upstream/downstream
> terminology in this paragraph is a good idea, but I'd instead just add a third
> option to the previous sentence, something like |, or "X is downstream from
Y"|.

Done.

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
File net/filter/filter_source_stream.cc (right):

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:140: if (bytes_output == OK &&
!upstream_end_reached_ && NeedMoreData())
On 2016/08/31 20:52:29, mmenke wrote:
> I think this may be clearer as:
> 
> // Received data or encountered an error.
> if (bytes_output != 0)
>   return bytes_output;
> 
> // Useful comment...
> if (!upstream_end_reached_ && NeedMoreData())
>   next_state_ = STATE_READ_DATA;
> return OK;

Done.

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:158: return !upstream_end_reached_;
On 2016/08/31 19:22:44, Randy Smith - Not in Fridays wrote:
> I like this architecture, but I'd like to avoid the duplication of the
> |!upstream_end_reached_| test between here and the if statement above.   I'd
> suggest either a) documenting that if the subclass overrides this it must also
> track if the upstream data end has been reached and removing the
> upstream_end_reached_ test in the conditional, or b) just having the default
> implementation here return true and document that a subclass overriding only
> needs to track filter-specific information and the framework will track EOF
from
> the upstream.  Having written out the two alternatives, I think I prefer (b),
> but up to you.

Done.

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:158: return !upstream_end_reached_;
On 2016/08/31 20:52:29, mmenke wrote:
> On 2016/08/31 19:22:44, Randy Smith - Not in Fridays wrote:
> > I like this architecture, but I'd like to avoid the duplication of the
> > |!upstream_end_reached_| test between here and the if statement above.   I'd
> > suggest either a) documenting that if the subclass overrides this it must
also
> > track if the upstream data end has been reached and removing the
> > upstream_end_reached_ test in the conditional, or b) just having the default
> > implementation here return true and document that a subclass overriding only
> > needs to track filter-specific information and the framework will track EOF
> from
> > the upstream.  Having written out the two alternatives, I think I prefer
(b),
> > but up to you.
> 
> I agree with Randy here, though I weakly prefer (a).
> 
> One concern I have is that this API makes it very difficult for the embedder
to
> demand more data, and to consider it an error if they don't have more data. 
> There's basically no reasonable way to do that, with this API.  We could say
> that's a feature, and not a bug, as that's how current filters work, but we
may
> want to change that, to better handle truncated streams.
> 
> There are some simple ways to work around that:
> 1) Pass in nullptr on repeat calls to FilterData, and have subclasses cache
the
> buffer if needed - could even separate out the method to pass in data, and
call
> it with an empty string when done which I think is much cleaner, though we'd
> probably need to keep NeedMoreData, or have a bool*/net error code that
> indicates we need more data.
> 2) Pass in nullptr after the final read that received no data, or add another
> method for that specific case).
> 3) If NeedMoreData() is true, but there is no more data, return an error. 
This
> would require all filters to know when their input stream is done, but they
> might not all know that.
> 
> Certainly other, possibly better, workaround, though they're require a bit
more
> reworking of the API.

Thanks a lot for the feedback and suggestions. I talked to Matt offline as well.
I agree with Matt that the biggest shortcoming of this API is that subclasses of
FilterSourceStream cannot demand/pull data from |upstream|. I have uploaded a
new patch that adds an explict Pull() method to address this. The upside is that
the API is more flexible, but downside is that there will be some duplicated
logic in subclasses -- which I think we can live with. PTAL?

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
net/filter/filter_source_stream.h:82: // or EOF. Subclass can override this to
skip reading all the input from
On 2016/08/31 19:22:44, Randy Smith - Not in Fridays wrote:
> nit, suggestion: "... can override this method to return false to skip ..."

Done.

xunjieli

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-01 21:27:34 UTC) #51

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/280001

4 years, 3 months ago (2016-09-01 21:28:22 UTC) #52

mmenke

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_source_stream.cc File net/filter/filter_source_stream.cc (right): https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_source_stream.cc#newcode158 net/filter/filter_source_stream.cc:158: return !upstream_end_reached_; On 2016/09/01 21:26:59, xunjieli wrote: > On ...

4 years, 3 months ago (2016-09-01 21:54:46 UTC) #53

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
File net/filter/filter_source_stream.cc (right):

https://codereview.chromium.org/2251853002/diff/240001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:158: return !upstream_end_reached_;
On 2016/09/01 21:26:59, xunjieli wrote:
> On 2016/08/31 20:52:29, mmenke wrote:
> > On 2016/08/31 19:22:44, Randy Smith - Not in Fridays wrote:
> > > I like this architecture, but I'd like to avoid the duplication of the
> > > |!upstream_end_reached_| test between here and the if statement above.  
I'd
> > > suggest either a) documenting that if the subclass overrides this it must
> also
> > > track if the upstream data end has been reached and removing the
> > > upstream_end_reached_ test in the conditional, or b) just having the
default
> > > implementation here return true and document that a subclass overriding
only
> > > needs to track filter-specific information and the framework will track
EOF
> > from
> > > the upstream.  Having written out the two alternatives, I think I prefer
> (b),
> > > but up to you.
> > 
> > I agree with Randy here, though I weakly prefer (a).
> > 
> > One concern I have is that this API makes it very difficult for the embedder
> to
> > demand more data, and to consider it an error if they don't have more data. 
> > There's basically no reasonable way to do that, with this API.  We could say
> > that's a feature, and not a bug, as that's how current filters work, but we
> may
> > want to change that, to better handle truncated streams.
> > 
> > There are some simple ways to work around that:
> > 1) Pass in nullptr on repeat calls to FilterData, and have subclasses cache
> the
> > buffer if needed - could even separate out the method to pass in data, and
> call
> > it with an empty string when done which I think is much cleaner, though we'd
> > probably need to keep NeedMoreData, or have a bool*/net error code that
> > indicates we need more data.
> > 2) Pass in nullptr after the final read that received no data, or add
another
> > method for that specific case).
> > 3) If NeedMoreData() is true, but there is no more data, return an error. 
> This
> > would require all filters to know when their input stream is done, but they
> > might not all know that.
> > 
> > Certainly other, possibly better, workaround, though they're require a bit
> more
> > reworking of the API.
> 
> Thanks a lot for the feedback and suggestions. I talked to Matt offline as
well.
> I agree with Matt that the biggest shortcoming of this API is that subclasses
of
> FilterSourceStream cannot demand/pull data from |upstream|. I have uploaded a
> new patch that adds an explict Pull() method to address this. The upside is
that
> the API is more flexible, but downside is that there will be some duplicated
> logic in subclasses -- which I think we can live with. PTAL?

Another weakness of this approahc that xunjieli brought up is that the API
wouldn't allow offthread decompression (Not something we're currently planning
on implementing, but nice to have as an option in the future).

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-01 22:45:51 UTC) #54

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-01 22:45:52 UTC) #55

xunjieli

Randy: any thought on the new design? I tried incorporating this in the larger CL. ...

4 years, 3 months ago (2016-09-07 14:24:18 UTC) #56

Randy Smith (Not in Mondays)

On 2016/09/07 14:24:18, xunjieli wrote: > Randy: any thought on the new design? > I ...

4 years, 3 months ago (2016-09-07 14:48:26 UTC) #57

mmenke

On 2016/09/07 14:48:26, Randy Smith - Not in Fridays wrote: > On 2016/09/07 14:24:18, xunjieli ...

4 years, 3 months ago (2016-09-07 15:01:10 UTC) #58

Randy Smith (Not in Mondays)

So thank you for https://codereview.chromium.org/2316053003/! Unfortunately, I'm still having the feeling I had this afternoon ...

4 years, 3 months ago (2016-09-08 00:03:32 UTC) #59

So thank you for https://codereview.chromium.org/2316053003/!  Unfortunately,
I'm still having the feeling I had this afternoon in our offline conversation,
of preferring a variant on the old interface;  it really looks to me like
there's a lot of repeated code in the subclasses that doesn't need to be there. 
 This will probably need to be hashed out in front of a whiteboard, and if you
feel strongly we can just keep the new interface, but let me at least try to
make my case.

I agree with the two requirements (allowing a filter/subclass to indicate that
it's an error if it doesn't get more data, and allowing the possibility of
off-thread processing (I'll extend this to off-process processing, as the attack
surface of VCDiff has occasionally come under discussion in the context of
SDCH)).  My preferences on top of those two requirements are a) minimal shared
code in the subclasses, and b) clean conceptual separation of responsibility
between FilterStreamSource and its subclasses (I think those two things are
often but not always correlated, so I'll call them both out).

I think the two options we're currently looking at are:
* PS 10 of this CL, which I'd describe as "Superclass passes subclass a buffer,
and the subclass can ask for more data if there isn't enough in the buffer." 
(I.e. I sorta consider this a combination push and pull model.)  (I'm not sure
whether or not the subclass has to empty the buffer before it asks for more
data--I'd vote in favor of "no" since I'd like to limit the subclass buffer
management to only what's required for the filter.  This means that the
superclass would have to do a copy if the subclass requested more data without
emptying the input IOBuffer, but I think that data has to be copied in that case
anyway; why not have it done in a single place?)

* A variant on PS9, expanded to handle our requirements.  I don't actually think
of PS9 as a push model (since the subclasses aren't responsible for sending the
data onward in the stream); it's more of a "Here's some input data, here's some
output space, filter what you can" model.   (Same issue as to whether
FilterData() can return 0 bytes with bytes left in the input buffer occurs here,
same personal preference applies, I don't think that distinguishes between the
models.)

The expansion of PS9 I'm thinking of is:
* Make it possible to FilterData() to be called asynchronously. 
* Add a mechanism for signaling EOF from the upstream source to filters.  Such a
mechanism would allow for the return of errors. 

WRT calling FilterData() asynchronously, I think that's an easy and obvious
enough expansion of the API (add a callback argument, allow return of
ERR_IO_PENDING) that I'm not actually inclined to put it in a TODO() rather than
expand the interface right now; it would be minimal work to do it in the future
and I don't see any boxes we're going to pain ourselves in by leaving it to the
future.  So let's wait until we need it; it'll be easy to add.  Even if we do
add it now, to my mind it really doesn't make the interface much more complex,
so I don't think that support for off-thread filtering drives a preference
between the two options.

Signaling EOF is trickier.  Assuming for the moment we don't do anything clever
with FilterData(), that means we have a three routine interface definition
between superclass and subclass.

// Can be defaulted for many classes.
bool NeedsData() { return true; }

// Give the subclass a chance to write out any remaining data
// or complain if it needs more data.  Note that this must be 
// called repeatedly until 0 bytes or an error is returned.
int UpstreamEof(IOBuffer* output_buffer, int output_buffer_size) {
  return 0;
}

// Read an arbitrary amount of data from input_buffer, write 
// an arbitrary (< output_buffer_size) amount of data to 
// output_buffer.  A return value of 0 indicates that more 
// data is needed before any more data can be produced.
int FilterData(IOBuffer* output_buffer, int output_buffer_size, 
         DrainableIOBuffer* input_buffer);

To my mind this means the only state the subclasses need to deal with is
algorithm-specific state (e.g. block tables for decompression), which I think
they need to deal with anyway.  The ownership of the buffers is clear: All
buffers named in the 
interface are owned by the superclass (FilterSourceStream).  The subclass may
drain bytes from the input_buffer (i.e. input_buffer is an In/out parameter
whose size can change) but that seems in keeping with the interface spec for
DrainableIOBuffer.   If the subclass can't make progress without more data but
can't do anything with the last couple of bytes in input_buffer, it doesn't need
to worry about it--if it doesn't drain them it'll get them back on the next go
round.

Similarly, I think the amount of duplicated code in the subclasses is minimal. 
Many subclasses won't need to handle the speciality calls, and the ones that do
may well have simple implementations.  And FilterData is about the simplest
interface to a filter I can imagine.

What am I missing?  (Feel free to wait and tell me in person.)

https://codereview.chromium.org/2251853002/diff/280001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/280001/net/filter/filter_sour...
net/filter/filter_source_stream.h:43: int Pull(const CompletionCallback&
callback);
nit: Document return value?  Also interface contract--is this always async? 
What are the subclass visible changes in state that occur?  Is it legal to call
this function if drainable_input_buffer() is not empty?

https://codereview.chromium.org/2251853002/diff/280001/net/filter/filter_sour...
net/filter/filter_source_stream.h:53: bool upstream_end_reached() const { return
upstream_end_reached_; }
?? This information is already provided to subclasses by their use of Pull(),
correct?  They've already seen an error or EOF from Pull.

https://codereview.chromium.org/2251853002/diff/280001/net/filter/filter_sour...
net/filter/filter_source_stream.h:56: // empty, subclasses can call Pull() to
read from |upstream_| as desired.
nit, suggestion: It seems a weird restriction that they can only call Pull if
this is empty; I could easily imagine the subclasses being in a situation where
there is some data here but not enough for them to do anything with, and that
restriction means they have to buffer that data themselves rather than just
relying on the superclass.

xunjieli

Matt and Randy PTAL at PS#11. I talked to Randy offline. I agree with Randy ...

4 years, 3 months ago (2016-09-08 16:12:09 UTC) #61

xunjieli

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-08 16:12:58 UTC) #62

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/320001

4 years, 3 months ago (2016-09-08 16:13:42 UTC) #63

mmenke

On 2016/09/08 16:12:09, xunjieli wrote: > Matt and Randy PTAL at PS#11. > > I ...

4 years, 3 months ago (2016-09-08 17:12:45 UTC) #64

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-08 17:38:13 UTC) #65

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-08 17:38:15 UTC) #66

Randy Smith (Not in Mondays)

So this still LGTM modulo the various comment and DCHECK suggestions I made. Matt, one ...

4 years, 3 months ago (2016-09-08 18:57:41 UTC) #67

So this still LGTM modulo the various comment and DCHECK suggestions I made.

Matt, one question: The final thing that I mind about this structure is the
requirement that if FilterData() returns 0 (indicating that it can't do anything
without more input data) it must have read everything from the input buffer;
i.e. it must buffer any extra information.  I'd rather have an interface where
that buffering was the superclass' responsibility, i.e. in which any data not
read was re-presented to the subclass on the next FilterData() call, even if
FilterData() indicated it needed more data.

However, I couldn't come up with a way to do this using the IOBuffer abstraction
without copying the entire result of the read from the upstream SourceStream,
and while I don't mind copying the remaining unconsumed bytes (I figure one of
the sub- or super- class has to do that) I didn't want to put an extra copy for
the large majority of bytes.  But it doesn't look like there's a way to wrap an
IOBuffer to allow the upstream to Read() into it starting at an offset, which is
what I think would be required.  Do you know of such functionality, or see
another way to do this?

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
File net/filter/filter_source_stream.cc (right):

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:143: next_state_ = STATE_READ_DATA;
nit, suggestion: I know it's redundant with the DCHECK in DoReadData(), but I'd
sorta like to have a DCHECK here confirming that all data's been drained from
drainable_input_buffer_, just so a failure will be close to the cause of the
failure and linked to it?  But up to you.

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:144: // TODO(xunjieli): if NeedMoreData() but
eof is reached, emit an error.
I don't think this is accurate, though I totally see where the confusion comes
from and maybe that means we should update the documentation.

The use case I see "NeedsMoreData()" as targeting is for the rare filter that
has output all its going to output no matter what the upstream provides.  SDCH's
meta-refresh is an (ugly) example; once SDCH decides that it can't decode, it
outputs meta-refresh and has no need to read anything else, so will start
returning false from NeedsMoreData().

Most filters will just default "NeedsMoreData()" to true, meaning "If upstream
has more data for me, I can send more data to downstream".  The filters that
want to say "Upstream has no more data for me?  That's an error" will be
returning errors from FilterData when EOF is signalled.

I think that this is reasonably clear from the header documentation, but if you
disagree, please feel free to update it (or if you disagree with this analysis,
let me know, though that generally goes without saying :-}).

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.h:70: // FilterData() return 0, at which point,
FilterSourceStream will read more
Should we not document the requirement that all data must have been read from
input_buffer if FilterData() returns zero?

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.h:71: // input data from |upstream_|. Upstream
EOF is reached when
nit: "... when FilterData() is called with |input_buffer->BytesRemaining() == 0|
(to distinguish from the above case when the subclass has drained all the
bytes).

Also, should we say something about FilterData being called repeatedly in this
case until 0 (EOF) or an error is returned from the subclass?

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.h:73: // TODO(xunjieli): consider allowing
asynchronous decompression.
nit, suggestion: "consider allowing asynchronous response via callback, to
support off-thread decompression"?

xunjieli

Matt, do you want to take a look before I CQ this? https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_source_stream.cc File net/filter/filter_source_stream.cc ...

4 years, 3 months ago (2016-09-08 19:11:43 UTC) #68

Matt, do you want to take a look before I CQ this?

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
File net/filter/filter_source_stream.cc (right):

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:143: next_state_ = STATE_READ_DATA;
On 2016/09/08 18:57:41, Randy Smith - Not in Fridays wrote:
> nit, suggestion: I know it's redundant with the DCHECK in DoReadData(), but
I'd
> sorta like to have a DCHECK here confirming that all data's been drained from
> drainable_input_buffer_, just so a failure will be close to the cause of the
> failure and linked to it?  But up to you.

Done.

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.cc:144: // TODO(xunjieli): if NeedMoreData() but
eof is reached, emit an error.
On 2016/09/08 18:57:41, Randy Smith - Not in Fridays wrote:
> I don't think this is accurate, though I totally see where the confusion comes
> from and maybe that means we should update the documentation.
> 
> The use case I see "NeedsMoreData()" as targeting is for the rare filter that
> has output all its going to output no matter what the upstream provides. 
SDCH's
> meta-refresh is an (ugly) example; once SDCH decides that it can't decode, it
> outputs meta-refresh and has no need to read anything else, so will start
> returning false from NeedsMoreData().
> 
> Most filters will just default "NeedsMoreData()" to true, meaning "If upstream
> has more data for me, I can send more data to downstream".  The filters that
> want to say "Upstream has no more data for me?  That's an error" will be
> returning errors from FilterData when EOF is signalled.
> 
> I think that this is reasonably clear from the header documentation, but if
you
> disagree, please feel free to update it (or if you disagree with this
analysis,
> let me know, though that generally goes without saying :-}).

Done.

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.h:70: // FilterData() return 0, at which point,
FilterSourceStream will read more
On 2016/09/08 18:57:41, Randy Smith - Not in Fridays wrote:
> Should we not document the requirement that all data must have been read from
> input_buffer if FilterData() returns zero?

Done.

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.h:71: // input data from |upstream_|. Upstream
EOF is reached when
On 2016/09/08 18:57:41, Randy Smith - Not in Fridays wrote:
> nit: "... when FilterData() is called with |input_buffer->BytesRemaining() ==
0|
> (to distinguish from the above case when the subclass has drained all the
> bytes).

Done.
> 
> Also, should we say something about FilterData being called repeatedly in this
> case until 0 (EOF) or an error is returned from the subclass?

The "repeated" part is in the first sentence of this paragraph.

https://codereview.chromium.org/2251853002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.h:73: // TODO(xunjieli): consider allowing
asynchronous decompression.
On 2016/09/08 18:57:41, Randy Smith - Not in Fridays wrote:
> nit, suggestion: "consider allowing asynchronous response via callback, to
> support off-thread decompression"?

Done.

xunjieli

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

4 years, 3 months ago (2016-09-08 19:11:52 UTC) #69

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/340001

4 years, 3 months ago (2016-09-08 19:12:34 UTC) #70

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 3 months ago (2016-09-08 20:19:13 UTC) #71

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 3 months ago (2016-09-08 20:19:14 UTC) #72

xunjieli

On 2016/09/08 19:11:43, xunjieli wrote: > Matt, do you want to take a look before ...

4 years, 3 months ago (2016-09-09 19:54:02 UTC) #73

xunjieli

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org Link to the ...

4 years, 3 months ago (2016-09-09 19:54:12 UTC) #75

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2251853002/340001

4 years, 3 months ago (2016-09-09 19:55:03 UTC) #76

commit-bot: I haz the power

Description was changed from ========== Add net::SourceStream and net::FilterSourceStream This CL adds two new classes ...

4 years, 3 months ago (2016-09-09 21:33:33 UTC) #77

commit-bot: I haz the power

Description was changed from ========== Add net::SourceStream and net::FilterSourceStream This CL adds two new classes ...

4 years, 3 months ago (2016-09-09 21:36:52 UTC) #79

commit-bot: I haz the power

Patchset 12 (id:??) landed as https://crrev.com/95ea25f5dd7e255a01efd1b10bd9250cbc3c0d69 Cr-Commit-Position: refs/heads/master@{#417716}

4 years, 3 months ago (2016-09-09 21:36:53 UTC) #80

Randy Smith (Not in Mondays)

My apologies for coming back to this CL after landing, but I think we should ...

4 years, 3 months ago (2016-09-12 21:05:03 UTC) #81

Message was sent while issue was closed.

My apologies for coming back to this CL after landing, but I think we should do
something about what looks to me like the inability for a filter to return an
error on upstream EOF signalling.  The key issue is the second comment; the
first is a target of opportunity suggestion :-}.  Let me know what you think.

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
net/filter/filter_source_stream.h:69: // FilterData() will be repeatedly invoked
with the same |input_buffer| until
Could we remove the phrase "with the same |input_buffer|"?  That seems like
something that we don't want the subclass to be counting on.  All we want to
guarantee is that we (superclass) won't lose any data they (subclass) haven't
drained.

(Suggestion: Matt raised the possibility of an interface where instead of
passing a DrainableIOBuffer, we pass an IOBuffer & size, so it's a pure input
argument rather than an input/output argument.  This would also have the benefit
of not allowing the possibility of the subclass assuming that we're passing the
same IOBuffer, since we wouldn't be--we'd rewrap on each call.  I like this
idea, but not with a deep passion, so just a suggestion.)

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
net/filter/filter_source_stream.h:72: // FilterData() is called with
|input_buffer->BytesRemaining() == 0|.
Helen, as I mentioned offline I think there's a problem with this interface, as
I don't think we can guarantee that FilterData() will never be called with
|input_buffer->BytesRemaining() == 0| in the non-EOF case.  If FilterData drains
everything from input_buffer, fills up output_buffer, and has stuff left over,
we need to provide some way for it to continue to output the data, which I think
means calling it again with input_buffer->BytesRemaining() == 0.  

Would it be reasonable to go back to the earlier suggestion of having a separate
interface for signaling EOF?  I'm inclined to make this dirt simple:

virtual int UpstreamEof() { return OK; }

And include in the interface contract that it can't be called until FilterData()
has returned 0 (indicating that it doesn't have any more data to return without
further input from the upstream).  In this case I think it's reasonable for
many/most subclasses not to bother to implement UpstreamEof(); they decompress
on the fly, and whenever the upstream is done, they are also done.  And if they
have more bufferred data to output, they'll output it on the previous calls to
FilterData().  WDYT?

mmenke

On 2016/09/12 21:05:03, Randy Smith - Not in Fridays wrote: > My apologies for coming ...

4 years, 3 months ago (2016-09-12 21:09:54 UTC) #82

Message was sent while issue was closed.

On 2016/09/12 21:05:03, Randy Smith - Not in Fridays wrote:
> My apologies for coming back to this CL after landing, but I think we should
do
> something about what looks to me like the inability for a filter to return an
> error on upstream EOF signalling.  The key issue is the second comment; the
> first is a target of opportunity suggestion :-}.  Let me know what you think.
> 
>
https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
> File net/filter/filter_source_stream.h (right):
> 
>
https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
> net/filter/filter_source_stream.h:69: // FilterData() will be repeatedly
invoked
> with the same |input_buffer| until
> Could we remove the phrase "with the same |input_buffer|"?  That seems like
> something that we don't want the subclass to be counting on.  All we want to
> guarantee is that we (superclass) won't lose any data they (subclass) haven't
> drained.
> 
> (Suggestion: Matt raised the possibility of an interface where instead of
> passing a DrainableIOBuffer, we pass an IOBuffer & size, so it's a pure input
> argument rather than an input/output argument.  This would also have the
benefit
> of not allowing the possibility of the subclass assuming that we're passing
the
> same IOBuffer, since we wouldn't be--we'd rewrap on each call.  I like this
> idea, but not with a deep passion, so just a suggestion.)
> 
>
https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
> net/filter/filter_source_stream.h:72: // FilterData() is called with
> |input_buffer->BytesRemaining() == 0|.
> Helen, as I mentioned offline I think there's a problem with this interface,
as
> I don't think we can guarantee that FilterData() will never be called with
> |input_buffer->BytesRemaining() == 0| in the non-EOF case.  If FilterData
drains
> everything from input_buffer, fills up output_buffer, and has stuff left over,
> we need to provide some way for it to continue to output the data, which I
think
> means calling it again with input_buffer->BytesRemaining() == 0.  
> 
> Would it be reasonable to go back to the earlier suggestion of having a
separate
> interface for signaling EOF?  I'm inclined to make this dirt simple:
> 
> virtual int UpstreamEof() { return OK; }
> 
> And include in the interface contract that it can't be called until
FilterData()
> has returned 0 (indicating that it doesn't have any more data to return
without
> further input from the upstream).  In this case I think it's reasonable for
> many/most subclasses not to bother to implement UpstreamEof(); they decompress
> on the fly, and whenever the upstream is done, they are also done.  And if
they
> have more bufferred data to output, they'll output it on the previous calls to
> FilterData().  WDYT?

I think it would be cleaner to add an is_eof bit that is passed to the
subclasses FileData method, rather than another virtual function call.

Randy Smith (Not in Mondays)

On 2016/09/12 21:09:54, mmenke wrote: > On 2016/09/12 21:05:03, Randy Smith - Not in Fridays ...

4 years, 3 months ago (2016-09-12 21:13:25 UTC) #83

Message was sent while issue was closed.

On 2016/09/12 21:09:54, mmenke wrote:
> On 2016/09/12 21:05:03, Randy Smith - Not in Fridays wrote:
> > My apologies for coming back to this CL after landing, but I think we should
> do
> > something about what looks to me like the inability for a filter to return
an
> > error on upstream EOF signalling.  The key issue is the second comment; the
> > first is a target of opportunity suggestion :-}.  Let me know what you
think.
> > 
> >
>
https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
> > File net/filter/filter_source_stream.h (right):
> > 
> >
>
https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
> > net/filter/filter_source_stream.h:69: // FilterData() will be repeatedly
> invoked
> > with the same |input_buffer| until
> > Could we remove the phrase "with the same |input_buffer|"?  That seems like
> > something that we don't want the subclass to be counting on.  All we want to
> > guarantee is that we (superclass) won't lose any data they (subclass)
haven't
> > drained.
> > 
> > (Suggestion: Matt raised the possibility of an interface where instead of
> > passing a DrainableIOBuffer, we pass an IOBuffer & size, so it's a pure
input
> > argument rather than an input/output argument.  This would also have the
> benefit
> > of not allowing the possibility of the subclass assuming that we're passing
> the
> > same IOBuffer, since we wouldn't be--we'd rewrap on each call.  I like this
> > idea, but not with a deep passion, so just a suggestion.)
> > 
> >
>
https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
> > net/filter/filter_source_stream.h:72: // FilterData() is called with
> > |input_buffer->BytesRemaining() == 0|.
> > Helen, as I mentioned offline I think there's a problem with this interface,
> as
> > I don't think we can guarantee that FilterData() will never be called with
> > |input_buffer->BytesRemaining() == 0| in the non-EOF case.  If FilterData
> drains
> > everything from input_buffer, fills up output_buffer, and has stuff left
over,
> > we need to provide some way for it to continue to output the data, which I
> think
> > means calling it again with input_buffer->BytesRemaining() == 0.  
> > 
> > Would it be reasonable to go back to the earlier suggestion of having a
> separate
> > interface for signaling EOF?  I'm inclined to make this dirt simple:
> > 
> > virtual int UpstreamEof() { return OK; }
> > 
> > And include in the interface contract that it can't be called until
> FilterData()
> > has returned 0 (indicating that it doesn't have any more data to return
> without
> > further input from the upstream).  In this case I think it's reasonable for
> > many/most subclasses not to bother to implement UpstreamEof(); they
decompress
> > on the fly, and whenever the upstream is done, they are also done.  And if
> they
> > have more bufferred data to output, they'll output it on the previous calls
to
> > FilterData().  WDYT?
> 
> I think it would be cleaner to add an is_eof bit that is passed to the
> subclasses FileData method, rather than another virtual function call.

No objection--I went with the extra function in the hopes that most subclasses
could avoid implementing it, and they can't avoid declaring another argument. 
But they can avoid testing it :-}.

xunjieli

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_source_stream.h File net/filter/filter_source_stream.h (right): https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_source_stream.h#newcode69 net/filter/filter_source_stream.h:69: // FilterData() will be repeatedly invoked with the same ...

4 years, 3 months ago (2016-09-13 19:45:25 UTC) #84

Message was sent while issue was closed.

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
net/filter/filter_source_stream.h:69: // FilterData() will be repeatedly invoked
with the same |input_buffer| until
On 2016/09/12 21:05:03, Randy Smith - Not in Fridays wrote:
> Could we remove the phrase "with the same |input_buffer|"?  That seems like
> something that we don't want the subclass to be counting on.  All we want to
> guarantee is that we (superclass) won't lose any data they (subclass) haven't
> drained.
> 
I added this comment per an earlier review commment. I agree this is an
implementation detail, which probably shouldn't be in the method documentation.
However, I feel that this is an important detail of the limitation of the
current design. I will keep it unless you feel strongly about removing it.

> (Suggestion: Matt raised the possibility of an interface where instead of
> passing a DrainableIOBuffer, we pass an IOBuffer & size, so it's a pure input
> argument rather than an input/output argument.  This would also have the
benefit
> of not allowing the possibility of the subclass assuming that we're passing
the
> same IOBuffer, since we wouldn't be--we'd rewrap on each call.  I like this
> idea, but not with a deep passion, so just a suggestion.)

Acknowledged. I will keep this as it is. I like the DrainableIOBuffer so
subclasses can have multiple routines to consume bytes from the buffer without
keeping track of the offsets themselves. You can see gzip_source_stream.cc for
an example. I will keep this as it is.

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
net/filter/filter_source_stream.h:72: // FilterData() is called with
|input_buffer->BytesRemaining() == 0|.
On 2016/09/12 21:05:03, Randy Smith - Not in Fridays wrote:
> Helen, as I mentioned offline I think there's a problem with this interface,
as
> I don't think we can guarantee that FilterData() will never be called with
> |input_buffer->BytesRemaining() == 0| in the non-EOF case.  If FilterData
drains
> everything from input_buffer, fills up output_buffer, and has stuff left over,
> we need to provide some way for it to continue to output the data, which I
think
> means calling it again with input_buffer->BytesRemaining() == 0.  
> 
> Would it be reasonable to go back to the earlier suggestion of having a
separate
> interface for signaling EOF?  I'm inclined to make this dirt simple:
> 
> virtual int UpstreamEof() { return OK; }
> 
> And include in the interface contract that it can't be called until
FilterData()
> has returned 0 (indicating that it doesn't have any more data to return
without
> further input from the upstream).  In this case I think it's reasonable for
> many/most subclasses not to bother to implement UpstreamEof(); they decompress
> on the fly, and whenever the upstream is done, they are also done.  And if
they
> have more bufferred data to output, they'll output it on the previous calls to
> FilterData().  WDYT?

Done. I will upload this change as a separate CL.

mmenke

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_source_stream.h File net/filter/filter_source_stream.h (right): https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_source_stream.h#newcode69 net/filter/filter_source_stream.h:69: // FilterData() will be repeatedly invoked with the same ...

4 years, 3 months ago (2016-09-13 19:50:57 UTC) #85

Message was sent while issue was closed.

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
net/filter/filter_source_stream.h:69: // FilterData() will be repeatedly invoked
with the same |input_buffer| until
On 2016/09/13 19:45:24, xunjieli wrote:
> On 2016/09/12 21:05:03, Randy Smith - Not in Fridays wrote:
> > Could we remove the phrase "with the same |input_buffer|"?  That seems like
> > something that we don't want the subclass to be counting on.  All we want to
> > guarantee is that we (superclass) won't lose any data they (subclass)
haven't
> > drained.
> > 
> I added this comment per an earlier review commment. I agree this is an
> implementation detail, which probably shouldn't be in the method
documentation.
> However, I feel that this is an important detail of the limitation of the
> current design. I will keep it unless you feel strongly about removing it.
> 
> > (Suggestion: Matt raised the possibility of an interface where instead of
> > passing a DrainableIOBuffer, we pass an IOBuffer & size, so it's a pure
input
> > argument rather than an input/output argument.  This would also have the
> benefit
> > of not allowing the possibility of the subclass assuming that we're passing
> the
> > same IOBuffer, since we wouldn't be--we'd rewrap on each call.  I like this
> > idea, but not with a deep passion, so just a suggestion.)
> 
> Acknowledged. I will keep this as it is. I like the DrainableIOBuffer so
> subclasses can have multiple routines to consume bytes from the buffer without
> keeping track of the offsets themselves. You can see gzip_source_stream.cc for
> an example. I will keep this as it is.

Don't think you understand the comment - the parent class would track the
offset, we'd just remove the weird contract that we always return the same
drainable IO buffer, which the subclass is responsible for managing itself.

i.e, we'd do something like:

FilterData(..., in_buffer, in_buffer_size, &consumed_bytes);
which then reads part of the data and sets consumed_bytes to if (say it's set to
8).

Then the next call to FilterData would return another IOBuffer, with 8 fewer
bytes in it (i.e., the parent class would completely manage the
DrainableIOBuffer).

xunjieli

4 years, 3 months ago (2016-09-13 19:54:59 UTC) #86

Message was sent while issue was closed.

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2251853002/diff/340001/net/filter/filter_sour...
net/filter/filter_source_stream.h:69: // FilterData() will be repeatedly invoked
with the same |input_buffer| until
On 2016/09/13 19:50:57, mmenke wrote:
> On 2016/09/13 19:45:24, xunjieli wrote:
> > On 2016/09/12 21:05:03, Randy Smith - Not in Fridays wrote:
> > > Could we remove the phrase "with the same |input_buffer|"?  That seems
like
> > > something that we don't want the subclass to be counting on.  All we want
to
> > > guarantee is that we (superclass) won't lose any data they (subclass)
> haven't
> > > drained.
> > > 
> > I added this comment per an earlier review commment. I agree this is an
> > implementation detail, which probably shouldn't be in the method
> documentation.
> > However, I feel that this is an important detail of the limitation of the
> > current design. I will keep it unless you feel strongly about removing it.
> > 
> > > (Suggestion: Matt raised the possibility of an interface where instead of
> > > passing a DrainableIOBuffer, we pass an IOBuffer & size, so it's a pure
> input
> > > argument rather than an input/output argument.  This would also have the
> > benefit
> > > of not allowing the possibility of the subclass assuming that we're
passing
> > the
> > > same IOBuffer, since we wouldn't be--we'd rewrap on each call.  I like
this
> > > idea, but not with a deep passion, so just a suggestion.)
> > 
> > Acknowledged. I will keep this as it is. I like the DrainableIOBuffer so
> > subclasses can have multiple routines to consume bytes from the buffer
without
> > keeping track of the offsets themselves. You can see gzip_source_stream.cc
for
> > an example. I will keep this as it is.
> 
> Don't think you understand the comment - the parent class would track the
> offset, we'd just remove the weird contract that we always return the same
> drainable IO buffer, which the subclass is responsible for managing itself.
> 
> i.e, we'd do something like:
> 
> FilterData(..., in_buffer, in_buffer_size, &consumed_bytes);
> which then reads part of the data and sets consumed_bytes to if (say it's set
to
> 8).
> 
> Then the next call to FilterData would return another IOBuffer, with 8 fewer
> bytes in it (i.e., the parent class would completely manage the
> DrainableIOBuffer).

Thanks for the follow-up. I will do that in a separate CL.

Issue 2251853002: Add net::SourceStream and net::FilterSourceStream (Closed)

Description

Patch Set 1 : Same as PS#18 of old CL but with upstream/downstream name change #

Patch Set 2 : Address Matt's comments #

Patch Set 3 : fix compile #

Patch Set 4 : fix tests #

Patch Set 5 : Fix a typo #

Patch Set 6 : address Matt's comments #

Patch Set 7 : self review #

Patch Set 8 : address Randy's comments (synced to r412509) #

Patch Set 9 : Add a method to allow filter to specify whether it is done #

Patch Set 10 : Have an explict Pull() method #

Patch Set 11 : PS#9 documentation TODOs (synced to r417301) #

Patch Set 12 : address comments #

Messages