Introduce a separate preference for 'proxy server mode'

The new preference is kProxyServerMode, which supersedes kProxyAutoDetect and kNoProxyServer. The point of this change is to represent 'use system proxy settings' in a more robust way. The proxy extension API is also adjusted to the preference system.

This is a continuation of gfeher's patch from issue 5701003.

BUG=65732, 66023
TEST=ProxyPrefsTest.*, and also covered by ExtensionApiTest.Porxy*, PrefProxyConfigServiceTest.*

Review URL: http://codereview.chromium.org/6004003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@70042 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 277 insertions, 245 deletions

diff --git a/chrome/browser/policy/config_dir_policy_provider_unittest.cc b/chrome/browser/policy/config_dir_policy_provider_unittest.cc
index 7197f490..8b7e9a0 100644
--- a/chrome/browser/policy/config_dir_policy_provider_unittest.cc
+++ b/chrome/browser/policy/config_dir_policy_provider_unittest.cc
@@ -259,8 +259,8 @@ INSTANTIATE_TEST_CASE_P(
             kPolicyDefaultSearchProviderEncodings,
             key::kDefaultSearchProviderEncodings),
         ValueTestParams::ForIntegerPolicy(
-            kPolicyProxyServerMode,
-            key::kProxyServerMode),
+            kPolicyProxyMode,
+            key::kProxyMode),
         ValueTestParams::ForStringPolicy(
             kPolicyProxyServer,
             key::kProxyServer),
diff --git a/chrome/browser/policy/configuration_policy_pref_store.cc b/chrome/browser/policy/configuration_policy_pref_store.cc
index 3971980..3c7ea6a 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store.cc
@@ -8,10 +8,12 @@
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/path_service.h"
+#include "base/stl_util-inl.h"
 #include "base/string16.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
+#include "chrome/browser/prefs/proxy_prefs.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/policy/configuration_policy_provider.h"
 #if defined(OS_WIN)
@@ -248,7 +250,7 @@ ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList() {
       key::kDefaultSearchProviderIconURL },
     { kPolicyDefaultSearchProviderEncodings, Value::TYPE_STRING,
       key::kDefaultSearchProviderEncodings },
-    { kPolicyProxyServerMode, Value::TYPE_INTEGER, key::kProxyServerMode },
+    { kPolicyProxyMode, Value::TYPE_INTEGER, key::kProxyMode },
     { kPolicyProxyServer, Value::TYPE_STRING, key::kProxyServer },
     { kPolicyProxyPacUrl, Value::TYPE_STRING, key::kProxyPacUrl },
     { kPolicyProxyBypassList, Value::TYPE_STRING, key::kProxyBypassList },
