Allow server to indicate a non-SDCH encoding

Allow server to indicate a non-SDCH encoding

When a server decides to not SDCH encode content
despite having an applicable dictionary, it can
send a header:
X-Sdch-Encode: 0
to tell the browser the lack of encoding was not
caused by a transit corruption (bad proxy?).

r=kmixter
bug=78489
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=84102

[kmixter1, 2011-05-04 17:01:05]

LGTM

http://codereview.chromium.org/6909033/diff/6001/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6909033/diff/6001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:395: // definately employ an SDCH filter
(or tentative sdch filter) when we get
s/definately/definitely/.  Maybe want to update this to mention the new header.

[jar (doing other things), 2011-05-04 17:08:20]

Wan Teh: Can you take a look at this patch?

It is pretty simple.  The critical point is that the SDCH error recovery looks
at "context" to decide if it has to plausibly deal with an SDCH issue.  When the
server absolutely asserts (via an HTTP header) that it is not an SDCH response,
then we can drop our conservative assumption that this should have been an SDCH
encoded response.

The underlying logic does not change... we just get to avoid this
ultra-conservative error recovery path (later on).

[wtc, 2011-05-04 17:22:36]

http://codereview.chromium.org/6909033/diff/6001/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6909033/diff/6001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:104: virtual void SetSdchResponse(bool
active);
This method should not be listed in this group because you
didn't add this method to the FilterContext interface.

This method doesn't need to be virtual.

Please document this method.

http://codereview.chromium.org/6909033/diff/6001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:149: job_->sdch_dictionary_advertised_ =
false;
Typo: false => active

Or perhaps you just need a ResetSdchResponse() method that
takes no argument?

[jar (doing other things), 2011-05-04 17:24:50]

Changes per wtc and kmixter comments.

http://codereview.chromium.org/6909033/diff/6001/net/url_request/url_request_...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6909033/diff/6001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:104: virtual void SetSdchResponse(bool
active);
On 2011/05/04 17:22:36, wtc wrote:
> This method should not be listed in this group because you
> didn't add this method to the FilterContext interface.
> 
> This method doesn't need to be virtual.
> 
> Please document this method.

Done.

http://codereview.chromium.org/6909033/diff/6001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:149: job_->sdch_dictionary_advertised_ =
false;
On 2011/05/04 17:22:36, wtc wrote:
> Typo: false => active
> 
> Or perhaps you just need a ResetSdchResponse() method that
> takes no argument?

Done.

http://codereview.chromium.org/6909033/diff/6001/net/url_request/url_request_...
net/url_request/url_request_http_job.cc:395: // definately employ an SDCH filter
(or tentative sdch filter) when we get
On 2011/05/04 17:01:05, kmixter1 wrote:
> s/definately/definitely/.  Maybe want to update this to mention the new
header.

Done.

[wtc, 2011-05-04 18:27:00]

LGTM.

http://codereview.chromium.org/6909033/diff/13001/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6909033/diff/13001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:110: // to an explicit HTTP header.
Nit: this comment doesn't even mention SDCH.

http://codereview.chromium.org/6909033/diff/13001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:155: DCHECK(!active);
It's strange to assert that a function argument can only
take a particular value.  You should just provide a method
that sets job_->sdch_dictionary_advertised_ to false.

[jar (doing other things), 2011-05-04 18:31:51]

Changes per comments by wtc.

http://codereview.chromium.org/6909033/diff/13001/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/6909033/diff/13001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:110: // to an explicit HTTP header.
On 2011/05/04 18:27:00, wtc wrote:
> Nit: this comment doesn't even mention SDCH.

Done.

http://codereview.chromium.org/6909033/diff/13001/net/url_request/url_request...
net/url_request/url_request_http_job.cc:155: DCHECK(!active);
On 2011/05/04 18:27:00, wtc wrote:
> It's strange to assert that a function argument can only
> take a particular value.  You should just provide a method
> that sets job_->sdch_dictionary_advertised_ to false.

Done.

[wtc, 2011-05-04 18:33:08]

LGTM!

[commit-bot: I haz the power, 2011-05-04 19:15:17]

Try job failure for 6909033-12002 on mac:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...