Remove GetInputStreamBufferSize() method from FilterContext.

net/base/filter.h

 // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // Filter performs filtering on data streams. Sample usage:
 //
 //   IStream* pre_filter_source;
 //   ...
 //   Filter* filter = Filter::Factory(filter_type, size);
 //   int pre_filter_data_len = filter->stream_buffer_size();
@@ skipping 20 matching lines @@
 #pragma once
 
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #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
 // filters, there is only one owner of all the chained filters, and that context
 // is passed to the constructor of all those filters.  To be clear, the context
 // does NOT reflect the position in a chain, or the fact that there are prior
 // or later filters in a chain.
 class FilterContext {
  public:
   // Enum to control what histograms are emitted near end-of-life of this
@@ skipping 29 matching lines @@
   virtual bool IsSdchResponse() const = 0;
 
   // How many bytes were read from the net or cache so far (and potentially
   // pushed into a filter for processing)?
   virtual int64 GetByteReadCount() const = 0;
 
   // What response code was received with the associated network transaction?
   // 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;
 };
 
 //------------------------------------------------------------------------------
 class Filter {
  public:
   // Return values of function ReadFilteredData.
   enum FilterStatus {
@@ skipping 63 matching lines @@
   // start filtering. Then after this function is called, the caller can get
   // post-filtered data using ReadFilteredData. The caller must not write to
   // stream_buffer_ and call this function again before stream_buffer_ is
   // emptied out by ReadFilteredData.
   //
   // The input stream_data_len is the length (in number of chars) of valid
   // data in stream_buffer_. It can not be greater than stream_buffer_size_.
   // 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);
 
   // Given a array of encoding_types, try to do some error recovery adjustment
   // to the list.  This includes handling known bugs in the Apache server (where
   // redundant gzip encoding is specified), as well as issues regarding SDCH
   // encoding, where various proxies and anti-virus products modify or strip the
   // encodings.  These fixups require context, which includes whether this
   // response was made to an SDCH request (i.e., an available dictionary was
@@ skipping 20 matching lines @@
   virtual FilterStatus ReadFilteredData(char* dest_buffer, int* dest_len);
 
   // Copy pre-filter data directly to destination buffer without decoding.
   FilterStatus CopyOut(char* dest_buffer, int* dest_len);
 
   FilterStatus last_status() const { return last_status_; }
 
   const FilterContext& filter_context() const { return filter_context_; }
 
   // Buffer to hold the data to be filtered (the input queue).
   scoped_refptr<net::IOBuffer> stream_buffer_;

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

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]

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.
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]

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:

 
   // Maximum size of stream_buffer_ in number of chars.
   int stream_buffer_size_;
 
   // Pointer to the next data in stream_buffer_ to be filtered.
   char* next_stream_data_;
 
   // Total number of remaining chars in stream_buffer_ to be filtered.
   int stream_data_len_;
 
  private:
   // A factory helper for creating filters for within a chain of potentially
   // multiple encodings.  If a chain of filters is created, then this may be
   // called multiple times during the filter creation process.  In most simple
   // cases, this is only called once. Returns NULL and cleans up (deleting
   // filter_list) if a new filter can't be constructed.
   static Filter* PrependNewFilter(FilterType type_id,
                                   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();
 
   // An optional filter to process output from this filter.
   scoped_ptr<Filter> next_filter_;
   // Remember what status or local filter last returned so we can better handle
   // chained filters.
   FilterStatus last_status_;
 
   // Context data from the owner of this filter.  Some filters need additional
   // context information (mime type, etc.) to properly function, and they access
   // this data via this reference member.
   const FilterContext& filter_context_;
 
   DISALLOW_COPY_AND_ASSIGN(Filter);
 };
 
 #endif  // NET_BASE_FILTER_H__