Make proxy settings one atomic dictionary in the PrefStores such that modifications are atomic.

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"
@@ skipping 68 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();
 
+  ProxyPrefsDictionary proxy_dict(pref_service_->GetDictionary(prefs::kProxy));

[danno, 2011/02/04 10:32:13]

Perhaps there should be a specific constructor on the ProxyPrefsDictionary that
just takes a pref_service. This eliminates the needs to clients of the proxy
prefs dictionary to have to know which preference they need access.

[battre, 2011/02/07 14:13:47]

I think this change is a great idea if we can handle read and write access by
the ProxyPrefsDictionary. Unfortunately, I think that this would make stuff more
complicated.

- CommandLinePrefStore sets values directly into itself (i.e.
ProxyPrefsDictionary would need to write into a ValueMapPrefStore).
- We want to be able to store settings for incognito and regular pref services -
though the incognito pref service might not exist at the time of setting a
value. We would require a dependency to an ExtensionPrefs which would make
testing very awkward.
- AdvancedOptionsHandler checks whether the Preference is extension managed.
- We need to be able to subscribe to proxy pref changes.

I think it is better to leave this as it is.

+
   ProxyPrefs::ProxyMode mode;
-  int proxy_mode = pref_service_->GetInteger(prefs::kProxyMode);
-  if (!ProxyPrefs::IntToProxyMode(proxy_mode, &mode)) {
+  if (!proxy_dict.GetMode(&mode)) {
     // Fall back to system settings if the mode preference is invalid.
     return false;
   }
 
   switch (mode) {
     case ProxyPrefs::MODE_SYSTEM:
       // Use system settings.
       return false;
     case ProxyPrefs::MODE_DIRECT:
       // Ignore all the other proxy config preferences if the use of a proxy
       // has been explicitly disabled.
       return true;
     case ProxyPrefs::MODE_AUTO_DETECT:
       config->set_auto_detect(true);
       return true;
     case ProxyPrefs::MODE_PAC_SCRIPT: {
-      if (!pref_service_->HasPrefPath(prefs::kProxyPacUrl)) {
+      std::string proxy_pac;
+      if (!proxy_dict.GetPacUrl(&proxy_pac)) {
         LOG(ERROR) << "Proxy settings request PAC script but do not specify "
                    << "its URL. Falling back to direct connection.";
         return true;
       }
-      std::string proxy_pac = pref_service_->GetString(prefs::kProxyPacUrl);
       GURL proxy_pac_url(proxy_pac);
       if (!proxy_pac_url.is_valid()) {
         LOG(ERROR) << "Invalid proxy PAC url: " << proxy_pac;
         return true;
       }
       config->set_pac_url(proxy_pac_url);
       return true;
     }
     case ProxyPrefs::MODE_FIXED_SERVERS: {
-      if (!pref_service_->HasPrefPath(prefs::kProxyServer)) {
+      std::string proxy_server;
+      if (!proxy_dict.GetProxyServer(&proxy_server)) {
         LOG(ERROR) << "Proxy settings request fixed proxy servers but do not "
                    << "specify their URLs. Falling back to direct connection.";
         return true;
       }
-      std::string proxy_server =
-          pref_service_->GetString(prefs::kProxyServer);
       config->proxy_rules().ParseFromString(proxy_server);
 
-      if (pref_service_->HasPrefPath(prefs::kProxyBypassList)) {
-        std::string proxy_bypass =
-            pref_service_->GetString(prefs::kProxyBypassList);
+      std::string proxy_bypass;
+      if (proxy_dict.GetBypassList(&proxy_bypass)) {
         config->proxy_rules().bypass_rules.ParseFromString(proxy_bypass);
       }
       return true;
     }
     case ProxyPrefs::kModeCount: {
       // Fall through to NOTREACHED().
     }
   }
   NOTREACHED() << "Unknown proxy mode, falling back to system settings.";
   return false;
@@ skipping 77 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->RegisterIntegerPref(prefs::kProxyMode, ProxyPrefs::MODE_SYSTEM);
-  pref_service->RegisterStringPref(prefs::kProxyServer, "");
-  pref_service->RegisterStringPref(prefs::kProxyPacUrl, "");
-  pref_service->RegisterStringPref(prefs::kProxyBypassList, "");
+  DictionaryValue* default_settings = ProxyPrefsDictionary::CreateSystem();
+  pref_service->RegisterDictionaryPref(prefs::kProxy, default_settings);
 }