Clean up filter and content encoding handling ...

Clean up filter and content encoding handling 

Centralize translation functions (text of "Content-Encoding" to enum) 
in filter.cc

Centralize error recovery (for damaged content encoding headers) in filter.cc
Error recovery includes a loss of SDCH encoding headers, plus handling
of Apache server bug with gzip files are tagged as also being gzip encoded.

Centralize and add a pile of unit tests to this filter code.

r=openvcdiff,huanr

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

[jar (doing other things), 2008-10-23 18:39:38]

This is predominantly a mechanical refactoring to facilitate
testing, and centralize error recovery (from header
corruption).

I also included flag settings to make our histograms upload
via UMA, so that I'd be gathering stats on the various
recovery actions.

Huan- You might be most interested in the testing in
filter_unittest.cc, as well as the refactoring of the
error recovery code. You had previously asked that some
of the test code move to a new filter_unittest.cc as well.

Lincoln- Some of this cleanup was in response to your
requests in prior CLs.  This includes more tests, and
enhanced testability.

Ken- The only really interesting function for you is
probably the Fixup...() method in filter.cc.  That is
where the bulk of the attempt to handle bogus
content-encoding specs resides.  Again, I'm copying you
mostly because you asked to be listed... and your review
is not critical (but it is very welcome!!!)

Thanks in advance,

Jim

[Lincoln, 2008-10-23 23:29:48]

Great changes.  Really nice work.  Hurrah for new unit tests and for knocking
out some TODOs.

http://codereview.chromium.org/8018/diff/263/271
File net/base/filter.cc (right):

http://codereview.chromium.org/8018/diff/263/271#newcode14
Line 14: // Filter types (using cannonical lower case only):
tiny nit: cannonical => canonical

http://codereview.chromium.org/8018/diff/263/271#newcode77
Line 77: void Filter::FixupEncodingTypes(bool is_sdch_response,
style nit: Since you can't align the other two arguments with the first one (the
third argument is too long to do so), move the first argument onto the following
line with a four-space indent.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

http://codereview.chromium.org/8018/diff/263/271#newcode99
Line 99: // Some proxies (found currently in Argentina) strip the gzip encoding,
and
This comment doesn't quite describe the proxies' behavior.  They strip the
Content-Encoding header while leaving the message body gzip-compressed.  To my
eyes, the comment seems to imply that the proxy actually gunzips the message
body before sending it to the client.  This is probably just a matter of using
more specific wording in the comment.

http://codereview.chromium.org/8018/diff/263/271#newcode128
Line 128: else if (encoding_types->size() == 1)
You could switch the operands to be consistent with line 80.

http://codereview.chromium.org/8018/diff/263/271#newcode140
Line 140: // to propose a dicitonary etc., but current SDCH spec does not
provide a nice
typo: dicitonary => dictionary
I write this same typo all the time  :-)

http://codereview.chromium.org/8018/diff/263/265
File net/base/gzip_filter_unittest.cc (left):

http://codereview.chromium.org/8018/diff/263/265#oldcode397
Line 397: TEST_F(GZipUnitTest, ApacheWorkaround) {
Add a comment block for this function, since the test name doesn't sufficiently
describe the expected behavior that the test is enforcing.

http://codereview.chromium.org/8018/diff/263/268
File net/base/sdch_filter.cc (right):

http://codereview.chromium.org/8018/diff/263/268#newcode189
Line 189: // Since dictionary was not found, check to see if has was even
plausible.
typo: has => hash

http://codereview.chromium.org/8018/diff/263/269
File net/base/sdch_manager.cc (right):

http://codereview.chromium.org/8018/diff/263/269#newcode61
Line 61: FieldTrialList::application_start_time() - Time::Now());
Are these backwards?  Seems like this would always produce a negative time
difference.

http://codereview.chromium.org/8018/diff/263/270
File net/base/sdch_manager.h (right):

http://codereview.chromium.org/8018/diff/263/270#newcode69
Line 69: DICTIONARY_FOUND_HAS_WRONG_DOMAIN = 10,
Won't this change all the problem code assignments below this point, and so
cause a mismatch with any histogram data generated by existing dev-channel
clients?  Perhaps it should be "= 11" instead.  Or is the new UmaHistogram
completely separate from the previous Histogram?

