Create abstraction for DataReductionProxyParams.

Create abstraction for DataReductionProxyParams.

This allows DRPConfig to use different parameter storage mechanisms (for
now, just DRP Params) and be isolated from their underlying
implementations.

BUG=466753
Committed: https://crrev.com/becf17df7886864575386a98829cd40ce5cf7212
Cr-Commit-Position: refs/heads/master@{#320855}

[jeremyim, 2015-03-12 20:04:30]

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

[jeremyim, 2015-03-12 20:04:31]

PTAL! =)

[sclittle, 2015-03-13 00:56:17]

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc:126:
EXPECT_CALL(
Is there some way we could avoid adding mocks here?

Maybe by bypassing the DRP in context_.proxy_server(), or setting the URL of the
URLRequest to something that will or won't be covered by local bypass rules?

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

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:153:
scoped_ptr<base::ListValue> proxy_list =
nit: Maybe not within the scope of this CL, but right now this function converts
the ProxyList of ProxyServers into a ListValue of strings, then converts each
string back into a ProxyServer :(

This function could just loop through proxies->GetAll(), which is a
vector<ProxyServer>&.

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

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:145:
bool AreProxiesBypassed(
Why is this method public? Would it be possible to just test
AreDataReductionProxiesBypassed instead?

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

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:312:
// The retry map has the scheme prefix for https but not for http
Comments should be complete sentences.

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_config_values.h
(right):

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_config_values.h:68:
virtual const net::ProxyServer& alt_origin() const = 0;
What about alt_fallback_origin?

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:166:
// Returns the alternative data reduction proxy fallback origin.
Either remove this comment, or add a comment to the other similar methods above.

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:167:
const net::ProxyServer& alt_fallback_origin() const;
Why isn't this exposed in ConfigValues?

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:171:
DataReductionProxyList GetAllowedProxies() const;
Could this be removed or made private?

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:174:
bool is_configured_on_command_line() const {
Could this be removed or made private?

[jeremyim, 2015-03-13 16:21:48]

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc:126:
EXPECT_CALL(
On 2015/03/13 00:56:16, sclittle wrote:
> Is there some way we could avoid adding mocks here?
> 
> Maybe by bypassing the DRP in context_.proxy_server(), or setting the URL of
the
> URLRequest to something that will or won't be covered by local bypass rules?

Mocking gives better control; if we try to play around with
context_.proxy_server(), it gets tightly bound to the implementation of
AreDataReductionProxiesBypassed - and returning true/false requires slightly
different nuanced setup - more complex than mocking the calls here.

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

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:153:
scoped_ptr<base::ListValue> proxy_list =
On 2015/03/13 00:56:16, sclittle wrote:
> nit: Maybe not within the scope of this CL, but right now this function
converts
> the ProxyList of ProxyServers into a ListValue of strings, then converts each
> string back into a ProxyServer :(
> 
> This function could just loop through proxies->GetAll(), which is a
> vector<ProxyServer>&.

Acknowledged. Bug 467024 filed.

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

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:145:
bool AreProxiesBypassed(
On 2015/03/13 00:56:16, sclittle wrote:
> Why is this method public? Would it be possible to just test
> AreDataReductionProxiesBypassed instead?

It was public before, but I can make it private.

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

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:312:
// The retry map has the scheme prefix for https but not for http
On 2015/03/13 00:56:16, sclittle wrote:
> Comments should be complete sentences.

Done.

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_config_values.h
(right):

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_config_values.h:68:
virtual const net::ProxyServer& alt_origin() const = 0;
On 2015/03/13 00:56:16, sclittle wrote:
> What about alt_fallback_origin?

It doesn't actually get called by anyone, but added.

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:166:
// Returns the alternative data reduction proxy fallback origin.
On 2015/03/13 00:56:16, sclittle wrote:
> Either remove this comment, or add a comment to the other similar methods
above.

Done.

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:167:
const net::ProxyServer& alt_fallback_origin() const;
On 2015/03/13 00:56:16, sclittle wrote:
> Why isn't this exposed in ConfigValues?

Done.

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:171:
DataReductionProxyList GetAllowedProxies() const;
On 2015/03/13 00:56:16, sclittle wrote:
> Could this be removed or made private?

Done.

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:174:
bool is_configured_on_command_line() const {
On 2015/03/13 00:56:16, sclittle wrote:
> Could this be removed or made private?

Done.

[sclittle, 2015-03-13 19:10:18]

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc:126:
EXPECT_CALL(
On 2015/03/13 16:21:47, jeremyim wrote:
> On 2015/03/13 00:56:16, sclittle wrote:
> > Is there some way we could avoid adding mocks here?
> > 
> > Maybe by bypassing the DRP in context_.proxy_server(), or setting the URL of
> the
> > URLRequest to something that will or won't be covered by local bypass rules?
> 
> Mocking gives better control; if we try to play around with
> context_.proxy_server(), it gets tightly bound to the implementation of
> AreDataReductionProxiesBypassed - and returning true/false requires slightly
> different nuanced setup - more complex than mocking the calls here.

nit: This might be giving the test too much control - it's relying on
WillStartUsingNetwork calling AreDataReductionProxiesBypassed exactly once. For
example, if for some reason someone makes an isometric change to
WillStartUsingNetwork that causes it to determine whether the proxies are
bypassed in some other way, then this test will break, even though the code
still works.

The test says "Expect ... when the data reduction proxy is bypassed", so I'm
suggesting actually bypassing the DRP.

This isn't a big deal here though, and restructuring these tests might be
outside the scope of this CL.

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:146:
virtual bool ContainsDataReductionProxy(
const?

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:153:
bool allowed();
const?

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:157:
bool alternative_allowed();
const?

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:161:
bool promo_allowed();
const?

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:206:
DataReductionProxyList proxies = GetAllowedProxies();
Remove GetAllowedProxies, and just test config()->allowed() and
config()->origin() and stuff directly in this test.

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:196:
bool alt_allowed_;
Why are these protected? Can they be private like |allowed_| and
|fallback_allowed_|?

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h
(right):

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h:62:
bool alt_allowed() const;
Doesn't ConfigValues already define a alternative_allowed() method? Same for
alternative_fallback_allowed().

[jeremyim, 2015-03-13 21:03:55]

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/1004853002/diff/20001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_debug_resource_throttle_unittest.cc:126:
EXPECT_CALL(
On 2015/03/13 19:10:17, sclittle wrote:
> On 2015/03/13 16:21:47, jeremyim wrote:
> > On 2015/03/13 00:56:16, sclittle wrote:
> > > Is there some way we could avoid adding mocks here?
> > > 
> > > Maybe by bypassing the DRP in context_.proxy_server(), or setting the URL
of
> > the
> > > URLRequest to something that will or won't be covered by local bypass
rules?
> > 
> > Mocking gives better control; if we try to play around with
> > context_.proxy_server(), it gets tightly bound to the implementation of
> > AreDataReductionProxiesBypassed - and returning true/false requires slightly
> > different nuanced setup - more complex than mocking the calls here.
> 
> nit: This might be giving the test too much control - it's relying on
> WillStartUsingNetwork calling AreDataReductionProxiesBypassed exactly once.
For
> example, if for some reason someone makes an isometric change to
> WillStartUsingNetwork that causes it to determine whether the proxies are
> bypassed in some other way, then this test will break, even though the code
> still works.
> 
> The test says "Expect ... when the data reduction proxy is bypassed", so I'm
> suggesting actually bypassing the DRP.
> 
> This isn't a big deal here though, and restructuring these tests might be
> outside the scope of this CL.

Yeah, it's definitely not ideal; I'm not sure what the best way to restructure
the tests would be, since crafting the DRP config + URLRequest objects seem
similarly fragile.

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:146:
virtual bool ContainsDataReductionProxy(
On 2015/03/13 19:10:18, sclittle wrote:
> const?

Done.

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:153:
bool allowed();
On 2015/03/13 19:10:17, sclittle wrote:
> const?

Done.

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:157:
bool alternative_allowed();
On 2015/03/13 19:10:17, sclittle wrote:
> const?

Done.

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:161:
bool promo_allowed();
On 2015/03/13 19:10:18, sclittle wrote:
> const?

Done.

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:206:
DataReductionProxyList proxies = GetAllowedProxies();
On 2015/03/13 19:10:18, sclittle wrote:
> Remove GetAllowedProxies, and just test config()->allowed() and
> config()->origin() and stuff directly in this test.

Honestly, this test doesn't really make any sense. Just removing it altogether.

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h
(right):

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:196:
bool alt_allowed_;
On 2015/03/13 19:10:18, sclittle wrote:
> Why are these protected? Can they be private like |allowed_| and
> |fallback_allowed_|?

Done.

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h
(right):

https://codereview.chromium.org/1004853002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/common/data_reduction_proxy_params_test_utils.h:62:
bool alt_allowed() const;
On 2015/03/13 19:10:18, sclittle wrote:
> Doesn't ConfigValues already define a alternative_allowed() method? Same for
> alternative_fallback_allowed().

Done.

[bengr, 2015-03-16 23:31:06]

lgtm, presuming you didn't change any of the current functionality.

[jeremyim, 2015-03-17 00:22:13]

The CQ bit was checked by jeremyim@chromium.org

[commit-bot: I haz the power, 2015-03-17 00:22:52]

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

[commit-bot: I haz the power, 2015-03-17 03:23:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-17 03:23:38]

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