@@ -331,17 +333,16 @@ ConfigurationPolicyPrefStore::GetChromePolicyDefinitionList() {
 ConfigurationPolicyPrefStore::ConfigurationPolicyPrefStore(
     ConfigurationPolicyProvider* provider)
     : provider_(provider),
-      prefs_(new DictionaryValue()),
-      lower_priority_proxy_settings_overridden_(false),
-      proxy_disabled_(false),
-      proxy_configuration_specified_(false),
-      use_system_proxy_(false) {
+      prefs_(new DictionaryValue()) {
   if (!provider_->Provide(this))
     LOG(WARNING) << "Failed to get policy from provider.";
+  FinalizeProxyPolicySettings();
   FinalizeDefaultSearchPolicySettings();
 }
 
-ConfigurationPolicyPrefStore::~ConfigurationPolicyPrefStore() {}
+ConfigurationPolicyPrefStore::~ConfigurationPolicyPrefStore() {
+  DCHECK(proxy_policies_.empty());
+}
 
 PrefStore::ReadResult ConfigurationPolicyPrefStore::GetValue(
     const std::string& key,
@@ -415,17 +416,6 @@ ConfigurationPolicyPrefStore::CreateRecommendedPolicyPrefStore() {
       g_configuration_policy_provider_keeper.Get().recommended_provider());
 }
 
-// static
-void ConfigurationPolicyPrefStore::GetProxyPreferenceSet(
-    ProxyPreferenceSet* proxy_pref_set) {
-  proxy_pref_set->clear();
-  for (size_t current = 0; current < arraysize(kProxyPolicyMap); ++current) {
-    proxy_pref_set->insert(kProxyPolicyMap[current].preference_path);
-  }
-  proxy_pref_set->insert(prefs::kNoProxyServer);
-  proxy_pref_set->insert(prefs::kProxyAutoDetect);
-}
-
 const ConfigurationPolicyPrefStore::PolicyToPreferenceMapEntry*
 ConfigurationPolicyPrefStore::FindPolicyInMap(
     ConfigurationPolicyType policy,
@@ -469,108 +459,152 @@ bool ConfigurationPolicyPrefStore::ApplyPolicyFromMap(
 bool ConfigurationPolicyPrefStore::ApplyProxyPolicy(
     ConfigurationPolicyType policy,
     Value* value) {
-  bool result = false;
-  bool warn_about_proxy_disable_config = false;
-  bool warn_about_proxy_system_config = false;
-
-  const PolicyToPreferenceMapEntry* match_entry =
-      FindPolicyInMap(policy, kProxyPolicyMap, arraysize(kProxyPolicyMap));
-
-  // When the first proxy-related policy is applied, ALL proxy-related
-  // preferences that have been set by command-line switches, extensions,
-  // user preferences or any other mechanism are overridden. Otherwise
-  // it's possible for a user to interfere with proxy policy by setting
-  // proxy-related command-line switches or set proxy-related prefs in an
-  // extension that are related, but not identical, to the ones set through
-  // policy.
-  if (!lower_priority_proxy_settings_overridden_ &&
-      (match_entry ||
-       policy == kPolicyProxyServerMode)) {
-    ProxyPreferenceSet proxy_preference_set;
-    GetProxyPreferenceSet(&proxy_preference_set);
-    for (ProxyPreferenceSet::const_iterator i = proxy_preference_set.begin();
-         i != proxy_preference_set.end(); ++i) {
-      // We use values of TYPE_NULL to mark preferences for which
-      // READ_USE_DEFAULT should be returned by GetValue().
-      prefs_->Set(*i, Value::CreateNullValue());
-    }
-    lower_priority_proxy_settings_overridden_ = true;
+  // We only collect the values until we have sufficient information when
+  // FinalizeProxyPolicySettings() is called to determine whether the presented
+  // values were correct and apply them in that case.
+  if (policy == kPolicyProxyMode ||
+      policy == kPolicyProxyServer ||
+      policy == kPolicyProxyPacUrl ||
+      policy == kPolicyProxyBypassList) {
+    delete proxy_policies_[policy];
+    proxy_policies_[policy] = value;
+    return true;
   }
+  // We are not interested in this policy.
+  return false;
+}
 
-  // Translate the proxy policy into preferences.
-  if (policy == kPolicyProxyServerMode) {
-    int int_value;
-    bool proxy_auto_detect = false;
-    if (value->GetAsInteger(&int_value)) {
-      result = true;
-      switch (int_value) {
-        case kPolicyNoProxyServerMode:
-          if (!proxy_disabled_) {
-            if (proxy_configuration_specified_)
-              warn_about_proxy_disable_config = true;
-            proxy_disabled_ = true;
-          }
-          break;
-        case kPolicyAutoDetectProxyMode:
-          proxy_auto_detect = true;
-          break;
-        case kPolicyManuallyConfiguredProxyMode:
-          break;
-        case kPolicyUseSystemProxyMode:
-          if (!use_system_proxy_) {
-            if (proxy_configuration_specified_)
-              warn_about_proxy_system_config = true;
-            use_system_proxy_ = true;
-          }
-          break;
-        default:
-          // Not a valid policy, don't assume ownership of |value|
-          result = false;
-          break;
-      }
+void ConfigurationPolicyPrefStore::FinalizeProxyPolicySettings() {
+  if (CheckProxySettings())
+    ApplyProxySettings();
 
-      if (int_value != kPolicyUseSystemProxyMode) {
-        prefs_->Set(prefs::kNoProxyServer,
-                    Value::CreateBooleanValue(proxy_disabled_));
-        prefs_->Set(prefs::kProxyAutoDetect,
-                    Value::CreateBooleanValue(proxy_auto_detect));
-      }
-    }
-  } else if (match_entry) {
-    // Determine if the applied proxy policy settings conflict and issue
-    // a corresponding warning if they do.
-    if (!proxy_configuration_specified_) {
-      if (proxy_disabled_)
-        warn_about_proxy_disable_config = true;
-      if (use_system_proxy_)
-        warn_about_proxy_system_config = true;
-      proxy_configuration_specified_ = true;
-    }
-    if (!use_system_proxy_ && !proxy_disabled_) {
-      prefs_->Set(match_entry->preference_path, value);
-      // The ownership of value has been passed on to |prefs_|,
-      // don't clean it up later.
-      value = NULL;
-    }
-    result = true;
+  STLDeleteContainerPairSecondPointers(proxy_policies_.begin(),
+                                       proxy_policies_.end());
+  proxy_policies_.clear();
+}
+
+bool ConfigurationPolicyPrefStore::HasProxyPolicy(
+    ConfigurationPolicyType policy) const {
+  std::map<ConfigurationPolicyType, Value*>::const_iterator iter;
+  iter = proxy_policies_.find(policy);
+  return iter != proxy_policies_.end() &&
+         iter->second && !iter->second->IsType(Value::TYPE_NULL);
+}
+
+bool ConfigurationPolicyPrefStore::CheckProxySettings() {
+  bool mode = HasProxyPolicy(kPolicyProxyMode);
+  bool server = HasProxyPolicy(kPolicyProxyServer);
+  bool pac_url = HasProxyPolicy(kPolicyProxyPacUrl);
+  bool bypass_list = HasProxyPolicy(kPolicyProxyBypassList);
+
+  if ((server || pac_url || bypass_list) && !mode) {
+    LOG(WARNING) << "A centrally-administered policy defines proxy setting"
+                 << " details without setting a proxy mode.";
+    return false;
   }
 
-  if (warn_about_proxy_disable_config) {
-    LOG(WARNING) << "A centrally-administered policy disables the use of"
-                 << " a proxy but also specifies an explicit proxy"
-                 << " configuration.";
+  if (!mode)
+    return true;
+
+  int mode_value;
+  if (!proxy_policies_[kPolicyProxyMode]->GetAsInteger(&mode_value)) {
+    LOG(WARNING) << "Invalid proxy mode value.";
+    return false;
   }
 
-  if (warn_about_proxy_system_config) {
-    LOG(WARNING) << "A centrally-administered policy dictates that the"
-                 << " system proxy settings should be used but also specifies"
-                 << " an explicit proxy configuration.";
+  switch (mode_value) {
+    case kPolicyNoProxyServerMode:
+      if (server || pac_url || bypass_list) {
+        LOG(WARNING) << "A centrally-administered policy disables the use of"
+                     << " a proxy but also specifies an explicit proxy"
+                     << " configuration.";
+        return false;
+      }
+      break;
+    case kPolicyAutoDetectProxyMode:
+      if (server || bypass_list) {
+        LOG(WARNING) << "A centrally-administered policy dictates that a proxy"
+                     << " shall be auto configured but specifies fixed proxy"
+                     << " servers or a by-pass list.";
+        return false;
+      }
+      break;
+    case kPolicyManuallyConfiguredProxyMode:
+      if (!server) {
+        LOG(WARNING) << "A centrally-administered policy dictates that the"
+                     << " system proxy settings should use fixed proxy servers"
+                     << " without specifying which ones.";
+        return false;
+      }
+      if (pac_url) {
+        LOG(WARNING) << "A centrally-administered policy dictates that the"
+                     << " system proxy settings should use fixed proxy servers"
+                     << " but also specifies a PAC script.";
+        return false;
+      }
+      break;
+    case kPolicyUseSystemProxyMode:
+      if (server || pac_url || bypass_list) {
+        LOG(WARNING) << "A centrally-administered policy dictates that the"
+                     << " system proxy settings should be used but also "
+                     << " specifies an explicit proxy configuration.";
+        return false;
+      }
+      break;
+    default:
+      LOG(WARNING) << "Invalid proxy mode " << mode_value;
+      return false;
   }
+  return true;
+}
 
-  // If the policy was a proxy policy, cleanup |value|.
-  if (result && value)
-    delete value;
-  return result;
+void ConfigurationPolicyPrefStore::ApplyProxySettings() {
+  if (!HasProxyPolicy(kPolicyProxyMode))
+    return;
+
+  int int_mode;
+  CHECK(proxy_policies_[kPolicyProxyMode]->GetAsInteger(&int_mode));
+  ProxyPrefs::ProxyMode mode;
+  switch (int_mode) {
+    case kPolicyNoProxyServerMode:
+      mode = ProxyPrefs::MODE_DIRECT;
+      break;
+    case kPolicyAutoDetectProxyMode:
+      mode = ProxyPrefs::MODE_AUTO_DETECT;
+      if (HasProxyPolicy(kPolicyProxyPacUrl))
+        mode = ProxyPrefs::MODE_PAC_SCRIPT;
+      break;
+    case kPolicyManuallyConfiguredProxyMode:
+      mode = ProxyPrefs::MODE_FIXED_SERVERS;
+      break;
+    case kPolicyUseSystemProxyMode:
+      mode = ProxyPrefs::MODE_SYSTEM;
+      break;
+    default:
+      mode = ProxyPrefs::MODE_DIRECT;
+      NOTREACHED();
+  }
+  prefs_->Set(prefs::kProxyMode, Value::CreateIntegerValue(mode));
+
+  if (HasProxyPolicy(kPolicyProxyServer)) {
+    prefs_->Set(prefs::kProxyServer, proxy_policies_[kPolicyProxyServer]);
+    proxy_policies_[kPolicyProxyServer] = NULL;
+  } else {
+    prefs_->Set(prefs::kProxyServer, Value::CreateNullValue());
+  }
+  if (HasProxyPolicy(kPolicyProxyPacUrl)) {
+    prefs_->Set(prefs::kProxyPacUrl, proxy_policies_[kPolicyProxyPacUrl]);
+    proxy_policies_[kPolicyProxyPacUrl] = NULL;
+  } else {
+    prefs_->Set(prefs::kProxyPacUrl, Value::CreateNullValue());
+  }
+  if (HasProxyPolicy(kPolicyProxyBypassList)) {
+    prefs_->Set(prefs::kProxyBypassList,
+                proxy_policies_[kPolicyProxyBypassList]);
+    proxy_policies_[kPolicyProxyBypassList] = NULL;
+  } else {
+    prefs_->Set(prefs::kProxyBypassList, Value::CreateNullValue());
+  }
 }
 
 bool ConfigurationPolicyPrefStore::ApplySyncPolicy(
diff --git a/chrome/browser/policy/configuration_policy_pref_store.h b/chrome/browser/policy/configuration_policy_pref_store.h
index 820306a..6d0ae15 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.h
+++ b/chrome/browser/policy/configuration_policy_pref_store.h
@@ -56,10 +56,6 @@ class ConfigurationPolicyPrefStore : public PrefStore,
   static const ConfigurationPolicyProvider::PolicyDefinitionList*
       GetChromePolicyDefinitionList();
 
-  // Returns the set of preference paths that can be affected by a proxy
-  // policy.
-  static void GetProxyPreferenceSet(ProxyPreferenceSet* proxy_pref_set);
-
  private:
   // Policies that map to a single preference are handled
   // by an automated converter. Each one of these policies
@@ -79,23 +75,11 @@ class ConfigurationPolicyPrefStore : public PrefStore,
   ConfigurationPolicyProvider* provider_;
   scoped_ptr<DictionaryValue> prefs_;
 
-  // Set to false until the first proxy-relevant policy is applied. At that
-  // time, default values are provided for all proxy-relevant prefs
-  // to override any values set from stores with a lower priority.
-  bool lower_priority_proxy_settings_overridden_;
-
-  // The following are used to track what proxy-relevant policy has been applied
-  // accross calls to Apply to provide a warning if a policy specifies a
-  // contradictory proxy configuration. |proxy_disabled_| is set to true if and
-  // only if the kPolicyNoProxyServer has been applied,
-  // |proxy_configuration_specified_| is set to true if and only if any other
-  // proxy policy other than kPolicyNoProxyServer has been applied.
-  bool proxy_disabled_;
-  bool proxy_configuration_specified_;
+  // Temporary cache that stores values until FinalizeProxyPolicySettings()
+  // is called.
+  std::map<ConfigurationPolicyType, Value*> proxy_policies_;
 
-  // Set to true if a the proxy mode policy has been set to force Chrome
-  // to use the system proxy.
-  bool use_system_proxy_;
+  bool HasProxyPolicy(ConfigurationPolicyType policy) const;
 
   // Returns the map entry that corresponds to |policy| in the map.
   const PolicyToPreferenceMapEntry* FindPolicyInMap(
@@ -136,6 +120,19 @@ class ConfigurationPolicyPrefStore : public PrefStore,
   // map entries from |prefs_|.
   void FinalizeDefaultSearchPolicySettings();
 
+  // If the required entries for the proxy settings are specified and valid,
+  // finalizes the policy-specified configuration by initializing the
+  // respective values in |prefs_|.
+  void FinalizeProxyPolicySettings();
+
+  // Returns true if the policy values stored in proxy_* represent a valid
+  // proxy configuration.
+  bool CheckProxySettings();
+
+  // Assumes CheckProxySettings returns true and applies the values stored
+  // in proxy_*.
+  void ApplyProxySettings();
+
   DISALLOW_COPY_AND_ASSIGN(ConfigurationPolicyPrefStore);
 };
 
diff --git a/chrome/browser/policy/configuration_policy_pref_store_unittest.cc b/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
index 43e1295..5dad5dc 100644
--- a/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/file_path.h"
 #include "chrome/browser/policy/configuration_policy_pref_store.h"
 #include "chrome/browser/policy/mock_configuration_policy_provider.h"
+#include "chrome/browser/prefs/proxy_prefs.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/chrome_switches.h"
 
@@ -96,7 +97,7 @@ TEST_P(ConfigurationPolicyPrefStoreStringTest, GetDefault) {
 
 TEST_P(ConfigurationPolicyPrefStoreStringTest, SetValue) {
   store_.Apply(GetParam().type(),
-              Value::CreateStringValue("http://chromium.org"));
+               Value::CreateStringValue("http://chromium.org"));
   std::string result;
   EXPECT_TRUE(store_.prefs()->GetString(GetParam().pref_name(), &result));
   EXPECT_EQ(result, "http://chromium.org");
@@ -108,12 +109,6 @@ INSTANTIATE_TEST_CASE_P(
     testing::Values(
         TypeAndName(kPolicyHomePage,
                     prefs::kHomePage),
-        TypeAndName(kPolicyProxyServer,
-                    prefs::kProxyServer),
-        TypeAndName(kPolicyProxyPacUrl,
-                    prefs::kProxyPacUrl),
-        TypeAndName(kPolicyProxyBypassList,
-                    prefs::kProxyBypassList),
         TypeAndName(kPolicyApplicationLocale,
                     prefs::kApplicationLocale),
         TypeAndName(kPolicyApplicationLocale,
@@ -225,6 +220,44 @@ INSTANTIATE_TEST_CASE_P(
 
 // Test cases for the proxy policy settings.
 class ConfigurationPolicyPrefStoreProxyTest : public testing::Test {
+ protected:
+  // Verify that all the proxy prefs are set to the specified expected values.
+  static void VerifyProxyPrefs(
+      const ConfigurationPolicyPrefStore& store,
+      const std::string& expected_proxy_server,
+      const std::string& expected_proxy_pac_url,
+      const std::string& expected_proxy_bypass_list,
+      const ProxyPrefs::ProxyMode& expected_proxy_mode) {
+    std::string string_result;
+
+    if (expected_proxy_server.empty()) {
+      EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyServer,
+                                            &string_result));
+    } else {
+      EXPECT_TRUE(store.prefs()->GetString(prefs::kProxyServer,
+                                           &string_result));
+      EXPECT_EQ(expected_proxy_server, string_result);
+    }
+    if (expected_proxy_pac_url.empty()) {
+      EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyPacUrl,
+                                            &string_result));
+    } else {
+      EXPECT_TRUE(store.prefs()->GetString(prefs::kProxyPacUrl,
+                                           &string_result));
+      EXPECT_EQ(expected_proxy_pac_url, string_result);
+    }
+    if (expected_proxy_bypass_list.empty()) {
+      EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyBypassList,
+                                            &string_result));
+    } else {
+      EXPECT_TRUE(store.prefs()->GetString(prefs::kProxyBypassList,
+                  &string_result));
+      EXPECT_EQ(expected_proxy_bypass_list, string_result);
+    }
+    int int_result = -1;
+    EXPECT_TRUE(store.prefs()->GetInteger(prefs::kProxyMode, &int_result));
+    EXPECT_EQ(expected_proxy_mode, int_result);
+  }
 };
 
 TEST_F(ConfigurationPolicyPrefStoreProxyTest, ManualOptions) {
@@ -232,140 +265,95 @@ TEST_F(ConfigurationPolicyPrefStoreProxyTest, ManualOptions) {
       new MockConfigurationPolicyProvider());
   provider->AddPolicy(kPolicyProxyBypassList,
                       Value::CreateStringValue("http://chromium.org/override"));
-  provider->AddPolicy(kPolicyProxyPacUrl,
-                      Value::CreateStringValue("http://short.org/proxy.pac"));
   provider->AddPolicy(kPolicyProxyServer,
                       Value::CreateStringValue("chromium.org"));
-  provider->AddPolicy(kPolicyProxyServerMode,
+  provider->AddPolicy(kPolicyProxyMode,
                       Value::CreateIntegerValue(
                           kPolicyManuallyConfiguredProxyMode));
 
   ConfigurationPolicyPrefStore store(provider.get());
-
-  std::string string_result;
-  EXPECT_TRUE(store.prefs()->GetString(prefs::kProxyBypassList,
-                                       &string_result));
-  EXPECT_EQ("http://chromium.org/override", string_result);
-  EXPECT_TRUE(store.prefs()->GetString(prefs::kProxyPacUrl, &string_result));
-  EXPECT_EQ("http://short.org/proxy.pac", string_result);
-  EXPECT_TRUE(store.prefs()->GetString(prefs::kProxyServer, &string_result));
-  EXPECT_EQ("chromium.org", string_result);
-  bool bool_result;
-  EXPECT_TRUE(store.prefs()->GetBoolean(prefs::kNoProxyServer, &bool_result));
-  EXPECT_FALSE(bool_result);
-  EXPECT_TRUE(store.prefs()->GetBoolean(prefs::kProxyAutoDetect, &bool_result));
-  EXPECT_FALSE(bool_result);
+  VerifyProxyPrefs(
+      store, "chromium.org", "", "http://chromium.org/override",
+      ProxyPrefs::MODE_FIXED_SERVERS);
 }
 
-TEST_F(ConfigurationPolicyPrefStoreProxyTest, NoProxy) {
+TEST_F(ConfigurationPolicyPrefStoreProxyTest, ManualOptionsReversedApplyOrder) {
   scoped_ptr<MockConfigurationPolicyProvider> provider(
       new MockConfigurationPolicyProvider());
+  provider->AddPolicy(kPolicyProxyMode,
+                      Value::CreateIntegerValue(
+                          kPolicyManuallyConfiguredProxyMode));
   provider->AddPolicy(kPolicyProxyBypassList,
                       Value::CreateStringValue("http://chromium.org/override"));
-  provider->AddPolicy(kPolicyProxyServerMode,
-                      Value::CreateIntegerValue(
-                          kPolicyNoProxyServerMode));
+  provider->AddPolicy(kPolicyProxyServer,
+                      Value::CreateStringValue("chromium.org"));
 
   ConfigurationPolicyPrefStore store(provider.get());
-
-  std::string string_result;
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyBypassList,
-                                        &string_result));
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyPacUrl, &string_result));
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyServer, &string_result));
-  bool bool_result;
-  EXPECT_TRUE(store.prefs()->GetBoolean(prefs::kNoProxyServer, &bool_result));
-  EXPECT_TRUE(bool_result);
-  EXPECT_TRUE(store.prefs()->GetBoolean(prefs::kProxyAutoDetect, &bool_result));
-  EXPECT_FALSE(bool_result);
+  VerifyProxyPrefs(
+      store, "chromium.org", "", "http://chromium.org/override",
+      ProxyPrefs::MODE_FIXED_SERVERS);
 }
 
-TEST_F(ConfigurationPolicyPrefStoreProxyTest, NoProxyReversedApplyOrder) {
+TEST_F(ConfigurationPolicyPrefStoreProxyTest, NoProxy) {
   scoped_ptr<MockConfigurationPolicyProvider> provider(
       new MockConfigurationPolicyProvider());
-  provider->AddPolicy(kPolicyProxyServerMode,
-                      Value::CreateIntegerValue(
-                          kPolicyNoProxyServerMode));
-  provider->AddPolicy(kPolicyProxyBypassList,
-                      Value::CreateStringValue("http://chromium.org/override"));
+  provider->AddPolicy(kPolicyProxyMode,
+                      Value::CreateIntegerValue(kPolicyNoProxyServerMode));
 
   ConfigurationPolicyPrefStore store(provider.get());
-
-  std::string string_result;
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyBypassList,
-                                        &string_result));
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyPacUrl, &string_result));
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyServer, &string_result));
-  bool bool_result;
-  EXPECT_TRUE(store.prefs()->GetBoolean(prefs::kNoProxyServer, &bool_result));
-  EXPECT_TRUE(bool_result);
-  EXPECT_TRUE(store.prefs()->GetBoolean(prefs::kProxyAutoDetect, &bool_result));
-  EXPECT_FALSE(bool_result);
+  VerifyProxyPrefs(store, "", "", "", ProxyPrefs::MODE_DIRECT);
 }
 
 TEST_F(ConfigurationPolicyPrefStoreProxyTest, AutoDetect) {
   scoped_ptr<MockConfigurationPolicyProvider> provider(
       new MockConfigurationPolicyProvider());
-  provider->AddPolicy(kPolicyProxyServerMode,
-                      Value::CreateIntegerValue(
-                          kPolicyAutoDetectProxyMode));
+  provider->AddPolicy(kPolicyProxyMode,
+                      Value::CreateIntegerValue(kPolicyAutoDetectProxyMode));
 
   ConfigurationPolicyPrefStore store(provider.get());
-
-  std::string string_result;
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyBypassList,
-                                        &string_result));
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyPacUrl, &string_result));
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyServer, &string_result));
-  bool bool_result;
-  EXPECT_TRUE(store.prefs()->GetBoolean(prefs::kNoProxyServer, &bool_result));
-  EXPECT_FALSE(bool_result);
-  EXPECT_TRUE(store.prefs()->GetBoolean(prefs::kProxyAutoDetect, &bool_result));
-  EXPECT_TRUE(bool_result);
+  VerifyProxyPrefs(store, "", "", "", ProxyPrefs::MODE_AUTO_DETECT);
 }
 
