Use filter context to track stats better in SDCH filtering

wtc: please look at URL related code, and hooks and nits
you might have commented on before.

huanr: please look at sdch_filter code.  The intent was
no semantic change, and only change in histograms and
stats gathered.  I wanted to be sure I had better stats
on several failure cases, as the turn-around time of
adding stats to instrument such cases after they surface
is just too long.

The big feature is the mechanism for getting the total
number of bytes passed to a filter.  We use the filter
context to achieve this, and then the SDCH filter can
calculate compression ratio (from pre-gunzip vs post
SDCH decompress).

The number of bytes read was also histogrammed in a number
of error scenarios, to better diagnose what is going on
when these cases arrise (example: When some data is still
buffered in the VCDIFF decoder).

The sdch_filter destructor was getting long and hard
to read with multiple if blocks, so I cleaned that up
as well a bit (less indentation, and use of early returns).

Nits not included in previous CL that earlier are listed
as well.

r=wtc,huanr

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

[huanr, 2009-03-09 22:00:07]

LGTM

http://codereview.chromium.org/40319/diff/1/2
File net/base/filter.h (right):

http://codereview.chromium.org/40319/diff/1/2#newcode74
Line 74: virtual int GetInputStreambufferSize() const = 0;
Is this between filters or only relevant between the first filer and owner?

[jar (doing other things), 2009-03-10 00:38:48]

Comments added per Huanr's request.

http://codereview.chromium.org/40319/diff/1/2
File net/base/filter.h (right):

http://codereview.chromium.org/40319/diff/1/2#newcode74
Line 74: virtual int GetInputStreambufferSize() const = 0;
On 2009/03/09 22:00:07, huanr wrote:
> Is this between filters or only relevant between the first filer and owner?

I added more text to the comment.

[wtc, 2009-03-10 20:41:32]

LGTM.

http://codereview.chromium.org/40319/diff/12/1002
File net/base/filter.h (right):

http://codereview.chromium.org/40319/diff/12/1002#newcode78
Line 78: virtual int GetInputStreambufferSize() const = 0;
Nit: Could you capitalize "buffer"?

[wtc, 2009-03-13 02:32:56]

LGTM!

http://codereview.chromium.org/40319/diff/3015/3030
File chrome/browser/net/sdch_dictionary_fetcher.cc (left):

http://codereview.chromium.org/40319/diff/3015/3030#oldcode6
Line 6: 
PUT THIS BLANK LINE BACK!!!!!

http://codereview.chromium.org/40319/diff/3015/3016
File net/base/filter.h (left):

http://codereview.chromium.org/40319/diff/3015/3016#oldcode228
Line 228: private:   // TODO(jar): Make more data private by moving this up
higher.
YOU TOLD ME TO REMIND YOU TO REMOVE THIS TO DO!!!!!

http://codereview.chromium.org/40319/diff/3015/3027
File net/base/sdch_filter.cc (right):

http://codereview.chromium.org/40319/diff/3015/3027#newcode158
Line 158: // Already accounted for when set.
Looks nicer with a 'return' here, to be consistent with the
other cases.

Also, I'd use 'break' instead of 'return' in these cases
because this switch statement is already at the end of the
function.  But this is a matter of personal preference.

http://codereview.chromium.org/40319/diff/3015/3027#newcode341
Line 341: SdchManager::Dictionary* dictionary(NULL);
How about dictioary = NULL?

http://codereview.chromium.org/40319/diff/3015/3026
File net/base/sdch_filter_unittest.cc (right):

http://codereview.chromium.org/40319/diff/3015/3026#newcode754
Line 754: filter_context.SetSdchResponse(true);
THE CODE BECOMES MORE READABLE NOW!!!!!

http://codereview.chromium.org/40319/diff/3015/3021
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/40319/diff/3015/3021#newcode207
Line 207: Filter::FixupEncodingTypes(*this, encoding_types);
ISN'T IT NICE TO SEE SIMPLER CODE?????

http://codereview.chromium.org/40319/diff/3015/3020
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/40319/diff/3015/3020#newcode511
Line 511: metrics_->total_bytes_read_ += bytes_read;
metrics_->total_bytes_read_ should also be changed to int64
to be able to handle downloads of large files (> 4GB).

http://codereview.chromium.org/40319/diff/3015/3022
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/40319/diff/3015/3022#newcode322
Line 322: size_t filter_input_byte_count_;
This should be int64 because this is a cumulative byte
count and you may be downloading a DVDROM image.

