Give the filter setup more context so it can figure out whether it's download...

Give the filter setup more context so it can figure out whether it's downloading a file or not. Only refuse to gunzip svgz files on download, so we can send an uncompressed svgz file to webkit.

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

[Lei Zhang, 2009-04-07 19:12:10]

eseidel: Do you have any thoughts on svgz handling?
hclam: Am I using net::LOAD_ENABLE_DOWNLOAD_FILE appropriately?

Please add anyone you feel would make a good reviewer.

[Alpha Left Google, 2009-04-07 20:08:15]

> hclam: Am I using net::LOAD_ENABLE_DOWNLOAD_FILE appropriately?

LOAD_ENABLE_DOWNLOAD_FILE is actually a bad name for what it does, it is used to
hint http cache to provide native file handle to renderer process of the cached
file. It seems like this is not what you want to do..

[Lei Zhang, 2009-04-07 22:16:06]

On 2009/04/07 20:08:15, Alpha wrote:
> > hclam: Am I using net::LOAD_ENABLE_DOWNLOAD_FILE appropriately?
> 
> LOAD_ENABLE_DOWNLOAD_FILE is actually a bad name for what it does, it is used
to
> hint http cache to provide native file handle to renderer process of the
cached
> file. It seems like this is not what you want to do..

Ok, I added a comment for LOAD_ENABLE_DOWNLOAD_FILE, and added
LOAD_BY_DOWNLOAD_MANAGER.

Also added net unit tests.

[Lei Zhang, 2009-04-07 22:19:50]

+jar who wrote a lot of the filter code.

[jar (doing other things), 2009-04-07 23:54:21]

I wanted wtc to confirm that we're grabbing the load_flags() from request_ at
the right level in the network hierarchy.  He might suggest we move it up or
down, and perhaps just save it so that other flags can be pulled on demand (I
know we pull an SDCH related LOAD_* flag at another level).

Also I had a few small suggestions and questions listed below.

Thanks,

Jim

http://codereview.chromium.org/62111/diff/8/1014
File net/base/filter_unittest.cc (right):

http://codereview.chromium.org/62111/diff/8/1014#newcode202
Line 202: EXPECT_FALSE(encoding_types.empty());
Please change the above to an ASSERT_FALSE(), and then add an
EXPECT_EQUAL(...FILTER_TYPE_GZIP).

Please add the same change below at line 210 to the EXPECT_FALSE.

thx! ;-)

http://codereview.chromium.org/62111/diff/8/1013
File net/base/load_flags.h (right):

http://codereview.chromium.org/62111/diff/8/1013#newcode42
Line 42: // of the acched file to the renderer process.
nit: typo: "acched"

If by any chance the word should be "attached," then I couldn't really grok the
comment.  Please clarify.

http://codereview.chromium.org/62111/diff/8/1013#newcode78
Line 78: LOAD_BY_DOWNLOAD_MANAGER = 1 << 16,
What is the difference between LOAD_BY_DOWNLOAD_MANAGER and
LOAD_ENABLE_DOWNLOAD_FILE?  Are both downloads... and hence both need to be
handled by your other logic??

http://codereview.chromium.org/62111/diff/8/1007
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/62111/diff/8/1007#newcode34
Line 34: is_download_ = (request_->load_flags() & net::LOAD_BY_DOWNLOAD_MANAGER)
> 0;
I'd suggest != 0 instead.
The values are ints, but we're using them as bit fields, and  I'd rather not
assume that the bit you're grabbing isn't the  high-order (sign) bit.

http://codereview.chromium.org/62111/diff/8/1008
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/62111/diff/8/1008#newcode300
Line 300: bool is_download_;
Is this actually const across the lifetime of the instance?  If so, perhaps
declare it as const and initialize in the constructor initialization list.

It is also tempting to just save the request_.load_flags(), as that is what is
used in derived class to determine SDCH content.

I'm going to cc wtc, as he may have stronger feeling about where we should pull
these values out of request_.

[Lei Zhang, 2009-04-08 00:28:33]

http://codereview.chromium.org/62111/diff/8/1014
File net/base/filter_unittest.cc (right):

