Remove GetInputStreamBufferSize() method from FilterContext.

net/base/filter.h

Index: net/base/filter.h
diff --git a/net/base/filter.h b/net/base/filter.h
index c6004e9d754385c96e419dd5fffc9d76947e68bb..b8788b8a2886a6a5b45cdb22475a947b0a23fae9 100644
--- a/net/base/filter.h
+++ b/net/base/filter.h
@@ -38,13 +38,10 @@
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "base/time.h"
+#include "net/base/io_buffer.h"
 
 class GURL;
 
-namespace net {
-class IOBuffer;
-}
-
 //------------------------------------------------------------------------------
 // Define an interface class that allows access to contextual information
 // supplied by the owner of this filter.  In the case where there are a chain of
@@ -94,13 +91,6 @@ class FilterContext {
   // For example: 200 is ok.   4xx are error codes. etc.
   virtual int GetResponseCode() const = 0;
 
-  // What is the desirable input buffer size for these filters?
-  // This value is currently supplied by the context, and is constant for all
-  // filters, even when they are part of a chain of filters. (i.e., we currently
-  // don't change the input buffer sizes for a linked chain of filters, and the
-  // buffer size for input to all filters in a chain is this one constant).
-  virtual int GetInputStreamBufferSize() const = 0;
-
   // The following method forces the context to emit a specific set of
   // statistics as selected by the argument.
   virtual void RecordPacketStats(StatisticSelector statistic) const = 0;
@@ -184,6 +174,15 @@ class Filter {
   // The function returns true if success, and false otherwise.
   bool FlushStreamBuffer(int stream_data_len);
 
+#if defined(UNIT_TEST)
+  // Resets the filter's internal buffer to the given size. Should only
+  // be called in unit tests, and only immediately after filter construction.
+  void ResetInputBufferForTest(int size) {

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

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.

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

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:

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

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.
 

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

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:

+    stream_buffer_ = new net::IOBuffer(size);
+    stream_buffer_size_ = size;
+  }
+#endif
+
   // Translate the text of a filter name (from Content-Encoding header) into a
   // FilterType.
   static FilterType ConvertEncodingToType(const std::string& filter_type);
@@ -245,11 +244,6 @@ class Filter {
                                   const FilterContext& filter_context,
                                   Filter* filter_list);
 
-  // Allocates and initializes stream_buffer_ based on filter_context_.
-  // Establishes a buffer large enough to handle the amount specified in
-  // filter_context_.GetInputStreamBufferSize().
-  bool InitBuffer();
-
   // Helper function to empty our output into the next filter's input.
   void PushDataIntoNextFilter();