-TEST_F(ConfigurationPolicyPrefStoreProxyTest, UseSystem) {
+TEST_F(ConfigurationPolicyPrefStoreProxyTest, AutoDetectPac) {
   scoped_ptr<MockConfigurationPolicyProvider> provider(
       new MockConfigurationPolicyProvider());
-  provider->AddPolicy(kPolicyProxyBypassList,
-                      Value::CreateStringValue("http://chromium.org/override"));
-  provider->AddPolicy(kPolicyProxyServerMode,
-                      Value::CreateIntegerValue(
-                          kPolicyUseSystemProxyMode));
+  provider->AddPolicy(kPolicyProxyPacUrl,
+                      Value::CreateStringValue("http://short.org/proxy.pac"));
+  provider->AddPolicy(kPolicyProxyMode,
+                      Value::CreateIntegerValue(kPolicyAutoDetectProxyMode));
 
   ConfigurationPolicyPrefStore store(provider.get());
-
-  std::string string_result;
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyBypassList,
-                                        &string_result));
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyPacUrl, &string_result));
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyServer, &string_result));
-  bool bool_result;
-  EXPECT_FALSE(store.prefs()->GetBoolean(prefs::kNoProxyServer, &bool_result));
-  EXPECT_FALSE(store.prefs()->GetBoolean(prefs::kProxyAutoDetect,
-                                         &bool_result));
+  VerifyProxyPrefs(
+      store, "", "http://short.org/proxy.pac", "", ProxyPrefs::MODE_PAC_SCRIPT);
 }
 
