Support for new data reduction proxy via header

Support for new data reduction proxy via header

This new header conforms to RFC 2616. With this change,
both the old and new header will be supported. Eventually,
support for the old header will be removed.

BUG=340333
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251256

[bengr, 2014-02-06 20:05:58]

mef: everything

This CL removes one code duplication and emphasizes that due to layering issues
there are still three. PTAL with an eye out for ways to dedup.

[mef, 2014-02-07 20:33:24]

Maybe it would make sense to add a method 'IsViaDataReductionProxy()' to
net::HttpResponseHeaders next to
HttpResponseHeaders::GetChromeProxyBypassDuration?

Also, decide whether to call it ChromeProxy or DataReductionProxy and stick with
it.

https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:218: const char
kDataReductionProxyViaValue[] = "Chrome-Compression-Proxy";
Any particular reason to skip version? 

Also, I think it may make sense to consistently call it either
'Data-Reduction-Proxy' or 'Chrome-Compression-Proxy' here and in function names.

[bengr, 2014-02-07 23:55:36]

https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:218: const char
kDataReductionProxyViaValue[] = "Chrome-Compression-Proxy";
On 2014/02/07 20:33:25, mef wrote:
> Any particular reason to skip version? 
> 
> Also, I think it may make sense to consistently call it either
> 'Data-Reduction-Proxy' or 'Chrome-Compression-Proxy' here and in function
names.

The version is the HTTP version from the origin, which could be 0.9, 1.0, or
1.1.

I agree about the names. The plan is to move to using 'ChromeProxy' in code, but
it will take a few CLs before that name is used everywhere.

[bengr, 2014-02-12 17:39:04]

On 2014/02/07 23:55:36, bengr1 wrote:
>
https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
> File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):
> 
>
https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
> chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:218: const char
> kDataReductionProxyViaValue[] = "Chrome-Compression-Proxy";
> On 2014/02/07 20:33:25, mef wrote:
> > Any particular reason to skip version? 
> > 
> > Also, I think it may make sense to consistently call it either
> > 'Data-Reduction-Proxy' or 'Chrome-Compression-Proxy' here and in function
> names.
> 
> The version is the HTTP version from the origin, which could be 0.9, 1.0, or
> 1.1.
> 
> I agree about the names. The plan is to move to using 'ChromeProxy' in code,
but
> it will take a few CLs before that name is used everywhere.

mef: ping?

[mef, 2014-02-12 17:55:45]

On 2014/02/12 17:39:04, bengr1 wrote:
> On 2014/02/07 23:55:36, bengr1 wrote:
> >
>
https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
> > File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):
> > 
> >
>
https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
> > chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:218: const
char
> > kDataReductionProxyViaValue[] = "Chrome-Compression-Proxy";
> > On 2014/02/07 20:33:25, mef wrote:
> > > Any particular reason to skip version? 
> > > 
> > > Also, I think it may make sense to consistently call it either
> > > 'Data-Reduction-Proxy' or 'Chrome-Compression-Proxy' here and in function
> > names.
> > 
> > The version is the HTTP version from the origin, which could be 0.9, 1.0, or
> > 1.1.
> > 
> > I agree about the names. The plan is to move to using 'ChromeProxy' in code,
> but
> > it will take a few CLs before that name is used everywhere.
> 
> mef: ping?

what about addning a method 'IsViaChromeProxyProxy()' to
net::HttpResponseHeaders next to
HttpResponseHeaders::GetChromeProxyBypassDuration?

[bengr, 2014-02-12 17:58:26]

On 2014/02/12 17:55:45, mef wrote:
> On 2014/02/12 17:39:04, bengr1 wrote:
> > On 2014/02/07 23:55:36, bengr1 wrote:
> > >
> >
>
https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
> > > File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
> > > chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:218: const
> char
> > > kDataReductionProxyViaValue[] = "Chrome-Compression-Proxy";
> > > On 2014/02/07 20:33:25, mef wrote:
> > > > Any particular reason to skip version? 
> > > > 
> > > > Also, I think it may make sense to consistently call it either
> > > > 'Data-Reduction-Proxy' or 'Chrome-Compression-Proxy' here and in
function
> > > names.
> > > 
> > > The version is the HTTP version from the origin, which could be 0.9, 1.0,
or
> > > 1.1.
> > > 
> > > I agree about the names. The plan is to move to using 'ChromeProxy' in
code,
> > but
> > > it will take a few CLs before that name is used everywhere.
> > 
> > mef: ping?
> 
> what about addning a method 'IsViaChromeProxyProxy()' to
> net::HttpResponseHeaders next to
> HttpResponseHeaders::GetChromeProxyBypassDuration?

I did that, but I called it IsChromeProxyResponse(). Is that ok?

[mef, 2014-02-12 17:59:46]

