Introduce a separate preference for 'proxy server mode'

chrome/browser/net/pref_proxy_config_service.cc

 // Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/net/pref_proxy_config_service.h"
 
 #include "base/values.h"
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/prefs/pref_set_observer.h"
+#include "chrome/browser/prefs/proxy_prefs.h"
 #include "chrome/common/notification_details.h"
 #include "chrome/common/notification_source.h"
 #include "chrome/common/notification_type.h"
 #include "chrome/common/pref_names.h"
 
-namespace {
-
-const bool kProxyPrefDefaultBoolean = false;
-const char kProxyPrefDefaultString[] = "";
-
-// Determines if a value of a proxy pref is set to its default. Default values
-// have a special role in the proxy pref system, because if all of the proxy
-// prefs are set to their defaults, then the system proxy settings are applied.
-// TODO(gfeher): Proxy preferences should be refactored to avoid the need
-// for such solutions. See crbug.com/65732
-bool IsDefaultValue(const Value* value) {
-  bool b = false;
-  std::string s;
-  if (value->IsType(Value::TYPE_BOOLEAN) &&
-      value->GetAsBoolean(&b)) {
-    return b == kProxyPrefDefaultBoolean;
-  } else if (value->IsType(Value::TYPE_STRING) &&
-             value->GetAsString(&s)) {
-    return s == kProxyPrefDefaultString;
-  } else {
-    NOTREACHED() << "Invalid type for a proxy preference.";
-    return false;
-  }
-}
-
-}  // namespace
-
 PrefProxyConfigTracker::PrefProxyConfigTracker(PrefService* pref_service)
     : pref_service_(pref_service) {
   valid_ = ReadPrefConfig(&pref_config_);
   proxy_prefs_observer_.reset(
       PrefSetObserver::CreateProxyPrefSetObserver(pref_service_, this));
 }
 
 PrefProxyConfigTracker::~PrefProxyConfigTracker() {
   DCHECK(pref_service_ == NULL);
 }
@@ skipping 52 matching lines @@
     FOR_EACH_OBSERVER(Observer, observers_, OnPrefProxyConfigChanged());
   }
 }
 
 bool PrefProxyConfigTracker::ReadPrefConfig(net::ProxyConfig* config) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Clear the configuration.
   *config = net::ProxyConfig();
 
