Intorduce a separate preference for 'proxy server mode'

chrome/browser/policy/configuration_policy_pref_store.cc

Index: chrome/browser/policy/configuration_policy_pref_store.cc
diff --git a/chrome/browser/policy/configuration_policy_pref_store.cc b/chrome/browser/policy/configuration_policy_pref_store.cc
index c777e35c701e0c93c9514f79d2ecff12add852a6..3040f657d3ee1a4136d7c705c4232fe0678b032e 100644
--- a/chrome/browser/policy/configuration_policy_pref_store.cc
+++ b/chrome/browser/policy/configuration_policy_pref_store.cc
@@ -12,6 +12,7 @@
 #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)
@@ -321,10 +322,7 @@ 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) {
+      lower_priority_proxy_settings_overridden_(false) {
   if (!provider_->Provide(this))
     LOG(WARNING) << "Failed to get policy from provider.";
   FinalizeDefaultSearchPolicySettings();
@@ -404,17 +402,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,
@@ -455,12 +442,25 @@ bool ConfigurationPolicyPrefStore::ApplyPolicyFromMap(
   return false;
 }
 
+bool ConfigurationPolicyPrefStore::NonNullValueExists(const std::string& path) {
+  Value* value = NULL;
+  if (!prefs_->Get(path, &value))
+    return false;
+  return value != NULL && !value->IsType(Value::TYPE_NULL);
+}
+
 bool ConfigurationPolicyPrefStore::ApplyProxyPolicy(
     ConfigurationPolicyType policy,
     Value* value) {
-  bool result = false;
-  bool warn_about_proxy_disable_config = false;
-  bool warn_about_proxy_system_config = false;
+
+  // Indicates if the specified proxy server mode conflicts with other proxy
+  // preferences.
+  bool conflict = false;
+  // If |conflict| is true, then this variable specifies the selected proxy
+  // server mode that lead to the conflict.

[Mattias Nissler (ping if slow), 2010/12/20 13:34:03]

s/lead/led/

[battre (please use the other), 2010/12/21 14:18:18]

Gone.

+  ProxyPrefs::ProxyServerMode conflicting_mode = ProxyPrefs::MANUAL;
+
+  std::string preference_path;

[Mattias Nissler (ping if slow), 2010/12/20 13:34:03]

Could use a comment saying what this is used for.

[battre (please use the other), 2010/12/21 14:18:18]

Gone.

 
   const PolicyToPreferenceMapEntry* match_entry =
       FindPolicyInMap(policy, kProxyPolicyMap, arraysize(kProxyPolicyMap));
@@ -473,93 +473,79 @@ bool ConfigurationPolicyPrefStore::ApplyProxyPolicy(
   // 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) {
+      (match_entry || policy == kPolicyProxyServerMode)) {
+    for (size_t current = 0; current < arraysize(kProxyPolicyMap); ++current) {
       // We use values of TYPE_NULL to mark preferences for which
       // READ_USE_DEFAULT should be returned by GetValue().
-      prefs_->Set(*i, Value::CreateNullValue());
+      prefs_->Set(kProxyPolicyMap[current].preference_path,
+                  Value::CreateNullValue());
     }
+    prefs_->Set(prefs::kProxyServerMode, Value::CreateNullValue());
     lower_priority_proxy_settings_overridden_ = true;
   }
 
-  // Translate the proxy policy into preferences.
   if (policy == kPolicyProxyServerMode) {
+    preference_path = prefs::kProxyServerMode;
+
+    // Don't claim ownership of the policy if we find it invalid.
     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;
-      }
+    if (!value->GetAsInteger(&int_value))
+      return false;
+    ProxyPrefs::ProxyServerMode mode;
+    if (!ProxyPrefs::IntToProxyMode(int_value, &mode))
+      return false;
 
-      if (int_value != kPolicyUseSystemProxyMode) {
-        prefs_->Set(prefs::kNoProxyServer,
-                    Value::CreateBooleanValue(proxy_disabled_));
-        prefs_->Set(prefs::kProxyAutoDetect,
-                    Value::CreateBooleanValue(proxy_auto_detect));
+    // Determine if the applied proxy policy settings conflict and issue
+    // a corresponding warning if they do.
+    if (mode == ProxyPrefs::DISABLED || mode == ProxyPrefs::SYSTEM) {
+      for (size_t current = 0; current < arraysize(kProxyPolicyMap);
+           ++current) {
+        if (NonNullValueExists(kProxyPolicyMap[current].preference_path)) {
+          conflict = true;
+          conflicting_mode = mode;
+        }
       }
     }
   } else if (match_entry) {
+    // Any proxy policy other than kPolicyProxyServerMode.
+
+    preference_path = match_entry->preference_path;
+
     // 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;
+    int int_mode = 0;
+    if (prefs_->GetInteger(prefs::kProxyServerMode, &int_mode)) {
+      if (ProxyPrefs::IntToProxyMode(int_mode, &conflicting_mode)) {
+        if (conflicting_mode == ProxyPrefs::DISABLED ||
+            conflicting_mode == ProxyPrefs::SYSTEM) {
+          conflict = true;
+        }
+      }
     }
-    result = true;
+  } else {
+    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.";
-  }
+  // We accept |value| as a policy and take ownership of it.
+  if (conflict) {
+    delete value;
 
-  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.";
+    if (conflicting_mode == ProxyPrefs::DISABLED) {
+      LOG(WARNING) << "A centrally-administered policy disables the use of"
+                   << " a proxy but also specifies an explicit proxy"
+                   << " configuration.";
+    } else if (conflicting_mode == ProxyPrefs::SYSTEM) {
+      LOG(WARNING) << "A centrally-administered policy dictates that the"
+                   << " system proxy settings should be used but also specifies"
+                   << " an explicit proxy configuration.";
+    } else {
+      NOTREACHED() << "Unexpected reason for a proxy policy conflict.";
+    }

[Mattias Nissler (ping if slow), 2010/12/20 13:34:03]

Hm, this whole logic for detecting we have a conflict is really cumbersome and
hard to understand. Maybe a two-step approach would be better? I'm thinking
along the lines of applying whatever we get fed in Apply() and then making the
sanity checks and printing the warning after Provide() is done? This would make
it obvious that the code behaves consistently no matter what order we Apply()
policies in.

[battre (please use the other), 2010/12/21 14:18:18]

I thought the same and implemented a Completed() method that is called by
Provide() at the end.

Done.

+  } else {
+    prefs_->Set(preference_path, value);
   }
 
-  // If the policy was a proxy policy, cleanup |value|.
-  if (result && value)
-    delete value;
-  return result;
+  return true;
 }
 
 bool ConfigurationPolicyPrefStore::ApplySyncPolicy(