Wrapped data reduction proxy initialization into its own class

Wrapped data reduction proxy initialization into its own class

The data reduction proxy code is initialized in profile code
in Chrome, primarily in ProfileImplIOData, and in the
AwBrowserContext and AwURLRequestContextGetter. This code is
rather involved. This change moves that initialization logic
to a new class, DataReductionProxyIOData, to avoid
duplication across platforms, and reduce the potential for regressions.

This CL also completely removes DRP support from the system network delegate.

BUG=429732, 447346
Committed: https://crrev.com/9463b577e5b356ee515c0b98ecacf3b6b779394b
Cr-Commit-Position: refs/heads/master@{#312975}

[bengr, 2014-12-16 00:25:00]

Patchset #1 (id:1) has been deleted

[bengr, 2014-12-16 00:25:11]

Patchset #1 (id:20001) has been deleted

[bengr, 2014-12-16 00:25:18]

Patchset #1 (id:40001) has been deleted

[bengr, 2014-12-16 00:25:26]

Patchset #1 (id:60001) has been deleted

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

Patchset #1 (id:80001) has been deleted

[bengr, 2014-12-16 00:40:01]

Patchset #1 (id:100001) has been deleted

[bengr, 2014-12-16 00:47:58]

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

[bengr, 2014-12-16 00:47:59]

mmenke: chrome/browser/* and please take a look at the approach, in general.

[mmenke, 2014-12-16 16:42:51]

On 2014/12/16 00:47:59, bengr wrote:
> mmenke: chrome/browser/* and please take a look at the approach, in general.

As much as I love cleanup CLs, may not be able to get to this until tomorrow.

[bengr, 2014-12-16 18:08:16]

On 2014/12/16 16:42:51, mmenke wrote:
> On 2014/12/16 00:47:59, bengr wrote:
> > mmenke: chrome/browser/* and please take a look at the approach, in general.
> 
> As much as I love cleanup CLs, may not be able to get to this until tomorrow.

There's no rush on this.

[mmenke, 2014-12-17 21:37:22]

Think this is much better, though there's still a lot of room for improvement,
here, and in future CLs.

Here are some preliminary comments, haven't really dug into it, though.

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

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:608:
globals_->data_reduction_proxy_io_data->network_delegate());
BUG:  You're storing the DRP's network delegate in two scoped_ptr's at once.

I suggest getting rid of the first and last parameters to Init() above, and
instead add a method:  scoped_ptr<...>
DRPIoData::CreateNetworkDelegate(wrapped_delegate.Pass())

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:25:
DataReductionProxyChromeConfigurator** configurator) {
This still seems like too much code hooking together DRP objects - suspect a lot
of this could be moved into DRP/ as well.

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:83:
base::Unretained(data_reduction_proxy_chrome_configurator)),
Passing a DataReductionProxyConfigurator and a bound method of it to the
DRPIOData is weird.  Instead, if it has to be a chrome/ class, you should make
an abstract interface in your component, with a virtual destructor and method to
get the proxy config, and then make the chrome/ class implement those methods,
and then just pass ownership here, without passing the callback.

If you need a configurator that always returns the same value for
CreateDataReductionProxyChromeIODataForSystem, then just make another subclass
of it that takes a configuration on completion.

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:87:
base::Unretained(data_reduction_proxy_chrome_settings)),
+4 indent

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:87:
base::Unretained(data_reduction_proxy_chrome_settings)),
Again, should just pass in the settings, and have the drp call its method
explicitly.  No need for bound callbacks.

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

https://codereview.chromium.org/778463002/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:76:
void DataReductionProxyIOData::CreateStatisticsPrefs(
I'd suggest calling this EnableStatisticsLogging.  Think it's clearer.

https://codereview.chromium.org/778463002/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:103:
if (capture_statistics) {
Can we just automatically do this if we have a statistics_prefs_ when creating
the proxy delegate?  See other suggestion about a rename, too.

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

https://codereview.chromium.org/778463002/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:43:
scoped_ptr<DataReductionProxyConfigurator> configurator,
Does this even compile?  DRP doesn't know how to delete this object, and it
can't depend on c/b/n/drp/DataReductionProxyConfigurator.

[bengr, 2014-12-19 20:05:58]

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

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:608:
globals_->data_reduction_proxy_io_data->network_delegate());
On 2014/12/17 21:37:22, mmenke (OOO 12-20 to 1-4) wrote:
> BUG:  You're storing the DRP's network delegate in two scoped_ptr's at once.
> 
> I suggest getting rid of the first and last parameters to Init() above, and
> instead add a method:  scoped_ptr<...>
> DRPIoData::CreateNetworkDelegate(wrapped_delegate.Pass())

Uggh. Thanks for catching this. I didn't do what you suggested, but did do
something.

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:25:
DataReductionProxyChromeConfigurator** configurator) {
On 2014/12/17 21:37:22, mmenke (OOO 12-20 to 1-4) wrote:
> This still seems like too much code hooking together DRP objects - suspect a
lot
> of this could be moved into DRP/ as well.

After my next CL, the event store and the configurator can go in the DRPIOData,
but not yet.

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:83:
base::Unretained(data_reduction_proxy_chrome_configurator)),
On 2014/12/17 21:37:22, mmenke (OOO 12-20 to 1-4) wrote:
> Passing a DataReductionProxyConfigurator and a bound method of it to the
> DRPIOData is weird.  Instead, if it has to be a chrome/ class, you should make
> an abstract interface in your component, with a virtual destructor and method
to
> get the proxy config, and then make the chrome/ class implement those methods,
> and then just pass ownership here, without passing the callback.
> 
> If you need a configurator that always returns the same value for
> CreateDataReductionProxyChromeIODataForSystem, then just make another subclass
> of it that takes a configuration on completion.

Done.

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:87:
base::Unretained(data_reduction_proxy_chrome_settings)),
On 2014/12/17 21:37:22, mmenke (OOO 12-20 to 1-4) wrote:
> +4 indent

Done.

https://codereview.chromium.org/778463002/diff/140001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:87:
base::Unretained(data_reduction_proxy_chrome_settings)),
On 2014/12/17 21:37:22, mmenke (OOO 12-20 to 1-4) wrote:
> Again, should just pass in the settings, and have the drp call its method
> explicitly.  No need for bound callbacks.

Done.

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

https://codereview.chromium.org/778463002/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:76:
void DataReductionProxyIOData::CreateStatisticsPrefs(
On 2014/12/17 21:37:22, mmenke (OOO 12-20 to 1-4) wrote:
> I'd suggest calling this EnableStatisticsLogging.  Think it's clearer.

Done.

https://codereview.chromium.org/778463002/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:103:
if (capture_statistics) {
On 2014/12/17 21:37:22, mmenke (OOO 12-20 to 1-4) wrote:
> Can we just automatically do this if we have a statistics_prefs_ when creating
> the proxy delegate?  See other suggestion about a rename, too.

No. This is for UMA, the other is for metrics that are presented to the user
iirc. I've renamed to make that clearer.

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

https://codereview.chromium.org/778463002/diff/140001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:43:
scoped_ptr<DataReductionProxyConfigurator> configurator,
On 2014/12/17 21:37:22, mmenke (OOO 12-20 to 1-4) wrote:
> Does this even compile?  DRP doesn't know how to delete this object, and it
> can't depend on c/b/n/drp/DataReductionProxyConfigurator.

I'm not sure what to do here.

[mmenke, 2015-01-08 18:21:06]

Sorry for slowness - probably won't get to this today, but will get to it
tomorrow.

[bengr, 2015-01-08 18:25:22]

On 2015/01/08 18:21:06, mmenke wrote:
> Sorry for slowness - probably won't get to this today, but will get to it
> tomorrow.

No problem. I'll need to update this to include the changes to support the DRP
as a best effort proxy.

[mmenke, 2015-01-09 21:09:50]

One quick relevant comment.  (Well..and a bonus nit).

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

https://codereview.chromium.org/778463002/diff/160001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1063:
globals.data_reduction_proxy_io_data->proxy_delegate();
This seems like a bad idea.  DataReductionProxyIOData is a per profile class,
and the system request context also has its own.  However, the
DataReductionProxyParams and DataReductionProxyAuthRequestHandler of the system
context are used to create this delegate, which we then share between profiles
and with the system as well.

If we need both global data and per-profile data, they should clearly be
separated.  If everything can be per-profile instead, we should create a
ProxyDelegate on a per-profile basis.  Also seems like we really shouldn't be
setting this for the incognito profile.

https://codereview.chromium.org/778463002/diff/160001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/778463002/diff/160001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:5: 
nit:  Should be one blank line here, not two.

[bengr, 2015-01-14 02:01:46]

Patchset #4 (id:180001) has been deleted

[bengr, 2015-01-14 02:01:58]

Patchset #4 (id:200001) has been deleted

[bengr, 2015-01-14 21:36:16]

Patchset #6 (id:260001) has been deleted

[bengr, 2015-01-14 21:36:25]

Patchset #5 (id:240001) has been deleted

[bengr, 2015-01-14 21:36:35]

Patchset #4 (id:220001) has been deleted

[bengr, 2015-01-14 21:39:53]

bengr@chromium.org changed reviewers:
+ sclittle@chromium.org, sgurun@chromium.org

[bengr, 2015-01-14 21:39:54]

sgurun: android_webview
sclittle: components/data_reduction_proxy* and chrome/browser/net/spdyproxy/*
mmenke: chrome/browser/profile*, chrome/browser/io_thread*,
chrome/browser/net/predictor.cc

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

https://codereview.chromium.org/778463002/diff/160001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:1063:
globals.data_reduction_proxy_io_data->proxy_delegate();
On 2015/01/09 21:09:50, mmenke wrote:
> This seems like a bad idea.  DataReductionProxyIOData is a per profile class,
> and the system request context also has its own.  However, the
> DataReductionProxyParams and DataReductionProxyAuthRequestHandler of the
system
> context are used to create this delegate, which we then share between profiles
> and with the system as well.
> 
> If we need both global data and per-profile data, they should clearly be
> separated.  If everything can be per-profile instead, we should create a
> ProxyDelegate on a per-profile basis.  Also seems like we really shouldn't be
> setting this for the incognito profile.

Done.

https://codereview.chromium.org/778463002/diff/160001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/778463002/diff/160001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:5: 
On 2015/01/09 21:09:50, mmenke wrote:
> nit:  Should be one blank line here, not two.

Done.

[sclittle, 2015-01-14 22:54:47]

https://codereview.chromium.org/778463002/diff/280001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/778463002/diff/280001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:161:
data_reduction_proxy_settings_.reset(
Would it be possible to pull the initialization of DRPSettings out of here as
well?

https://codereview.chromium.org/778463002/diff/280001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:270: if
(data_reduction_proxy_settings_.get()) {
nit: do you need the .get() here?

https://codereview.chromium.org/778463002/diff/280001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:271:
data_reduction_proxy_settings_->InitDataReductionProxySettings(
Would it be possible to pull this initialization of DRPSettings out somehow?

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/io_threa...
chrome/browser/io_thread.h:42: class DataReductionProxyIOData;
nit: remove, since it's not used in this file

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:17: 
nit: add '#include "content/public/browser/browser_context.h"'

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

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:38: #include
"components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
Is this needed here?

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

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:886: 
nit: remove blank line

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

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:25: #include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h"
Remove this

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:258: bool
IsDataReductionProxyEnabled() {
Make const and define in .cc file

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h:28:
class ProxyConfig;
Remove, since you already #include ProxyConfig

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:25:
namespace data_reduction_proxy {
nit: add blank line before constructor

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:50:

nit: remove blank line

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:17:
class URLRequestInterceptor;
nit: either remove this forward declaration or remove the #include for
URLRequestInterceptor above

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:29:
// Contains and initializes all Data Reduction Proxy objects that operate on
nit: inconsistent capitalization of Data Reduction Proxy vs. data reduction
proxy

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:37:
// |proxy_config_getter| provides the current effective proxy configuration,
what |proxy_config_getter|?

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:39:
// The |unavailable_callback| is called when the Data Reduction Proxy is
what |unavailable_callback|?

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:56:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
Maybe remove the io_task_runner parameter, and use the member instead?

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:66:
// bypass protocol.
Does the caller own the returned interceptor?

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:93: 
nit: remove blank line

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:131:
scoped_ptr<DataReductionProxyUsageStats> bypass_stats_;
Why |bypass_stats_|, and not |usage_stats_|?

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:138:
scoped_ptr<DataReductionProxyNetworkDelegate> network_delegate_;
|network_delegate_| is never set.

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

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:14:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.h"
Why do you need this include?

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

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:156:
void OnResolveProxyHandler(const GURL& url,
Maybe move this function to an anonymous namespace in the .cc file, or inline it
into OnProxyResolvedInternal? It doesn't need to be globally visible; you could
just change the tests to call NotifyResolveProxy on a network delegate.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:191:
if (settings_)
When would this ever be null? Also, why not move this null check into
NotifyUnavailabilityIfChange, before you post the task?

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:47:
// MessageLoopProxy instance is owned by io_thread. |params| outlives
Update comment.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:135:
DataReductionProxySettings* settings_;
Can this be null?

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:162:
void RecordBypassedBytes(
nit: why is this declared below all the private members?

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc:217:
net::ERR_PROXY_CONNECTION_FAILED);
nit: indentation

[bengr, 2015-01-15 00:30:32]

https://codereview.chromium.org/778463002/diff/280001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/778463002/diff/280001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:161:
data_reduction_proxy_settings_.reset(
On 2015/01/14 22:54:46, sclittle wrote:
> Would it be possible to pull the initialization of DRPSettings out of here as
> well?

I considered that. For now, I'd like to leave it as is, though we could
certainly do that in a follow-up.

https://codereview.chromium.org/778463002/diff/280001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:270: if
(data_reduction_proxy_settings_.get()) {
On 2015/01/14 22:54:46, sclittle wrote:
> nit: do you need the .get() here?

Done. Call me old fashioned.

https://codereview.chromium.org/778463002/diff/280001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:271:
data_reduction_proxy_settings_->InitDataReductionProxySettings(
On 2015/01/14 22:54:46, sclittle wrote:
> Would it be possible to pull this initialization of DRPSettings out somehow?

Eventually, yes. Not in this CL, which is too large as it is.

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/io_threa...
chrome/browser/io_thread.h:42: class DataReductionProxyIOData;
On 2015/01/14 22:54:46, sclittle wrote:
> nit: remove, since it's not used in this file

Done.

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:17: 
On 2015/01/14 22:54:46, sclittle wrote:
> nit: add '#include "content/public/browser/browser_context.h"'

I forward declared it.

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

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:38: #include
"components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
On 2015/01/14 22:54:46, sclittle wrote:
> Is this needed here?

Nope. Done.

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

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:886: 
On 2015/01/14 22:54:46, sclittle wrote:
> nit: remove blank line

Done.

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

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:25: #include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h"
On 2015/01/14 22:54:46, sclittle wrote:
> Remove this

Done.

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.h:258: bool
IsDataReductionProxyEnabled() {
On 2015/01/14 22:54:46, sclittle wrote:
> Make const and define in .cc file

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator.h:28:
class ProxyConfig;
On 2015/01/14 22:54:46, sclittle wrote:
> Remove, since you already #include ProxyConfig

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:25:
namespace data_reduction_proxy {
On 2015/01/14 22:54:46, sclittle wrote:
> nit: add blank line before constructor

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:50:

On 2015/01/14 22:54:46, sclittle wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:17:
class URLRequestInterceptor;
On 2015/01/14 22:54:47, sclittle wrote:
> nit: either remove this forward declaration or remove the #include for
> URLRequestInterceptor above

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:29:
// Contains and initializes all Data Reduction Proxy objects that operate on
On 2015/01/14 22:54:46, sclittle wrote:
> nit: inconsistent capitalization of Data Reduction Proxy vs. data reduction
> proxy

Done, at least in this file.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:37:
// |proxy_config_getter| provides the current effective proxy configuration,
On 2015/01/14 22:54:47, sclittle wrote:
> what |proxy_config_getter|?

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:39:
// The |unavailable_callback| is called when the Data Reduction Proxy is
On 2015/01/14 22:54:46, sclittle wrote:
> what |unavailable_callback|?

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:56:
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner);
On 2015/01/14 22:54:47, sclittle wrote:
> Maybe remove the io_task_runner parameter, and use the member instead?

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:66:
// bypass protocol.
On 2015/01/14 22:54:47, sclittle wrote:
> Does the caller own the returned interceptor?

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:93: 
On 2015/01/14 22:54:47, sclittle wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:131:
scoped_ptr<DataReductionProxyUsageStats> bypass_stats_;
On 2015/01/14 22:54:46, sclittle wrote:
> Why |bypass_stats_|, and not |usage_stats_|?

We should rename DataReductionProxyUsageStats as DataReductionProxyBypassStats,
but not in this CL.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:138:
scoped_ptr<DataReductionProxyNetworkDelegate> network_delegate_;
On 2015/01/14 22:54:47, sclittle wrote:
> |network_delegate_| is never set.

Done.

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

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:14:
#include
"components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol.h"
On 2015/01/14 22:54:47, sclittle wrote:
> Why do you need this include?

Done.

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

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:156:
void OnResolveProxyHandler(const GURL& url,
On 2015/01/14 22:54:47, sclittle wrote:
> Maybe move this function to an anonymous namespace in the .cc file, or inline
it
> into OnProxyResolvedInternal? It doesn't need to be globally visible; you
could
> just change the tests to call NotifyResolveProxy on a network delegate.

Leaving as is. The function is used by tests.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.cc:191:
if (settings_)
On 2015/01/14 22:54:47, sclittle wrote:
> When would this ever be null? Also, why not move this null check into
> NotifyUnavailabilityIfChange, before you post the task?

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:47:
// MessageLoopProxy instance is owned by io_thread. |params| outlives
On 2015/01/14 22:54:47, sclittle wrote:
> Update comment.

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:135:
DataReductionProxySettings* settings_;
On 2015/01/14 22:54:47, sclittle wrote:
> Can this be null?

Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:162:
void RecordBypassedBytes(
On 2015/01/14 22:54:47, sclittle wrote:
> nit: why is this declared below all the private members?

Beats me. I fixed up the ordering of all functions so .cc matches .h Done.

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc
(right):

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats_unittest.cc:217:
net::ERR_PROXY_CONNECTION_FAILED);
On 2015/01/14 22:54:47, sclittle wrote:
> nit: indentation

Done.

[sclittle, 2015-01-15 02:18:56]

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/280001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:17: 
On 2015/01/15 00:30:31, bengr wrote:
> On 2015/01/14 22:54:46, sclittle wrote:
> > nit: add '#include "content/public/browser/browser_context.h"'
> 
> I forward declared it.

Actually, you don't need to forward declare it here unless you really want to,
since you already do in the header file.

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

https://codereview.chromium.org/778463002/diff/280001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.h:156:
void OnResolveProxyHandler(const GURL& url,
On 2015/01/15 00:30:32, bengr wrote:
> On 2015/01/14 22:54:47, sclittle wrote:
> > Maybe move this function to an anonymous namespace in the .cc file, or
inline
> it
> > into OnProxyResolvedInternal? It doesn't need to be globally visible; you
> could
> > just change the tests to call NotifyResolveProxy on a network delegate.
> 
> Leaving as is. The function is used by tests.

OK, could you add a TODO to factor this out then? You can assign the TODO to me
if you'd like.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:30:
class DataReductionProxyIOData {
Could you add unit test coverage for this?

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:36:
// proxy. |settings| provides a UI hook to signal when the proxy is
can |settings| be null? should |settings| be allowed to be null?

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:121:
// TODO(bengr): Rename as DataReductionProxyBypassStats.
Maybe move this TODO to usage_stats.h?

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:122:
scoped_ptr<DataReductionProxyUsageStats> bypass_stats_;
shouldn't this be |usage_stats_| until that renaming happens?

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h
(right):

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:47:
// MessageLoopProxy instance is owned by io_thread. |params| outlives
Where's the MessageLoopProxy instance?

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:58:
// Callback intended to be called from |ChromeNetworkDelegate| when a
nit: DataReductionProxyNetworkDelegate, not ChromeNetworkDelegate

[sgurun-gerrit only, 2015-01-16 19:02:03]

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:168:
data_reduction_proxy_settings_->params()->Clone(),
you are already passing settings to DataReductionProxyIOData constructor, and it
seems settings owns the params anyway, why not move the clone there and drop an
extra parameter.

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:274:
GetAwURLRequestContext()->GetNetLog(),
you are already passing the urlrequestcontext above. I think you should get the
netlog from it, and then drop one parameter here.

[mmenke, 2015-01-16 19:03:21]

I'm relying on the other reviewers to check for correctness of the changes, but
I'm really happy with this.

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

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:597: globals_->system_network_delegate =
chrome_network_delegate.Pass();
So we're completely removing DRP support from the system network delegate? 
Should have a comment about this in the CL description.

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:658:
&data_reduction_proxy_configurator).Pass();
Does any of this stuff need to be done in profile_impl?  Seems like it really
belongs in ProfileImplIOData.cc now...Either in Handle::Init or
Handle::LazyInitialize.

Could even put it in the non-off-the-record-block of
ProfileIOData::InitializeOnUIThread.

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:675:
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(this)->
Cant this call be moved before io_data_.Init?  I assume not, because this
depends on the request context.  If we could, it could be nice if we could make
CreateDataReductionProxyChromeIOData do that call as well.

Also seems kinda weird to set up the IO thread stuff first.

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:678: g_browser_process->local_state(),
GetRequestContext(), net_log,
Question:  Not for this CL, but for the future...Do we need to use the profile's
RequestContext here?  If we could use the system request context, we'd get rid
of the circular dependency.  We're bypassing the proxy settings anyways.

The downside is that I don't think we currently create a URLRequestContextGetter
on the UI thread for the system context (Or the PAC context, for that matter).

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

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:346:
io_data_->data_reduction_proxy_io_data()->InitPrefsOnUIThread(pref_service);
We're already passing the prefs to CreateDataReductionProxyChromeIOData, which
returns the DRP_io_data in the first place.  Can't that method also call this
one?

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:5: 
nit:  Remove extra linebreak.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:40:
ui_task_runner_(ui_task_runner) {
Strongly suggest having methods DCHECK that the correct task runners run tasks
on the current thread.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:60:
if (compression_stats_.get())
Do we ever create one of these without compression_stats now?  If that, that's
fine, but if not, shouldn't have the checks.

(Also, don't need the .get() for these checks)

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:76:
net::URLRequestInterceptor* DataReductionProxyIOData::CreateInterceptor() {
Know you'll have to call release() on it, but this should still probably be
returning a scoped_ptr

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:93:
bool track_proxy_bypass_statistics) {
Is track_proxy_bypass_statistics ever false now?  Fine if it is, just making
sure.

[sgurun-gerrit only, 2015-01-16 19:04:40]

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

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:205:
builder.set_network_delegate(
also please add a comment here explaining what false means.

[bengr, 2015-01-17 00:20:48]

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:168:
data_reduction_proxy_settings_->params()->Clone(),
On 2015/01/16 19:02:03, sgurun wrote:
> you are already passing settings to DataReductionProxyIOData constructor, and
it
> seems settings owns the params anyway, why not move the clone there and drop
an
> extra parameter.

Done.

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:274:
GetAwURLRequestContext()->GetNetLog(),
On 2015/01/16 19:02:03, sgurun wrote:
> you are already passing the urlrequestcontext above. I think you should get
the
> netlog from it, and then drop one parameter here.

That's a different object. Above I create a DRPIOData object. Here I initialize
a DRPSettings object.

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

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
android_webview/browser/net/aw_url_request_context_getter.cc:205:
builder.set_network_delegate(
On 2015/01/16 19:04:40, sgurun wrote:
> also please add a comment here explaining what false means.

Done.

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

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:597: globals_->system_network_delegate =
chrome_network_delegate.Pass();
On 2015/01/16 19:03:20, mmenke wrote:
> So we're completely removing DRP support from the system network delegate? 
> Should have a comment about this in the CL description.

Done.

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:658:
&data_reduction_proxy_configurator).Pass();
On 2015/01/16 19:03:20, mmenke wrote:
> Does any of this stuff need to be done in profile_impl?  Seems like it really
> belongs in ProfileImplIOData.cc now...Either in Handle::Init or
> Handle::LazyInitialize.
> 
> Could even put it in the non-off-the-record-block of
> ProfileIOData::InitializeOnUIThread.

I think the answer is yes, for now, because InitDataReductionSettings below
needs byproducts of this construction.

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:675:
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(this)->
On 2015/01/16 19:03:20, mmenke wrote:
> Cant this call be moved before io_data_.Init?  I assume not, because this
> depends on the request context.  If we could, it could be nice if we could
make
> CreateDataReductionProxyChromeIOData do that call as well.
> 
> Also seems kinda weird to set up the IO thread stuff first.

Ack that this is kinda weird and not ideal. Unfortunately it can't be moved
before io_data_.Init(). I think jeremyim's upcoming changes will move the io
dependencies out of this call, so hopefully we'll be able to find a better place
for it then.

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:678: g_browser_process->local_state(),
GetRequestContext(), net_log,
On 2015/01/16 19:03:20, mmenke wrote:
> Question:  Not for this CL, but for the future...Do we need to use the
profile's
> RequestContext here?  If we could use the system request context, we'd get rid
> of the circular dependency.  We're bypassing the proxy settings anyways.
> 
> The downside is that I don't think we currently create a
URLRequestContextGetter
> on the UI thread for the system context (Or the PAC context, for that matter).

See above reply. Even better is that we'd remove the need for a request context
at all from DRPSettings.

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

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:346:
io_data_->data_reduction_proxy_io_data()->InitPrefsOnUIThread(pref_service);
On 2015/01/16 19:03:20, mmenke wrote:
> We're already passing the prefs to CreateDataReductionProxyChromeIOData, which
> returns the DRP_io_data in the first place.  Can't that method also call this
> one?

Done.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:5: 
On 2015/01/16 19:03:21, mmenke wrote:
> nit:  Remove extra linebreak.

Done.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:40:
ui_task_runner_(ui_task_runner) {
On 2015/01/16 19:03:21, mmenke wrote:
> Strongly suggest having methods DCHECK that the correct task runners run tasks
> on the current thread.

Done.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:60:
if (compression_stats_.get())
On 2015/01/16 19:03:20, mmenke wrote:
> Do we ever create one of these without compression_stats now?  If that, that's
> fine, but if not, shouldn't have the checks.
> 
> (Also, don't need the .get() for these checks)

Done. Yeah, Webview constructs the io data object without compression_stats and
creates it later..

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:76:
net::URLRequestInterceptor* DataReductionProxyIOData::CreateInterceptor() {
On 2015/01/16 19:03:20, mmenke wrote:
> Know you'll have to call release() on it, but this should still probably be
> returning a scoped_ptr

Done.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:93:
bool track_proxy_bypass_statistics) {
On 2015/01/16 19:03:21, mmenke wrote:
> Is track_proxy_bypass_statistics ever false now?  Fine if it is, just making
> sure.

Yeah. in Webview.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h
(right):

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:30:
class DataReductionProxyIOData {
On 2015/01/15 02:18:56, sclittle wrote:
> Could you add unit test coverage for this?

Done.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:36:
// proxy. |settings| provides a UI hook to signal when the proxy is
On 2015/01/15 02:18:56, sclittle wrote:
> can |settings| be null? should |settings| be allowed to be null?

Done.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:121:
// TODO(bengr): Rename as DataReductionProxyBypassStats.
On 2015/01/15 02:18:56, sclittle wrote:
> Maybe move this TODO to usage_stats.h?

Done.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.h:122:
scoped_ptr<DataReductionProxyUsageStats> bypass_stats_;
On 2015/01/15 02:18:56, sclittle wrote:
> shouldn't this be |usage_stats_| until that renaming happens?

Done.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h
(right):

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:47:
// MessageLoopProxy instance is owned by io_thread. |params| outlives
On 2015/01/15 02:18:56, sclittle wrote:
> Where's the MessageLoopProxy instance?

Done.

https://codereview.chromium.org/778463002/diff/300001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_usage_stats.h:58:
// Callback intended to be called from |ChromeNetworkDelegate| when a
On 2015/01/15 02:18:56, sclittle wrote:
> nit: DataReductionProxyNetworkDelegate, not ChromeNetworkDelegate

Done.

[mmenke, 2015-01-17 00:29:55]

Quick response

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:658:
&data_reduction_proxy_configurator).Pass();
On 2015/01/17 00:20:47, bengr wrote:
> On 2015/01/16 19:03:20, mmenke wrote:
> > Does any of this stuff need to be done in profile_impl?  Seems like it
really
> > belongs in ProfileImplIOData.cc now...Either in Handle::Init or
> > Handle::LazyInitialize.
> > 
> > Could even put it in the non-off-the-record-block of
> > ProfileIOData::InitializeOnUIThread.
> 
> I think the answer is yes, for now, because InitDataReductionSettings below
> needs byproducts of this construction.

Right, I was thinking we could move that, too.

[sclittle, 2015-01-17 00:38:27]

On 2015/01/17 00:29:55, mmenke wrote:
> Quick response
> 
>
https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
> File chrome/browser/profiles/profile_impl.cc (right):
> 
>
https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
> chrome/browser/profiles/profile_impl.cc:658:
> &data_reduction_proxy_configurator).Pass();
> On 2015/01/17 00:20:47, bengr wrote:
> > On 2015/01/16 19:03:20, mmenke wrote:
> > > Does any of this stuff need to be done in profile_impl?  Seems like it
> really
> > > belongs in ProfileImplIOData.cc now...Either in Handle::Init or
> > > Handle::LazyInitialize.
> > > 
> > > Could even put it in the non-off-the-record-block of
> > > ProfileIOData::InitializeOnUIThread.
> > 
> > I think the answer is yes, for now, because InitDataReductionSettings below
> > needs byproducts of this construction.
> 
> Right, I was thinking we could move that, too.

components/data_reduction_proxy* and chrome/browser/net/spdyproxy/* LGTM

[sgurun-gerrit only, 2015-01-17 00:44:34]

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:274:
GetAwURLRequestContext()->GetNetLog(),
On 2015/01/17 00:20:47, bengr wrote:
> On 2015/01/16 19:02:03, sgurun wrote:
> > you are already passing the urlrequestcontext above. I think you should get
> the
> > netlog from it, and then drop one parameter here.
> 
> That's a different object. Above I create a DRPIOData object. Here I
initialize
> a DRPSettings object.

That is not what I mean. The second parameter of InitDataReductionProxySettings 
is a UrlRequestContextGetter which you can receive the netlog from, so there is
no need to pass one more param.

[bengr, 2015-01-21 23:28:02]

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
android_webview/browser/aw_browser_context.cc:274:
GetAwURLRequestContext()->GetNetLog(),
On 2015/01/17 00:44:34, sgurun wrote:
> On 2015/01/17 00:20:47, bengr wrote:
> > On 2015/01/16 19:02:03, sgurun wrote:
> > > you are already passing the urlrequestcontext above. I think you should
get
> > the
> > > netlog from it, and then drop one parameter here.
> > 
> > That's a different object. Above I create a DRPIOData object. Here I
> initialize
> > a DRPSettings object.
> 
> That is not what I mean. The second parameter of
InitDataReductionProxySettings 
> is a UrlRequestContextGetter which you can receive the netlog from, so there
is
> no need to pass one more param.

I'd like to use this interface on all platforms and there is no guarantee that a
request context will have a netlog.

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/778463002/diff/300001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl.cc:658:
&data_reduction_proxy_configurator).Pass();
On 2015/01/17 00:29:54, mmenke wrote:
> On 2015/01/17 00:20:47, bengr wrote:
> > On 2015/01/16 19:03:20, mmenke wrote:
> > > Does any of this stuff need to be done in profile_impl?  Seems like it
> really
> > > belongs in ProfileImplIOData.cc now...Either in Handle::Init or
> > > Handle::LazyInitialize.
> > > 
> > > Could even put it in the non-off-the-record-block of
> > > ProfileIOData::InitializeOnUIThread.
> > 
> > I think the answer is yes, for now, because InitDataReductionSettings below
> > needs byproducts of this construction.
> 
> Right, I was thinking we could move that, too.

Done.

[sgurun-gerrit only, 2015-01-21 23:30:16]

On 2015/01/21 23:28:02, bengr wrote:
>
https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
> File android_webview/browser/aw_browser_context.cc (right):
> 
>
https://codereview.chromium.org/778463002/diff/300001/android_webview/browser...
> android_webview/browser/aw_browser_context.cc:274:
> GetAwURLRequestContext()->GetNetLog(),
> On 2015/01/17 00:44:34, sgurun wrote:
> > On 2015/01/17 00:20:47, bengr wrote:
> > > On 2015/01/16 19:02:03, sgurun wrote:
> > > > you are already passing the urlrequestcontext above. I think you should
> get
> > > the
> > > > netlog from it, and then drop one parameter here.
> > > 
> > > That's a different object. Above I create a DRPIOData object. Here I
> > initialize
> > > a DRPSettings object.
> > 
> > That is not what I mean. The second parameter of
> InitDataReductionProxySettings 
> > is a UrlRequestContextGetter which you can receive the netlog from, so there
> is
> > no need to pass one more param.
> 
> I'd like to use this interface on all platforms and there is no guarantee that
a
> request context will have a netlog.
> 
ok then. aw lgtm

[mmenke, 2015-01-22 17:14:49]

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:10: #include
"chrome/browser/net/chrome_net_log.h"
Per suggestion in other file, can get rid of this.

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:41: //
(http://crbug.com/408264)
Doesn't belong in this CL, but could consider a 10-30 second delay here on
Android.  I don't think it's the end of the world if we lost a little data, and
mobile often has very bad storage performance.  Also, overwriting a lot to flash
is just, in general, not a great idea.

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.h (right):

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.h:11: class
ChromeNetLog;
Suggest just making this take a net::NetLog.  Doesn't seem to be any reason to
need a ChromeNetLog here.  The cc file will also not need to depend on the
net_log header, since it'll then just be passing the log around, without
implicitly casting it.

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

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:184:
io_data_->data_reduction_proxy_io_data()->configurator(),
Optional:  Can we just pass in the data_reduction_proxy_io_data, and have
InitDataReductionProxySettings grab the event store and configurator itself? 
Maybe the NetLog, too, actually, as the event_store presumably has access to it.

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

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:887: data_reduction_proxy_io_data_ =
data_reduction_proxy_io_data.Pass();
2 space indent...  Don't suppose we could just move
data_reduction_proxy_io_data_ over to the profile_impl_io_data, and get rid of
these methods?  Doesn't seem like there's any reason
ShutdownStatisicsPrefsOnUIThread needs to be in this file.

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1258:
data_reduction_proxy_io_data()->ShutdownStatisicsPrefsOnUIThread();
Suggest naming this ShutdownOnUIThread (The embedder shouldn't know what it's
actually shutting down, and that way, you don't have to rename it if you ever
need to have it do more stuff).

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:53:
DataReductionProxyIOData::~DataReductionProxyIOData() {
Can we DCHECK that ShutdownStatisicsPrefsOnUIThread was called first?

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:63:
DCHECK(thread_checker_.CalledOnValidThread());
This is really weird.  This class is called "...IOData", but the
"thread_checker_" makes sure we're being called on the UI thread.  Since we have
ui_task_runner_, can we get rid of the thread checker, and just make sure we're
called on that task runner's thread instead?

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:69:
void  DataReductionProxyIOData::ShutdownStatisicsPrefsOnUIThread() {
I don't think we need both this and DestroyPrefsOnUIThread.  They're currently
both called synchronously, one after another, on shutdown, so having both
doesn't seem to get us anything.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:89:
void DataReductionProxyIOData::EnableCompressionStatisticsLogging(
Suggest moving this up (Here and in the header file) with the other setup
methods that must be called on the UI thread.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc
(right):

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:25:
class CountingNetworkDelegate : public net::NetworkDelegateImpl {
This class should be in an anonymous namespace.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:50:

No blank line after public

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:63:

Remove blank line.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:66:
protected:
Google style guide forbids protect member variables.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:70:
net::CapturingNetLog net_log_;
Since you aren't actually digging through the log, can just use a standard
net::NetLog here.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:136:
}
}  // namespace data_reduction_proxy

[mmenke, 2015-01-22 17:17:22]

And sorry for the slow response - I completely forgot this one was in my court
(I have 15 CLs in my review list updated in the past 2 days, which makes it hard
to keep track of which ones I still need to respond to).

[bengr, 2015-01-23 20:09:04]

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:10: #include
"chrome/browser/net/chrome_net_log.h"
On 2015/01/22 17:14:48, mmenke wrote:
> Per suggestion in other file, can get rid of this.

Done.

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.cc:41: //
(http://crbug.com/408264)
On 2015/01/22 17:14:48, mmenke wrote:
> Doesn't belong in this CL, but could consider a 10-30 second delay here on
> Android.  I don't think it's the end of the world if we lost a little data,
and
> mobile often has very bad storage performance.  Also, overwriting a lot to
flash
> is just, in general, not a great idea.

It's not clear the data loss will be small. Chrome on Android and iOS is
commonly backgrounded immediately after the first page load or first few page
loads, and backgrounding sometimes shuts down the browser. We could investigate.
Agreed that it doesn't belong in this CL.

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/net/spdy...
File chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.h (right):

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/net/spdy...
chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_io_data.h:11: class
ChromeNetLog;
On 2015/01/22 17:14:48, mmenke wrote:
> Suggest just making this take a net::NetLog.  Doesn't seem to be any reason to
> need a ChromeNetLog here.  The cc file will also not need to depend on the
> net_log header, since it'll then just be passing the log around, without
> implicitly casting it.

Done.

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

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:184:
io_data_->data_reduction_proxy_io_data()->configurator(),
On 2015/01/22 17:14:48, mmenke wrote:
> Optional:  Can we just pass in the data_reduction_proxy_io_data, and have
> InitDataReductionProxySettings grab the event store and configurator itself? 
> Maybe the NetLog, too, actually, as the event_store presumably has access to
it.

Done.

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

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:887: data_reduction_proxy_io_data_ =
data_reduction_proxy_io_data.Pass();
On 2015/01/22 17:14:48, mmenke wrote:
> 2 space indent...  Don't suppose we could just move
> data_reduction_proxy_io_data_ over to the profile_impl_io_data, and get rid of
> these methods?  Doesn't seem like there's any reason
> ShutdownStatisicsPrefsOnUIThread needs to be in this file.

Not entirely. It is still used by safe browsing and to set up the ProxyDelegate.
Leaving as is.

https://codereview.chromium.org/778463002/diff/360001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1258:
data_reduction_proxy_io_data()->ShutdownStatisicsPrefsOnUIThread();
On 2015/01/22 17:14:48, mmenke wrote:
> Suggest naming this ShutdownOnUIThread (The embedder shouldn't know what it's
> actually shutting down, and that way, you don't have to rename it if you ever
> need to have it do more stuff).
> 

Done.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc
(right):

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:53:
DataReductionProxyIOData::~DataReductionProxyIOData() {
On 2015/01/22 17:14:49, mmenke wrote:
> Can we DCHECK that ShutdownStatisicsPrefsOnUIThread was called first?

Done.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:63:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/01/22 17:14:49, mmenke wrote:
> This is really weird.  This class is called "...IOData", but the
> "thread_checker_" makes sure we're being called on the UI thread.  Since we
have
> ui_task_runner_, can we get rid of the thread checker, and just make sure
we're
> called on that task runner's thread instead?

Done.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:69:
void  DataReductionProxyIOData::ShutdownStatisicsPrefsOnUIThread() {
On 2015/01/22 17:14:49, mmenke wrote:
> I don't think we need both this and DestroyPrefsOnUIThread.  They're currently
> both called synchronously, one after another, on shutdown, so having both
> doesn't seem to get us anything.

Done.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data.cc:89:
void DataReductionProxyIOData::EnableCompressionStatisticsLogging(
On 2015/01/22 17:14:48, mmenke wrote:
> Suggest moving this up (Here and in the header file) with the other setup
> methods that must be called on the UI thread.

Done.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc
(right):

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:25:
class CountingNetworkDelegate : public net::NetworkDelegateImpl {
On 2015/01/22 17:14:49, mmenke wrote:
> This class should be in an anonymous namespace.

Done.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:50:

On 2015/01/22 17:14:49, mmenke wrote:
> No blank line after public

Done.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:63:

On 2015/01/22 17:14:49, mmenke wrote:
> Remove blank line.

Done.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:66:
protected:
On 2015/01/22 17:14:49, mmenke wrote:
> Google style guide forbids protect member variables.

Done.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:70:
net::CapturingNetLog net_log_;
On 2015/01/22 17:14:49, mmenke wrote:
> Since you aren't actually digging through the log, can just use a standard
> net::NetLog here.

Done.

https://codereview.chromium.org/778463002/diff/360001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_io_data_unittest.cc:136:
}
On 2015/01/22 17:14:49, mmenke wrote:
> }  // namespace data_reduction_proxy

Done.

[mmenke, 2015-01-23 20:48:54]

LGTM!  Thanks for the cleanup!  The profile hooks look so much nicer!

Note that I haven't verified correctness of the DRP component changes.

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

https://codereview.chromium.org/778463002/diff/380001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:173: ChromeNetLog* const net_log
= g_browser_process->io_thread()->net_log();
optional:  Don't think const is necessary here.

[bengr, 2015-01-23 22:33:42]

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

https://codereview.chromium.org/778463002/diff/380001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:173: ChromeNetLog* const net_log
= g_browser_process->io_thread()->net_log();
On 2015/01/23 20:48:54, mmenke wrote:
> optional:  Don't think const is necessary here.

Leaving as is for now, because this is how the code was before the refactor.

[bengr, 2015-01-23 22:33:50]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2015-01-23 22:35:14]

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

[commit-bot: I haz the power, 2015-01-24 00:25:29]

Committed patchset #10 (id:400001)

[commit-bot: I haz the power, 2015-01-24 00:27:06]

Patchset 10 (id:??) landed as
https://crrev.com/9463b577e5b356ee515c0b98ecacf3b6b779394b
Cr-Commit-Position: refs/heads/master@{#312975}