Remove GetInputStreamBufferSize() method from FilterContext.

Remove GetInputStreamBufferSize() method from FilterContext.
This virtual method, implemented only by URLRequestJob and MockFilterContext,
was only used for testing purposes. The kFilterBufSize constant now lives
in filter.cc (the only place it was used), and for the few tests that needed
to override the buffer size, I've added a test-only method in filter.h.

The result is a smaller interface surface in URLRequestJob and simpler tests for most cases in gzip_filter_unittest.cc and sdch_filter_unittest.cc.  I've done some further refactoring of the former to remove redundancy (most of Filter's complexity is exercised only in the SDCH test).

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77315

[adamk, 2011-02-15 01:36:37]

Adding jam as a possible alternate reviewer, as Will mentioned that he was also
recently doing cleanup in the general area of URLRequest.

[willchan no longer on Chromium, 2011-02-15 01:42:25]

Dropped jam@. I think there may have been miscommunication over this, he's
probably not appropriate for this area of code. jar@ may be.

In any case, I just got through the rest of my review queue and will be looking
at this shortly.

[willchan no longer on Chromium, 2011-02-15 01:52:02]

This cleanup lgtm. But I think I'm going to defer to jar@ here.

[jar (doing other things), 2011-02-15 06:48:53]

Bottom line: I'd like to better understand why you're going in this
direction(?), and where you expect this to lead.

Details:

Having fought with the filter architecture, and worked to make the filters chain
together, I can certainly sympathize and agree that I didn't like the
architecture.  It was very hard to chain, and a lot of confusing logic was
needed to connect several filters (and to test the chaining).

That said, I *think* the goal was to make it possible to hand whole (fully
filled) buffers from the network stack to the filter input, without having to
copy them. Compared to all the processing that goes on, that copy was probably
the least of the issues, and was perchance a premature optimization.  This
change starts to take a step toward letting each filter handle their input
buffer allocation more directly, which seems reasonable.  

I would be really nice to some day have a cleaner chaining mechanism, and I'd
feel much better about this change if I was convinced it was heading towards
such a point.

