RDH: Don't attempt to sniff the mime type of requests that...

RDH: Don't attempt to sniff the mime type of requests that
don't have a body.

BUG=16873
TEST=unittests


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

[abarth-chromium, 2009-08-27 02:46:39]

http://codereview.chromium.org/173536/diff/8/10
File chrome/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/173536/diff/8/10#newcode157
Line 157: response_->response_head.headers->response_code() == 204);
I'm not sure this condition is right.  For example, why can't we sniff a 404
response?

[rvargas (doing something else), 2009-08-27 02:59:31]

On 2009/08/27 02:46:39, abarth wrote:
> http://codereview.chromium.org/173536/diff/8/10
> File chrome/browser/renderer_host/buffered_resource_handler.cc (right):
> 
> http://codereview.chromium.org/173536/diff/8/10#newcode157
> Line 157: response_->response_head.headers->response_code() == 204);
> I'm not sure this condition is right.  For example, why can't we sniff a 404
> response?

To be totally honest, it just seemed simpler to write the code and fix it after
receiving feedback.

What do you think we should have here?

[abarth-chromium, 2009-08-27 03:10:49]

> To be totally honest, it just seemed simpler to write the code and fix it
after
> receiving feedback.
> 
> What do you think we should have here?

One option is to use 304 and 204 for here.  If the response really doesn't have
a body, we can make the sniffing algorithm always return the type_hint for zero
length buffers.

[rvargas (doing something else), 2009-08-28 18:21:47]

Please take another look.

[abarth-chromium, 2009-08-29 05:41:41]

LGTM.  Thanks.

[eroman, 2009-09-01 03:30:57]

http://codereview.chromium.org/173536/diff/2001/2003
File chrome/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/173536/diff/2001/2003#newcode156
Line 156: (response_->response_head.headers->response_code() == 304 ||
I'm not sure that it is appropriate to put HTTP-code specific stuff outside of
the net/http module (I know we do this in other places, but that seems wrong).

In particular, this seems like logic duplicated from
http_network_transaction.cc.

Moreover, I don't think this list is exhaustive. For example, HEAD requests can
never have a body, regardless of the HTTP status code.

See HttpNetworkTransaction::DidReadResponseHeaders() which tries to handle all
of these cases in setting up |response_body_length_|.

I think it would be better to unify these interpretations of HTTP codes --
either by using shared functions living in src/net/http, or by exporting the
information from the network module through some field in the response object.

(Sorry to pitch in late, only just got back from vacation).

[abarth-chromium, 2009-09-01 14:09:20]

On 2009/09/01 03:30:57, eroman wrote:
> I think it would be better to unify these interpretations of HTTP codes --
> either by using shared functions living in src/net/http, or by exporting the
> information from the network module through some field in the response object.

Yeah, that makes sense.  Can you file a bug about this so we don't forget?

Thanks,
Adam

[eroman, 2009-09-08 22:51:34]

On Tue, Sep 1, 2009 at 7:09 AM, <abarth@chromium.org> wrote:

> On 2009/09/01 03:30:57, eroman wrote:
>
>> I think it would be better to unify these interpretations of HTTP
>>
> codes --
>
>> either by using shared functions living in src/net/http, or by
>>
> exporting the
>
>> information from the network module through some field in the response
>>
> object.
>
> Yeah, that makes sense.  Can you file a bug about this so we don't
> forget?
>
> Thanks,
> Adam


Done. Filed http://code.google.com/p/chromium/issues/detail?id=21325


>
>
>
> http://codereview.chromium.org/173536
>