Add Accept-Encoding: br for secure data reduction proxy requests.

Add Accept-Encoding: br for secure Data Reduction Proxy requests.

When the connection to the Data Reduction Proxy is secure, Brotli is
requested. This will allow better compression. On secure connections,
intermediate proxies are not a threat.

BUG=634061
Committed: https://crrev.com/7bbc70483e552b9ef1630a00cde36a3f3bba4c88
Cr-Commit-Position: refs/heads/master@{#410825}

[harringtond, 2016-08-09 18:19:54]

Description was changed from

==========
Add Accept-Encoding: br for secure data reduction proxy requests.

When the connection to the data reduction proxy is secure, Brotli is
requested. This will allow better compression. On secure connections,
intermediate proxies are not a threat.

BUG=634061
==========

to

==========
Add Accept-Encoding: br for secure data reduction proxy requests.

When the connection to the data reduction proxy is secure, Brotli is
requested. This will allow better compression. On secure connections,
intermediate proxies are not a threat.

BUG=634061
==========

[harringtond, 2016-08-09 18:19:54]

harringtond@google.com changed reviewers:
+ sclittle@chromium.org

[sclittle, 2016-08-09 19:55:20]

LGTM % nits

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:228:
// Data reduction proxy handles Accept-Encoding: Brotli correctly, but don't
tiny nit: in comments, typically we capitalize Data Reduction Proxy, just for
consistency, although there are lots of places that break this rule. Could you
capitalize it here too?

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:817:
std::string accept_encoding;
nit: for clarity, could you name this something more along the lines of
|expected_accept_encoding| or something that indicates that this is the expected
result?

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:836:
net::ProxyRetryInfoMap proxy_retry_info;
nit: |proxy_retry_info| isn't used, so you could just inline it directly into
the NotifyBeforeSendHeaders call, e.g.

NotifyBeforeSendHeaders(request.get(), data_reduction_proxy_info,
net::ProxyRetryInfoMap(), &headers);

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:26:
#include "net/http/http_request_headers.h"
FYI: Thanks for adding this! I'm surprised it wasn't here already.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:42:
bool HasBrotliAcceptEncoding(const std::string& accept_encoding_header) {
nit: Unless you really need an std::string, typically it's better to pass in a
base::StringPiece, so that if in the future someone wants to just pass in a
const char* or base::StringPiece, then it doesn't need to be converted into an
std::string.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:44:
base::SplitString(accept_encoding_header, ",", base::TRIM_WHITESPACE,
nit: use base::SplitStringPiece here instead of base::SplitString, to avoid
copying all the substrings.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:171:
net::URLRequest* request,
nit: pass in a const net::URLRequest&, which will ensure that (a) |request| is
never null and (b) this code doesn't accidentally modify |request|.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:177:
!HasBrotliAcceptEncoding(accept_encoding_header)) {
nit: could you change the if-statement structure here so that it does an early
return if these conditions don't hold? This is primarily just a style
consistency thing in Chrome, plus it reduces the indent of the lower lines.

e.g. instead of:
if (... conditions ...) {
  ... do stuff ...
}

do:

if (... not conditions ...)
  return;
... do stuff ...

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
(right):

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:381:
if (test.initial_accept_encoding != nullptr) {
nit: you can leave out the "!= nullptr".

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:388:
std::string got_accept_encoding;
nit: typically in chromium, these are names "actual_..." instead of "got_...".
Could you change this to match?

[harringtond, 2016-08-09 20:17:32]

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:228:
// Data reduction proxy handles Accept-Encoding: Brotli correctly, but don't
On 2016/08/09 19:55:20, sclittle wrote:
> tiny nit: in comments, typically we capitalize Data Reduction Proxy, just for
> consistency, although there are lots of places that break this rule. Could you
> capitalize it here too?

Done.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:817:
std::string accept_encoding;
On 2016/08/09 19:55:20, sclittle wrote:
> nit: for clarity, could you name this something more along the lines of
> |expected_accept_encoding| or something that indicates that this is the
expected
> result?

Done.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:836:
net::ProxyRetryInfoMap proxy_retry_info;
On 2016/08/09 19:55:20, sclittle wrote:
> nit: |proxy_retry_info| isn't used, so you could just inline it directly into
> the NotifyBeforeSendHeaders call, e.g.
> 
> NotifyBeforeSendHeaders(request.get(), data_reduction_proxy_info,
> net::ProxyRetryInfoMap(), &headers);

Done.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:26:
#include "net/http/http_request_headers.h"
On 2016/08/09 19:55:20, sclittle wrote:
> FYI: Thanks for adding this! I'm surprised it wasn't here already.

Acknowledged.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:42:
bool HasBrotliAcceptEncoding(const std::string& accept_encoding_header) {
On 2016/08/09 19:55:20, sclittle wrote:
> nit: Unless you really need an std::string, typically it's better to pass in a
> base::StringPiece, so that if in the future someone wants to just pass in a
> const char* or base::StringPiece, then it doesn't need to be converted into an
> std::string.

Done.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:42:
bool HasBrotliAcceptEncoding(const std::string& accept_encoding_header) {
On 2016/08/09 19:55:20, sclittle wrote:
> nit: Unless you really need an std::string, typically it's better to pass in a
> base::StringPiece, so that if in the future someone wants to just pass in a
> const char* or base::StringPiece, then it doesn't need to be converted into an
> std::string.

Done.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:44:
base::SplitString(accept_encoding_header, ",", base::TRIM_WHITESPACE,
On 2016/08/09 19:55:20, sclittle wrote:
> nit: use base::SplitStringPiece here instead of base::SplitString, to avoid
> copying all the substrings.

Done.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:177:
!HasBrotliAcceptEncoding(accept_encoding_header)) {
On 2016/08/09 19:55:20, sclittle wrote:
> nit: could you change the if-statement structure here so that it does an early
> return if these conditions don't hold? This is primarily just a style
> consistency thing in Chrome, plus it reduces the indent of the lower lines.
> 
> e.g. instead of:
> if (... conditions ...) {
>   ... do stuff ...
> }
> 
> do:
> 
> if (... not conditions ...)
>   return;
> ... do stuff ...

Done.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc
(right):

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:381:
if (test.initial_accept_encoding != nullptr) {
On 2016/08/09 19:55:20, sclittle wrote:
> nit: you can leave out the "!= nullptr".

Done.

https://codereview.chromium.org/2224003004/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options_unittest.cc:388:
std::string got_accept_encoding;
On 2016/08/09 19:55:20, sclittle wrote:
> nit: typically in chromium, these are names "actual_..." instead of "got_...".
> Could you change this to match?

Done.

[harringtond, 2016-08-09 20:17:59]

The CQ bit was checked by harringtond@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 20:19:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[harringtond, 2016-08-09 20:33:04]

Description was changed from

==========
Add Accept-Encoding: br for secure data reduction proxy requests.

When the connection to the data reduction proxy is secure, Brotli is
requested. This will allow better compression. On secure connections,
intermediate proxies are not a threat.

BUG=634061
==========

to

==========
Add Accept-Encoding: br for secure Data Reduction Proxy requests.

When the connection to the Data Reduction Proxy is secure, Brotli is
requested. This will allow better compression. On secure connections,
intermediate proxies are not a threat.

BUG=634061
==========

[harringtond, 2016-08-09 20:33:31]

The CQ bit was checked by harringtond@google.com

[harringtond, 2016-08-09 20:33:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/2224003004/#ps40001
(title: "Add Accept-Encoding: br for secure data reduction proxy requests")

[commit-bot: I haz the power, 2016-08-09 20:34:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 21:25:22]

Description was changed from

==========
Add Accept-Encoding: br for secure Data Reduction Proxy requests.

When the connection to the Data Reduction Proxy is secure, Brotli is
requested. This will allow better compression. On secure connections,
intermediate proxies are not a threat.

BUG=634061
==========

to

==========
Add Accept-Encoding: br for secure Data Reduction Proxy requests.

When the connection to the Data Reduction Proxy is secure, Brotli is
requested. This will allow better compression. On secure connections,
intermediate proxies are not a threat.

BUG=634061
==========

[commit-bot: I haz the power, 2016-08-09 21:25:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-09 21:26:56]

Description was changed from

==========
Add Accept-Encoding: br for secure Data Reduction Proxy requests.

When the connection to the Data Reduction Proxy is secure, Brotli is
requested. This will allow better compression. On secure connections,
intermediate proxies are not a threat.

BUG=634061
==========

to

==========
Add Accept-Encoding: br for secure Data Reduction Proxy requests.

When the connection to the Data Reduction Proxy is secure, Brotli is
requested. This will allow better compression. On secure connections,
intermediate proxies are not a threat.

BUG=634061

Committed: https://crrev.com/7bbc70483e552b9ef1630a00cde36a3f3bba4c88
Cr-Commit-Position: refs/heads/master@{#410825}
==========

[commit-bot: I haz the power, 2016-08-09 21:26:57]

Patchset 3 (id:??) landed as
https://crrev.com/7bbc70483e552b9ef1630a00cde36a3f3bba4c88
Cr-Commit-Position: refs/heads/master@{#410825}