-TEST_F(ConfigurationPolicyPrefStoreProxyTest, UseSystemReversedApplyOrder) {
+TEST_F(ConfigurationPolicyPrefStoreProxyTest, UseSystem) {
   scoped_ptr<MockConfigurationPolicyProvider> provider(
       new MockConfigurationPolicyProvider());
-  provider->AddPolicy(kPolicyProxyServerMode,
-                      Value::CreateIntegerValue(
-                          kPolicyUseSystemProxyMode));
-  provider->AddPolicy(kPolicyProxyBypassList,
-                      Value::CreateStringValue("http://chromium.org/override"));
+  provider->AddPolicy(kPolicyProxyMode,
+                      Value::CreateIntegerValue(kPolicyUseSystemProxyMode));
 
   ConfigurationPolicyPrefStore store(provider.get());
+  VerifyProxyPrefs(store, "", "", "", ProxyPrefs::MODE_SYSTEM);
+}
 
-  std::string string_result;
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyBypassList,
-                                        &string_result));
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyPacUrl, &string_result));
-  EXPECT_FALSE(store.prefs()->GetString(prefs::kProxyServer, &string_result));
-  bool bool_result;
-  EXPECT_FALSE(store.prefs()->GetBoolean(prefs::kNoProxyServer, &bool_result));
-  EXPECT_FALSE(store.prefs()->GetBoolean(prefs::kProxyAutoDetect,
-                                         &bool_result));
+TEST_F(ConfigurationPolicyPrefStoreProxyTest, ProxyInvalid) {
+  for (int i = 0; i < MODE_COUNT; ++i) {
+    scoped_ptr<MockConfigurationPolicyProvider> provider(
+        new MockConfigurationPolicyProvider());
+    provider->AddPolicy(kPolicyProxyMode, Value::CreateIntegerValue(i));
+    // No mode expects all three parameters being set.
+    provider->AddPolicy(kPolicyProxyPacUrl,
+                        Value::CreateStringValue("http://short.org/proxy.pac"));
+    provider->AddPolicy(kPolicyProxyBypassList,
+                        Value::CreateStringValue(
+                            "http://chromium.org/override"));
+    provider->AddPolicy(kPolicyProxyServer,
+                        Value::CreateStringValue("chromium.org"));
+
+    ConfigurationPolicyPrefStore store(provider.get());
+    EXPECT_FALSE(store.prefs()->HasKey(prefs::kProxyMode));
+  }
 }
 
 class ConfigurationPolicyPrefStoreDefaultSearchTest : public testing::Test {
diff --git a/chrome/browser/policy/configuration_policy_provider_mac_unittest.cc b/chrome/browser/policy/configuration_policy_provider_mac_unittest.cc
index 5690375..345a3f6 100644
--- a/chrome/browser/policy/configuration_policy_provider_mac_unittest.cc
+++ b/chrome/browser/policy/configuration_policy_provider_mac_unittest.cc
@@ -233,8 +233,8 @@ INSTANTIATE_TEST_CASE_P(
             kPolicyDefaultSearchProviderEncodings,
             key::kDefaultSearchProviderEncodings),
         PolicyTestParams::ForIntegerPolicy(
-            kPolicyProxyServerMode,
-            key::kProxyServerMode),
+            kPolicyProxyMode,
+            key::kProxyMode),
         PolicyTestParams::ForStringPolicy(
             kPolicyProxyServer,
             key::kProxyServer),