-  // Scan for all "enable" type proxy switches.
-  static const char* proxy_prefs[] = {
-    prefs::kProxyPacUrl,
-    prefs::kProxyServer,
-    prefs::kProxyBypassList,
-    prefs::kProxyAutoDetect
-  };
-
-  // Check whether the preference system holds a valid proxy configuration. Note
-  // that preferences coming from a lower-priority source than the user settings
-  // are ignored. That's because chrome treats the system settings as the
-  // default values, which should apply if there's no explicit value forced by
-  // policy or the user.
-  // Preferences that are set to their default values are also ignored,
-  // regardless of their controlling source. This is because 'use system proxy
-  // settings' is currently encoded by all the preferences being set to their
-  // defaults. This will change when crbug.com/65732 is addressed.
-  bool found_enable_proxy_pref = false;
-  for (size_t i = 0; i < arraysize(proxy_prefs); i++) {
-    const PrefService::Preference* pref =
-        pref_service_->FindPreference(proxy_prefs[i]);
-    DCHECK(pref);
-    if (pref && (!pref->IsUserModifiable() || pref->HasUserSetting()) &&
-        !IsDefaultValue(pref->GetValue())) {
-      found_enable_proxy_pref = true;
-      break;
-    }
-  }
-
-  if (!found_enable_proxy_pref &&
-      !pref_service_->GetBoolean(prefs::kNoProxyServer)) {
+  ProxyPrefs::ProxyMode mode;
+  int proxy_mode = pref_service_->GetInteger(prefs::kProxyMode);
+  if (!ProxyPrefs::IntToProxyMode(proxy_mode, &mode)) {
+    // Fall back to system settings if the mode preference is invalid.
     return false;
   }
 
-  if (pref_service_->GetBoolean(prefs::kNoProxyServer)) {
-    // Ignore all the other proxy config preferences if the use of a proxy
-    // has been explicitly disabled.
-    return true;
+  switch (mode) {
+    case ProxyPrefs::SYSTEM:
+      // Use system settings.
+      return false;
+    case ProxyPrefs::DISABLED:
+      // Ignore all the other proxy config preferences if the use of a proxy
+      // has been explicitly disabled.
+      return true;
+    case ProxyPrefs::AUTO_DETECT:
+      config->set_auto_detect(true);
+      return true;
+    case ProxyPrefs::PAC_SCRIPT:
+      if (pref_service_->HasPrefPath(prefs::kProxyPacUrl)) {
+        std::string proxy_pac = pref_service_->GetString(prefs::kProxyPacUrl);
+        config->set_pac_url(GURL(proxy_pac));

[eroman, 2010/12/21 22:55:21]

You may want to check that the URL was valid as well, in-case it is something
weird.

[battre, 2010/12/22 10:01:27]

Done.

+        return true;
+      } else {

[eroman, 2010/12/21 22:55:21]

nit: omit "else" when previous clause returns.

[battre, 2010/12/22 10:01:27]

Done.

+        LOG(ERROR) << "Proxy settings request PAC script but do not specify "
+                   << "its URL. Falling back to direct connection.";
+        return true;
+      }
+    case ProxyPrefs::FIXED_SERVERS:
+      if (pref_service_->HasPrefPath(prefs::kProxyServer)) {
+        std::string proxy_server =
+            pref_service_->GetString(prefs::kProxyServer);
+        config->proxy_rules().ParseFromString(proxy_server);

[eroman, 2010/12/21 22:55:21]

I don't think it is a good idea to rely on the internal string format (which is
not guaranteed stable).

How about splitting these out into separate preferences? i.e. have a
kProxyForHttpUrls, kProxyForFtpUrls, kProxyForHttpsUrls, kProxyForAllSchemes
(this latter is where you would insert socks proxies).

Similarly, the extension API should be specifying these in a structured format
rather than as a string (it could just pass a dictionary from scheme --> proxy
server).

[battre, 2010/12/22 10:01:27]

Created http://crbug.com/67779

+      } else {
+        LOG(ERROR) << "Proxy settings request fixed proxy servers but do not "
+                   << "specify their URLs. Falling back to direct connection.";
+        return true;
+      }
+
+      if (pref_service_->HasPrefPath(prefs::kProxyBypassList)) {
+        std::string proxy_bypass =
+            pref_service_->GetString(prefs::kProxyBypassList);

[eroman, 2010/12/21 22:55:21]

Can this be stored as a ValueList instead?

[battre, 2010/12/22 10:01:27]

Created http://crbug.com/67778

+        config->proxy_rules().bypass_rules.ParseFromString(proxy_bypass);
+      }
+      return true;
+    default:
+      NOTREACHED() << "Unknown proxy mode, falling back to system settings.";
+      return false;
   }
-
-  if (pref_service_->HasPrefPath(prefs::kProxyServer)) {
-    std::string proxy_server = pref_service_->GetString(prefs::kProxyServer);
-    config->proxy_rules().ParseFromString(proxy_server);
-  }
-
-  if (pref_service_->HasPrefPath(prefs::kProxyPacUrl)) {
-    std::string proxy_pac = pref_service_->GetString(prefs::kProxyPacUrl);
-    config->set_pac_url(GURL(proxy_pac));
-  }
-
-  config->set_auto_detect(pref_service_->GetBoolean(prefs::kProxyAutoDetect));
-
-  if (pref_service_->HasPrefPath(prefs::kProxyBypassList)) {
-    std::string proxy_bypass =
-        pref_service_->GetString(prefs::kProxyBypassList);
-    config->proxy_rules().bypass_rules.ParseFromString(proxy_bypass);
-  }
-
-  return true;
 }
 
 PrefProxyConfigService::PrefProxyConfigService(
     PrefProxyConfigTracker* tracker,
     net::ProxyConfigService* base_service)
     : base_service_(base_service),
       pref_config_tracker_(tracker),
       registered_observers_(false) {
 }
 
@@ skipping 67 matching lines @@
   if (!registered_observers_) {
     base_service_->AddObserver(this);
     pref_config_tracker_->AddObserver(this);
     registered_observers_ = true;
   }
 }
 
 // static
 void PrefProxyConfigService::RegisterUserPrefs(
     PrefService* pref_service) {
-  pref_service->RegisterBooleanPref(prefs::kNoProxyServer,
-                                    kProxyPrefDefaultBoolean);
-  pref_service->RegisterBooleanPref(prefs::kProxyAutoDetect,
-                                    kProxyPrefDefaultBoolean);
-  pref_service->RegisterStringPref(prefs::kProxyServer,
-                                   kProxyPrefDefaultString);
-  pref_service->RegisterStringPref(prefs::kProxyPacUrl,
-                                   kProxyPrefDefaultString);
-  pref_service->RegisterStringPref(prefs::kProxyBypassList,
-                                   kProxyPrefDefaultString);
+  pref_service->RegisterIntegerPref(prefs::kProxyMode, ProxyPrefs::SYSTEM);
+  pref_service->RegisterStringPref(prefs::kProxyServer, "");
+  pref_service->RegisterStringPref(prefs::kProxyPacUrl, "");
+  pref_service->RegisterStringPref(prefs::kProxyBypassList, "");
 }