http://codereview.chromium.org/62111/diff/8/1014#newcode202
Line 202: EXPECT_FALSE(encoding_types.empty());
On 2009/04/07 23:54:21, jar wrote:
> Please change the above to an ASSERT_FALSE(), and then add an
> EXPECT_EQUAL(...FILTER_TYPE_GZIP).
> 
> Please add the same change below at line 210 to the EXPECT_FALSE.
> 
> thx! ;-)

I changed it to EXPECT_EQ(1U, encoding_types.size());

http://codereview.chromium.org/62111/diff/8/1013
File net/base/load_flags.h (right):

http://codereview.chromium.org/62111/diff/8/1013#newcode42
Line 42: // of the acched file to the renderer process.
On 2009/04/07 23:54:21, jar wrote:
> nit: typo: "acched"
> 
> If by any chance the word should be "attached," then I couldn't really grok
the
> comment.  Please clarify.

Whoops, should have been "cached". See hclam's earlier comment in this review.

http://codereview.chromium.org/62111/diff/8/1007
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/62111/diff/8/1007#newcode34
Line 34: is_download_ = (request_->load_flags() & net::LOAD_BY_DOWNLOAD_MANAGER)
> 0;
On 2009/04/07 23:54:21, jar wrote:
> I'd suggest != 0 instead.
> The values are ints, but we're using them as bit fields, and  I'd rather not
> assume that the bit you're grabbing isn't the  high-order (sign) bit.

Done.

http://codereview.chromium.org/62111/diff/8/1008
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/62111/diff/8/1008#newcode300
Line 300: bool is_download_;
On 2009/04/07 23:54:21, jar wrote:
> Is this actually const across the lifetime of the instance?  If so, perhaps
> declare it as const and initialize in the constructor initialization list.
> 
> It is also tempting to just save the request_.load_flags(), as that is what is
> used in derived class to determine SDCH content.
> 
> I'm going to cc wtc, as he may have stronger feeling about where we should
pull
> these values out of request_. 

Yes, it's const. I suppose we don't even need it and we can just return
GetLoadState() & net::LOAD_BY_DOWNLOAD_MANAGER in IsDownload(). If anyone has
strong feelings, let me know.

[Lei Zhang, 2009-04-08 19:42:24]

Whoops, I got load state and load flags confused. I changed UrlRequestJob to
grab the load flags.

[jar (doing other things), 2009-04-09 01:05:40]

I sent some email... but here are a few specific comments on the CL.

Thanks,

Jim

http://codereview.chromium.org/62111/diff/1027/24
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/62111/diff/1027/24#newcode528
Line 528: request->set_load_flags(request->load_flags() |
net::LOAD_BY_DOWNLOAD_MANAGER);
style nit: Please wrap the line at the bit-wise or.

http://codereview.chromium.org/62111/diff/1027/27
File net/base/filter_unittest.cc (right):

http://codereview.chromium.org/62111/diff/1027/27#newcode185
Line 185: EXPECT_EQ(1U, encoding_types.size());
Please change that EXPECT to an ASSERT, so that we don't crash on the next line
if the size() is zero.
I think this applies to the other similar EXPECTs as wel.

http://codereview.chromium.org/62111/diff/1027/27#newcode186
Line 186: EXPECT_EQ(Filter::FILTER_TYPE_SDCH, encoding_types.front());
typo: You probably meant FILTER_TYPE_GZIP (here and at lines  204 and 213 as
well).  The good news is that should have failed the try test.  If it didn't
fail... then something is wrong.  Be sure to run this in your build before
landing.

[Lei Zhang, 2009-04-09 18:04:24]

http://codereview.chromium.org/62111/diff/1027/24
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/62111/diff/1027/24#newcode528
Line 528: request->set_load_flags(request->load_flags() |
net::LOAD_BY_DOWNLOAD_MANAGER);
On 2009/04/09 01:05:40, jar wrote:
> style nit: Please wrap the line at the bit-wise or.

Done. (not used to msvc)

http://codereview.chromium.org/62111/diff/1027/27
File net/base/filter_unittest.cc (right):

http://codereview.chromium.org/62111/diff/1027/27#newcode185
Line 185: EXPECT_EQ(1U, encoding_types.size());
On 2009/04/09 01:05:40, jar wrote:
> Please change that EXPECT to an ASSERT, so that we don't crash on the next
line
> if the size() is zero.
> I think this applies to the other similar EXPECTs as wel.