diff --git a/chrome/browser/policy/configuration_policy_provider_win_unittest.cc b/chrome/browser/policy/configuration_policy_provider_win_unittest.cc
index 78c6f7b..beed450 100644
--- a/chrome/browser/policy/configuration_policy_provider_win_unittest.cc
+++ b/chrome/browser/policy/configuration_policy_provider_win_unittest.cc
@@ -391,8 +391,8 @@ INSTANTIATE_TEST_CASE_P(
             kPolicyDefaultSearchProviderEncodings,
             key::kDefaultSearchProviderEncodings),
         PolicyTestParams::ForIntegerPolicy(
-            kPolicyProxyServerMode,
-            key::kProxyServerMode),
+            kPolicyProxyMode,
+            key::kProxyMode),
         PolicyTestParams::ForStringPolicy(
             kPolicyProxyServer,
             key::kProxyServer),
diff --git a/chrome/browser/policy/configuration_policy_store_interface.h b/chrome/browser/policy/configuration_policy_store_interface.h
index 00ebf30..b6d1f3b 100644
--- a/chrome/browser/policy/configuration_policy_store_interface.h
+++ b/chrome/browser/policy/configuration_policy_store_interface.h
@@ -25,7 +25,7 @@ enum ConfigurationPolicyType {
   kPolicyDefaultSearchProviderIconURL,
   kPolicyDefaultSearchProviderEncodings,
   kPolicyDisableSpdy,
-  kPolicyProxyServerMode,
+  kPolicyProxyMode,
   kPolicyProxyServer,
   kPolicyProxyPacUrl,
   kPolicyProxyBypassList,
@@ -69,10 +69,24 @@ enum ConfigurationPolicyType {
   kPolicyDisable3DAPIs
 };
 
-static const int kPolicyNoProxyServerMode = 0;
-static const int kPolicyAutoDetectProxyMode = 1;
-static const int kPolicyManuallyConfiguredProxyMode = 2;
-static const int kPolicyUseSystemProxyMode = 3;
+
+// Constants for the "Proxy Server Mode" defined in the policies.
+// Note that these diverge from internal presentation defined in
+// ProxyPrefs::ProxyMode for legacy reasons. The following four
+// PolicyProxyModeType types were not very precise and had overlapping use
+// cases.
+enum PolicyProxyModeType {
+  // Disable Proxy, connect directly.
+  kPolicyNoProxyServerMode = 0,
+  // Auto detect proxy or use specific PAC script if given.
+  kPolicyAutoDetectProxyMode = 1,
+  // Use manually configured proxy servers (fixed servers).
+  kPolicyManuallyConfiguredProxyMode = 2,
+  // Use system proxy server.
+  kPolicyUseSystemProxyMode = 3,
+
+  MODE_COUNT
+};
 
 // An abstract super class for policy stores that provides a method that can be
 // called by a |ConfigurationPolicyProvider| to specify a policy.
diff --git a/chrome/browser/policy/managed_prefs_banner_base.cc b/chrome/browser/policy/managed_prefs_banner_base.cc
index 1351da4..7c20a09 100644
--- a/chrome/browser/policy/managed_prefs_banner_base.cc
+++ b/chrome/browser/policy/managed_prefs_banner_base.cc
@@ -84,8 +84,7 @@ void ManagedPrefsBannerBase::Init(PrefService* local_state,
 #if defined(GOOGLE_CHROME_BUILD)
       AddLocalStatePref(prefs::kMetricsReportingEnabled);
 #endif
-      AddUserPref(prefs::kNoProxyServer);
-      AddUserPref(prefs::kProxyAutoDetect);
+      AddUserPref(prefs::kProxyMode);
       AddUserPref(prefs::kProxyServer);
       AddUserPref(prefs::kProxyPacUrl);
       AddUserPref(prefs::kProxyBypassList);