Move data reduction proxy logic out of chrome and android webview network delegate

Move data reduction proxy logic out of chrome and android webview network delegates

In order to move the  data reduction proxy logic from
ChromeNetworkDelegate and AwNetworkDelegate to
DataReductionProxyNetworkDelegate, pure virtual methods in
NetworkDelegate move to the new subclass NetworkDelegateImpl.
All subclasses of NetworkDelegate now subclass
NetworkDelegateImpl.

The DataReductionProxyNetworkDelegate subclasses
WrappingNetworkDelegate. WrappingNetworkDelegate subclasses
NetworkDelegateand and takes a NetworkDelegate in its
constructor and owns it. For every override OnFoo,
WrappingNetworkDelegate provides a protected virtual void
OnFooInternal(), with an empty default implementation.

DataReductionProxyNetworkDelegate subclasses
WrappingNetworkDelegate and moves implementations from
ChromeNetworkDelegate and AwNetworkDelegate inside the
On*Internal() methods.

BUG=429734
Committed: https://crrev.com/c1751458b9409f52c56b3f84d8226c57c39369d5
Cr-Commit-Position: refs/heads/master@{#307523}

[megjablon, 2014-11-18 18:36:13]

megjablon@chromium.org changed reviewers:
+ bengr@chromium.org

[bengr, 2014-11-18 19:30:56]

https://codereview.chromium.org/734263003/diff/1/android_webview/browser/net/...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/734263003/diff/1/android_webview/browser/net/...
android_webview/browser/net/aw_url_request_context_getter.cc:243:
data_reduction_proxy_auth_request_handler_.reset(
Can this be moved above the delegate construction so that the setter can be part
of the DRPNetworkDelegate constructor?

https://codereview.chromium.org/734263003/diff/1/android_webview/browser/net/...
android_webview/browser/net/aw_url_request_context_getter.cc:253:
data_reduction_proxy_settings->params());
Make this part of the  DRPNetworkDelegate constructor or put inside a
DRPNetworkDelegate::Init(). Ideally, the embedder wouldn't need to think about
these added hookups.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/734263003/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:1170:
network_delegate->set_data_reduction_proxy_params(
Likewise, this stuff should all be part of the DRPNetworkDelegate constructor or
Init().

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:108: void
RecordContentLengthHistograms(
This should be moved to the DRPNetworkDelegate.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:421: "x-original-content-length");
This should be moved to the DRPNetworkDelegate.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:423: base::TimeDelta
freshness_lifetime =
This should be moved to the DRPNetworkDelegate.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:427:
RecordContentLengthHistograms(received_content_length,
This should be moved to the DRPNetworkDelegate.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:431: DVLOG(2) << __FUNCTION__
This should be moved to the DRPNetworkDelegate.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/734263003/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl_io_data.cc:448: network_delegate()->
These can be put into a single DRPNetworkDelegate::Init() method.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/734263003/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:1042: profile->GetPrefs());
The DRPND constructor should take the pref service and the message loop proxy.

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

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:5:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h"
For tests, you'll want to test that (1) wrapping works. (2) the added
functionality in this class works.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:9:
#include "base/prefs/pref_member.h"
Is this needed?

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:13:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_metrics.h"
Is this needed?

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:15:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"
This can be forward declared until you move UpdateContentLengthPrefs to here.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:17:
#include
"components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
Not needed.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:20:
#include "net/proxy/proxy_config.h"
This can be forward declared.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:21:
#include "net/proxy/proxy_info.h"
This can be forward declared.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:22:
#include "net/proxy/proxy_server.h"
This can be forward declared.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:23:
#include "net/url_request/url_request.h"
You need an include for URLRequestStatus too.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:24:

You also need an include for ProxyService.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:25:
namespace data_reduction_proxy {
You should forward declare GURL, ProxyInfo, ProxyConfig, ProxyServer, DRPParams,
RequestHeaders. Includes and forward declarations don't need to be added here
again if they are already in the .h.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:94:
proxy_service.proxy_retry_info(),
Is this the same as the result?

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:178:

Remove blank line.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:41:
class DataReductionProxyNetworkDelegate : public net::WrappingNetworkDelegate {
Please add a comment describing what this class does and add comments to *every*
method and member variable, unless it is overwhelmingly obvious why it is there
and what it does.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:45:
~DataReductionProxyNetworkDelegate() override;
I think you want this to be virtual.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:103:
void set_on_resolve_proxy_handler(OnResolveProxyHandler handler) {
Can this be a const &?

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:132:
void OnCompletedInternal(net::URLRequest* request,
Add a blank line above and between all methods.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:157:
DataReductionProxyUsageStats* data_reduction_proxy_usage_stats_;
Add a blank line between each member variable.

https://codereview.chromium.org/734263003/diff/1/net/base/network_delegate_im...
File net/base/network_delegate_impl.h (right):

https://codereview.chromium.org/734263003/diff/1/net/base/network_delegate_im...
net/base/network_delegate_impl.h:50: virtual int OnBeforeURLRequest(URLRequest*
request,
Remove the virtual keyword from all of these overrides.

https://codereview.chromium.org/734263003/diff/1/net/base/wrapping_network_de...
File net/base/wrapping_network_delegate.h (right):

https://codereview.chromium.org/734263003/diff/1/net/base/wrapping_network_de...
net/base/wrapping_network_delegate.h:28: class NET_EXPORT
WrappingNetworkDelegate : public NetworkDelegate {
Should this subclass NetworkDelegateImpl?

[megjablon, 2014-11-19 19:23:41]

https://codereview.chromium.org/734263003/diff/1/android_webview/browser/net/...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/734263003/diff/1/android_webview/browser/net/...
android_webview/browser/net/aw_url_request_context_getter.cc:243:
data_reduction_proxy_auth_request_handler_.reset(
On 2014/11/18 19:30:55, bengr wrote:
> Can this be moved above the delegate construction so that the setter can be
part
> of the DRPNetworkDelegate constructor?

Done.

https://codereview.chromium.org/734263003/diff/1/android_webview/browser/net/...
android_webview/browser/net/aw_url_request_context_getter.cc:253:
data_reduction_proxy_settings->params());
On 2014/11/18 19:30:55, bengr wrote:
> Make this part of the  DRPNetworkDelegate constructor or put inside a
> DRPNetworkDelegate::Init(). Ideally, the embedder wouldn't need to think about
> these added hookups.

Done.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/734263003/diff/1/chrome/browser/io_thread.cc#...
chrome/browser/io_thread.cc:1170:
network_delegate->set_data_reduction_proxy_params(
On 2014/11/18 19:30:55, bengr wrote:
> Likewise, this stuff should all be part of the DRPNetworkDelegate constructor
or
> Init().

Done.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:108: void
RecordContentLengthHistograms(
On 2014/11/18 19:30:55, bengr wrote:
> This should be moved to the DRPNetworkDelegate.

Done.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:421: "x-original-content-length");
On 2014/11/18 19:30:55, bengr wrote:
> This should be moved to the DRPNetworkDelegate.

Done.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:423: base::TimeDelta
freshness_lifetime =
On 2014/11/18 19:30:55, bengr wrote:
> This should be moved to the DRPNetworkDelegate.

Done.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:427:
RecordContentLengthHistograms(received_content_length,
On 2014/11/18 19:30:55, bengr wrote:
> This should be moved to the DRPNetworkDelegate.

Done.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/net/chrome_ne...
chrome/browser/net/chrome_network_delegate.cc:431: DVLOG(2) << __FUNCTION__
On 2014/11/18 19:30:55, bengr wrote:
> This should be moved to the DRPNetworkDelegate.

Done.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/734263003/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl_io_data.cc:448: network_delegate()->
On 2014/11/18 19:30:55, bengr wrote:
> These can be put into a single DRPNetworkDelegate::Init() method.

Done.

https://codereview.chromium.org/734263003/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/734263003/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:1042: profile->GetPrefs());
On 2014/11/18 19:30:55, bengr wrote:
> The DRPND constructor should take the pref service and the message loop proxy.

Done.

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

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:5:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h"
On 2014/11/18 19:30:56, bengr wrote:
> For tests, you'll want to test that (1) wrapping works. (2) the added
> functionality in this class works.

Next patch

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:9:
#include "base/prefs/pref_member.h"
On 2014/11/18 19:30:56, bengr wrote:
> Is this needed?

Removed.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:13:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_metrics.h"
On 2014/11/18 19:30:56, bengr wrote:
> Is this needed?

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:15:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h"
On 2014/11/18 19:30:56, bengr wrote:
> This can be forward declared until you move UpdateContentLengthPrefs to here.

Acknowledged.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:17:
#include
"components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
On 2014/11/18 19:30:56, bengr wrote:
> Not needed.

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:20:
#include "net/proxy/proxy_config.h"
On 2014/11/18 19:30:56, bengr wrote:
> This can be forward declared.

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:21:
#include "net/proxy/proxy_info.h"
On 2014/11/18 19:30:55, bengr wrote:
> This can be forward declared.

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:22:
#include "net/proxy/proxy_server.h"
On 2014/11/18 19:30:56, bengr wrote:
> This can be forward declared.

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:23:
#include "net/url_request/url_request.h"
On 2014/11/18 19:30:55, bengr wrote:
> You need an include for URLRequestStatus too.

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:24:

On 2014/11/18 19:30:56, bengr wrote:
> You also need an include for ProxyService.

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:25:
namespace data_reduction_proxy {
On 2014/11/18 19:30:56, bengr wrote:
> You should forward declare GURL, ProxyInfo, ProxyConfig, ProxyServer,
DRPParams,
> RequestHeaders. Includes and forward declarations don't need to be added here
> again if they are already in the .h.

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:94:
proxy_service.proxy_retry_info(),
On 2014/11/18 19:30:55, bengr wrote:
> Is this the same as the result?

I'm not sure what you're asking here.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:178:

On 2014/11/18 19:30:55, bengr wrote:
> Remove blank line.

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:41:
class DataReductionProxyNetworkDelegate : public net::WrappingNetworkDelegate {
On 2014/11/18 19:30:56, bengr wrote:
> Please add a comment describing what this class does and add comments to
*every*
> method and member variable, unless it is overwhelmingly obvious why it is
there
> and what it does.

Next patch

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:45:
~DataReductionProxyNetworkDelegate() override;
On 2014/11/18 19:30:56, bengr wrote:
> I think you want this to be virtual.

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:103:
void set_on_resolve_proxy_handler(OnResolveProxyHandler handler) {
On 2014/11/18 19:30:56, bengr wrote:
> Can this be a const &?

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:132:
void OnCompletedInternal(net::URLRequest* request,
On 2014/11/18 19:30:56, bengr wrote:
> Add a blank line above and between all methods.

Done.

https://codereview.chromium.org/734263003/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:157:
DataReductionProxyUsageStats* data_reduction_proxy_usage_stats_;
On 2014/11/18 19:30:56, bengr wrote:
> Add a blank line between each member variable.

Done.

https://codereview.chromium.org/734263003/diff/1/net/base/network_delegate_im...
File net/base/network_delegate_impl.h (right):

https://codereview.chromium.org/734263003/diff/1/net/base/network_delegate_im...
net/base/network_delegate_impl.h:50: virtual int OnBeforeURLRequest(URLRequest*
request,
On 2014/11/18 19:30:56, bengr wrote:
> Remove the virtual keyword from all of these overrides.

Done.

https://codereview.chromium.org/734263003/diff/1/net/base/wrapping_network_de...
File net/base/wrapping_network_delegate.h (right):

https://codereview.chromium.org/734263003/diff/1/net/base/wrapping_network_de...
net/base/wrapping_network_delegate.h:28: class NET_EXPORT
WrappingNetworkDelegate : public NetworkDelegate {
On 2014/11/18 19:30:56, bengr wrote:
> Should this subclass NetworkDelegateImpl?

Acknowledged.

[megjablon, 2014-11-25 19:19:20]

Patchset #4 (id:60001) has been deleted

[megjablon, 2014-11-25 22:20:30]

megjablon@chromium.org changed reviewers:
+ damienv@chromium.org, jamescook@chromium.org, jochen@chromium.org,
mmenke@chromium.org, sgurun@chromium.org

[megjablon, 2014-11-25 22:20:31]

damienv: chromecast/*

jamescook: extensions/*

sgurun: android_webview/*

jochen: components/components_tests.gyp
        content/*

mmenke: net/*
        components/cronet/*
        chrome/*

Thanks!

[James Cook, 2014-11-25 23:41:59]

extensions/shell lgtm

[damienv1, 2014-11-26 01:25:26]

damienv@chromium.org changed reviewers:
+ lcwu@chromium.org

[jochen (gone - plz use gerrit), 2014-11-26 14:04:07]

content and components lgtm

https://codereview.chromium.org/734263003/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/734263003/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:72:
virtual ~DataReductionProxyNetworkDelegate() override;
either virtual or override, one is enough

[mmenke, 2014-11-26 15:23:53]

I haven't yet dug through the tests.  May not get to them until next week.

https://codereview.chromium.org/734263003/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:65:
}
This should be in an anonymous namespace.

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

https://codereview.chromium.org/734263003/diff/100001/net/base/network_delega...
net/base/network_delegate.h:112: // This is the interface for subclasses of
NetworkDelegate to implement. These
Can we keep these private, and have the wrapped delegate call the corresponding
Notify* methods of the delegate it wraps, rather than the On* ones?

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
File net/base/wrapping_network_delegate.cc (right):

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.cc:7: #include "net/base/net_errors.h"
Don't think we need this?

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
File net/base/wrapping_network_delegate.h (right):

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:1: // TODO: Insert description here.
(generated by megjablon)
Need copyright header.

Also, descriptions generally go above class definitions, not at the top of a
file (Where they'll often be ignored, along with the copyright notice)

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:28: class NET_EXPORT
WrappingNetworkDelegate : public NetworkDelegate {
Maybe call this the LayeredNetworkDelegate?

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:32: // the On*() of |network_delegate| is
called.
This should be just above the class declaration.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:32: // the On*() of |network_delegate| is
called.
Should also mention that the internal methods have no return values, and cannot
prevent calling into the nested network delegate.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:33:
WrappingNetworkDelegate(scoped_ptr<NetworkDelegate> network_delegate);
Single argument methods should be explicit.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:33:
WrappingNetworkDelegate(scoped_ptr<NetworkDelegate> network_delegate);
Suggest network_delegate be renamed to nested_network_delegate, here and the
private variable as well.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:34: virtual ~WrappingNetworkDelegate();
nit:  -virtual + override

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:36: // Protected for testing purposes.
+1 indent...Also, not really sure these need to be protected.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:50: // The default implementation returns
OK (continue with request).
Should not duplicate comments like this (They'll become out of date if only the
public verison is updated).  Think we probably should have / don't need any
method comments in this file, except maybe the constructor.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:53: GURL* new_url) final;
The overridden methods should be grouped together, with no lines between them,
and a comment above them along the liens of "NetworkDelegate implementation:"

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:53: GURL* new_url) final;
-virtual + override (And keep the final) for all of the overridden methods.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:275: // WrappingNetworkDelegate takes
ownership of this.
Not needed (It's implied by the fact it's a scoped_ptr)

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
File net/base/wrapping_network_delegate_unittest.cc (right):

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate_unittest.cc:23: namespace {
Should nest this in the net namespace, and remove all net::'s

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate_unittest.cc:25: typedef std::map<const char*,
int64> CountersMap;
Don't think you really need an int64 here

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate_unittest.cc:55: virtual
~TestNetworkDelegateImpl() {
-virtual + override

[megjablon, 2014-12-01 19:26:56]

https://codereview.chromium.org/734263003/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:65:
}
On 2014/11/26 15:23:52, mmenke wrote:
> This should be in an anonymous namespace.

Done.

https://codereview.chromium.org/734263003/diff/100001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/734263003/diff/100001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:72:
virtual ~DataReductionProxyNetworkDelegate() override;
On 2014/11/26 14:04:07, jochen (slow) wrote:
> either virtual or override, one is enough

Done.

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

https://codereview.chromium.org/734263003/diff/100001/net/base/network_delega...
net/base/network_delegate.h:112: // This is the interface for subclasses of
NetworkDelegate to implement. These
On 2014/11/26 15:23:52, mmenke wrote:
> Can we keep these private, and have the wrapped delegate call the
corresponding
> Notify* methods of the delegate it wraps, rather than the On* ones?

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
File net/base/wrapping_network_delegate.cc (right):

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.cc:7: #include "net/base/net_errors.h"
On 2014/11/26 15:23:52, mmenke wrote:
> Don't think we need this?

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
File net/base/wrapping_network_delegate.h (right):

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:1: // TODO: Insert description here.
(generated by megjablon)
On 2014/11/26 15:23:52, mmenke wrote:
> Need copyright header.
> 
> Also, descriptions generally go above class definitions, not at the top of a
> file (Where they'll often be ignored, along with the copyright notice)

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:28: class NET_EXPORT
WrappingNetworkDelegate : public NetworkDelegate {
On 2014/11/26 15:23:53, mmenke wrote:
> Maybe call this the LayeredNetworkDelegate?

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:32: // the On*() of |network_delegate| is
called.
On 2014/11/26 15:23:52, mmenke wrote:
> Should also mention that the internal methods have no return values, and
cannot
> prevent calling into the nested network delegate.

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:33:
WrappingNetworkDelegate(scoped_ptr<NetworkDelegate> network_delegate);
On 2014/11/26 15:23:52, mmenke wrote:
> Single argument methods should be explicit.

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:34: virtual ~WrappingNetworkDelegate();
On 2014/11/26 15:23:52, mmenke wrote:
> nit:  -virtual + override

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:36: // Protected for testing purposes.
On 2014/11/26 15:23:52, mmenke wrote:
> +1 indent...Also, not really sure these need to be protected.

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:50: // The default implementation returns
OK (continue with request).
On 2014/11/26 15:23:52, mmenke wrote:
> Should not duplicate comments like this (They'll become out of date if only
the
> public verison is updated).  Think we probably should have / don't need any
> method comments in this file, except maybe the constructor.

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:53: GURL* new_url) final;
On 2014/11/26 15:23:53, mmenke wrote:
> -virtual + override (And keep the final) for all of the overridden methods.

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate.h:275: // WrappingNetworkDelegate takes
ownership of this.
On 2014/11/26 15:23:52, mmenke wrote:
> Not needed (It's implied by the fact it's a scoped_ptr)

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
File net/base/wrapping_network_delegate_unittest.cc (right):

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate_unittest.cc:23: namespace {
On 2014/11/26 15:23:53, mmenke wrote:
> Should nest this in the net namespace, and remove all net::'s

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate_unittest.cc:25: typedef std::map<const char*,
int64> CountersMap;
On 2014/11/26 15:23:53, mmenke wrote:
> Don't think you really need an int64 here

Done.

https://codereview.chromium.org/734263003/diff/100001/net/base/wrapping_netwo...
net/base/wrapping_network_delegate_unittest.cc:55: virtual
~TestNetworkDelegateImpl() {
On 2014/11/26 15:23:53, mmenke wrote:
> -virtual + override

Done.

[megjablon, 2014-12-01 19:53:52]

Patchset #6 (id:120001) has been deleted

[bengr, 2014-12-02 17:20:01]

https://codereview.chromium.org/734263003/diff/140001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/734263003/diff/140001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:221: NULL, NULL,
NULL,
I don't understand this Init call. If you need to call it from two different
places to initialize different subsets of the parameters, please make it two
separate functions. I'm hoping, however, that by moving some of this to the
constructor, you can do it with a single Init() called once.

https://codereview.chromium.org/734263003/diff/140001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:223:
data_reduction_proxy_auth_request_handler_.get(), NULL,
Who needs this besides the data_reduction_proxy_network_delegate? Can this be
instantiated inside the DRPND constructor?

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

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:358: #if defined(OS_ANDROID)
Why did this get added?

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

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:420: }
Remove curly braces.

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:452: network_delegate()->Init(
Try to tighten this up. I think these parameters should be put into three
groups:
1) parameters that could be passed in on network delegate construction, (2)
parameters that must be passed in later during an Init() call, and (3)
parameters that the network delegate should instantiate itself (and not be
passed in).

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1038: if (profile &&
!profile->IsOffTheRecord()) {
When would provide be NULL?

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1047:
data_reduction_proxy_network_delegate->Init(
Again, this Init needs to be rewritten.

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1048: profile_prefs, ui_thread_proxy,
NULL, NULL, NULL, NULL, NULL,
This confuses me. You always instantiate the DRPNetworkDelegate, even on
incognito, but you don't pass in prefs or the message loop proxy if incognito?
Why is that?

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:678: mutable ChromeNetworkDelegate*
chrome_network_delegate_;
Is this just a convenience? Can't you get this from
data_reduction_proxy_network_delegate_->network_delegate() or some such?

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1070:
static_cast<data_reduction_proxy::DataReductionProxyNetworkDelegate*>(
This seems like a layering violation.

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:249:
profile_prefs_,
Make sure that incognito does not pass profile prefs.

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:144:
//Weak, owned by our owner.
space

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:147:
// |data_reduction_proxy_params_| must outlive this
Must outlive this...

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:151:
// |data_reduction_proxy_usage_stats_| must outlive this
Must outlive this...

[megjablon, 2014-12-02 22:33:00]

Patchset #7 (id:160001) has been deleted

[megjablon, 2014-12-02 22:33:21]

https://codereview.chromium.org/734263003/diff/140001/android_webview/browser...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/734263003/diff/140001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:221: NULL, NULL,
NULL,
On 2014/12/02 17:20:00, bengr wrote:
> I don't understand this Init call. If you need to call it from two different
> places to initialize different subsets of the parameters, please make it two
> separate functions. I'm hoping, however, that by moving some of this to the
> constructor, you can do it with a single Init() called once.

Moved all to constructor.

https://codereview.chromium.org/734263003/diff/140001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:223:
data_reduction_proxy_auth_request_handler_.get(), NULL,
On 2014/12/02 17:20:00, bengr wrote:
> Who needs this besides the data_reduction_proxy_network_delegate? Can this be
> instantiated inside the DRPND constructor?

It is used by SetKeyOnIO

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

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:358: #if defined(OS_ANDROID)
On 2014/12/02 17:20:00, bengr wrote:
> Why did this get added?

Just got moved. received_content_length is only used in the #if
defined(OS_ANDROID) block now so it should be included otherwise we have an
unused variable on other platforms.

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

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:420: }
On 2014/12/02 17:20:00, bengr wrote:
> Remove curly braces.

Done.

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:452: network_delegate()->Init(
On 2014/12/02 17:20:00, bengr wrote:
> Try to tighten this up. I think these parameters should be put into three
> groups:
> 1) parameters that could be passed in on network delegate construction, (2)
> parameters that must be passed in later during an Init() call, and (3)
> parameters that the network delegate should instantiate itself (and not be
> passed in).

I have moved everything to the constructor and gotten rid of the Init call. I
don't see anything that between AwURLRequestContextGetter, ProfileImplIOData,
and IOThread that I can instantiate in the constructor. Can you take a look at
this Ben to see if I'm missing something?

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1038: if (profile &&
!profile->IsOffTheRecord()) {
On 2014/12/02 17:20:00, bengr wrote:
> When would provide be NULL?

Removed in moved code

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1047:
data_reduction_proxy_network_delegate->Init(
On 2014/12/02 17:20:00, bengr wrote:
> Again, this Init needs to be rewritten.

Removed.

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1048: profile_prefs, ui_thread_proxy,
NULL, NULL, NULL, NULL, NULL,
On 2014/12/02 17:20:00, bengr wrote:
> This confuses me. You always instantiate the DRPNetworkDelegate, even on
> incognito, but you don't pass in prefs or the message loop proxy if incognito?
> Why is that?

Moved into ProfileImplIOData as discussed.

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:678: mutable ChromeNetworkDelegate*
chrome_network_delegate_;
On 2014/12/02 17:20:00, bengr wrote:
> Is this just a convenience? Can't you get this from
> data_reduction_proxy_network_delegate_->network_delegate() or some such?

This doesn't work because we need the ChromeNetworkDelegate for some setters
which DataReductionProxyNetworkDelegate does not have.

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/734263003/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1070:
static_cast<data_reduction_proxy::DataReductionProxyNetworkDelegate*>(
On 2014/12/02 17:20:00, bengr wrote:
> This seems like a layering violation.

Following the existing pattern.

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:249:
profile_prefs_,
On 2014/12/02 17:20:00, bengr wrote:
> Make sure that incognito does not pass profile prefs.

Done.

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:144:
//Weak, owned by our owner.
On 2014/12/02 17:20:00, bengr wrote:
> space

Done.

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:147:
// |data_reduction_proxy_params_| must outlive this
On 2014/12/02 17:20:01, bengr wrote:
> Must outlive this...

Done.

https://codereview.chromium.org/734263003/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:151:
// |data_reduction_proxy_usage_stats_| must outlive this
On 2014/12/02 17:20:00, bengr wrote:
> Must outlive this...

Done.

[megjablon, 2014-12-02 23:06:17]

Patchset #8 (id:60002) has been deleted

[megjablon, 2014-12-03 01:07:22]

Patchset #9 (id:230001) has been deleted

[mmenke, 2014-12-03 18:50:39]

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:603: // usage stats.
nit:  Don't use "we" in comments.

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

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:425:
scoped_ptr<ChromeNetworkDelegate> chrome_network_delegate,
This passing ownership back and forth seems weird: 
ProfileData->ProfileImplIOData->ProfileIOData.  Can we just have the
profileImplIOData own it?

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:464: profile->GetPrefs(),
BUG:  We're on the IO thread.  it's not safe to dereference to the profile here.

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:472:
chrome_network_delegate_ptr->set_predictor(predictor_.get());
Can this just be moved to where chrome_network_delegate_ptr is, so we can get
rid of chrome_network_delegate_ptr?

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:620:
chrome_network_delegate_ptr->set_domain_reliability_monitor(monitor);
Again, think it's simpler if we can just move this up

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:460: net::NetworkDelegate*
network_delegate() const {
Think we can get rid of this - can just grab it from the appropriate context, or
off the stack, where it's needed, I believe.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
File net/base/layered_network_delegate_unittest.cc (right):

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:28: typedef std::map<const char*,
int> CountersMap;
include map.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:30: void
VerifyCounters(CountersMap counters, int64 count) {
int64 not needed

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:30: void
VerifyCounters(CountersMap counters, int64 count) {
Actually, can you just get rid of this method, and check that the corresponding
counter is 1 in each of the callbacks?  That'll also print a more useful error
on failure.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:37: class TestProxyConfigService :
public ProxyConfigService {
Not needed.  Can just use ProxyService::CreateDirect() and stick it in a
scoped_ptr to create a proxy service for the test.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:69:
++counters_["on_before_url_request_count"];
Hrm...  It's a pain, but if we want to be complete, perhaps we should have check
that the TestLayeredNetworkDelegate's corresponding counter has already been
incremented on all of these.  Via a method, of
course...IncrementAndCompareCounter("...");

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:220:
OnRawBytesRead(*request.get(), 0);
Think *request can be used instead of *request.get()

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:231: *request.get(), GURL(),
GURL());
Maybe use EXPECT_EQ for those that return values?

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:331: CountersMap counters_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:343:
MOCK_CONST_METHOD2(OnCanAccessFileInternal,
Probably shouldn't be using gmock, so hacking on Chrome doesn't require
knowledge of it.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:385:
OnCanAccessFileInternal(testing::_, testing::_));
Why are these calls separated out?

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:397:
layered_network_delegate_->VerifyCalled(1);
Should compare the total number of calls to the two is the same.

[megjablon, 2014-12-04 00:22:21]

Patchset #10 (id:270001) has been deleted

[megjablon, 2014-12-04 01:23:02]

Patchset #10 (id:290001) has been deleted

[megjablon, 2014-12-04 01:23:47]

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:603: // usage stats.
On 2014/12/03 18:50:38, mmenke wrote:
> nit:  Don't use "we" in comments.

Done.

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

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:425:
scoped_ptr<ChromeNetworkDelegate> chrome_network_delegate,
On 2014/12/03 18:50:38, mmenke wrote:
> This passing ownership back and forth seems weird: 
> ProfileData->ProfileImplIOData->ProfileIOData.  Can we just have the
> profileImplIOData own it?

I'm not sure I understand. Can you explain? If we have ProfileImplIOData own it,
we would still have to pass it from ProfileIOData to ProfileImplIOData. Also, we
would have to add ownership of the nested network delegate to all other classes
that create a DRPNetworkDelegate. I think it makes the most sense to have the
LayeredNetworkDelegate take ownership of the nested network delegate. Right now
we create the ChromeNetworkDelegate in ProfileIOData, pass ownership to
ProfileImplIOData, do the remaining ChromeNetworkDelegate specific calls, and
then have DRPNetworkDelegate take ownership of it.

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:464: profile->GetPrefs(),
On 2014/12/03 18:50:38, mmenke wrote:
> BUG:  We're on the IO thread.  it's not safe to dereference to the profile
here.

Done.

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:472:
chrome_network_delegate_ptr->set_predictor(predictor_.get());
On 2014/12/03 18:50:38, mmenke wrote:
> Can this just be moved to where chrome_network_delegate_ptr is, so we can get
> rid of chrome_network_delegate_ptr?

Done.

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:620:
chrome_network_delegate_ptr->set_domain_reliability_monitor(monitor);
On 2014/12/03 18:50:38, mmenke wrote:
> Again, think it's simpler if we can just move this up

Done.

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/734263003/diff/250001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:460: net::NetworkDelegate*
network_delegate() const {
On 2014/12/03 18:50:38, mmenke wrote:
> Think we can get rid of this - can just grab it from the appropriate context,
or
> off the stack, where it's needed, I believe.

Done.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
File net/base/layered_network_delegate_unittest.cc (right):

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:28: typedef std::map<const char*,
int> CountersMap;
On 2014/12/03 18:50:38, mmenke wrote:
> include map.

Done.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:30: void
VerifyCounters(CountersMap counters, int64 count) {
On 2014/12/03 18:50:38, mmenke wrote:
> Actually, can you just get rid of this method, and check that the
corresponding
> counter is 1 in each of the callbacks?  That'll also print a more useful error
> on failure.

Done.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:37: class TestProxyConfigService :
public ProxyConfigService {
On 2014/12/03 18:50:38, mmenke wrote:
> Not needed.  Can just use ProxyService::CreateDirect() and stick it in a
> scoped_ptr to create a proxy service for the test.

Done.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:69:
++counters_["on_before_url_request_count"];
On 2014/12/03 18:50:38, mmenke wrote:
> Hrm...  It's a pain, but if we want to be complete, perhaps we should have
check
> that the TestLayeredNetworkDelegate's corresponding counter has already been
> incremented on all of these.  Via a method, of
> course...IncrementAndCompareCounter("...");

Done.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:220:
OnRawBytesRead(*request.get(), 0);
On 2014/12/03 18:50:38, mmenke wrote:
> Think *request can be used instead of *request.get()

Done.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:231: *request.get(), GURL(),
GURL());
On 2014/12/03 18:50:38, mmenke wrote:
> Maybe use EXPECT_EQ for those that return values?

Done.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:331: CountersMap counters_;
On 2014/12/03 18:50:38, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:343:
MOCK_CONST_METHOD2(OnCanAccessFileInternal,
On 2014/12/03 18:50:38, mmenke wrote:
> Probably shouldn't be using gmock, so hacking on Chrome doesn't require
> knowledge of it.

Done.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:385:
OnCanAccessFileInternal(testing::_, testing::_));
On 2014/12/03 18:50:38, mmenke wrote:
> Why are these calls separated out?

Done.

https://codereview.chromium.org/734263003/diff/250001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:397:
layered_network_delegate_->VerifyCalled(1);
On 2014/12/03 18:50:38, mmenke wrote:
> Should compare the total number of calls to the two is the same.

This is now checked in IncrementAndCompareCounter

[mmenke, 2014-12-04 19:20:07]

Want to do another pass over the net/ tests, hopefully later today, but I think
this is looking really good, from a profile/ standpoint.

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

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:122:
io_data_->data_reduction_proxy_enabled_io()->Destroy();
Rather than having accessors for these, the handle can just access the private
fields directly (Inner classes have full access to the cuts of their parents -
see the code above to muck with the predictor_, for instance).

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:428:
scoped_ptr<ChromeNetworkDelegate> chrome_network_delegate,
On 2014/12/04 01:23:46, megjablon wrote:
> On 2014/12/03 18:50:38, mmenke wrote:
> > This passing ownership back and forth seems weird: 
> > ProfileData->ProfileImplIOData->ProfileIOData.  Can we just have the
> > profileImplIOData own it?
> 
> I'm not sure I understand. Can you explain? If we have ProfileImplIOData own
it,
> we would still have to pass it from ProfileIOData to ProfileImplIOData. Also,
we
> would have to add ownership of the nested network delegate to all other
classes
> that create a DRPNetworkDelegate. I think it makes the most sense to have the
> LayeredNetworkDelegate take ownership of the nested network delegate. Right
now
> we create the ChromeNetworkDelegate in ProfileIOData, pass ownership to
> ProfileImplIOData, do the remaining ChromeNetworkDelegate specific calls, and
> then have DRPNetworkDelegate take ownership of it.

With your current code, the ProfileIOData creates it, passes ownership of it to
ProfileImplIOData.  The ProfileImplIOData wraps in a DRPNetworkDelegate, and
then passes ownership of that back to the ProfileIOData.  Can we skip the last
step, and have the ProfileIOData keep ownership of the DRPNetworkDelegate (Or,
in the case of incognito mode, the ChromeNetworkDelegate).

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:462:
scoped_ptr<data_reduction_proxy::DataReductionProxyNetworkDelegate>
Maybe a linebreak here?  Or better, just pass in everything the
DRPNetworkDelegate needs into its constructor, rather than having 4 calls into
it initializing various things.  The profile code shouldn't know or care how it
uses the stuff we're passing into it.

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:474:
&data_reduction_proxy_enabled_ui_,
Why are you passing a preference that lives on the UI thread to an IO thread
object on the IO thread?  If the NetworkDelegate needs to check the value on the
UI thread, better to grab the value from data_reduction_proxy_enabled_io_ on the
IO thread, and pass that value back.  Otherwise, the profile may have been
deleted by the time you run the task on the UI thread, which would get you into
trouble.

Even if you have protects against that, very odd to be passing this at all on
the IO thread, and seems regression prone.

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:863: bool
ProfileImplIOData::IsDataReductionProxyEnabled() const {
Is this called from anywhere?  If so, fine to keep it.  If not, let's get rid of
it.

[mmenke, 2014-12-04 19:20:57]

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

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:428:
scoped_ptr<ChromeNetworkDelegate> chrome_network_delegate,
On 2014/12/04 19:20:07, mmenke wrote:
> On 2014/12/04 01:23:46, megjablon wrote:
> > On 2014/12/03 18:50:38, mmenke wrote:
> > > This passing ownership back and forth seems weird: 
> > > ProfileData->ProfileImplIOData->ProfileIOData.  Can we just have the
> > > profileImplIOData own it?
> > 
> > I'm not sure I understand. Can you explain? If we have ProfileImplIOData own
> it,
> > we would still have to pass it from ProfileIOData to ProfileImplIOData.
Also,
> we
> > would have to add ownership of the nested network delegate to all other
> classes
> > that create a DRPNetworkDelegate. I think it makes the most sense to have
the
> > LayeredNetworkDelegate take ownership of the nested network delegate. Right
> now
> > we create the ChromeNetworkDelegate in ProfileIOData, pass ownership to
> > ProfileImplIOData, do the remaining ChromeNetworkDelegate specific calls,
and
> > then have DRPNetworkDelegate take ownership of it.
> 
> With your current code, the ProfileIOData creates it, passes ownership of it
to
> ProfileImplIOData.  The ProfileImplIOData wraps in a DRPNetworkDelegate, and
> then passes ownership of that back to the ProfileIOData.  Can we skip the last
> step, and have the ProfileIOData keep ownership of the DRPNetworkDelegate (Or,
> in the case of incognito mode, the ChromeNetworkDelegate).

Have the ProfileImplIOData keep ownership, rather.

[megjablon, 2014-12-04 20:31:01]

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

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:122:
io_data_->data_reduction_proxy_enabled_io()->Destroy();
On 2014/12/04 19:20:07, mmenke wrote:
> Rather than having accessors for these, the handle can just access the private
> fields directly (Inner classes have full access to the cuts of their parents -
> see the code above to muck with the predictor_, for instance).

Done.

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:428:
scoped_ptr<ChromeNetworkDelegate> chrome_network_delegate,
On 2014/12/04 19:20:57, mmenke wrote:
> On 2014/12/04 19:20:07, mmenke wrote:
> > On 2014/12/04 01:23:46, megjablon wrote:
> > > On 2014/12/03 18:50:38, mmenke wrote:
> > > > This passing ownership back and forth seems weird: 
> > > > ProfileData->ProfileImplIOData->ProfileIOData.  Can we just have the
> > > > profileImplIOData own it?
> > > 
> > > I'm not sure I understand. Can you explain? If we have ProfileImplIOData
own
> > it,
> > > we would still have to pass it from ProfileIOData to ProfileImplIOData.
> Also,
> > we
> > > would have to add ownership of the nested network delegate to all other
> > classes
> > > that create a DRPNetworkDelegate. I think it makes the most sense to have
> the
> > > LayeredNetworkDelegate take ownership of the nested network delegate.
Right
> > now
> > > we create the ChromeNetworkDelegate in ProfileIOData, pass ownership to
> > > ProfileImplIOData, do the remaining ChromeNetworkDelegate specific calls,
> and
> > > then have DRPNetworkDelegate take ownership of it.
> > 
> > With your current code, the ProfileIOData creates it, passes ownership of it
> to
> > ProfileImplIOData.  The ProfileImplIOData wraps in a DRPNetworkDelegate, and
> > then passes ownership of that back to the ProfileIOData.  Can we skip the
last
> > step, and have the ProfileIOData keep ownership of the DRPNetworkDelegate
(Or,
> > in the case of incognito mode, the ChromeNetworkDelegate).
> 
> Have the ProfileImplIOData keep ownership, rather.

Done. Let me know if I misunderstood what you wanted.

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:462:
scoped_ptr<data_reduction_proxy::DataReductionProxyNetworkDelegate>
On 2014/12/04 19:20:07, mmenke wrote:
> Maybe a linebreak here?  Or better, just pass in everything the
> DRPNetworkDelegate needs into its constructor, rather than having 4 calls into
> it initializing various things.  The profile code shouldn't know or care how
it
> uses the stuff we're passing into it.

On webview we don't need all of these various arguments. This was so that we
could add specific DRP behavior to various platforms and know which arguments we
need to get that behavior. Since the Prefs and UMA are now sharing the
drp_enabled_ boolean pref I grouped those into one init call.

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:474:
&data_reduction_proxy_enabled_ui_,
On 2014/12/04 19:20:07, mmenke wrote:
> Why are you passing a preference that lives on the UI thread to an IO thread
> object on the IO thread?  If the NetworkDelegate needs to check the value on
the
> UI thread, better to grab the value from data_reduction_proxy_enabled_io_ on
the
> IO thread, and pass that value back.  Otherwise, the profile may have been
> deleted by the time you run the task on the UI thread, which would get you
into
> trouble.
> 
> Even if you have protects against that, very odd to be passing this at all on
> the IO thread, and seems regression prone.

Good to know! Thanks. Fixed.

https://codereview.chromium.org/734263003/diff/310001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:863: bool
ProfileImplIOData::IsDataReductionProxyEnabled() const {
On 2014/12/04 19:20:07, mmenke wrote:
> Is this called from anywhere?  If so, fine to keep it.  If not, let's get rid
of
> it.

Used here
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ren...

[mmenke, 2014-12-04 20:50:57]

LGTM

https://codereview.chromium.org/734263003/diff/330001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/734263003/diff/330001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_io_data.cc:203: 
nit:  Remove blank line

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

https://codereview.chromium.org/734263003/diff/330001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:491: network_delegate_ =
data_reduction_proxy_network_delegate.Pass();
nit:  Suggest moving this line to just below where
data_reduction_proxy_network_delegate is initialized, and modifying the above
one to use network_delegate_.get()

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
File net/base/layered_network_delegate_unittest.cc (right):

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:29: typedef std::map<const char*,
int> CountersMap;
Typedef and all other classes besides the test fixture need to be in an
anonymous namespace.  In this case, it looks like you can just put everything in
the anonymous namespace (Inside the net namespace).

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:34: :
layered_network_delegate_counters_(layered_network_delegate_counters) {
nit:  +1 indent

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:47: int
OnBeforeURLRequest(URLRequest* request,
nit:  "// NetworkDelegateImpl implementation:"

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:47: int
OnBeforeURLRequest(URLRequest* request,
nit:  Suggest making all the overrides public (But keeping
IncrementAndCompareCounter and the member variables private) - it's a little
more common to do that.

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:160:
"on_cancel_url_request_with_policy_violating_referrer_header_count");
+1 indent

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:167:
DISALLOW_COPY_AND_ASSIGN(TestNetworkDelegateImpl);
nit:  Should include base/macros.h for DISALLOW_COPY_AND_ASSIGN.

[sgurun-gerrit only, 2014-12-04 22:16:30]

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:187:
int64 received_content_length = request->received_response_content_length();
I think you should early out if the proxy is not enabled or data reduction proxy
usage stats are not collected. Webview does not have UMA for example.

[bengr, 2014-12-04 22:58:58]

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:146:
on_resolve_proxy_handler_.Run(url, load_flags,
nit: move these parameters to the next line, so you can fit more parameters per
line.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:39:
#if defined(OS_ANDROID)
DataReductionProxyAuthRequestHandler should provide a function to return the
client type.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:68:
: loop_proxy_(base::MessageLoopProxy::current().get()),
I think you should just be able to declare:

base::MessageLoopForIO loop_;

and then do:

base::RunLoop().Run();

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:80:
scoped_ptr<net::URLRequest> CreateURLRequest(
Maybe rename as FetchURL

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:81:
const GURL& url) {
Move up a line.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:121:
base::MessageLoop::current()->RunUntilIdle();
Factor out as much as you can from this function and the one above. It's
repetitive.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:128:
void SetDelegate(net::NetworkDelegate* delegate) {
Rename: set_network_delegate()

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:133:
// Required for base::MessageLoopProxy::current().
Shouldn't these be private too?

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:134:
base::MessageLoopForUI loop_;
Are you intending to run on UI or IO?

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:182:
EXPECT_TRUE(headers.HasHeader(kChromeProxyHeader));
You should probably also check that this header has ps and sid directives

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:298:
base::DictionaryValue* dict = NULL;
use nullptr

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:299:
base::Value* stats_value =
Use a scoped_ptr here so you don't have to remember to delete.

[megjablon, 2014-12-05 22:17:03]

Patchset #12 (id:350001) has been deleted

[megjablon, 2014-12-05 22:24:21]

https://codereview.chromium.org/734263003/diff/330001/chrome/browser/profiles...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/734263003/diff/330001/chrome/browser/profiles...
chrome/browser/profiles/off_the_record_profile_io_data.cc:203: 
On 2014/12/04 20:50:56, mmenke wrote:
> nit:  Remove blank line

Done.

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

https://codereview.chromium.org/734263003/diff/330001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:491: network_delegate_ =
data_reduction_proxy_network_delegate.Pass();
On 2014/12/04 20:50:56, mmenke wrote:
> nit:  Suggest moving this line to just below where
> data_reduction_proxy_network_delegate is initialized, and modifying the above
> one to use network_delegate_.get()

Done.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:146:
on_resolve_proxy_handler_.Run(url, load_flags,
On 2014/12/04 22:58:57, bengr wrote:
> nit: move these parameters to the next line, so you can fit more parameters
per
> line.

Done.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:187:
int64 received_content_length = request->received_response_content_length();
On 2014/12/04 22:16:30, sgurun wrote:
> I think you should early out if the proxy is not enabled or data reduction
proxy
> usage stats are not collected. Webview does not have UMA for example.

We still record content length histograms and update the prefs if the data
reduction proxy isn't enabled and usage stats are not collected.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:39:
#if defined(OS_ANDROID)
On 2014/12/04 22:58:57, bengr wrote:
> DataReductionProxyAuthRequestHandler should provide a function to return the
> client type. 

You have to give the DataReductionProxyAuthRequestHandler constructor the client
type and I'm using this to construct it. We usually use
DataReductionProxyChromeSettings::GetClient() but that lives in the chrome
directory.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:68:
: loop_proxy_(base::MessageLoopProxy::current().get()),
On 2014/12/04 22:58:57, bengr wrote:
> I think you should just be able to declare:
> 
> base::MessageLoopForIO loop_;
> 
> and then do:
> 
> base::RunLoop().Run();

Done.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:80:
scoped_ptr<net::URLRequest> CreateURLRequest(
On 2014/12/04 22:58:58, bengr wrote:
> Maybe rename as FetchURL

Done.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:81:
const GURL& url) {
On 2014/12/04 22:58:57, bengr wrote:
> Move up a line.

Done.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:121:
base::MessageLoop::current()->RunUntilIdle();
On 2014/12/04 22:58:58, bengr wrote:
> Factor out as much as you can from this function and the one above. It's
> repetitive.

Done.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:128:
void SetDelegate(net::NetworkDelegate* delegate) {
On 2014/12/04 22:58:57, bengr wrote:
> Rename: set_network_delegate()

Done.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:133:
// Required for base::MessageLoopProxy::current().
On 2014/12/04 22:58:57, bengr wrote:
> Shouldn't these be private too?

I'm using loop_ to get the MessageLoopProxy in the test below. I cant write a
getter for this if you prefer.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:134:
base::MessageLoopForUI loop_;
On 2014/12/04 22:58:57, bengr wrote:
> Are you intending to run on UI or IO?

Done.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:182:
EXPECT_TRUE(headers.HasHeader(kChromeProxyHeader));
On 2014/12/04 22:58:57, bengr wrote:
> You should probably also check that this header has ps and sid directives

Done.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:298:
base::DictionaryValue* dict = NULL;
On 2014/12/04 22:58:57, bengr wrote:
> use nullptr

Done.

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:299:
base::Value* stats_value =
On 2014/12/04 22:58:58, bengr wrote:
> Use a scoped_ptr here so you don't have to remember to delete.

Done.

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
File net/base/layered_network_delegate_unittest.cc (right):

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:29: typedef std::map<const char*,
int> CountersMap;
On 2014/12/04 20:50:56, mmenke wrote:
> Typedef and all other classes besides the test fixture need to be in an
> anonymous namespace.  In this case, it looks like you can just put everything
in
> the anonymous namespace (Inside the net namespace).

Done.

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:34: :
layered_network_delegate_counters_(layered_network_delegate_counters) {
On 2014/12/04 20:50:57, mmenke wrote:
> nit:  +1 indent

Done.

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:47: int
OnBeforeURLRequest(URLRequest* request,
On 2014/12/04 20:50:57, mmenke wrote:
> nit:  "// NetworkDelegateImpl implementation:"

Done.

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:160:
"on_cancel_url_request_with_policy_violating_referrer_header_count");
On 2014/12/04 20:50:57, mmenke wrote:
> +1 indent

Done.

https://codereview.chromium.org/734263003/diff/330001/net/base/layered_networ...
net/base/layered_network_delegate_unittest.cc:167:
DISALLOW_COPY_AND_ASSIGN(TestNetworkDelegateImpl);
On 2014/12/04 20:50:56, mmenke wrote:
> nit:  Should include base/macros.h for DISALLOW_COPY_AND_ASSIGN.

Done.

[sgurun-gerrit only, 2014-12-05 22:41:30]

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:187:
int64 received_content_length = request->received_response_content_length();
On 2014/12/05 22:24:20, megjablon wrote:
> On 2014/12/04 22:16:30, sgurun wrote:
> > I think you should early out if the proxy is not enabled or data reduction
> proxy
> > usage stats are not collected. Webview does not have UMA for example.
> 
> We still record content length histograms and update the prefs if the data
> reduction proxy isn't enabled and usage stats are not collected.

Why are we doing that if usage stats are not collected? I don't think it makes
sense for webview. does it?

[megjablon, 2014-12-06 00:37:22]

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:187:
int64 received_content_length = request->received_response_content_length();
On 2014/12/05 22:41:30, sgurun wrote:
> On 2014/12/05 22:24:20, megjablon wrote:
> > On 2014/12/04 22:16:30, sgurun wrote:
> > > I think you should early out if the proxy is not enabled or data reduction
> > proxy
> > > usage stats are not collected. Webview does not have UMA for example.
> > 
> > We still record content length histograms and update the prefs if the data
> > reduction proxy isn't enabled and usage stats are not collected.
> 
> Why are we doing that if usage stats are not collected? I don't think it makes
> sense for webview. does it?

This is a refactor so it would be best to not change the functionality in this
CL. Can you file a bug on us and I'll address it separately?

[bengr, 2014-12-06 01:05:39]

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:39:
#if defined(OS_ANDROID)
On 2014/12/05 22:24:20, megjablon wrote:
> On 2014/12/04 22:58:57, bengr wrote:
> > DataReductionProxyAuthRequestHandler should provide a function to return the
> > client type. 
> 
> You have to give the DataReductionProxyAuthRequestHandler constructor the
client
> type and I'm using this to construct it. We usually use
> DataReductionProxyChromeSettings::GetClient() but that lives in the chrome
> directory.

Why can't that live in the component?

https://codereview.chromium.org/734263003/diff/330001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:133:
// Required for base::MessageLoopProxy::current().
On 2014/12/05 22:24:20, megjablon wrote:
> On 2014/12/04 22:58:57, bengr wrote:
> > Shouldn't these be private too?
> 
> I'm using loop_ to get the MessageLoopProxy in the test below. I cant write a
> getter for this if you prefer.

Yes, please.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_metrics.cc
(right):

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_metrics.cc:470:
int received_content_length,
Can this be moved up a line?

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_metrics.cc:475:
int64 total_received = prefs->GetInt64(
DCHECK(prefs)

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:74:
DataReductionProxyAuthRequestHandler* handler,
params and handler must be non-null. Add a DCHECK for each.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:102:
DataReductionProxyUsageStats* usage_stats) {
Can UsageStats or StatisticsPrefs be NULL? If not, add DCHECKs.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:119:
base::DictionaryValue* dict = new base::DictionaryValue();
This is error prone. Return a scoped_ptr that you explicitly pass instead.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:131:
// Use strings to avoid overflow.  base::Value only supports 32-bit integers.
remove space before base::Value

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:45:
// DataReductionProxyNetworkDelegate is a LayeredNetworkDelegate that takes a
takes -> wraps

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:46:
// NetworkDelegate and adds data reduction proxy specific logic.
I think we should always capitalize "Data Reduction Proxy".

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:64:
// proxy resolution.
While you're here, please add a sentence describing how we use this.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:67:
// DataReductionProxyNetworkDelegate takes a |network_delegate| and extends
// Wraps a |network_delegate|, calling an internal implementation for each
delegate method. For example, the implementation of OnHeadersReceived() calls
OnHeadersReceivedInternal().


Also, please describe what the other params are.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:77:
// Initializes instance variables used for overriding the proxy config.
instance variables -> members (or member variables)

here and everywhere.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:79:
void InitProxyConfigOverrides(const OnResolveProxyHandler& proxy_handler);
maybe rename as InitProxyConfigOverrider?

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:89:
// Creates a Value summary of the persistent state of the network session.
|Value|

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:95:
// Creates a Value summary of the state of the network session. The caller is
|Value|

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:148:
BooleanPrefMember* data_reduction_proxy_enabled_;
Is this used?

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:69:
: context_(true) {
This can fit on the previous line.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:76:

Remove blank line.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:80:
scoped_ptr<net::URLRequest> FetchURLRequest(const GURL& url) {
Why can't this be implemented by passing in std::string() as the
raw_response_headers of the second FetchURLRequest()?

You'd also have to fix up the EXPECT_TRUE(...) there to only expect that if
non-empty.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:81:
scoped_ptr<net::URLRequest> fake_request = context_.CreateRequest(
What makes this request fake? Suggest removing "fake_"

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:100:
scoped_ptr<net::URLRequest> FetchURLRequest(
You shouldn't overload function names.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:132:
base::MessageLoopForIO loop_;
Make this private and add:

const base::MessageLoopForIO& loop() const {
  return loop_;
}

[megjablon, 2014-12-08 22:25:38]

Patchset #13 (id:390001) has been deleted

[megjablon, 2014-12-08 22:26:02]

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_metrics.cc
(right):

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_metrics.cc:470:
int received_content_length,
On 2014/12/06 01:05:38, bengr wrote:
> Can this be moved up a line?

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_metrics.cc:475:
int64 total_received = prefs->GetInt64(
On 2014/12/06 01:05:38, bengr wrote:
> DCHECK(prefs)

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:74:
DataReductionProxyAuthRequestHandler* handler,
On 2014/12/06 01:05:38, bengr wrote:
> params and handler must be non-null. Add a DCHECK for each.

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:102:
DataReductionProxyUsageStats* usage_stats) {
On 2014/12/06 01:05:38, bengr wrote:
> Can UsageStats or StatisticsPrefs be NULL? If not, add DCHECKs.

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:119:
base::DictionaryValue* dict = new base::DictionaryValue();
On 2014/12/06 01:05:38, bengr wrote:
> This is error prone. Return a scoped_ptr that you explicitly pass instead.

Let's file a bug for this and change outside of the refactor.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:131:
// Use strings to avoid overflow.  base::Value only supports 32-bit integers.
On 2014/12/06 01:05:38, bengr wrote:
> remove space before base::Value

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h
(right):

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:45:
// DataReductionProxyNetworkDelegate is a LayeredNetworkDelegate that takes a
On 2014/12/06 01:05:38, bengr wrote:
> takes -> wraps

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:46:
// NetworkDelegate and adds data reduction proxy specific logic.
On 2014/12/06 01:05:38, bengr wrote:
> I think we should always capitalize "Data Reduction Proxy".

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:64:
// proxy resolution.
On 2014/12/06 01:05:38, bengr wrote:
> While you're here, please add a sentence describing how we use this.

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:67:
// DataReductionProxyNetworkDelegate takes a |network_delegate| and extends
On 2014/12/06 01:05:38, bengr wrote:
> // Wraps a |network_delegate|, calling an internal implementation for each
> delegate method. For example, the implementation of OnHeadersReceived() calls
> OnHeadersReceivedInternal().
> 
> 
> Also, please describe what the other params are.

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:77:
// Initializes instance variables used for overriding the proxy config.
On 2014/12/06 01:05:38, bengr wrote:
> instance variables -> members (or member variables)
> 
> here and everywhere.

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:79:
void InitProxyConfigOverrides(const OnResolveProxyHandler& proxy_handler);
On 2014/12/06 01:05:38, bengr wrote:
> maybe rename as InitProxyConfigOverrider?

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:89:
// Creates a Value summary of the persistent state of the network session.
On 2014/12/06 01:05:38, bengr wrote:
> |Value|

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:95:
// Creates a Value summary of the state of the network session. The caller is
On 2014/12/06 01:05:38, bengr wrote:
> |Value|

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:148:
BooleanPrefMember* data_reduction_proxy_enabled_;
On 2014/12/06 01:05:38, bengr wrote:
> Is this used?

Yes. Used as an argument in RecordByptesHistograms and UpdateContentLengthPrefs.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:69:
: context_(true) {
On 2014/12/06 01:05:39, bengr wrote:
> This can fit on the previous line.

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:76:

On 2014/12/06 01:05:39, bengr wrote:
> Remove blank line.

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:80:
scoped_ptr<net::URLRequest> FetchURLRequest(const GURL& url) {
On 2014/12/06 01:05:38, bengr wrote:
> Why can't this be implemented by passing in std::string() as the
> raw_response_headers of the second FetchURLRequest()?
> 
> You'd also have to fix up the EXPECT_TRUE(...) there to only expect that if
> non-empty.

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:81:
scoped_ptr<net::URLRequest> fake_request = context_.CreateRequest(
On 2014/12/06 01:05:39, bengr wrote:
> What makes this request fake? Suggest removing "fake_"

Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:100:
scoped_ptr<net::URLRequest> FetchURLRequest(
On 2014/12/06 01:05:39, bengr wrote:
> You shouldn't overload function names.

The previous function was supposed to be deleted. Done.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:132:
base::MessageLoopForIO loop_;
On 2014/12/06 01:05:39, bengr wrote:
> Make this private and add:
> 
> const base::MessageLoopForIO& loop() const {
>   return loop_;
> }

Just made a getter for MessageLoopProxy instead.

[bengr, 2014-12-08 22:36:26]

lgtm with nits.

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/734263003/diff/370001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:119:
base::DictionaryValue* dict = new base::DictionaryValue();
On 2014/12/08 22:26:01, megjablon wrote:
> On 2014/12/06 01:05:38, bengr wrote:
> > This is error prone. Return a scoped_ptr that you explicitly pass instead.
> 
> Let's file a bug for this and change outside of the refactor.

Please do. Thanks.

https://codereview.chromium.org/734263003/diff/410001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/734263003/diff/410001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:111:
scoped_refptr<base::MessageLoopProxy> message_loop_proxy() {
You should be able to do:
const scoped_refptr<base::MessageLoopProxy>& message_loop_proxy() const {
  // ...
}

[megjablon, 2014-12-08 23:16:48]

https://codereview.chromium.org/734263003/diff/410001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/734263003/diff/410001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:111:
scoped_refptr<base::MessageLoopProxy> message_loop_proxy() {
On 2014/12/08 22:36:26, bengr wrote:
> You should be able to do:
> const scoped_refptr<base::MessageLoopProxy>& message_loop_proxy() const {
>   // ...
> }

Done.

[lcwu1, 2014-12-08 23:31:42]

lgtm on chromecast/.

[sgurun-gerrit only, 2014-12-09 00:12:48]

On 2014/12/08 23:31:42, lcwu1 wrote:
> lgtm on chromecast/.

aw lgtm

[megjablon, 2014-12-09 18:31:07]

The CQ bit was checked by megjablon@chromium.org

[commit-bot: I haz the power, 2014-12-09 18:31:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/734263003/450001

[commit-bot: I haz the power, 2014-12-09 19:47:19]

Committed patchset #15 (id:450001)

[commit-bot: I haz the power, 2014-12-09 19:48:06]

Patchset 15 (id:??) landed as
https://crrev.com/c1751458b9409f52c56b3f84d8226c57c39369d5
Cr-Commit-Position: refs/heads/master@{#307523}