Move data reduction proxy to Chrome-Proxy header for authentication

Move data reduction proxy to Chrome-Proxy header for authentication

BUG=345907, 367268
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279957

[bengr, 2014-06-23 23:54:20]

sgurun: android_webview/*
marq:
 -- components/*
 -- chrome/browser/net/spdyproxy/*
mmenke:
 -- net/*
 -- chrome/browser/net/chrome_network_delegate*
 -- chrome/browser/profiles/*
nyquist: chrome/android/*
pfeldman: chrome/browser/devtools/*
thestig: chrome/browser/io_thread.*

[bengr, 2014-06-24 16:35:26]

Misha, PTAL at the net changes. See below. 

On 2014/06/23 23:54:20, bengr1 wrote:
> sgurun: android_webview/*
> marq:
>  -- components/*
>  -- chrome/browser/net/spdyproxy/*
> mef:
>  -- net/*
>  -- chrome/browser/net/chrome_network_delegate*
>  -- chrome/browser/profiles/*
> nyquist: chrome/android/*
> pfeldman: chrome/browser/devtools/*
> thestig: chrome/browser/io_thread.*

[sgurun-gerrit only, 2014-06-24 16:43:33]

https://codereview.chromium.org/333113002/diff/240001/android_webview/native/...
File android_webview/native/aw_contents_statics.cc (right):

https://codereview.chromium.org/333113002/diff/240001/android_webview/native/...
android_webview/native/aw_contents_statics.cc:60: ConvertJavaStringToUTF8(env,
key), "webview", "0");
are you keeping the keys as name/value pairs in some storage where webview is
the name? not sure why this would be needed since network stack here won't be
shared with others.

[bengr, 2014-06-24 16:59:40]

https://codereview.chromium.org/333113002/diff/240001/android_webview/native/...
File android_webview/native/aw_contents_statics.cc (right):

https://codereview.chromium.org/333113002/diff/240001/android_webview/native/...
android_webview/native/aw_contents_statics.cc:60: ConvertJavaStringToUTF8(env,
key), "webview", "0");
On 2014/06/24 16:43:33, sgurun wrote:
> are you keeping the keys as name/value pairs in some storage where webview is
> the name? not sure why this would be needed since network stack here won't be
> shared with others.

This name and proxy is transmitted to the proxy, so that the proxy can tailor
its service to the client.

[bengr, 2014-06-24 17:00:07]

https://codereview.chromium.org/333113002/diff/240001/android_webview/native/...
File android_webview/native/aw_contents_statics.cc (right):

https://codereview.chromium.org/333113002/diff/240001/android_webview/native/...
android_webview/native/aw_contents_statics.cc:60: ConvertJavaStringToUTF8(env,
key), "webview", "0");
I mean this name and version...

On 2014/06/24 16:59:40, bengr1 wrote:
> On 2014/06/24 16:43:33, sgurun wrote:
> > are you keeping the keys as name/value pairs in some storage where webview
is
> > the name? not sure why this would be needed since network stack here won't
be
> > shared with others.
> 
> This name and proxy is transmitted to the proxy, so that the proxy can tailor
> its service to the client.

[sgurun-gerrit only, 2014-06-24 17:17:55]

On 2014/06/24 17:00:07, bengr1 wrote:
>
https://codereview.chromium.org/333113002/diff/240001/android_webview/native/...
> File android_webview/native/aw_contents_statics.cc (right):
> 
>
https://codereview.chromium.org/333113002/diff/240001/android_webview/native/...
> android_webview/native/aw_contents_statics.cc:60: ConvertJavaStringToUTF8(env,
> key), "webview", "0");
> I mean this name and version...
> 
> On 2014/06/24 16:59:40, bengr1 wrote:
> > On 2014/06/24 16:43:33, sgurun wrote:
> > > are you keeping the keys as name/value pairs in some storage where webview
> is
> > > the name? not sure why this would be needed since network stack here won't
> be
> > > shared with others.
> > 
> > This name and proxy is transmitted to the proxy, so that the proxy can
tailor
> > its service to the client.

nit: suggest adding a constant in data reduction proxy for webview and use it
instead of using it like that.  (and I think you should mention that this is
expected at the server side)

aw lgtm

[mef, 2014-06-24 17:41:56]

Wow, that's a big CL. Here are a few initial comments.
One question - is the callback ever expected to return anything but Ok?

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:73: #if defined(OS_ANDROID)
Why is this included on Android only?

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:493:
data_reduction_proxy_auth_request_handler_->MaybeAddRequestHeader(
Should there be some meaningful return here?

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc
(right):

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:119:
EXPECT_EQ(GURL(expected_params_->DefaultOrigin()),
Could you explain this change?

https://codereview.chromium.org/333113002/diff/240001/net/base/network_delega...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/240001/net/base/network_delega...
net/base/network_delegate.cc:35: const ProxyInfo* proxy_info,
Add DCHECK(proxy_info)?

https://codereview.chromium.org/333113002/diff/240001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/333113002/diff/240001/net/base/network_delega...
net/base/network_delegate.h:136: virtual int
OnBeforeSendProxyHeaders(URLRequest* request,
Please document the return value.

https://codereview.chromium.org/333113002/diff/240001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/333113002/diff/240001/net/http/http_network_t...
net/http/http_network_transaction.cc:887: if (using_proxy &&
!before_proxy_headers_sent_callback_.is_null())
Why is it here and not where OnBeforeSendHeaders is?

https://codereview.chromium.org/333113002/diff/240001/net/url_request/url_req...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/333113002/diff/240001/net/url_request/url_req...
net/url_request/url_request_test_util.cc:404:
observed_before_proxy_headers_sent_callbacks_++;
I think coding style dictates pre-increment.

[nyquist, 2014-06-24 18:11:21]

https://codereview.chromium.org/333113002/diff/240001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java
(left):

https://codereview.chromium.org/333113002/diff/240001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:148:
public boolean isAcceptableAuthChallenge(String host, String realm) {
Wouldn't this break the downstream build? Is there any way you could temporarily
deprecate this instead (optionally keeping the same functionality), and then
clean it up later?

[bengr, 2014-06-24 18:24:11]

https://codereview.chromium.org/333113002/diff/240001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java
(left):

https://codereview.chromium.org/333113002/diff/240001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:148:
public boolean isAcceptableAuthChallenge(String host, String realm) {
On 2014/06/24 18:11:21, nyquist wrote:
> Wouldn't this break the downstream build? Is there any way you could
temporarily
> deprecate this instead (optionally keeping the same functionality), and then
> clean it up later?

I sent you a downstream CL to review. I intend to land the downstream CL first.

[bengr, 2014-06-24 19:01:51]

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:73: #if defined(OS_ANDROID)
On 2014/06/24 17:41:56, mef wrote:
> Why is this included on Android only?

Uggh. Rebase automerge. Done.

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:493:
data_reduction_proxy_auth_request_handler_->MaybeAddRequestHeader(
On 2014/06/24 17:41:56, mef wrote:
> Should there be some meaningful return here?

I guess there could be. I wasn't sure about the return. bool could indicate that
the request headers have been changed. void might be better. What do you think?

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/spdy...
File
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc
(right):

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:119:
EXPECT_EQ(GURL(expected_params_->DefaultOrigin()),
On 2014/06/24 17:41:56, mef wrote:
> Could you explain this change?

I'm hoping marq@ can review this code. In a nutshell, I added a
TestDataReductionProxyParams class so that we could test such expectations more
consistently across all data reduction proxy tests.

https://codereview.chromium.org/333113002/diff/240001/net/base/network_delega...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/240001/net/base/network_delega...
net/base/network_delegate.cc:35: const ProxyInfo* proxy_info,
On 2014/06/24 17:41:56, mef wrote:
> Add DCHECK(proxy_info)?

Done.

https://codereview.chromium.org/333113002/diff/240001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/333113002/diff/240001/net/base/network_delega...
net/base/network_delegate.h:136: virtual int
OnBeforeSendProxyHeaders(URLRequest* request,
On 2014/06/24 17:41:56, mef wrote:
> Please document the return value.

Done.

https://codereview.chromium.org/333113002/diff/240001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/333113002/diff/240001/net/http/http_network_t...
net/http/http_network_transaction.cc:887: if (using_proxy &&
!before_proxy_headers_sent_callback_.is_null())
On 2014/06/24 17:41:56, mef wrote:
> Why is it here and not where OnBeforeSendHeaders is?

Because OnBeforeSendHeaders is called before we know that we are connecting
though a proxy. This is the point of this new interface.

https://codereview.chromium.org/333113002/diff/240001/net/url_request/url_req...
File net/url_request/url_request_test_util.cc (right):

https://codereview.chromium.org/333113002/diff/240001/net/url_request/url_req...
net/url_request/url_request_test_util.cc:404:
observed_before_proxy_headers_sent_callbacks_++;
On 2014/06/24 17:41:56, mef wrote:
> I think coding style dictates pre-increment.

Done. Bad habit.

[sgurun-gerrit only, 2014-06-24 21:26:56]

still lgtm

[nyquist, 2014-06-24 23:34:50]

chrome/android lgtm

[bengr, 2014-06-25 16:12:36]

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/240001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:493:
data_reduction_proxy_auth_request_handler_->MaybeAddRequestHeader(
On 2014/06/24 19:01:50, bengr1 wrote:
> On 2014/06/24 17:41:56, mef wrote:
> > Should there be some meaningful return here?
> 
> I guess there could be. I wasn't sure about the return. bool could indicate
that
> the request headers have been changed. void might be better. What do you
think?

Changed to a void return.

[marq (ping after 24h), 2014-06-25 16:39:27]

Mostly style comments, some implementation questions.

FWIW, I think the unit test cleanup could have been in a separate CL.

https://codereview.chromium.org/333113002/diff/300001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/300001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:486: DCHECK(proxy_info);
Method body is indented two spaces too far.

https://codereview.chromium.org/333113002/diff/300001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
(left):

https://codereview.chromium.org/333113002/diff/300001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:152:
jboolean DataReductionProxySettingsAndroid::IsAcceptableAuthChallenge(
Please file a bug to deprecate the equivalent iOS API.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc
(right):

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:72:
base::Time DataReductionProxyAuthRequestHandler::Now() const {
Why wrap base::Time::Now() like this?

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:76:
void DataReductionProxyAuthRequestHandler::RandBytes(
Why wrap RandBytes like this?

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:86:
LOG(WARNING) << "Invalid proxy";
Clean up the logging -- this looks like debugging residue.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:97:
else {
} else {

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:134:
<< "]";
Align << for continued log lines.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:148:
const CommandLine& command_line = *CommandLine::ForCurrentProcess();
Why doesn't this get the key from a data_reduction_proxy_params object?

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h
(right):

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:40:
void MaybeAddRequestHeader(net::URLRequest* request,
All methods in public header files must have comments.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:76:
std::string client_;
From reading the .cc, client_ is a string identifying the OS of the client.
client_os_ might be clearer. Also, it's not part of the authentication state if
I understand correctly, so the comments here need to be cleaned up.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler_unittest.cc
(right):

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler_unittest.cc:21:

Remove extra newlines.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler_unittest.cc:90:
else
#else, right?

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:149: //
Returns true if any dns names are set on the command line.
s/dns names/proxy origins/, yes?

[bengr, 2014-06-25 17:15:19]

https://codereview.chromium.org/333113002/diff/300001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/300001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:486: DCHECK(proxy_info);
On 2014/06/25 16:39:26, marq_ wrote:
> Method body is indented two spaces too far.

Done.

https://codereview.chromium.org/333113002/diff/300001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc
(left):

https://codereview.chromium.org/333113002/diff/300001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:152:
jboolean DataReductionProxySettingsAndroid::IsAcceptableAuthChallenge(
On 2014/06/25 16:39:26, marq_ wrote:
> Please file a bug to deprecate the equivalent iOS API.

Done.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc
(right):

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:72:
base::Time DataReductionProxyAuthRequestHandler::Now() const {
On 2014/06/25 16:39:26, marq_ wrote:
> Why wrap base::Time::Now() like this?

This facilitates testing. There's a comment in the .h

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:76:
void DataReductionProxyAuthRequestHandler::RandBytes(
On 2014/06/25 16:39:27, marq_ wrote:
> Why wrap RandBytes like this?

This facilitates testing also.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:86:
LOG(WARNING) << "Invalid proxy";
On 2014/06/25 16:39:26, marq_ wrote:
> Clean up the logging -- this looks like debugging residue.

Aargh. Sorry. Done.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:97:
else {
On 2014/06/25 16:39:27, marq_ wrote:
> } else {

Done.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:134:
<< "]";
On 2014/06/25 16:39:27, marq_ wrote:
> Align << for continued log lines.

Done.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.cc:148:
const CommandLine& command_line = *CommandLine::ForCurrentProcess();
On 2014/06/25 16:39:26, marq_ wrote:
> Why doesn't this get the key from a data_reduction_proxy_params object?

Key has been pulled out of params, bucause it is more closely tied to the auth.
It just seemed a bit cleaner that way.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h
(right):

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:40:
void MaybeAddRequestHeader(net::URLRequest* request,
On 2014/06/25 16:39:27, marq_ wrote:
> All methods in public header files must have comments.

Done.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler.h:76:
std::string client_;
On 2014/06/25 16:39:27, marq_ wrote:
> From reading the .cc, client_ is a string identifying the OS of the client.
> client_os_ might be clearer. Also, it's not part of the authentication state
if
> I understand correctly, so the comments here need to be cleaned up.

Done. Client can be things other than an OS, e.g. Android Webview.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler_unittest.cc
(right):

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler_unittest.cc:21:

On 2014/06/25 16:39:27, marq_ wrote:
> Remove extra newlines.

Done.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_auth_request_handler_unittest.cc:90:
else
On 2014/06/25 16:39:27, marq_ wrote:
> #else, right?

Wow. That's dangerous. Done.

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/333113002/diff/300001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_params.h:149: //
Returns true if any dns names are set on the command line.
On 2014/06/25 16:39:27, marq_ wrote:
> s/dns names/proxy origins/, yes?

Done.

[mef, 2014-06-25 17:21:34]

https://codereview.chromium.org/333113002/diff/320001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/320001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:487: if
(data_reduction_proxy_auth_request_handler_) {
BUG? Is this initialized in constructor?

https://codereview.chromium.org/333113002/diff/320001/net/base/network_delega...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/320001/net/base/network_delega...
net/base/network_delegate.cc:38: DCHECK(proxy_info);
Suggest: make |proxy_info| a reference?

https://codereview.chromium.org/333113002/diff/320001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/333113002/diff/320001/net/base/network_delega...
net/base/network_delegate.h:68: const ProxyInfo* proxy_info,
Can |proxy_info| be NULL? If not, should it be a reference instead of pointer?
Also below.

https://codereview.chromium.org/333113002/diff/320001/net/http/http_transacti...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/333113002/diff/320001/net/http/http_transacti...
net/http/http_transaction.h:37: typedef base::Callback<void(const ProxyInfo*)>
BeforeProxyHeadersSentCallback;
&

[bengr, 2014-06-25 19:25:23]

https://codereview.chromium.org/333113002/diff/320001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/320001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:487: if
(data_reduction_proxy_auth_request_handler_) {
On 2014/06/25 17:21:33, mef wrote:
> BUG? Is this initialized in constructor?

Done.

https://codereview.chromium.org/333113002/diff/320001/net/base/network_delega...
File net/base/network_delegate.cc (right):

https://codereview.chromium.org/333113002/diff/320001/net/base/network_delega...
net/base/network_delegate.cc:38: DCHECK(proxy_info);
On 2014/06/25 17:21:33, mef wrote:
> Suggest: make |proxy_info| a reference?

Done.

https://codereview.chromium.org/333113002/diff/320001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/333113002/diff/320001/net/base/network_delega...
net/base/network_delegate.h:68: const ProxyInfo* proxy_info,
On 2014/06/25 17:21:33, mef wrote:
> Can |proxy_info| be NULL? If not, should it be a reference instead of pointer?
> Also below.

Done.

https://codereview.chromium.org/333113002/diff/320001/net/http/http_transacti...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/333113002/diff/320001/net/http/http_transacti...
net/http/http_transaction.h:37: typedef base::Callback<void(const ProxyInfo*)>
BeforeProxyHeadersSentCallback;
On 2014/06/25 17:21:33, mef wrote:
> &

Done.

[Lei Zhang, 2014-06-25 19:31:41]

c/b/io_thread.* logtm

https://codereview.chromium.org/333113002/diff/360001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/333113002/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:35: #include
"components/data_reduction_proxy/common/data_reduction_proxy_pref_names.h"
Please remove the data reduction headers too

[bengr, 2014-06-25 20:07:48]

https://codereview.chromium.org/333113002/diff/360001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/333113002/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:35: #include
"components/data_reduction_proxy/common/data_reduction_proxy_pref_names.h"
On 2014/06/25 19:31:41, Lei Zhang wrote:
> Please remove the data reduction headers too

Done.

[Lei Zhang, 2014-06-25 20:09:07]

On 2014/06/25 19:31:41, Lei Zhang wrote:
> c/b/io_thread.* logtm
> 
>
https://codereview.chromium.org/333113002/diff/360001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_impl_io_data.cc (right):
> 
>
https://codereview.chromium.org/333113002/diff/360001/chrome/browser/profiles...
> chrome/browser/profiles/profile_impl_io_data.cc:35: #include
> "components/data_reduction_proxy/common/data_reduction_proxy_pref_names.h"
> Please remove the data reduction headers too

lgtm, even

[mef, 2014-06-25 20:57:40]

net/*, chrome/browser/net/*, chrome/browser/profiles/* lgtm

https://codereview.chromium.org/333113002/diff/380001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/333113002/diff/380001/net/base/network_delega...
net/base/network_delegate.h:134: // |headers] before they get sent out.
|headers| is valid only until
nit: ] -> |

https://codereview.chromium.org/333113002/diff/380001/net/http/http_transacti...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/333113002/diff/380001/net/http/http_transacti...
net/http/http_transaction.h:37: typedef base::Callback<void(const ProxyInfo&)>
BeforeProxyHeadersSentCallback;
Please add a comment.

[marq (ping after 24h), 2014-06-25 21:30:34]

lgtm

[bengr, 2014-06-25 21:32:20]

https://codereview.chromium.org/333113002/diff/380001/net/base/network_delega...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/333113002/diff/380001/net/base/network_delega...
net/base/network_delegate.h:134: // |headers] before they get sent out.
|headers| is valid only until
On 2014/06/25 20:57:39, mef wrote:
> nit: ] -> |

Done.

https://codereview.chromium.org/333113002/diff/380001/net/http/http_transacti...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/333113002/diff/380001/net/http/http_transacti...
net/http/http_transaction.h:37: typedef base::Callback<void(const ProxyInfo&)>
BeforeProxyHeadersSentCallback;
On 2014/06/25 20:57:39, mef wrote:
> Please add a comment.

Done.

[pfeldman, 2014-06-26 01:06:38]

devtools rslgtm

[bengr, 2014-06-26 04:26:36]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-06-26 04:28:52]

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

[commit-bot: I haz the power, 2014-06-26 08:12:30]

Change committed as 279957