On 2014/02/12 17:58:26, bengr1 wrote:
> On 2014/02/12 17:55:45, mef wrote:
> > On 2014/02/12 17:39:04, bengr1 wrote:
> > > On 2014/02/07 23:55:36, bengr1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
> > > > File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
> > > > chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:218: const
> > char
> > > > kDataReductionProxyViaValue[] = "Chrome-Compression-Proxy";
> > > > On 2014/02/07 20:33:25, mef wrote:
> > > > > Any particular reason to skip version? 
> > > > > 
> > > > > Also, I think it may make sense to consistently call it either
> > > > > 'Data-Reduction-Proxy' or 'Chrome-Compression-Proxy' here and in
> function
> > > > names.
> > > > 
> > > > The version is the HTTP version from the origin, which could be 0.9,
1.0,
> or
> > > > 1.1.
> > > > 
> > > > I agree about the names. The plan is to move to using 'ChromeProxy' in
> code,
> > > but
> > > > it will take a few CLs before that name is used everywhere.
> > > 
> > > mef: ping?
> > 
> > what about addning a method 'IsViaChromeProxyProxy()' to
> > net::HttpResponseHeaders next to
> > HttpResponseHeaders::GetChromeProxyBypassDuration?
> 
> I did that, but I called it IsChromeProxyResponse(). Is that ok?

Oh, I didn't realize that. Sorry, I'll take a look.

[mef, 2014-02-12 18:12:18]

https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:218: const char
kDataReductionProxyViaValue[] = "Chrome-Compression-Proxy";
On 2014/02/07 23:55:36, bengr1 wrote:
> The version is the HTTP version from the origin, which could be 0.9, 1.0, or
> 1.1.
Are you sure? From this comment in rfc2616 it seems that it specifies the
protocol used to communicate with Chrome Proxy:

For example, a request message could be sent from an HTTP/1.0 user agent to an
internal proxy code-named "fred", which uses HTTP/1.1 to forward the request to
a public proxy at nowhere.com, which completes the request by forwarding it to
the origin server at www.ics.uci.edu. The request received by www.ics.uci.edu
would then have the following Via header field:

       Via: 1.0 fred, 1.1 nowhere.com (Apache/1.1)

https://codereview.chromium.org/156373002/diff/160001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/156373002/diff/160001/net/http/http_response_...
net/http/http_response_headers.cc:1433: while (EnumerateHeader(&iter, "via",
&value)) {
If it is case sensitive, shouldn't it be 'Via' instead of 'via'? Also below.

[bengr, 2014-02-12 21:14:00]

https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc (right):

https://codereview.chromium.org/156373002/diff/80001/chrome/browser/net/spdyp...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings.cc:218: const char
kDataReductionProxyViaValue[] = "Chrome-Compression-Proxy";
It is always the protocol used for the response from the upstream server. This
is what rfc2616 says:

The received-protocol indicates the protocol version of the message received by
the server or client along each segment of the request/response chain. The
received-protocol version is appended to the Via field value when the message is
forwarded so that information about the protocol capabilities of upstream
applications remains visible to all recipients.

On 2014/02/12 18:12:18, mef wrote:
> On 2014/02/07 23:55:36, bengr1 wrote:
> > The version is the HTTP version from the origin, which could be 0.9, 1.0, or
> > 1.1.
> Are you sure? From this comment in rfc2616 it seems that it specifies the
> protocol used to communicate with Chrome Proxy:
> 
> For example, a request message could be sent from an HTTP/1.0 user agent to an
> internal proxy code-named "fred", which uses HTTP/1.1 to forward the request
to
> a public proxy at http://nowhere.com, which completes the request by
forwarding it to
> the origin server at http://www.ics.uci.edu. The request received by
http://www.ics.uci.edu
> would then have the following Via header field:
> 
>        Via: 1.0 fred, 1.1 http://nowhere.com (Apache/1.1)

https://codereview.chromium.org/156373002/diff/160001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/156373002/diff/160001/net/http/http_response_...
net/http/http_response_headers.cc:1433: while (EnumerateHeader(&iter, "via",
&value)) {
The value comparison is case sensitive. The Header name comparison (see
FindHeader(), which is used by EnumerateHeader()) is not case sensitive. I've
changed the comment to be clearer.

On 2014/02/12 18:12:19, mef wrote:
> If it is case sensitive, shouldn't it be 'Via' instead of 'via'? Also below.

[mef, 2014-02-12 21:36:50]

https://codereview.chromium.org/156373002/diff/360001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (left):

https://codereview.chromium.org/156373002/diff/360001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:200: return ViaHeaderContains(frame,
kDatReductionProxyViaValue);
Given that ViaHeaderContains is still used in GetPreviewState maybe this module
should use that instead of hacking together HttpResponseHeaders?

https://codereview.chromium.org/156373002/diff/360001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/156373002/diff/360001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:214: std::replace(headers.begin(),
headers.end(), '\n', '\0');
Is it worth adding a comment that this produces a raw headers, expected by
HttpResponseHeaders constructor?

