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

chrome/browser/policy/configuration_policy_pref_store_unittest.cc

 // Copyright (c) 2011 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 "base/file_path.h"
 #include "base/ref_counted.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/notification_service.h"
@@ skipping 220 matching lines @@
 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) {
     Value* value = NULL;
-
+    ASSERT_EQ(PrefStore::READ_OK,
+              store.GetValue(prefs::kProxy, &value));
+    ASSERT_EQ(Value::TYPE_DICTIONARY, value->GetType());
+    ProxyPrefsDictionary dict(static_cast<DictionaryValue*>(value));
+    std::string s;
     if (expected_proxy_server.empty()) {
-      EXPECT_EQ(PrefStore::READ_USE_DEFAULT,
-                store.GetValue(prefs::kProxyServer, NULL));
+      EXPECT_FALSE(dict.GetProxyServer(&s));
     } else {
-      EXPECT_EQ(PrefStore::READ_OK,
-                store.GetValue(prefs::kProxyServer, &value));
-      EXPECT_TRUE(StringValue(expected_proxy_server).Equals(value));
+      ASSERT_TRUE(dict.GetProxyServer(&s));
+      EXPECT_EQ(expected_proxy_server, s);
     }
     if (expected_proxy_pac_url.empty()) {
-      EXPECT_EQ(PrefStore::READ_USE_DEFAULT,
-                store.GetValue(prefs::kProxyPacUrl, NULL));
+      EXPECT_FALSE(dict.GetPacUrl(&s));
     } else {
-      EXPECT_EQ(PrefStore::READ_OK,
-                store.GetValue(prefs::kProxyPacUrl, &value));
-      EXPECT_TRUE(StringValue(expected_proxy_pac_url).Equals(value));
+      ASSERT_TRUE(dict.GetPacUrl(&s));
+      EXPECT_EQ(expected_proxy_pac_url, s);
     }
     if (expected_proxy_bypass_list.empty()) {
-      EXPECT_EQ(PrefStore::READ_USE_DEFAULT,
-                store.GetValue(prefs::kProxyBypassList, NULL));
+      EXPECT_FALSE(dict.GetBypassList(&s));
     } else {
-      EXPECT_EQ(PrefStore::READ_OK,
-                store.GetValue(prefs::kProxyBypassList, &value));
-      EXPECT_TRUE(StringValue(expected_proxy_bypass_list).Equals(value));
+      ASSERT_TRUE(dict.GetBypassList(&s));
+      EXPECT_EQ(expected_proxy_bypass_list, s);
     }
-    EXPECT_EQ(PrefStore::READ_OK, store.GetValue(prefs::kProxyMode, &value));
-    EXPECT_TRUE(FundamentalValue(expected_proxy_mode).Equals(value));
+    ProxyPrefs::ProxyMode mode;
+    ASSERT_TRUE(dict.GetMode(&mode));
+    EXPECT_EQ(expected_proxy_mode, mode);
   }
 };
 
 TEST_F(ConfigurationPolicyPrefStoreProxyTest, ManualOptions) {
   MockConfigurationPolicyProvider provider;
   provider.AddPolicy(kPolicyProxyBypassList,
                      Value::CreateStringValue("http://chromium.org/override"));
   provider.AddPolicy(kPolicyProxyServer,
                      Value::CreateStringValue("chromium.org"));
   provider.AddPolicy(kPolicyProxyServerMode,
@@ skipping 124 matching lines @@
     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"));
 
     scoped_refptr<ConfigurationPolicyPrefStore> store(
         new ConfigurationPolicyPrefStore(&provider));
-    EXPECT_EQ(PrefStore::READ_NO_VALUE,
-              store->GetValue(prefs::kProxyMode, NULL));
+    Value* value = NULL;
+    ASSERT_EQ(PrefStore::READ_NO_VALUE,

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

Why is this an assert? Asserts are for fatal errors that should abort test
execution.

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

Done.

+              store->GetValue(prefs::kProxy, &value));
   }
 }
 
 class ConfigurationPolicyPrefStoreDefaultSearchTest : public testing::Test {
 };
 
 // Checks that if the policy for default search is valid, i.e. there's a
 // search URL, that all the elements have been given proper defaults.
 TEST_F(ConfigurationPolicyPrefStoreDefaultSearchTest, MinimallyDefined) {
   const char* const search_url = "http://test.com/search?t={searchTerms}";
@@ skipping 275 matching lines @@
 
   provider_.SetInitializationComplete(true);
   EXPECT_FALSE(store_->IsInitializationComplete());
 
   store_->OnUpdatePolicy();
   Mock::VerifyAndClearExpectations(&observer_);
   EXPECT_TRUE(store_->IsInitializationComplete());
 }
 
 }  // namespace policy