Add the ability to use DataReductionProxyParams from DataReductionProxyConfigServiceClient.

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc

Index: components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
diff --git a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
index 39a9481bc782694f431a6400fc82c34519b5d0f1..77a058ab643325d9da0d593f1249cc2619552873 100644
--- a/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
+++ b/components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.cc
@@ -4,27 +4,159 @@
 
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client.h"
 
+#include <vector>

[sclittle, 2015/03/24 21:34:55]

also include string

[jeremyim, 2015/03/24 23:20:28]

Done.

+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/json/json_writer.h"
+#include "base/location.h"
 #include "base/logging.h"
+#include "base/single_thread_task_runner.h"
+#include "base/time/time.h"
 #include "base/values.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h"
+#include "components/data_reduction_proxy/core/browser/data_reduction_proxy_mutable_config_values.h"
 #include "components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h"
+#include "components/data_reduction_proxy/core/common/data_reduction_proxy_client_config_parser.h"
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_params.h"
+#include "components/data_reduction_proxy/proto/client_config.pb.h"
+#include "net/base/host_port_pair.h"
+#include "net/proxy/proxy_server.h"
 
 namespace data_reduction_proxy {
 
+namespace {
+
+const net::BackoffEntry::Policy kDefaultBackoffPolicy = {

[bengr, 2015/03/24 21:37:41]

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.

[jeremyim, 2015/03/24 23:20:28]

Done.

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

+    // num_errors_to_ignore

[bengr, 2015/03/24 21:37:40]

Might be more readable if you put the comments to the right of the values.

[jeremyim, 2015/03/24 23:20:28]

Done.

+    0,
+    // initial_delay_ms
+    10 * 1000,
+    // multiply_factor
+    2,
+    // jitter_factor,
+    0.10,
+    // maximum_backoff_ms
+    30 * 60 * 1000,
+    // entry_lifetime_ms
+    -1,
+    // always_use_initial_delay
+    true,
+};
+
+}  // namespace
+
+const net::BackoffEntry::Policy& GetBackoffPolicy() {
+  return kDefaultBackoffPolicy;
+}
+
 DataReductionProxyConfigServiceClient::DataReductionProxyConfigServiceClient(
-    DataReductionProxyParams* params,
-    DataReductionProxyRequestOptions* request_options)
-    : params_(params),
-      request_options_(request_options) {
-  DCHECK(params);
+    scoped_ptr<DataReductionProxyParams> params,
+    const net::BackoffEntry::Policy& backoff_policy,
+    DataReductionProxyRequestOptions* request_options,
+    DataReductionProxyMutableConfigValues* config_values,
+    DataReductionProxyConfig* config,
+    scoped_refptr<base::SingleThreadTaskRunner> io_task_runner)
+    : params_(params.Pass()),
+      request_options_(request_options),
+      config_values_(config_values),
+      config_(config),
+      io_task_runner_(io_task_runner),
+      backoff_entry_(&backoff_policy) {
   DCHECK(request_options);
+  DCHECK(config_values);
+  DCHECK(config);
+  DCHECK(io_task_runner.get());
+  io_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&DataReductionProxyConfigServiceClient::RetrieveConfig,
+                 base::Unretained(this)));
 }
 
 DataReductionProxyConfigServiceClient::
     ~DataReductionProxyConfigServiceClient() {
 }
 
+void DataReductionProxyConfigServiceClient::RetrieveConfig() {

[bengr, 2015/03/24 21:37:40]

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. 

[jeremyim, 2015/03/24 23:20:27]

Acknowledged.

This makes more sense in the next CL. :)

+  DCHECK(io_task_runner_->BelongsToCurrentThread());
+
+  std::string static_response = ConstructStaticResponse();
+  ClientConfig config;
+  bool succeeded = false;
+  if (config_parser::ParseClientConfig(static_response, &config)) {
+    if (config.has_proxy_config()) {
+      net::ProxyServer origin;
+      net::ProxyServer fallback_origin;
+      if (config.proxy_config().http_proxy_servers_size() > 0) {

[bengr, 2015/03/24 21:37:41]

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;

[jeremyim, 2015/03/24 23:20:28]

Done.

+        const ProxyServer& server = config.proxy_config().http_proxy_servers(0);
+        if (server.scheme() != ProxyServer_ProxyScheme_UNSPECIFIED) {
+          origin = net::ProxyServer(
+              config_parser::SchemeFromProxyScheme(server.scheme()),
+              net::HostPortPair(server.host(), server.port()));
+        }
+
+        if (config.proxy_config().http_proxy_servers_size() > 1) {
+          const ProxyServer& fallback =

[sclittle, 2015/03/24 21:34:55]

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.

[jeremyim, 2015/03/24 23:20:28]

Done.

+              config.proxy_config().http_proxy_servers(1);
+          if (fallback.scheme() != ProxyServer_ProxyScheme_UNSPECIFIED) {
+            fallback_origin = net::ProxyServer(
+                config_parser::SchemeFromProxyScheme(fallback.scheme()),
+                net::HostPortPair(fallback.host(), fallback.port()));
+          }
+        }
+
+        std::string session;
+        std::string credentials;
+        if (DataReductionProxyRequestOptions::ParseLocalSessionKey(
+                config.session_key(), &session, &credentials)) {
+          request_options_->SetCredentials(session, credentials);
+          config_values_->UpdateValues(origin, fallback_origin);
+          config_->ReloadConfig();
+          succeeded = true;
+        }
+      }
+    }
+  }
+
+  base::TimeDelta next_config_refresh_time;
+  if (succeeded) {
+    GetBackoffEntry()->InformOfRequest(true);
+    base::TimeDelta minimum_refresh_time =
+        GetBackoffEntry()->GetTimeUntilRelease();
+    base::Time expiration_time =
+        config_parser::TimestampToTime(config.expire_time());
+    next_config_refresh_time = (expiration_time - Now());
+    if (next_config_refresh_time < minimum_refresh_time)
+      next_config_refresh_time = minimum_refresh_time;
+  } else {
+    GetBackoffEntry()->InformOfRequest(false);
+    next_config_refresh_time = GetBackoffEntry()->GetTimeUntilRelease();
+  }
+
+  SetConfigRefreshTimer(next_config_refresh_time);

[bengr, 2015/03/24 21:37:41]

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) {...}

[jeremyim, 2015/03/24 23:20:28]

Done.

+}
+
+const net::BackoffEntry*
+DataReductionProxyConfigServiceClient::GetBackoffEntry() const {
+  return &backoff_entry_;
+}
+
+net::BackoffEntry* DataReductionProxyConfigServiceClient::GetBackoffEntry() {
+  return &backoff_entry_;
+}
+
+void DataReductionProxyConfigServiceClient::SetConfigRefreshTimer(
+    const base::TimeDelta& delay) {
+  config_refresh_timer_.Stop();
+  config_refresh_timer_.Start(
+      FROM_HERE, delay, this,
+      &DataReductionProxyConfigServiceClient::RetrieveConfig);
+}
+
+base::Time DataReductionProxyConfigServiceClient::Now() const {
+  return base::Time::Now();
+}
+
 std::string
 DataReductionProxyConfigServiceClient::ConstructStaticResponse() const {
   std::string response;