Add the ability to use DataReductionProxyParams from DataReductionProxyConfigServiceClient.

Add the ability to use DataReductionProxyParams from DataReductionProxyConfigServiceClient.

- Permit DRP RequestOptions to take auth credentials instead of always internally
  generating them.
- Permit DRP Config to perform a reload of the configuration
- Create a new DRP MutableConfigValues class which can have its underlying values
  updated.
- Add a configuration refresh timer to DRP ConfigServiceClient, along with
  backoff logic.

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

[jeremyim, 2015-03-24 18:06:06]

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

[jeremyim, 2015-03-24 18:06:06]

PTAL. =)

[sclittle, 2015-03-24 21:34:56]

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:7:
#include <vector>
also include string

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:99:
const ProxyServer& fallback =
It's confusing to have both ProxyServer and net::ProxyServer here meaning
different things. 

Consider renaming data_reduction_proxy::ProxyServer to something else, or
otherwise differentiating between them somehow. The same issue might also come
up with data_reduction_proxy::ProxyConfig vs. net::ProxyConfig.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:44:
class DataReductionProxyConfigClientTest : public testing::Test {
Rename to DataReductionProxyConfigServiceClientTest

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:106:
EXPECT_CALL(*mock_config_client(), ConstructStaticResponse())
Why are you mocking the methods of DRPConfigClient while testing
DRPConfigClient? This seems to defeat the purpose of testing DRPConfigClient.

Couldn't you just set up DRPParams such that ConstructStaticResponse works
normally? You could even mock DRPParams if necessary.

IIUC, mocks should only be used for other interfaces that the class being tested
depends on, otherwise the test is just testing less of the class, and even then
fakes are preferable than mocks where feasible due to the overhead of learning
googlemock.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:110:
EXPECT_CALL(*mock_config_client(), SetConfigRefreshTimer(expected_delay))
Same as above: mocking the class being tested means that it tests less of the
class.

Maybe you could actually simulate that a day passes, and then check that it
constructs a new config. You could use a base::MockTimer or something; maybe you
could set up DRPConfigServiceClient to take in the timer in the constructor to
make this easier. Then you could just call MockTimer::Fire() to advance a day.

Ideally, you shouldn't have to test a fake or mock version of
DRPConfigServiceClient here at all, although you might still need to fake out
Now() somehow.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:45:
scoped_ptr<DataReductionProxyConfigValues> config_values,
Could this be changed to scoped_ptr<DataReductionProxyMutableConfigValues>, to
ensure that it's mutable?

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:73:
bool DataReductionProxyMutableConfigValues::IsDataReductionProxy(
This looks like duplicated code from DRPParams. Could this be put in a common
place, maybe by providing a default implementation under ConfigValues?

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:30:
DataReductionProxyParams* params);
Could this be const DataReductionProxyParams*?

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:331:
if (use_config_client_) {
Maybe DCHECK(!(use_mock_config_ && use_config_client_)) so that no one tries to
set both?

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:104:
MOCK_METHOD1(SetConfigRefreshTimer, void(const base::TimeDelta& delay));
These methods aren't part of any public interface, why are you mocking them? Is
there really no other way?

Why not just set up Params in the tests such that the right config is
constructed when RetrieveConfig is called?

[bengr, 2015-03-24 21:37:42]

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:93:
void DataReductionProxyConfig::ReloadConfig() {
Why is this method needed?

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:93:
// Reloads and applies the proxy configuration.
What does it mean to reload adn apply a configuration. Please explain.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:30:
const net::BackoffEntry::Policy kDefaultBackoffPolicy = {
Add a comment. Does this mean if you can't fetch a config, wait 10 seconds
before retrying, and then on each subsequent failure, double that delay until
you reach 30 minutes at which time you bail? Does that mean 8 attempts before
giving up?

I'd probably set initial_delay_ms to 1000, and multiply_factor to 4. That way
it's more aggressive up front and takes one fewer attempts before giving up.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:31:
// num_errors_to_ignore
Might be more readable if you put the comments to the right of the values.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:80:
void DataReductionProxyConfigServiceClient::RetrieveConfig() {
I'd expect RetrieveConfig to be asynchronous, or possibly to be sometimes
asynchronous. It could return a bool if the config is available synchronously.
I'd also expect it to take a config pointer, which it would populate.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:90:
if (config.proxy_config().http_proxy_servers_size() > 0) {
You could probably factor out the stuff that determines origin and fallback
origin into a helper.

E.g., std::vector<net::ProxyServer> GetProxiesForHTTP(const ClientConfig&
config) const;

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:136:
SetConfigRefreshTimer(next_config_refresh_time);
Perhaps factor out everything that determines next_config_refresh_time into a
helper?

e.g.:

base::TimeDelta GetNextConfigRefreshTime(bool last_fetch_status, base::Time
expiration_time) {...}

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:44:
TestDataReductionProxyConfig(
Add comment that explains how this Constructor is different.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:82:
}
bad indentation.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:23:
class DataReductionProxyMutableConfigValues
Add a class-level comment.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:30:
DataReductionProxyParams* params);
Can this be a const& ?

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:43:
bool fallback_allowed() const override;
The DRPParams code made the assumption that there would never be more than two
proxies for HTTP traffic. I didn't think we were still operating under this
assumption. I don't think the change has to be in this CL, but we should switch
to using a proxy list, and start reporting UMA based on attributes of the proxy
(e.g., scheme=https) instead of on position in the list.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:51:
const net::ProxyServer& fallback_origin() const override;
What's the case for exposing all of these values? Ideally, we'd have a proxy
config that provides a very simple interface, e.g.:

std::vector<net::ProxcyServer> GetProxies(const std::string& scheme) const;
bool IsDataReductionProxy(const net::ProxyServer& proxy) const;

[jeremyim, 2015-03-24 23:20:29]

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:93:
void DataReductionProxyConfig::ReloadConfig() {
On 2015/03/24 21:37:40, bengr wrote:
> Why is this method needed?

The config service client needs a way to notify that the configuration has
changed, and should be reloaded by config.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:93:
// Reloads and applies the proxy configuration.
On 2015/03/24 21:37:40, bengr wrote:
> What does it mean to reload adn apply a configuration. Please explain.

Done.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:7:
#include <vector>
On 2015/03/24 21:34:55, sclittle wrote:
> also include string

Done.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:30:
const net::BackoffEntry::Policy kDefaultBackoffPolicy = {
On 2015/03/24 21:37:41, bengr wrote:
> Add a comment. Does this mean if you can't fetch a config, wait 10 seconds
> before retrying, and then on each subsequent failure, double that delay until
> you reach 30 minutes at which time you bail? Does that mean 8 attempts before
> giving up?
> 
> I'd probably set initial_delay_ms to 1000, and multiply_factor to 4. That way
> it's more aggressive up front and takes one fewer attempts before giving up.

Done.

maximum_backoff_ms is the maximum permitted, but will be continually returned
(so we would try every 30 minutes).

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:31:
// num_errors_to_ignore
On 2015/03/24 21:37:40, bengr wrote:
> Might be more readable if you put the comments to the right of the values.

Done.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:80:
void DataReductionProxyConfigServiceClient::RetrieveConfig() {
On 2015/03/24 21:37:40, bengr wrote:
> I'd expect RetrieveConfig to be asynchronous, or possibly to be sometimes
> asynchronous. It could return a bool if the config is available synchronously.
> I'd also expect it to take a config pointer, which it would populate. 

Acknowledged.

This makes more sense in the next CL. :)

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:90:
if (config.proxy_config().http_proxy_servers_size() > 0) {
On 2015/03/24 21:37:41, bengr wrote:
> You could probably factor out the stuff that determines origin and fallback
> origin into a helper.
> 
> E.g., std::vector<net::ProxyServer> GetProxiesForHTTP(const ClientConfig&
> config) const;

Done.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:99:
const ProxyServer& fallback =
On 2015/03/24 21:34:55, sclittle wrote:
> It's confusing to have both ProxyServer and net::ProxyServer here meaning
> different things. 
> 
> Consider renaming data_reduction_proxy::ProxyServer to something else, or
> otherwise differentiating between them somehow. The same issue might also come
> up with data_reduction_proxy::ProxyConfig vs. net::ProxyConfig.

Done.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:136:
SetConfigRefreshTimer(next_config_refresh_time);
On 2015/03/24 21:37:41, bengr wrote:
> Perhaps factor out everything that determines next_config_refresh_time into a
> helper?
> 
> e.g.:
> 
> base::TimeDelta GetNextConfigRefreshTime(bool last_fetch_status, base::Time
> expiration_time) {...}

Done.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:44:
class DataReductionProxyConfigClientTest : public testing::Test {
On 2015/03/24 21:34:55, sclittle wrote:
> Rename to DataReductionProxyConfigServiceClientTest

Done.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:106:
EXPECT_CALL(*mock_config_client(), ConstructStaticResponse())
On 2015/03/24 21:34:55, sclittle wrote:
> Why are you mocking the methods of DRPConfigClient while testing
> DRPConfigClient? This seems to defeat the purpose of testing DRPConfigClient.
> 
> Couldn't you just set up DRPParams such that ConstructStaticResponse works
> normally? You could even mock DRPParams if necessary.
> 
> IIUC, mocks should only be used for other interfaces that the class being
tested
> depends on, otherwise the test is just testing less of the class, and even
then
> fakes are preferable than mocks where feasible due to the overhead of learning
> googlemock.

Done.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:110:
EXPECT_CALL(*mock_config_client(), SetConfigRefreshTimer(expected_delay))
On 2015/03/24 21:34:55, sclittle wrote:
> Same as above: mocking the class being tested means that it tests less of the
> class.
> 
> Maybe you could actually simulate that a day passes, and then check that it
> constructs a new config. You could use a base::MockTimer or something; maybe
you
> could set up DRPConfigServiceClient to take in the timer in the constructor to
> make this easier. Then you could just call MockTimer::Fire() to advance a day.
> 
> Ideally, you shouldn't have to test a fake or mock version of
> DRPConfigServiceClient here at all, although you might still need to fake out
> Now() somehow.

Done.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:44:
TestDataReductionProxyConfig(
On 2015/03/24 21:37:41, bengr wrote:
> Add comment that explains how this Constructor is different.

Done.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:45:
scoped_ptr<DataReductionProxyConfigValues> config_values,
On 2015/03/24 21:34:55, sclittle wrote:
> Could this be changed to scoped_ptr<DataReductionProxyMutableConfigValues>, to
> ensure that it's mutable?

I actually pass TestDataReductionProxyParams in from the other constructor.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:73:
bool DataReductionProxyMutableConfigValues::IsDataReductionProxy(
On 2015/03/24 21:34:55, sclittle wrote:
> This looks like duplicated code from DRPParams. Could this be put in a common
> place, maybe by providing a default implementation under ConfigValues?

Acknowledged.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:82:
}
On 2015/03/24 21:37:41, bengr wrote:
> bad indentation.

Done.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:23:
class DataReductionProxyMutableConfigValues
On 2015/03/24 21:37:41, bengr wrote:
> Add a class-level comment.

Done.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:30:
DataReductionProxyParams* params);
On 2015/03/24 21:34:55, sclittle wrote:
> Could this be const DataReductionProxyParams*?

Done.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:43:
bool fallback_allowed() const override;
On 2015/03/24 21:37:41, bengr wrote:
> The DRPParams code made the assumption that there would never be more than two
> proxies for HTTP traffic. I didn't think we were still operating under this
> assumption. I don't think the change has to be in this CL, but we should
switch
> to using a proxy list, and start reporting UMA based on attributes of the
proxy
> (e.g., scheme=https) instead of on position in the list.

Acknowledged.

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:51:
const net::ProxyServer& fallback_origin() const override;
On 2015/03/24 21:37:41, bengr wrote:
> What's the case for exposing all of these values? Ideally, we'd have a proxy
> config that provides a very simple interface, e.g.:
> 
> std::vector<net::ProxcyServer> GetProxies(const std::string& scheme) const;
> bool IsDataReductionProxy(const net::ProxyServer& proxy) const;

Acknowledged.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:331:
if (use_config_client_) {
On 2015/03/24 21:34:56, sclittle wrote:
> Maybe DCHECK(!(use_mock_config_ && use_config_client_)) so that no one tries
to
> set both?

Done.

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:104:
MOCK_METHOD1(SetConfigRefreshTimer, void(const base::TimeDelta& delay));
On 2015/03/24 21:34:56, sclittle wrote:
> These methods aren't part of any public interface, why are you mocking them?
Is
> there really no other way?
> 
> Why not just set up Params in the tests such that the right config is
> constructed when RetrieveConfig is called?

Done.

[bengr, 2015-03-24 23:35:37]

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

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:38:
30 * 60 * 1000, // maximum_backoff_ms
I think style says comments like these should be at least two spaces away from
code.

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:149:
DataReductionProxyConfigServiceClient::GetProxiesForHTTP(
This could be put in an anonymous namespace.

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:169:
GetBackoffEntry()->InformOfRequest(true);
This and the one below could be moved outside this function and called first,
e.g.,
GetBackoffEntry()->InformOfRequest(success);

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:171:
GetBackoffEntry()->GetTimeUntilRelease();
I'd also pass in GetBackoffEntry()->GetTimeUntilRelease(), and then put this
method in an anonymous namespace.

[sclittle, 2015-03-25 00:03:23]

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

https://codereview.chromium.org/1024553009/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:73:
bool DataReductionProxyMutableConfigValues::IsDataReductionProxy(
On 2015/03/24 23:20:28, jeremyim wrote:
> On 2015/03/24 21:34:55, sclittle wrote:
> > This looks like duplicated code from DRPParams. Could this be put in a
common
> > place, maybe by providing a default implementation under ConfigValues?
> 
> Acknowledged.

What do you mean? If this will be rectified in a later CL, could you at least
add a comment or a TODO saying that this code duplication is temporary?

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

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:88:
class RequestOptionsPopulator {
This class and methods should be in an anonymous namespace.

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

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:32:
const scoped_ptr<DataReductionProxyParams>& params);
Why const scoped_ptr&? Is it important that the caller of CreateFromParams
directly owns the DRPParams? If so, please document that; otherwise, change to
const DataReductionProxyParams* since it's simpler.

[jeremyim, 2015-03-25 00:37:21]

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

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:38:
30 * 60 * 1000, // maximum_backoff_ms
On 2015/03/24 23:35:36, bengr wrote:
> I think style says comments like these should be at least two spaces away from
> code.

Done.

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:149:
DataReductionProxyConfigServiceClient::GetProxiesForHTTP(
On 2015/03/24 23:35:36, bengr wrote:
> This could be put in an anonymous namespace.

Done.

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:169:
GetBackoffEntry()->InformOfRequest(true);
On 2015/03/24 23:35:37, bengr wrote:
> This and the one below could be moved outside this function and called first,
> e.g.,
> GetBackoffEntry()->InformOfRequest(success);

Done.

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:171:
GetBackoffEntry()->GetTimeUntilRelease();
On 2015/03/24 23:35:37, bengr wrote:
> I'd also pass in GetBackoffEntry()->GetTimeUntilRelease(), and then put this
> method in an anonymous namespace.

Done.

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

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:88:
class RequestOptionsPopulator {
On 2015/03/25 00:03:22, sclittle wrote:
> This class and methods should be in an anonymous namespace.

Done.

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

https://codereview.chromium.org/1024553009/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h:32:
const scoped_ptr<DataReductionProxyParams>& params);
On 2015/03/25 00:03:22, sclittle wrote:
> Why const scoped_ptr&? Is it important that the caller of CreateFromParams
> directly owns the DRPParams? If so, please document that; otherwise, change to
> const DataReductionProxyParams* since it's simpler.

Done.

[sclittle, 2015-03-25 02:58:38]

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc
(right):

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:125:
base::Time::UnixEpoch() + base::TimeDelta::FromDays(1),
Where is it tested that a new config is fetched after the old one expires?

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h
(right):

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:69:
DataReductionProxyMutableConfigValues* mutable_config_values();
This method and test_params() are mutually exclusive such that only one is safe,
but there's no DCHECKs to ensure that the right one gets called.

Why not just expose a DRPConfigValues* config_values() method and let the caller
decide what to cast it to?

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
(right):

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:558:
return config()->mutable_config_values();
Why not just cast the config's config_values to a MutableConfigValues right here
and remove the config()->mutable_config_values() method?

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:119:
class TestTickClock : public base::TickClock {
Could you use SimpleTestTickClock here?
https://code.google.com/p/chromium/codesearch#chromium/src/base/test/simple_t...

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:127:
void SetNowTicks(const base::TimeTicks& time);
This method is never called.

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:138:
base::TickClock* tick_clock);
Do you really need a tick clock here? Why not just convert the base::Time now_
into a base::TimeTicks in ImplGetTimeNow() when needed? Is TimeTicks::Now
significantly different for testing purposes than Time::Now?

Otherwise, please document why a TickClock is needed here.

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:148:
base::Time now_;
Why have both a base::Time and a TickClock that return the current time? Or is
the TickClock not for the current time?

[jeremyim, 2015-03-25 05:05:38]

More CR comments

[jeremyim, 2015-03-25 05:07:39]

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc
(right):

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:125:
base::Time::UnixEpoch() + base::TimeDelta::FromDays(1),
On 2015/03/25 02:58:38, sclittle wrote:
> Where is it tested that a new config is fetched after the old one expires?

Tweaked the test slightly; I don't plan on testing the OneShotTimer itself (or
its behavior).

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h
(right):

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:69:
DataReductionProxyMutableConfigValues* mutable_config_values();
On 2015/03/25 02:58:38, sclittle wrote:
> This method and test_params() are mutually exclusive such that only one is
safe,
> but there's no DCHECKs to ensure that the right one gets called.
> 
> Why not just expose a DRPConfigValues* config_values() method and let the
caller
> decide what to cast it to?

This is a little messy during the transition, since there are several callers of
test_params(). TODO added.

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
(right):

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:558:
return config()->mutable_config_values();
On 2015/03/25 02:58:38, sclittle wrote:
> Why not just cast the config's config_values to a MutableConfigValues right
here
> and remove the config()->mutable_config_values() method?

Done.

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:119:
class TestTickClock : public base::TickClock {
On 2015/03/25 02:58:38, sclittle wrote:
> Could you use SimpleTestTickClock here?
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/test/simple_t...

Changed to use a clock which returns base::Time and base::TimeTicks

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:127:
void SetNowTicks(const base::TimeTicks& time);
On 2015/03/25 02:58:38, sclittle wrote:
> This method is never called.

Done.

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:138:
base::TickClock* tick_clock);
On 2015/03/25 02:58:38, sclittle wrote:
> Do you really need a tick clock here? Why not just convert the base::Time now_
> into a base::TimeTicks in ImplGetTimeNow() when needed? Is TimeTicks::Now
> significantly different for testing purposes than Time::Now?
> 
> Otherwise, please document why a TickClock is needed here.

Hopefully the comment for TestTickClock is sufficient.

https://codereview.chromium.org/1024553009/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:148:
base::Time now_;
On 2015/03/25 02:58:38, sclittle wrote:
> Why have both a base::Time and a TickClock that return the current time? Or is
> the TickClock not for the current time?

Done.

[jeremyim, 2015-03-25 05:39:36]

Rebase

[bengr, 2015-03-25 17:15:11]

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:301:
fallback_origin = config_values_->fallback_origin().ToURI();
Why do you convert from net::ProxyServer to std::string, and then back to
net::ProxyServer?

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:67:
base::TimeDelta success_delay = config_expiration - now;
It would probably be a good idea to enforce assumptions here. E.g., should
config_expiration always be in the future?

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h:57:
virtual const net::BackoffEntry* GetBackoffEntry() const;
Why do you need both const and non-const versions of this function?

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:28:
.Pass(),
If you move 'new...' to a new line, you can probably move up '.Pass()'.

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:86:
if (fallback_allowed() && fallback_origin().is_valid() &&
I believe you can get rid of the nesting by doing:

if (!fallback_allowed() || ...)
  return false:

if (proxy_info) {
}

return true;

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:144:
base::Time TestDataReductionProxyConfigServiceClient::TestTickClock::Now()
Put the return type on a separate line and move 'const' up.

[jeremyim, 2015-03-25 17:53:15]

Address bengr CR comments

[jeremyim, 2015-03-25 17:54:19]

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:301:
fallback_origin = config_values_->fallback_origin().ToURI();
On 2015/03/25 17:15:11, bengr wrote:
> Why do you convert from net::ProxyServer to std::string, and then back to
> net::ProxyServer?

TODO added.

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc:67:
base::TimeDelta success_delay = config_expiration - now;
On 2015/03/25 17:15:11, bengr wrote:
> It would probably be a good idea to enforce assumptions here. E.g., should
> config_expiration always be in the future?

Done.

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h:57:
virtual const net::BackoffEntry* GetBackoffEntry() const;
On 2015/03/25 17:15:11, bengr wrote:
> Why do you need both const and non-const versions of this function?

Done.

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.cc:28:
.Pass(),
On 2015/03/25 17:15:11, bengr wrote:
> If you move 'new...' to a new line, you can probably move up '.Pass()'.

Done.

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.cc:86:
if (fallback_allowed() && fallback_origin().is_valid() &&
On 2015/03/25 17:15:11, bengr wrote:
> I believe you can get rid of the nesting by doing:
> 
> if (!fallback_allowed() || ...)
>   return false:
> 
> if (proxy_info) {
> }
> 
> return true;

Done.

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc:144:
base::Time TestDataReductionProxyConfigServiceClient::TestTickClock::Now()
On 2015/03/25 17:15:11, bengr wrote:
> Put the return type on a separate line and move 'const' up.

Done.

FWIW, I believe the git cl format prefers const on its own line and return type
with the method.

[bengr, 2015-03-25 17:58:36]

lgtm

[sclittle, 2015-03-25 18:03:56]

LGTM with nit

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h
(right):

https://codereview.chromium.org/1024553009/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h:125:
base::TimeTicks NowTicks() override;
nit: If you make TestBackoffEntry take in a TestTickClock instead of a
base::TickClock, you could make TestTickClock not implement base::TickClock and
make this NowTicks() method const.

Then you could pass in a const TestTickClock to TestBackoffEntry, which might be
a bit cleaner.

[jeremyim, 2015-03-25 18:22:01]

Address sclittle comment

[jeremyim, 2015-03-25 18:22:10]

The CQ bit was checked by jeremyim@chromium.org

[jeremyim, 2015-03-25 18:22:11]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1024553009/#ps140001
(title: "Address sclittle comment")

[jeremyim, 2015-03-25 18:22:30]

Thanks for looking at this!

[commit-bot: I haz the power, 2015-03-25 18:22:42]

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

[commit-bot: I haz the power, 2015-03-25 19:51:36]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-25 19:52:37]

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