[huanr, 2009-03-13 06:04:33]

Good change.

http://codereview.chromium.org/40319/diff/3015/3016
File net/base/filter.h (left):

http://codereview.chromium.org/40319/diff/3015/3016#oldcode47
Line 47: FilterContext() {};
Why remove this constructor and DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/40319/diff/3015/3027
File net/base/sdch_filter.cc (right):

http://codereview.chromium.org/40319/diff/3015/3027#newcode56
Line 56: filter_context().GetByteReadCount());
indent

[wtc, 2009-03-13 14:53:57]

http://codereview.chromium.org/40319/diff/3015/3016
File net/base/filter.h (left):

http://codereview.chromium.org/40319/diff/3015/3016#oldcode47
Line 47: FilterContext() {};
On 2009/03/13 06:04:33, huanr wrote:
> Why remove this constructor and DISALLOW_COPY_AND_ASSIGN?

I asked Jim to make this change to match how the other
"interface classes" in the net module are defined.

[huanr, 2009-03-13 15:53:33]

LGTM

[jar (doing other things), 2009-03-13 21:52:50]

http://codereview.chromium.org/40319/diff/3015/3030
File chrome/browser/net/sdch_dictionary_fetcher.cc (left):

http://codereview.chromium.org/40319/diff/3015/3030#oldcode6
Line 6: 
On 2009/03/13 02:32:56, wtc wrote:
> PUT THIS BLANK LINE BACK!!!!!

DONE :-)

http://codereview.chromium.org/40319/diff/3015/3016
File net/base/filter.h (left):

http://codereview.chromium.org/40319/diff/3015/3016#oldcode228
Line 228: private:   // TODO(jar): Make more data private by moving this up
higher.
On 2009/03/13 02:32:56, wtc wrote:
> YOU TOLD ME TO REMIND YOU TO REMOVE THIS TO DO!!!!!

Done.

http://codereview.chromium.org/40319/diff/3015/3027
File net/base/sdch_filter.cc (right):

http://codereview.chromium.org/40319/diff/3015/3027#newcode158
Line 158: // Already accounted for when set.
On 2009/03/13 02:32:56, wtc wrote:
> Looks nicer with a 'return' here, to be consistent with the
> other cases.
> 
> Also, I'd use 'break' instead of 'return' in these cases
> because this switch statement is already at the end of the
> function.  But this is a matter of personal preference.

Done.

http://codereview.chromium.org/40319/diff/3015/3027#newcode341
Line 341: SdchManager::Dictionary* dictionary(NULL);
On 2009/03/13 02:32:56, wtc wrote:
> How about dictioary = NULL?

Done.

http://codereview.chromium.org/40319/diff/3015/3026
File net/base/sdch_filter_unittest.cc (right):

http://codereview.chromium.org/40319/diff/3015/3026#newcode754
Line 754: filter_context.SetSdchResponse(true);
On 2009/03/13 02:32:56, wtc wrote:
> THE CODE BECOMES MORE READABLE NOW!!!!!

AND I'M NOT GONNA CHANGE IT!!! (yet)

http://codereview.chromium.org/40319/diff/3015/3021
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/40319/diff/3015/3021#newcode207
Line 207: Filter::FixupEncodingTypes(*this, encoding_types);
On 2009/03/13 02:32:56, wtc wrote:
> ISN'T IT NICE TO SEE SIMPLER CODE?????
Simple is better... and less is better...
...and if I had more time, I'd write even fewer lines of code!

http://codereview.chromium.org/40319/diff/3015/3020
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/40319/diff/3015/3020#newcode511
Line 511: metrics_->total_bytes_read_ += bytes_read;
On 2009/03/13 02:32:56, wtc wrote:
> metrics_->total_bytes_read_ should also be changed to int64
> to be able to handle downloads of large files (> 4GB).

I changed it, but ended up having to cast in the (seemingly) only case it was
used.  (in a printf style call).

http://codereview.chromium.org/40319/diff/3015/3022
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/40319/diff/3015/3022#newcode322
Line 322: size_t filter_input_byte_count_;
On 2009/03/13 02:32:56, wtc wrote:
> This should be int64 because this is a cumulative byte
> count and you may be downloading a DVDROM image.

Since this was ONLY for histogramming, when I changed it to a larger type, I
ended up needing casts.  I think it ended up being cleaner to use as a size_t.