I also have a specific concern about the details below.

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h
File net/base/filter.h (right):

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h#newcode180
net/base/filter.h:180: void ResetInputBufferForTest(int size) {
This strikes me as a bad or perilous interface for several reasons.  First off,
it adds an odd time-element (must call at a certain time after construction, and
before use), and discards constructor-time buffers.  

Secondly, from a purely C++ perspective, it bothers me that the class does not
have a singular definition, and some users (unit tests) view the class as having
this method, while other consumers (linked against this "same" class) would
assert that there is no such method. I suspect this is problematic for some
compilers, and it seems pretty unnecessary.  As an example, with an IDE, what
should the system display for the list of available methods?

  I'd much rather see the (currently proposed) const buffer size changed into a
non-const size, with an accessor that can be called at startup, and never again
reconsidered (during testing). 

It is IMO more desirable to change the size of the buffers once (in a test
scenario, at startup) and then leave it fixed for that duration of a run, than
to have to hack each and every instance to comply with this goal.  

I don't like the existing architecture that much, but I'm wary that this CL may
have worse elements.

[adamk, 2011-02-15 19:00:30]

On Mon, Feb 14, 2011 at 10:48 PM, <jar@chromium.org> wrote:

> Bottom line: I'd like to better understand why you're going in this
> direction(?), and where you expect this to lead.
>

Thanks for taking a look. Let me first explain the motivations that started
me down this road,
and then I'll address individual points below. I recently had the task of
implementing a new
URLRequestJob to support "filesystem:" URIs (a proposed part of the Files &
Directories spec
that Eric Urhane is writing). In doing so, I found the interface of
URLRequestJob rather unwieldy:
it was hard to figure out which methods I was supposed to implement, or what
they were supposed
to do. A good number of these came from FilterContext.  And
GetInputStreamBufferSize() happened
to be one of them that didn't really seem to have much reason to exist
(beyond a few tests).

So my motivation here was first and foremost to reduce the number of virtual
methods in URLRequestJob
that subclasses might have to implement.  A secondary goal (and a little
longer term) is to think of
a more comprehensible way to handle filters, but it wasn't my intention to
tackle that with this change,
and I'd be very happy to discuss that design with you in more detail.


> Details:
>
> Having fought with the filter architecture, and worked to make the filters
> chain
> together, I can certainly sympathize and agree that I didn't like the
> architecture.  It was very hard to chain, and a lot of confusing logic was
> needed to connect several filters (and to test the chaining).
>
> That said, I *think* the goal was to make it possible to hand whole (fully
> filled) buffers from the network stack to the filter input, without having
> to
> copy them. Compared to all the processing that goes on, that copy was
> probably
> the least of the issues, and was perchance a premature optimization.  This
> change starts to take a step toward letting each filter handle their input
> buffer allocation more directly, which seems reasonable.
>

If I read you right, you're saying that GetInputStreamBufferSize() was
intended
to be part of a larger optimization scheme that was never fully implemented?
As the code stands now, I couldn't see a reason why kFilterBufSize shouldn't
just live in Filter, since that was the only place it was used.


> I would be really nice to some day have a cleaner chaining mechanism, and
> I'd
> feel much better about this change if I was convinced it was heading
> towards
> such a point.
>
> I also have a specific concern about the details below.
>
>
> http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h
> File net/base/filter.h (right):
>
>
> http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h#newcode180
> net/base/filter.h:180: void ResetInputBufferForTest(int size) {
> This strikes me as a bad or perilous interface for several reasons.
> First off, it adds an odd time-element (must call at a certain time
> after construction, and before use), and discards constructor-time
> buffers.
>
> Secondly, from a purely C++ perspective, it bothers me that the class
> does not have a singular definition, and some users (unit tests) view
> the class as having this method, while other consumers (linked against
> this "same" class) would assert that there is no such method. I suspect
> this is problematic for some compilers, and it seems pretty unnecessary.
>  As an example, with an IDE, what should the system display for the list
> of available methods?
>

I agree that this is rather odd, and I only implemented this way as I'd
recently
seen several other examples of Chromium code that do similar things.


>  I'd much rather see the (currently proposed) const buffer size changed
> into a non-const size, with an accessor that can be called at startup,
> and never again reconsidered (during testing).
>
> It is IMO more desirable to change the size of the buffers once (in a
> test scenario, at startup) and then leave it fixed for that duration of
> a run, than to have to hack each and every instance to comply with this
> goal.
>

Can you explain a bit more about what you mean by "startup"? Most of the
tests don't care what the buffer size is, but a few want to make sure the
buffer
is large, and a few want to make sure it's small. I could make the buffer
size a
static referenced inside the constructors, with a way to change it globally,
but then I'd really like a way to make sure that only gets modified in
tests.


>
> I don't like the existing architecture that much, but I'm wary that this
> CL may have worse elements.
>
>
> http://codereview.chromium.org/6516025/
>

[adamk, 2011-02-22 23:32:47]

Hi Jim,

Can you take another look at this?

Thanks,
Adam

On Tue, Feb 15, 2011 at 11:00 AM, Adam Klein <adamk@chromium.org> wrote:

> On Mon, Feb 14, 2011 at 10:48 PM, <jar@chromium.org> wrote:
>
>> Bottom line: I'd like to better understand why you're going in this
>> direction(?), and where you expect this to lead.
>>
>
> Thanks for taking a look. Let me first explain the motivations that started
> me down this road,
> and then I'll address individual points below. I recently had the task of
> implementing a new
> URLRequestJob to support "filesystem:" URIs (a proposed part of the Files &
> Directories spec
> that Eric Urhane is writing). In doing so, I found the interface of
> URLRequestJob rather unwieldy:
> it was hard to figure out which methods I was supposed to implement, or
> what they were supposed
> to do. A good number of these came from FilterContext.  And
> GetInputStreamBufferSize() happened
> to be one of them that didn't really seem to have much reason to exist
> (beyond a few tests).
>
> So my motivation here was first and foremost to reduce the number of
> virtual methods in URLRequestJob
> that subclasses might have to implement.  A secondary goal (and a little
> longer term) is to think of
> a more comprehensible way to handle filters, but it wasn't my intention to
> tackle that with this change,
> and I'd be very happy to discuss that design with you in more detail.
>
>
>> Details:
>>
>> Having fought with the filter architecture, and worked to make the filters
>> chain
>> together, I can certainly sympathize and agree that I didn't like the
>> architecture.  It was very hard to chain, and a lot of confusing logic was
>> needed to connect several filters (and to test the chaining).
>>
>> That said, I *think* the goal was to make it possible to hand whole (fully
>> filled) buffers from the network stack to the filter input, without having
>> to
>> copy them. Compared to all the processing that goes on, that copy was
>> probably
>> the least of the issues, and was perchance a premature optimization.  This
>> change starts to take a step toward letting each filter handle their input
>> buffer allocation more directly, which seems reasonable.
>>
>
> If I read you right, you're saying that GetInputStreamBufferSize() was
> intended
> to be part of a larger optimization scheme that was never fully
> implemented?
> As the code stands now, I couldn't see a reason why kFilterBufSize
> shouldn't
> just live in Filter, since that was the only place it was used.
>
>
>> I would be really nice to some day have a cleaner chaining mechanism, and
>> I'd
>> feel much better about this change if I was convinced it was heading
>> towards
>> such a point.
>>
>> I also have a specific concern about the details below.
>>
>>
>> http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h
>> File net/base/filter.h (right):
>>
>>
>> http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h#newcode180
>> net/base/filter.h:180: void ResetInputBufferForTest(int size) {
>> This strikes me as a bad or perilous interface for several reasons.
>> First off, it adds an odd time-element (must call at a certain time
>> after construction, and before use), and discards constructor-time
>> buffers.
>>
>> Secondly, from a purely C++ perspective, it bothers me that the class
>> does not have a singular definition, and some users (unit tests) view
>> the class as having this method, while other consumers (linked against
>> this "same" class) would assert that there is no such method. I suspect
>> this is problematic for some compilers, and it seems pretty unnecessary.
>>  As an example, with an IDE, what should the system display for the list
>> of available methods?
>>
>
> I agree that this is rather odd, and I only implemented this way as I'd
> recently
> seen several other examples of Chromium code that do similar things.
>
>
>>  I'd much rather see the (currently proposed) const buffer size changed
>> into a non-const size, with an accessor that can be called at startup,
>> and never again reconsidered (during testing).
>>
>> It is IMO more desirable to change the size of the buffers once (in a
>> test scenario, at startup) and then leave it fixed for that duration of
>> a run, than to have to hack each and every instance to comply with this
>> goal.
>>
>
> Can you explain a bit more about what you mean by "startup"? Most of the
> tests don't care what the buffer size is, but a few want to make sure the
> buffer
> is large, and a few want to make sure it's small. I could make the buffer
> size a
> static referenced inside the constructors, with a way to change it
> globally,
> but then I'd really like a way to make sure that only gets modified in
> tests.
>
>
>>
>> I don't like the existing architecture that much, but I'm wary that this
>> CL may have worse elements.
>>
>>
>> http://codereview.chromium.org/6516025/
>>
>
>

[jar (doing other things), 2011-02-25 18:37:24]

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.cc
File net/base/filter.cc (right):

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.cc#newcode38
net/base/filter.cc:38: const int kFilterBufSize = 32 * 1024;
As per comment in filter.h: Please make this non-const, and probably a static
member of the class.  Please provide a static accessor (perhaps private?  used
only by the testing infrastructure?) to vary this value for testing purposes.

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h
File net/base/filter.h (right):

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h#newcode180
net/base/filter.h:180: void ResetInputBufferForTest(int size) {
I spoke with others on the Chrome team about this pattern.  we all eventually
agreed it is a bad pattern, and should not be used (re: in a single executable,
we should NOT have two or more different definitions of a class.).   It is
probably non-portable, opposed by the C++ standard, and fraught with peril when
a virtual method is added/removed, or a slot is added/removed (even at the end!)
with packing optimization enabled.  It may be problematic with various IDE's,
etc. etc.

Please don't use the pattern.  In this case, please remove the #if
defined(UNIT_TEST).

The good news is that the linker would probably discard the unused code in a
production release (since the method is only referenced in a test module).  I
assume that was the underlying optimization you were after anyway.

I also don't like this as an interface for test.  Discarding and recreating a
buffer seems like a significant variation in performance of the class.  It seems
like an unnecessary difference from the "regular use/flow of the class."

I'd like to see an] testing interface which changes the otherwise constant
buffer size.  That interface, presumably through a static method, can be called
before constructing an instance, and then the standard constructor paths can be
used.


On 2011/02/15 06:48:53, jar wrote:
> This strikes me as a bad or perilous interface for several reasons.  First
off,
> it adds an odd time-element (must call at a certain time after construction,
and
> before use), and discards constructor-time buffers.  
> 
> Secondly, from a purely C++ perspective, it bothers me that the class does not
> have a singular definition, and some users (unit tests) view the class as
having
> this method, while other consumers (linked against this "same" class) would
> assert that there is no such method. I suspect this is problematic for some
> compilers, and it seems pretty unnecessary.  As an example, with an IDE, what
> should the system display for the list of available methods?
> 
>   I'd much rather see the (currently proposed) const buffer size changed into
a
> non-const size, with an accessor that can be called at startup, and never
again
> reconsidered (during testing). 
> 
> It is IMO more desirable to change the size of the buffers once (in a test
> scenario, at startup) and then leave it fixed for that duration of a run, than
> to have to hack each and every instance to comply with this goal.  
> 
> I don't like the existing architecture that much, but I'm wary that this CL
may
> have worse elements.

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h#newcode226
net/base/filter.h:226: scoped_refptr<net::IOBuffer> stream_buffer_;
Please consider whether this can be changed to be an actual stream buffer,
rather than a scoped_refptr.

Given the progression in this CL, it seems doubtful that it is handed around, or
changed.... but I haven't carefully checked all uses in derived classes to see
if this is manipulated as a pointer.

[adamk, 2011-02-25 20:48:13]

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h
File net/base/filter.h (right):

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h#newcode180
net/base/filter.h:180: void ResetInputBufferForTest(int size) {
On 2011/02/25 18:37:24, jar wrote:
> I spoke with others on the Chrome team about this pattern.  we all eventually
> agreed it is a bad pattern, and should not be used (re: in a single
executable,
> we should NOT have two or more different definitions of a class.).   It is
> probably non-portable, opposed by the C++ standard, and fraught with peril
when
> a virtual method is added/removed, or a slot is added/removed (even at the
end!)
> with packing optimization enabled.  It may be problematic with various IDE's,
> etc. etc.
> 
> Please don't use the pattern.  In this case, please remove the #if
> defined(UNIT_TEST).
> 
> The good news is that the linker would probably discard the unused code in a
> production release (since the method is only referenced in a test module).  I
> assume that was the underlying optimization you were after anyway.
> 
> I also don't like this as an interface for test.  Discarding and recreating a
> buffer seems like a significant variation in performance of the class.  It
seems
> like an unnecessary difference from the "regular use/flow of the class."
> 
> I'd like to see an] testing interface which changes the otherwise constant
> buffer size.  That interface, presumably through a static method, can be
called
> before constructing an instance, and then the standard constructor paths can
be
> used.

I'm happy to get rid of the UNIT_TEST nonsense and use, say, a private method
while making the few tests that need access friends.  But I'm not convinced
there's an advantage to making the default (and production) buffer size
non-constant.  You cite "significant variation in performance". But surely using
a tiny buffer is a bigger variation than the cost of a few mallocs?

My worry with using a static is that it's easy for a test to forget to reset the
buffer size when the test is over, thus endangering the independence of each
test.  Since there are only 5 callsites where this needs to happen, it seems
more straightforward to me for them to replace the buffer in the
already-constructed Filter, thus avoiding the possibility of leaking local
modifications into other tests.
 
> 
> On 2011/02/15 06:48:53, jar wrote:
> > This strikes me as a bad or perilous interface for several reasons.  First
> off,
> > it adds an odd time-element (must call at a certain time after construction,
> and
> > before use), and discards constructor-time buffers.  
> > 
> > Secondly, from a purely C++ perspective, it bothers me that the class does
not
> > have a singular definition, and some users (unit tests) view the class as
> having
> > this method, while other consumers (linked against this "same" class) would
> > assert that there is no such method. I suspect this is problematic for some
> > compilers, and it seems pretty unnecessary.  As an example, with an IDE,
what
> > should the system display for the list of available methods?
> > 
> >   I'd much rather see the (currently proposed) const buffer size changed
into
> a
> > non-const size, with an accessor that can be called at startup, and never
> again
> > reconsidered (during testing). 
> > 
> > It is IMO more desirable to change the size of the buffers once (in a test
> > scenario, at startup) and then leave it fixed for that duration of a run,
than
> > to have to hack each and every instance to comply with this goal.  
> > 
> > I don't like the existing architecture that much, but I'm wary that this CL
> may
> > have worse elements.
>

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h#newcode226
net/base/filter.h:226: scoped_refptr<net::IOBuffer> stream_buffer_;
On 2011/02/25 18:37:24, jar wrote:
> Please consider whether this can be changed to be an actual stream buffer,
> rather than a scoped_refptr.

Not sure what you mean by "actual stream buffer".  It needs to be an IOBuffer
because that's exposed in the public interface (stream_buffer()), and IOBuffers
are refcounted.

> Given the progression in this CL, it seems doubtful that it is handed around,
or
> changed.... but I haven't carefully checked all uses in derived classes to see
> if this is manipulated as a pointer.

The stream buffer is passed down to URLRequestJob::ReadRawData(), so the
refcounting is actually used in those places where it's passed to something
doing async IO which needs the IOBuffer to live long enough to handle a read.

[jar (doing other things), 2011-02-26 01:36:52]

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h
File net/base/filter.h (right):

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h#newcode180
net/base/filter.h:180: void ResetInputBufferForTest(int size) {
Perhaps I chose the wrong words....

I was trying to say that this would be the only place (that  I could see) where
a the stream_buffer_ was discarded and replaced. The lifetime of the object
seems vastly different with that scenario.  When running tests that link
together filters, I'm wary of how that linking will be tested, given that the
common inter-filter-buffers may need to be discarded in order to test things.  I
felt that would cause the lifetime, as seen in tests, to be vastly different
from their use in production.  Perhaps I should have said "significant change in
the lifecycle of the instances" to use this testing interface.

In contrast, I think that if you set the sizes, and then ran tests, the instance
would probably function much more like the way they do in production.

Your concern about messing up global settings is very reasonable, but I think we
have to face that all over the place in testing.  Any time we change a global
value, or static value, we face this problem.  If you're concerned about making
it harder to err in setting/reseting the value (which is a good thing to worry
about!), it would be reasonable to have the setup/teardown in a Test class
handle the resetting to a default, and perhaps you could even have the Setter
restore to a "default" value when passed a buffer size of zero.  Such a
"default" would help to assure the setting was done properly.  If you are really
concerned, you could also use a counter or such, to ensure that no more than N
buffers were allocated with a "non-default" size.

Hopefully that explanation is clearer, and I apologize for using the word
"performance," it I think it may have obscured my concern.

On 2011/02/25 20:48:13, Adam Klein wrote:
> On 2011/02/25 18:37:24, jar wrote:
> > I spoke with others on the Chrome team about this pattern.  we all
eventually
> > agreed it is a bad pattern, and should not be used (re: in a single
> executable,
> > we should NOT have two or more different definitions of a class.).   It is
> > probably non-portable, opposed by the C++ standard, and fraught with peril
> when
> > a virtual method is added/removed, or a slot is added/removed (even at the
> end!)
> > with packing optimization enabled.  It may be problematic with various
IDE's,
> > etc. etc.
> > 
> > Please don't use the pattern.  In this case, please remove the #if
> > defined(UNIT_TEST).
> > 
> > The good news is that the linker would probably discard the unused code in a
> > production release (since the method is only referenced in a test module). 
I
> > assume that was the underlying optimization you were after anyway.
> > 
> > I also don't like this as an interface for test.  Discarding and recreating
a
> > buffer seems like a significant variation in performance of the class.  It
> seems
> > like an unnecessary difference from the "regular use/flow of the class."
> > 
> > I'd like to see an] testing interface which changes the otherwise constant
> > buffer size.  That interface, presumably through a static method, can be
> called
> > before constructing an instance, and then the standard constructor paths can
> be
> > used.
> 
> I'm happy to get rid of the UNIT_TEST nonsense and use, say, a private method
> while making the few tests that need access friends.  But I'm not convinced
> there's an advantage to making the default (and production) buffer size
> non-constant.  You cite "significant variation in performance". But surely
using
> a tiny buffer is a bigger variation than the cost of a few mallocs?
> 
> My worry with using a static is that it's easy for a test to forget to reset
the
> buffer size when the test is over, thus endangering the independence of each
> test.  Since there are only 5 callsites where this needs to happen, it seems
> more straightforward to me for them to replace the buffer in the
> already-constructed Filter, thus avoiding the possibility of leaking local
> modifications into other tests.
>  
> > 
> > On 2011/02/15 06:48:53, jar wrote:
> > > This strikes me as a bad or perilous interface for several reasons.  First
> > off,
> > > it adds an odd time-element (must call at a certain time after
construction,
> > and
> > > before use), and discards constructor-time buffers.  
> > > 
> > > Secondly, from a purely C++ perspective, it bothers me that the class does
> not
> > > have a singular definition, and some users (unit tests) view the class as
> > having
> > > this method, while other consumers (linked against this "same" class)
would
> > > assert that there is no such method. I suspect this is problematic for
some
> > > compilers, and it seems pretty unnecessary.  As an example, with an IDE,
> what
> > > should the system display for the list of available methods?
> > > 
> > >   I'd much rather see the (currently proposed) const buffer size changed
> into
> > a
> > > non-const size, with an accessor that can be called at startup, and never
> > again
> > > reconsidered (during testing). 
> > > 
> > > It is IMO more desirable to change the size of the buffers once (in a test
> > > scenario, at startup) and then leave it fixed for that duration of a run,
> than
> > > to have to hack each and every instance to comply with this goal.  
> > > 
> > > I don't like the existing architecture that much, but I'm wary that this
CL
> > may
> > > have worse elements.
> > 
>

http://codereview.chromium.org/6516025/diff/4001/net/base/filter.h#newcode226
net/base/filter.h:226: scoped_refptr<net::IOBuffer> stream_buffer_;
OK.  I hadn't seen places where the ownership was assumed by anyone else (as I
said... I didn't look far).

On 2011/02/25 20:48:13, Adam Klein wrote:
> On 2011/02/25 18:37:24, jar wrote:
> > Please consider whether this can be changed to be an actual stream buffer,
> > rather than a scoped_refptr.
> 
> Not sure what you mean by "actual stream buffer".  It needs to be an IOBuffer
> because that's exposed in the public interface (stream_buffer()), and
IOBuffers
> are refcounted.
> 
> > Given the progression in this CL, it seems doubtful that it is handed
around,
> or
> > changed.... but I haven't carefully checked all uses in derived classes to
see
> > if this is manipulated as a pointer.
> 
> The stream buffer is passed down to URLRequestJob::ReadRawData(), so the
> refcounting is actually used in those places where it's passed to something
> doing async IO which needs the IOBuffer to live long enough to handle a read.

http://codereview.chromium.org/6516025/diff/15001/net/base/sdch_filter_unitte...
File net/base/sdch_filter_unittest.cc (right):

http://codereview.chromium.org/6516025/diff/15001/net/base/sdch_filter_unitte...
net/base/sdch_filter_unittest.cc:857: ASSERT_GT(kLargeInputBufferSize,
expanded_.size());
It is OK to use asserts, but it was not clear why you changed them here.  

I'm only inclined to use an assert if code will segv if it continues (or
infinite loop, etc.).  If that is not a risk, it always seems preferable to use
an EXPECT so that we can get as much additional data in the failure scenario as
possible.

Also note that these premature returns are cases where it is probably important
to have teardown of the test class put things right (and I *think* you get that
from the test classes).

http://codereview.chromium.org/6516025/diff/15001/net/base/sdch_filter_unitte...
net/base/sdch_filter_unittest.cc:895:
SdchFilterChainingTest::ResetInputBuffer(filter.get(), kLargeInputBufferSize);
You could probably put this into the teardown of your test class, so you can't
accidentally exit and fail to restore it.

[adamk, 2011-03-01 01:53:09]

One more shot, this time with an approach that very closely matches the previous
code.  I've added a new test-only factory method that takes a buffer size, and
re-instituted the InitBuffer() method (simplified) that's called immediately
after initialization.

In re-implementing InitBuffer, I really wanted to tighten up Filter::Factory,
and so I've made the GZipFilter and SdchFilter constructors private, with Filter
as a friend.  If this bothers you, I can switch it back; it just struck me as
the right thing to do.

I've also done some refactoring in the Gzip test to get rid of lots of
redundancy.

[jar (doing other things), 2011-03-01 22:44:02]

See small comments below.

http://codereview.chromium.org/6516025/diff/17001/net/base/gzip_filter_unitte...
File net/base/gzip_filter_unittest.cc (right):

http://codereview.chromium.org/6516025/diff/17001/net/base/gzip_filter_unitte...
net/base/gzip_filter_unittest.cc:261: deflate_encode_len_);
With this code not being explicit about buffer size, the above code (which I
didn't write) shows off the use of memcpy() without an assert or checking.  

Could you add one:
ASSERT_GT(filter_->stream_buffer().size() > deflate_encode_len_);

IMO, the same assert should preface any memcpy() calls like this below.

http://codereview.chromium.org/6516025/diff/17001/net/base/sdch_filter.h
File net/base/sdch_filter.h (right):

http://codereview.chromium.org/6516025/diff/17001/net/base/sdch_filter.h#newc...
net/base/sdch_filter.h:57: explicit SdchFilter(const FilterContext&
filter_context);
The moving of these constructors to being private is good.

Thanks!

http://codereview.chromium.org/6516025/diff/17001/net/base/sdch_filter_unitte...
File net/base/sdch_filter_unittest.cc (left):

http://codereview.chromium.org/6516025/diff/17001/net/base/sdch_filter_unitte...
net/base/sdch_filter_unittest.cc:196: EXPECT_EQ(kInputBufferSize,
input_buffer_size);
This test appears to (among other things) test that the requested input buffer
size is indeed obtained.  Please add that test somewhere (it works well here),
but it is critical to know that we are indeed modulating the size as needed.

[adamk, 2011-03-01 23:13:53]

Added some sanity checks as requested: buffers are big enough for memcpy(), and,
when we request a size, we actually get that size.

http://codereview.chromium.org/6516025/diff/17001/net/base/gzip_filter_unitte...
File net/base/gzip_filter_unittest.cc (right):

http://codereview.chromium.org/6516025/diff/17001/net/base/gzip_filter_unitte...
net/base/gzip_filter_unittest.cc:261: deflate_encode_len_);
On 2011/03/01 22:44:03, jar wrote:
> With this code not being explicit about buffer size, the above code (which I
> didn't write) shows off the use of memcpy() without an assert or checking.  
> 
> Could you add one:
> ASSERT_GT(filter_->stream_buffer().size() > deflate_encode_len_);
> 
> IMO, the same assert should preface any memcpy() calls like this below.

I've added this assert to InitBuffer().  There are already ASSERTs in SetUp()
that these member buffers are <= kDefaultBufferSize; same goes for those tests
that call DecodeAllWithFilter().

http://codereview.chromium.org/6516025/diff/17001/net/base/sdch_filter_unitte...
File net/base/sdch_filter_unittest.cc (left):

http://codereview.chromium.org/6516025/diff/17001/net/base/sdch_filter_unitte...
net/base/sdch_filter_unittest.cc:196: EXPECT_EQ(kInputBufferSize,
input_buffer_size);
On 2011/03/01 22:44:03, jar wrote:
> This test appears to (among other things) test that the requested input buffer
> size is indeed obtained.  Please add that test somewhere (it works well here),
> but it is critical to know that we are indeed modulating the size as needed.

This test doesn't actually seem care how big the buffer is, as far as I can
tell, only that it's big enough to hold the input data.  And so it no longer
specifies a size and uses the default buffer size.

If what you want me to test is that Filter::FactoryForTests() is appropriately
setting the buffer size, it seems to me that those tests belong with the tests
that care about the buffer size, and I've added them in the four tests (three in
GZip, one in this file).

If I'm mistaken, and it's actually important that this test be run with a
30-byte buffer, please point that out.

[adamk, 2011-03-07 19:00:59]

Jim, do you have time to take another look at this?

Thanks!

[jar (doing other things), 2011-03-08 02:39:01]

LGTM

http://codereview.chromium.org/6516025/diff/17001/net/base/sdch_filter_unitte...
File net/base/sdch_filter_unittest.cc (left):

http://codereview.chromium.org/6516025/diff/17001/net/base/sdch_filter_unitte...
net/base/sdch_filter_unittest.cc:196: EXPECT_EQ(kInputBufferSize,
input_buffer_size);
You're correct.  This and the next few tests don't use the buffer... but it is
always nice to use a somewhat inductive set of tests.  Initially, you test basic
items, often as simple as whether a constructor sets args (or such as buffer
being as expected), and later you test complicated interactions, and don't need
to re-test basic elements.

It is equally fine to put the checks everywhere... I just noticed the change
looking at the delta (and I personally like the inductive buliding... but it is
your choice).

On 2011/03/01 23:13:54, Adam Klein wrote:
> On 2011/03/01 22:44:03, jar wrote:
> > This test appears to (among other things) test that the requested input
buffer
> > size is indeed obtained.  Please add that test somewhere (it works well
here),
> > but it is critical to know that we are indeed modulating the size as needed.
> 
> This test doesn't actually seem care how big the buffer is, as far as I can
> tell, only that it's big enough to hold the input data.  And so it no longer
> specifies a size and uses the default buffer size.
> 
> If what you want me to test is that Filter::FactoryForTests() is appropriately
> setting the buffer size, it seems to me that those tests belong with the tests
> that care about the buffer size, and I've added them in the four tests (three
in
> GZip, one in this file).
> 
> If I'm mistaken, and it's actually important that this test be run with a
> 30-byte buffer, please point that out.