Create a new DataReductionProxyConfig class

Create a new DataReductionProxyConfig class

This instance will be created by the DataReductionProxySettings class,
and take ownership of the DataReductionProxyParams (currently owned by
DataReductionProxySettings).

BUG=452773
Committed: https://crrev.com/32a19e6ad65292e1efc67986e6a61c0f29e393d3
Cr-Commit-Position: refs/heads/master@{#313980}

[jeremyim, 2015-01-28 04:36:31]

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

[jeremyim, 2015-01-28 04:39:42]

Patchset #1 (id:1) has been deleted

[jeremyim, 2015-01-28 04:40:18]

PTAL!

[bengr, 2015-01-29 01:37:26]

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:60:
net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
Does this work even if InitDataReductionProxyConfig was never called?

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:8:
#include "base/macros.h"
Is this needed?

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:16:
#include "net/url_request/url_fetcher.h"
Is this needed?

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:68:
// DataReductionProxyConfig will take ownership of |params|.
will take -> takes

And this would be clearer if you passed a scoped_ptr explicitly.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:72:
// Initializes the data reduction proxy config with a  |network_task_runner|,
Capitalize Data Reduction Proxy everywhere. At some point we should probably
change the name everywhere to "Data Saver" but not yet.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:79:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
I think we've been using the name io_task_runner, which is shorter. I don't care
too much.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:86:
DataReductionProxyParams* params() const { return params_.get(); }
Move the return to the next line.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:90:
// the proxy to use SPDY.
to use TLS.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:91:
virtual net::URLFetcher* GetURLFetcherForProbe();
Can this method be const?

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:111:
// Virtualized for mocking. Returns the list of network interfaces in use.
Must interfaces be non-null?

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:116:
friend class DataReductionProxyConfigTest;
Do you really need to friend the test class and the tests?

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:129:
// Adds the default proxy bypass rules for the data reduction proxy.
Capitalize DRP everywhere.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:157:
net::NetLog* net_log_;
Please explain why there's a NetLog* and a BoundNetLog in thi class.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:175:
net::BoundNetLog bound_net_log_;
Why must this be bound?

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:46:
// AddProxyToCommandLine();
Remove dead code.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:158:
scoped_ptr<DataReductionProxyConfigurator> configurator_;
These should be private.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:167:
// SetUp() adds the origin to the command line, which should be returned here.
It does?

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:198:
TestDataReductionProxyParams drp_params(
The test class creates params too. Do we need both?

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:259:
// Holdback.
What does this comment mean?

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:281:
CheckProbeOnIPChange(kProbeURLWithBadResponse,
The parameters here and in calls below could be merged into one or two lines.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.cc
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.cc:42:
ssl_origin_ = "";
std::string() instead of ""

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:30:
void Enable(bool restricted,
Add a comment:

// Overrides of DataReductionProxyConfigurator:

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:37:
void AddURLPatternToBypass(const std::string& pattern) override {}
Add a blank line between each method.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:41:
bool enabled_;
Member variables should all be private.

[jeremyim, 2015-01-29 06:29:24]

Patchset #2 (id:40001) has been deleted

[jeremyim, 2015-01-29 06:38:12]

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:60:
net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
On 2015/01/29 01:37:25, bengr wrote:
> Does this work even if InitDataReductionProxyConfig was never called?

Yes; if the observer can't be found, it short circuits and returns
(ObserverListThreadSafe::RemoveObserver)

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:8:
#include "base/macros.h"
On 2015/01/29 01:37:25, bengr wrote:
> Is this needed?

This is where DISALLOW_COPY_AND_ASSIGN is defined

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:16:
#include "net/url_request/url_fetcher.h"
On 2015/01/29 01:37:26, bengr wrote:
> Is this needed?

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:68:
// DataReductionProxyConfig will take ownership of |params|.
On 2015/01/29 01:37:25, bengr wrote:
> will take -> takes
> 
> And this would be clearer if you passed a scoped_ptr explicitly.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:72:
// Initializes the data reduction proxy config with a  |network_task_runner|,
On 2015/01/29 01:37:25, bengr wrote:
> Capitalize Data Reduction Proxy everywhere. At some point we should probably
> change the name everywhere to "Data Saver" but not yet.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:79:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
On 2015/01/29 01:37:25, bengr wrote:
> I think we've been using the name io_task_runner, which is shorter. I don't
care
> too much.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:86:
DataReductionProxyParams* params() const { return params_.get(); }
On 2015/01/29 01:37:26, bengr wrote:
> Move the return to the next line.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:90:
// the proxy to use SPDY.
On 2015/01/29 01:37:25, bengr wrote:
> to use TLS.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:91:
virtual net::URLFetcher* GetURLFetcherForProbe();
On 2015/01/29 01:37:25, bengr wrote:
> Can this method be const?

No, since we pass "this" as the URLFetcherDelegate into the create method.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:111:
// Virtualized for mocking. Returns the list of network interfaces in use.
On 2015/01/29 01:37:25, bengr wrote:
> Must interfaces be non-null?

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:116:
friend class DataReductionProxyConfigTest;
On 2015/01/29 01:37:26, bengr wrote:
> Do you really need to friend the test class and the tests?

Otherwise, we have to raise the visibility of some methods and add additional
test methods.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:129:
// Adds the default proxy bypass rules for the data reduction proxy.
On 2015/01/29 01:37:25, bengr wrote:
> Capitalize DRP everywhere.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:157:
net::NetLog* net_log_;
On 2015/01/29 01:37:25, bengr wrote:
> Please explain why there's a NetLog* and a BoundNetLog in thi class.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:175:
net::BoundNetLog bound_net_log_;
On 2015/01/29 01:37:25, bengr wrote:
> Why must this be bound?

Explained in the comment for net_log_.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:46:
// AddProxyToCommandLine();
On 2015/01/29 01:37:26, bengr wrote:
> Remove dead code.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:158:
scoped_ptr<DataReductionProxyConfigurator> configurator_;
On 2015/01/29 01:37:26, bengr wrote:
> These should be private.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:167:
// SetUp() adds the origin to the command line, which should be returned here.
On 2015/01/29 01:37:26, bengr wrote:
> It does?

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:198:
TestDataReductionProxyParams drp_params(
On 2015/01/29 01:37:26, bengr wrote:
> The test class creates params too. Do we need both?

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:259:
// Holdback.
On 2015/01/29 01:37:26, bengr wrote:
> What does this comment mean?

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:281:
CheckProbeOnIPChange(kProbeURLWithBadResponse,
On 2015/01/29 01:37:26, bengr wrote:
> The parameters here and in calls below could be merged into one or two lines.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.cc
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.cc:42:
ssl_origin_ = "";
On 2015/01/29 01:37:26, bengr wrote:
> std::string() instead of ""

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:30:
void Enable(bool restricted,
On 2015/01/29 01:37:26, bengr wrote:
> Add a comment:
> 
> // Overrides of DataReductionProxyConfigurator:

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:37:
void AddURLPatternToBypass(const std::string& pattern) override {}
On 2015/01/29 01:37:26, bengr wrote:
> Add a blank line between each method.

Done.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:41:
bool enabled_;
On 2015/01/29 01:37:26, bengr wrote:
> Member variables should all be private.

Done.

[bengr, 2015-01-30 18:28:15]

lgtm, but please address the cosmetic issues.

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/880963006/diff/20001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:8:
#include "base/macros.h"
On 2015/01/29 06:38:12, jeremyim wrote:
> On 2015/01/29 01:37:25, bengr wrote:
> > Is this needed?
> 
> This is where DISALLOW_COPY_AND_ASSIGN is defined

Right.

https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc
(right):

https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc:100:
new TestDataReductionProxyParams(
Why so indented?

https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h
(right):

https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:31:
// Overrides DataReductionProxyConfigurator:
I was thinking that you'd have one comment:

// Overrides of DataReductionProxyConfigurator

You don't need to repeat this for each override.

https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:47:
bool enabled() const { return enabled_; }
Move the return to its own line, here and below.

[jeremyim, 2015-01-30 18:35:14]

jeremyim@chromium.org changed reviewers:
+ sgurun@chromium.org

[jeremyim, 2015-01-30 18:35:15]

sgurun -> PTAL at aw_browser_context.cc

Thank you!

https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc
(right):

https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc:100:
new TestDataReductionProxyParams(
On 2015/01/30 18:28:15, bengr wrote:
> Why so indented?

Done.

https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h
(right):

https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:31:
// Overrides DataReductionProxyConfigurator:
On 2015/01/30 18:28:15, bengr wrote:
> I was thinking that you'd have one comment:
> 
> // Overrides of DataReductionProxyConfigurator
> 
> You don't need to repeat this for each override.

Done.

https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:47:
bool enabled() const { return enabled_; }
On 2015/01/30 18:28:15, bengr wrote:
> Move the return to its own line, here and below.

Done.

[sgurun-gerrit only, 2015-01-30 19:10:47]

On 2015/01/30 18:35:15, jeremyim wrote:
> sgurun -> PTAL at aw_browser_context.cc
> 
> Thank you!
> 
>
https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
> File
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_protocol_unittest.cc:100:
> new TestDataReductionProxyParams(
> On 2015/01/30 18:28:15, bengr wrote:
> > Why so indented?
> 
> Done.
> 
>
https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
> File
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h
> (right):
> 
>
https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:31:
> // Overrides DataReductionProxyConfigurator:
> On 2015/01/30 18:28:15, bengr wrote:
> > I was thinking that you'd have one comment:
> > 
> > // Overrides of DataReductionProxyConfigurator
> > 
> > You don't need to repeat this for each override.
> 
> Done.
> 
>
https://codereview.chromium.org/880963006/diff/80001/components/data_reductio...
>
components/data_reduction_proxy/core/browser/data_reduction_proxy_configurator_test_utils.h:47:
> bool enabled() const { return enabled_; }
> On 2015/01/30 18:28:15, bengr wrote:
> > Move the return to its own line, here and below.
> 
> Done.

aw lgtm

[jeremyim, 2015-01-30 19:14:12]

The CQ bit was checked by jeremyim@chromium.org

[commit-bot: I haz the power, 2015-01-30 19:15:36]

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

[commit-bot: I haz the power, 2015-01-30 20:38:00]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-01-30 20:39:55]

Patchset 4 (id:??) landed as
https://crrev.com/32a19e6ad65292e1efc67986e6a61c0f29e393d3
Cr-Commit-Position: refs/heads/master@{#313980}