I changed all of them in the file.

http://codereview.chromium.org/62111/diff/1027/27#newcode186
Line 186: EXPECT_EQ(Filter::FILTER_TYPE_SDCH, encoding_types.front());
On 2009/04/09 01:05:40, jar wrote:
> typo: You probably meant FILTER_TYPE_GZIP (here and at lines  204 and 213 as
> well).  The good news is that should have failed the try test.  If it didn't
> fail... then something is wrong.  Be sure to run this in your build before
> landing.

Typo fixed, it passes.

[wtc, 2009-04-09 23:19:23]

This CL gives me the impression that it is a hacky solution.
It fixes the bug, but I hope there is a better fix.

My suggestion below that you rename the new load flag
LOAD_DISABLE_GUNZIP_SVGZ really drives this point home --
we're telling the net module that a URLRequest is a
download, but that fact is irrelevant to the net module,
and its only effect is to suppress the gunzip of .svgz
files to work around some bug.

Perhaps I should study this bug to see if there is a
better fix.

I have some comments below, mostly style nits.  Thanks!

http://codereview.chromium.org/62111/diff/29/33
File net/base/filter.cc (right):

http://codereview.chromium.org/62111/diff/29/33#newcode111
Line 111: filter_context.IsDownload()) {
Nit: I know the style guide is ambiguous about this
situation -- I would align this line with the opening
parenthesis '(' of the previous line.

Actually, you can just do the following to avoid repeating
encoding_types->clear():

    if (0 == extension.compare(FILE_PATH_LITERAL(".gz")) ||
        0 == extension.compare(FILE_PATH_LITERAL(".tgz")) ||
        (0 == extension.compare(FILE_PATH_LITERAL(".svgz")) &&
        filter_context.IsDownload())) {
      encoding_types->clear();
    }

Please add a comment to explain why .svgz requires special
treatment.

http://codereview.chromium.org/62111/diff/29/36
File net/base/load_flags.h (right):

http://codereview.chromium.org/62111/diff/29/36#newcode43
Line 43: LOAD_ENABLE_DOWNLOAD_FILE = 1 << 7,
I'm actually not familiar with most of the load flags.
hclam added the LOAD_ENABLE_DOWNLOAD_FILE flag in r10972,
so he's the best person to advise on the meaning of
LOAD_ENABLE_DOWNLOAD_FILE and whether we should add the
LOAD_BY_DOWNLOAD_MANAGER.

I do think the name of the LOAD_ENABLE_DOWNLOAD_FILE flag
doesn't tell me what it means.  If this flag is not set,
what does the http cache provide to the renderer process?
Just bytes?  Perhaps this flag could be renamed
LOAD_RETURN_FILE_INSTEAD_OF_BYTES or LOAD_RETURN_FILE_HANDLE?

http://codereview.chromium.org/62111/diff/29/36#newcode78
Line 78: LOAD_BY_DOWNLOAD_MANAGER = 1 << 16,
Nit: to make the net module usable by other products, we
may want to avoid mention of Chromium-specific terms such
as "download manager".  For this purpose, even
LOAD_IS_DOWNLOAD would be better.

Another approach is to name this flag by its goal.  You're
using this flag to suppress gunzip of .svgz files, so
perhaps it could be named LOAD_DISABLE_GUNZIP_SVGZ.

http://codereview.chromium.org/62111/diff/29/30
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/62111/diff/29/30#newcode532
Line 532: bool URLRequestJob::IsDownload() const {
Nit: you may follow the suggestion of the style guide:
  Method definitions in the corresponding .cc file should
  be the same as the declaration order, as much as possible.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

[wtc, 2009-04-09 23:53:29]

Lei,

Just to clarify what I asked you to change:

1. Please rename the new load flag LOAD_IS_DOWNLOAD
(and update its comment) to avoid reference to the
download manager, which is outside the net module.

2. Please reformat the code in filter.cc as follows to
do avoid repeating encoding_types->clear():

    if (0 == extension.compare(FILE_PATH_LITERAL(".gz")) ||
        0 == extension.compare(FILE_PATH_LITERAL(".tgz")) ||
        (0 == extension.compare(FILE_PATH_LITERAL(".svgz")) &&
        filter_context.IsDownload())) {
      encoding_types->clear();
    }

3. Please add a comment to explain why .svgz requires
special treatment.

The other changes I suggested are optional and at your
discretion.

[Lei Zhang, 2009-04-10 00:57:13]

http://codereview.chromium.org/62111/diff/29/33
File net/base/filter.cc (right):

http://codereview.chromium.org/62111/diff/29/33#newcode111
Line 111: filter_context.IsDownload()) {
On 2009/04/09 23:19:24, wtc wrote:
> Nit: I know the style guide is ambiguous about this
> situation -- I would align this line with the opening
> parenthesis '(' of the previous line.
> 
> Actually, you can just do the following to avoid repeating
> encoding_types->clear():
> 
>     if (0 == extension.compare(FILE_PATH_LITERAL(".gz")) ||
>         0 == extension.compare(FILE_PATH_LITERAL(".tgz")) ||
>         (0 == extension.compare(FILE_PATH_LITERAL(".svgz")) &&
>         filter_context.IsDownload())) {
>       encoding_types->clear();
>     }
> 
> Please add a comment to explain why .svgz requires special
> treatment.
> 

Done.

http://codereview.chromium.org/62111/diff/29/36
File net/base/load_flags.h (right):

http://codereview.chromium.org/62111/diff/29/36#newcode43
Line 43: LOAD_ENABLE_DOWNLOAD_FILE = 1 << 7,
On 2009/04/09 23:19:24, wtc wrote:
> I'm actually not familiar with most of the load flags.
> hclam added the LOAD_ENABLE_DOWNLOAD_FILE flag in r10972,
> so he's the best person to advise on the meaning of
> LOAD_ENABLE_DOWNLOAD_FILE and whether we should add the
> LOAD_BY_DOWNLOAD_MANAGER.
> 
> I do think the name of the LOAD_ENABLE_DOWNLOAD_FILE flag
> doesn't tell me what it means.  If this flag is not set,
> what does the http cache provide to the renderer process?
> Just bytes?  Perhaps this flag could be renamed
> LOAD_RETURN_FILE_INSTEAD_OF_BYTES or LOAD_RETURN_FILE_HANDLE?

hclam: any thoughts on this? Do you want to give it a better name in a separate
CL?

http://codereview.chromium.org/62111/diff/29/36#newcode78
Line 78: LOAD_BY_DOWNLOAD_MANAGER = 1 << 16,
On 2009/04/09 23:19:24, wtc wrote:
> Nit: to make the net module usable by other products, we
> may want to avoid mention of Chromium-specific terms such
> as "download manager".  For this purpose, even
> LOAD_IS_DOWNLOAD would be better.

LOAD_IS_DOWNLOAD sounds good, done.

http://codereview.chromium.org/62111/diff/29/30
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/62111/diff/29/30#newcode532
Line 532: bool URLRequestJob::IsDownload() const {
On 2009/04/09 23:19:24, wtc wrote:
> Nit: you may follow the suggestion of the style guide:
>   Method definitions in the corresponding .cc file should
>   be the same as the declaration order, as much as possible.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Moved it to just above SetupFilter().

[Alpha Left Google, 2009-04-10 01:01:27]

Sorry for the delay, I just did more studies about this bug. First of all sorry
for the confusion about LOAD_ENABLE_DOWNLOAD_FILE, it's badly named I'm going to
refactor it in another CL and give it a better name.

As for another solution of fixing the bug here's my ideas:
1. Add DisableDecodeFiltering() to URLRequest
2. Remove '0 == extension.compare(FILE_PATH_LITERAL(".svgz")' from
Filter::FixupEncodingTypes(...)
3. In DownloadResourceHandler::OnResponseStarted we perform the logic of
checking ".svgz" and content-encoding and disable the decoding filtering from
there.

[wtc, 2009-04-10 18:05:41]

LGTM.  Thanks.  Please fix the two nits below.

Here are my thoughts on this CL and an alternative solution
I considered.

For a solution along this line, this CL is probably the
best we can do.  Ideally, I'd like to name the load flag
by its objective/effect:
    LOAD_DISABLE_CONTENT_DECODING_FOR_SVGZ

But it's weird to see SVGZ there.  The next choice is:
    LOAD_DISABLE_CONTENT_DECODING

To make this work, we may need to move the check for.svgz
from Filter::FixupEncodingTypes to
ResourceDispatcherHost::BeginDownload.  So this seems to be
a variant of the solution that hclam proposed.  What I
don't like about this approach is that the logic will be
split between Filter::FixupEncodingTypes and
ResourceDispatcherHost::BeginDownload.

http://codereview.chromium.org/62111/diff/5002/5006
File net/base/filter.cc (right):

http://codereview.chromium.org/62111/diff/5002/5006#newcode107
Line 107: // For the case of svgz files, we use the extension to distinguishing
Nit: "distinguishing" => "distinguish"

http://codereview.chromium.org/62111/diff/5002/5009
File net/base/load_flags.h (right):

http://codereview.chromium.org/62111/diff/5002/5009#newcode77
Line 77: // Indicate this request is for a download.
You may want to add "as opposed to viewing".

It'd be nice to describe the effect this flag has on the
URLRequest.

[Lei Zhang, 2009-04-10 19:28:09]

On 2009/04/10 18:05:41, wtc wrote:
> To make this work, we may need to move the check for.svgz
> from Filter::FixupEncodingTypes to
> ResourceDispatcherHost::BeginDownload.  So this seems to be
> a variant of the solution that hclam proposed.  What I
> don't like about this approach is that the logic will be
> split between Filter::FixupEncodingTypes and
> ResourceDispatcherHost::BeginDownload.

That was my thought too. I'd prefer to keep all the logic in one place. I'll fix
the nits and commit.

[jar (doing other things), 2009-04-10 20:27:14]

http://codereview.chromium.org/62111/diff/6012/5015
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/62111/diff/6012/5015#newcode127
Line 127: virtual bool IsSdchResponse() const { return false; }
One element that I think is questionable is the difference in implementation of
IsSdchResponse() and IsDownload().  Both methods are implemented for use only by
the filter.cc as a context for a derived class.  I suspect that IsDownload()
also "only makes sense for some types of requests."  

As I result, I'd suggest that IsDownload() should also have a trivial (return
false) definition here, and then should have a precise (variable) definition in
the class where it is applicable (probably along side of IsSdchResponse().

I had pulled WTC in to try to get a judgement call on where this should be done
(perhaps it should be the case that IsSdchResponse() should be fully defined
here... but I didn't think so).

WTC didn't weigh in strongly on this, but he agreed that these two methods
should be implemented using the same patter in the same places.

Which place do you think is better, and why?  If you don't have a justification
for being different from IsSdchResponse(), I'd suggest following its pattern.

[Lei Zhang, 2009-04-10 21:25:21]

http://codereview.chromium.org/62111/diff/6012/5015
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/62111/diff/6012/5015#newcode127
Line 127: virtual bool IsSdchResponse() const { return false; }
SDCH only makes sense for http right? To me, download is more general and
happens for many different types of requests.

[jar (doing other things), 2009-04-10 22:10:09]

http://codereview.chromium.org/62111/diff/6012/5015
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/62111/diff/6012/5015#newcode127
Line 127: virtual bool IsSdchResponse() const { return false; }
SDCH is currently outlawed by the spec for simplicity/security reasons in other
schemes (like ftp and https).  Conceptually, it could appear anywhere (i.e. The
general implementation could have been pushed to the base class... and it would
have returned false based on the flags exactly as your accessor gets a bool).

Note that in both cases, the only current consumer of these accessors are in
filter.cc, and hence it is only "really used"  (read: needs to be implemented)
in the derived class.

It is also interesting that the IsSdchResponse() could plausibly be renamed to
IsSdchRequest() (it really flags the fact that the request was a full sdch
request with an applicable dictionary), and IsDownload() might plausibly be
renamed IsDownloadRequest(), making them more clearly similar.

As a result, I go back and forth about where it is best to define these... but 
I suspect they should be defined next to each other, as they both derive from
LOAD_ flags. 


On 2009/04/10 21:25:21, Lei Zhang wrote:
> SDCH only makes sense for http right? To me, download is more general and
> happens for many different types of requests.