Use data reduction proxy when managed proxy config returns direct

chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc

Index: chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc
diff --git a/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc b/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc
index 1201cf0e19838275ccd5640411f073af5c667f40..cb9e788a19141c70da51ee1a9136e3174f006b5a 100644
--- a/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc
+++ b/chrome/browser/net/spdyproxy/data_reduction_proxy_chrome_configurator.cc
@@ -6,13 +6,18 @@
 
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
+#include "base/sequenced_task_runner.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/prefs/proxy_prefs.h"
 #include "chrome/common/pref_names.h"
+#include "net/proxy/proxy_config.h"
+#include "net/proxy/proxy_info.h"
+#include "net/proxy/proxy_service.h"
 
 DataReductionProxyChromeConfigurator::DataReductionProxyChromeConfigurator(
-    PrefService* prefs) : prefs_(prefs) {
-  DCHECK(prefs);
+    PrefService* prefs,
+    scoped_refptr<base::SequencedTaskRunner> network_task_runner)
+    : prefs_(prefs), network_task_runner_(network_task_runner) {
 }
 
 DataReductionProxyChromeConfigurator::~DataReductionProxyChromeConfigurator() {
@@ -26,6 +31,8 @@ void DataReductionProxyChromeConfigurator::Enable(
     const std::string& ssl_origin) {
   DCHECK(prefs_);
   DictionaryPrefUpdate update(prefs_, prefs::kProxy);
+  // TODO(bengr): Consider relying on the proxy config for all data reduction
+  // proxy configuration.
   base::DictionaryValue* dict = update.Get();
 
   std::vector<std::string> proxies;
@@ -60,6 +67,21 @@ void DataReductionProxyChromeConfigurator::Enable(
   dict->SetString("server", server);
   dict->SetString("mode", ProxyModeToString(ProxyPrefs::MODE_FIXED_SERVERS));
   dict->SetString("bypass_list", JoinString(bypass_rules_, ", "));
+
+  net::ProxyConfig config;
+  config.proxy_rules().ParseFromString(server);
+  config.proxy_rules().bypass_rules.ParseFromString(
+      JoinString(bypass_rules_, ", "));
+  // The ID is set to a bogus value. It cannot be left uninitialized, else the
+  // config will return invalid.
+  net::ProxyConfig::ID unused_id = 1;
+  config.set_id(unused_id);

[Ryan Sleevi, 2014/08/14 19:19:51]

why not just set_id(1)? Why do you need to declare the local ID?

+  network_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &DataReductionProxyChromeConfigurator::UpdateProxyConfigOnIO,
+          base::Unretained(this),

[Ryan Sleevi, 2014/08/14 19:19:50]

This is a very dangerous usage.

No where in the header are the lifetime requirements spelled out, so you have no
guarantee that this object ("this") will still be valid when this code executes
on the IO thread.

While you could spell it out with a comment, a better design would be to avoid
this requirement altogether.

That is, favour complexity *in* this class if it means you can *avoid*
complexity in users of this class (which lifetime management is often the
high-order bit of complexity)

+          config));
 }
 
 void DataReductionProxyChromeConfigurator::Disable() {
@@ -69,6 +91,13 @@ void DataReductionProxyChromeConfigurator::Disable() {
   dict->SetString("mode", ProxyModeToString(ProxyPrefs::MODE_SYSTEM));
   dict->SetString("server", "");
   dict->SetString("bypass_list", "");

[Ryan Sleevi, 2014/08/14 19:19:50]

cleanup: These should all be std::string() instead of ""

+  net::ProxyConfig config = net::ProxyConfig::CreateDirect();
+  network_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(
+          &DataReductionProxyChromeConfigurator::UpdateProxyConfigOnIO,
+          base::Unretained(this),
+          config));

[Ryan Sleevi, 2014/08/14 19:19:50]

Did git-cl-format do this? It seems odd for it.

 }
 
 void DataReductionProxyChromeConfigurator::AddHostPatternToBypass(
@@ -90,3 +119,13 @@ void DataReductionProxyChromeConfigurator::AddURLPatternToBypass(
 
   AddHostPatternToBypass(host_pattern);
 }
+
+void DataReductionProxyChromeConfigurator::UpdateProxyConfigOnIO(

[Ryan Sleevi, 2014/08/14 19:19:50]

If you decide to keep these methods (ideally, you wouldn't), you should add
DCHECKs to these methods to annotate throughout this class that asserts it's
used correctly

However, from looking at this (rather than just the header), it doesn't seem
necessary to
1) Expose these as public methods (as opposed to private)
2) Require they be methods on the class (rather, you could have a helper ::Core
class that lives entirely on the IO thread, and which the parent marshals
messages to/from)

+    const net::ProxyConfig& config) {
+  config_ = config;
+}
+
+const net::ProxyConfig&
+DataReductionProxyChromeConfigurator::GetProxyConfigOnIO() const {
+  return config_;
+}