http://codereview.chromium.org/8018/diff/263/273
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/8018/diff/263/273#newcode165
Line 165: bool
URLRequestHttpJob::GetContentEncodings(std::vector<Filter::FilterType>*
I would probably put the line break right after the parenthesis, as in the
previous version of this code.  Optional, but I think it would help readability.

http://codereview.chromium.org/8018/diff/263/274
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/8018/diff/263/274#newcode44
Line 44: virtual bool GetContentEncodings(std::vector<Filter::FilterType>*
I would probably put the line break right after the parenthesis, with a 4-space
indent on the following line.  Optional, but I think it would help readability.

http://codereview.chromium.org/8018/diff/263/275
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/8018/diff/263/275#newcode117
Line 117: // content.
nit: trailing whitespace
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whi...

[jar (doing other things), 2008-10-24 00:31:53]

This includes responses and changes for all of Lincoln's comments.

Jim

http://codereview.chromium.org/8018/diff/263/271
File net/base/filter.cc (right):

http://codereview.chromium.org/8018/diff/263/271#newcode14
Line 14: // Filter types (using cannonical lower case only):
On 2008/10/23 23:29:48, Lincoln wrote:
> tiny nit: cannonical => canonical

Done.

http://codereview.chromium.org/8018/diff/263/271#newcode77
Line 77: void Filter::FixupEncodingTypes(bool is_sdch_response,
On 2008/10/23 23:29:48, Lincoln wrote:
> style nit: Since you can't align the other two arguments with the first one
(the
> third argument is too long to do so), move the first argument onto the
following
> line with a four-space indent.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Calls

Done.

http://codereview.chromium.org/8018/diff/263/271#newcode99
Line 99: // Some proxies (found currently in Argentina) strip the gzip encoding,
and
On 2008/10/23 23:29:48, Lincoln wrote:
> This comment doesn't quite describe the proxies' behavior.  They strip the
> Content-Encoding header while leaving the message body gzip-compressed.  To my
> eyes, the comment seems to imply that the proxy actually gunzips the message
> body before sending it to the client.  This is probably just a matter of using
> more specific wording in the comment.

Done.

http://codereview.chromium.org/8018/diff/263/271#newcode128
Line 128: else if (encoding_types->size() == 1)
On 2008/10/23 23:29:48, Lincoln wrote:
> You could switch the operands to be consistent with line 80.

Done.

http://codereview.chromium.org/8018/diff/263/271#newcode140
Line 140: // to propose a dicitonary etc., but current SDCH spec does not
provide a nice
On 2008/10/23 23:29:48, Lincoln wrote:
> typo: dicitonary => dictionary
> I write this same typo all the time  :-)

Done.

http://codereview.chromium.org/8018/diff/263/265
File net/base/gzip_filter_unittest.cc (left):

http://codereview.chromium.org/8018/diff/263/265#oldcode397
Line 397: TEST_F(GZipUnitTest, ApacheWorkaround) {
On 2008/10/23 23:29:48, Lincoln wrote:
> Add a comment block for this function, since the test name doesn't
sufficiently
> describe the expected behavior that the test is enforcing.

This is the old code which has since moved to filter_unittest.cc, and has been
changed.  Please see if comments there are sufficient.

http://codereview.chromium.org/8018/diff/263/268
File net/base/sdch_filter.cc (right):

http://codereview.chromium.org/8018/diff/263/268#newcode189
Line 189: // Since dictionary was not found, check to see if has was even
plausible.
On 2008/10/23 23:29:48, Lincoln wrote:
> typo: has => hash

Done.

http://codereview.chromium.org/8018/diff/263/269
File net/base/sdch_manager.cc (right):

http://codereview.chromium.org/8018/diff/263/269#newcode61
Line 61: FieldTrialList::application_start_time() - Time::Now());
On 2008/10/23 23:29:48, Lincoln wrote:
> Are these backwards?  Seems like this would always produce a negative time
> difference.

Done.

http://codereview.chromium.org/8018/diff/263/270
File net/base/sdch_manager.h (right):

http://codereview.chromium.org/8018/diff/263/270#newcode69
Line 69: DICTIONARY_FOUND_HAS_WRONG_DOMAIN = 10,
On 2008/10/23 23:29:48, Lincoln wrote:
> Won't this change all the problem code assignments below this point, and so
> cause a mismatch with any histogram data generated by existing dev-channel
> clients?  Perhaps it should be "= 11" instead.  Or is the new UmaHistogram
> completely separate from the previous Histogram?

Up until this check-in, I was careful to avoid setting the UMA upload flags
(either via the UMA_ prefix, or manually setting the flag in an underlying
histogram instance).  As a result, the stats have not yet begun to be gathered
via UMA.  I tend to prefer to experiment and look at my test machine for a while
before setting that flag, precisely so that I can make such finalizing changes
as shown above.

http://codereview.chromium.org/8018/diff/263/273
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/8018/diff/263/273#newcode165
Line 165: bool
URLRequestHttpJob::GetContentEncodings(std::vector<Filter::FilterType>*
On 2008/10/23 23:29:48, Lincoln wrote:
> I would probably put the line break right after the parenthesis, as in the
> previous version of this code.  Optional, but I think it would help
readability.

Done.

http://codereview.chromium.org/8018/diff/263/274
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/8018/diff/263/274#newcode44
Line 44: virtual bool GetContentEncodings(std::vector<Filter::FilterType>*
On 2008/10/23 23:29:48, Lincoln wrote:
> I would probably put the line break right after the parenthesis, with a
4-space
> indent on the following line.  Optional, but I think it would help
readability.

Done.

http://codereview.chromium.org/8018/diff/263/275
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/8018/diff/263/275#newcode117
Line 117: // content.
On 2008/10/23 23:29:48, Lincoln wrote:
> nit: trailing whitespace
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Horizontal_Whi...

Done.

[Lincoln, 2008-10-24 00:39:25]

LGTM

[huanr, 2008-10-24 22:55:42]

http://codereview.chromium.org/8018/diff/70/312
File net/base/filter.cc (right):

http://codereview.chromium.org/8018/diff/70/312#newcode86
Line 86: LowerCaseEqualsASCII(mime_type, kApplicationXGunzip))
this seems a behavior change. previously we check mime_type and apply work
around for every filer type in the chain. but now we only do it for the first
one?

http://codereview.chromium.org/8018/diff/70/317
File net/build/net_unittests.vcproj (right):

http://codereview.chromium.org/8018/diff/70/317#newcode330
Line 330: RelativePath="..\base\filter_unittest.cc"
did you add it in both debug and release settings?

[kmixter, 2008-10-24 23:27:11]

LGTM

[jar (doing other things), 2008-10-26 07:21:44]

Comments below in response to Huanr's comments.

Thanks,

Jim

http://codereview.chromium.org/8018/diff/70/312
File net/base/filter.cc (right):

http://codereview.chromium.org/8018/diff/70/312#newcode86
Line 86: LowerCaseEqualsASCII(mime_type, kApplicationXGunzip))
On 2008/10/24 22:55:42, huanr wrote:
> this seems a behavior change. previously we check mime_type and apply work
> around for every filer type in the chain. but now we only do it for the first
> one?

It is a slight behavior change.... 

The original code only supported at most 1 encoding filter.  Recall that a
stream of filters was not supported.  As a result, the "original" bug fix (found
in FireFox to deal with Apache) only processed a singleton encoding.  

This change restored (minimized?) the Apache patch to only work on the
singleton.  Unless we have evidence of it being needed more broadly, it seemed
best to leave it restricted.

http://codereview.chromium.org/8018/diff/70/317
File net/build/net_unittests.vcproj (right):

http://codereview.chromium.org/8018/diff/70/317#newcode330
Line 330: RelativePath="..\base\filter_unittest.cc"
On 2008/10/24 22:55:42, huanr wrote:
> did you add it in both debug and release settings?

I think there is only one place to add it (and it runs in both settings).  What
am I missing?