https://codereview.chromium.org/156373002/diff/360001/net/http/http_response_...
File net/http/http_response_headers_unittest.cc (right):

https://codereview.chromium.org/156373002/diff/360001/net/http/http_response_...
net/http/http_response_headers_unittest.cc:2067: "Via: 1.1 Foo Bar, 1.1
Chrome-Compression-Proxy\n",
I think Foo Bar is not a valid token, is it?

[bengr, 2014-02-13 00:17:37]

https://codereview.chromium.org/156373002/diff/360001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (left):

https://codereview.chromium.org/156373002/diff/360001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:200: return ViaHeaderContains(frame,
kDatReductionProxyViaValue);
On 2014/02/12 21:36:50, mef wrote:
> Given that ViaHeaderContains is still used in GetPreviewState maybe this
module
> should use that instead of hacking together HttpResponseHeaders?

ViaHeaderContains is only used by Promise as non-conforming to RFC 2616. I'd
prefer not to use it. Promise should fix their via header too.

https://codereview.chromium.org/156373002/diff/360001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/156373002/diff/360001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:214: std::replace(headers.begin(),
headers.end(), '\n', '\0');
On 2014/02/12 21:36:50, mef wrote:
> Is it worth adding a comment that this produces a raw headers, expected by
> HttpResponseHeaders constructor?

Done.

https://codereview.chromium.org/156373002/diff/360001/net/http/http_response_...
File net/http/http_response_headers_unittest.cc (right):

https://codereview.chromium.org/156373002/diff/360001/net/http/http_response_...
net/http/http_response_headers_unittest.cc:2067: "Via: 1.1 Foo Bar, 1.1
Chrome-Compression-Proxy\n",
On 2014/02/12 21:36:50, mef wrote:
> I think Foo Bar is not a valid token, is it?

It isn't. Changed.

[mef, 2014-02-13 00:46:42]

lgtm

[bengr, 2014-02-13 01:19:05]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-02-13 01:20:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/156373002/430001

[bengr, 2014-02-13 01:20:41]

The CQ bit was unchecked by bengr@chromium.org

[bengr, 2014-02-13 01:24:53]

thestig: chrome/renderer/page_load_histograms.cc

yfriedman: intercept_download_resource_throttle.cc

[Lei Zhang, 2014-02-13 01:33:06]

https://codereview.chromium.org/156373002/diff/430001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/156373002/diff/430001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:220: return false;
shouldn't this be inside: #else ... #endif ?

Otherwise, the preprocessor writes this out as:

...
return ...
return false;

[Lei Zhang, 2014-02-13 01:36:31]

https://codereview.chromium.org/156373002/diff/430001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/156373002/diff/430001/net/http/http_response_...
net/http/http_response_headers.cc:1442: if (value ==
kDeprecatedChromeProxyViaValue) return true;
nit: return true should be on its own line.

[Yaron, 2014-02-13 06:48:53]

On 2014/02/13 01:36:31, Lei Zhang wrote:
>
https://codereview.chromium.org/156373002/diff/430001/net/http/http_response_...
> File net/http/http_response_headers.cc (right):
> 
>
https://codereview.chromium.org/156373002/diff/430001/net/http/http_response_...
> net/http/http_response_headers.cc:1442: if (value ==
> kDeprecatedChromeProxyViaValue) return true;
> nit: return true should be on its own line.

lgtm

[bengr, 2014-02-13 16:57:35]

https://codereview.chromium.org/156373002/diff/430001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/156373002/diff/430001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:220: return false;
Done. Good catch. Thanks!

On 2014/02/13 01:33:07, Lei Zhang wrote:
> shouldn't this be inside: #else ... #endif ?
> 
> Otherwise, the preprocessor writes this out as:
> 
> ...
> return ...
> return false;

https://codereview.chromium.org/156373002/diff/430001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/156373002/diff/430001/net/http/http_response_...
net/http/http_response_headers.cc:1442: if (value ==
kDeprecatedChromeProxyViaValue) return true;
On 2014/02/13 01:36:32, Lei Zhang wrote:
> nit: return true should be on its own line.

Done.

[Lei Zhang, 2014-02-13 19:17:52]

lgtm!

[bengr, 2014-02-13 19:18:13]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-02-13 19:21:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/156373002/610001

[commit-bot: I haz the power, 2014-02-13 20:03:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/156373002/610001

[commit-bot: I haz the power, 2014-02-13 20:48:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 20:48:27]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for dinner.
Please email commit-bot@chromium.org with the CL url.

[bengr, 2014-02-13 22:31:14]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-02-13 22:33:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/156373002/610001

[commit-bot: I haz the power, 2014-02-13 23:18:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 23:18:31]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[bengr, 2014-02-14 01:24:35]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-02-14 01:25:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/156373002/1290001

[commit-bot: I haz the power, 2014-02-14 04:19:07]

Change committed as 251256