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

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

BUG=67779, 70904
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74284

[battre, 2011-01-27 15:19:35]

Please review. Thanks. :-)

http://codereview.chromium.org/6240013/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/6240013/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:242:
ProxyPrefsDictionary dict((DictionaryValue*) value);
changed this and two other occurrences to static_cast<DictionaryValue*> already
locally

[battre, 2011-02-02 11:08:28]

Ping (haven't received any reviews, yet)

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

Pretty good, mostly nits.

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_api.h (right):

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_api.h:46: bool
GetProxyRules(DictionaryValue* proxy_rules, std::string* out);
Ad bit of documentation would actually help here.

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_apitest.cc (right):

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_apitest.cc:23: void ExpectSettings(int
expected_mode,
s/Expect/Validate/g

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:89: ProxyPrefsDictionary
proxy_dict(pref_service_->GetDictionary(prefs::kProxy));
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.

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:412:
ASSERT_EQ(PrefStore::READ_NO_VALUE,
Why is this an assert? Asserts are for fatal errors that should abort test
execution.

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/prefs/proxy_...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:64: class ProxyPrefsDictionary {
How about putting this in the ProxyPrefs namespace and calling in
ProxyConfigDictionary?

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:86: const DictionaryValue* dict_;  // weak
pointer
Would it not be better/safer to copy the dictionary and own the copy?

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

Thanks, Danno. I have addressed your comments.

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_api.h (right):

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_api.h:46: bool
GetProxyRules(DictionaryValue* proxy_rules, std::string* out);
On 2011/02/04 10:32:13, danno wrote:
> Ad bit of documentation would actually help here. 

Done.

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_proxy_apitest.cc (right):

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_proxy_apitest.cc:23: void ExpectSettings(int
expected_mode,
On 2011/02/04 10:32:13, danno wrote:
> s/Expect/Validate/g

Done.

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/net/pref_pro...
File chrome/browser/net/pref_proxy_config_service.cc (right):

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/net/pref_pro...
chrome/browser/net/pref_proxy_config_service.cc:89: ProxyPrefsDictionary
proxy_dict(pref_service_->GetDictionary(prefs::kProxy));
On 2011/02/04 10:32:13, danno wrote:
> 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.

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.

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:412:
ASSERT_EQ(PrefStore::READ_NO_VALUE,
On 2011/02/04 10:32:13, danno wrote:
> Why is this an assert? Asserts are for fatal errors that should abort test
> execution.

Done.

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/prefs/proxy_...
File chrome/browser/prefs/proxy_prefs.h (right):

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:64: class ProxyPrefsDictionary {
On 2011/02/04 10:32:13, danno wrote:
> How about putting this in the ProxyPrefs namespace and calling in
> ProxyConfigDictionary? 

I have renamed ProxyPrefsDictionary to ProxyConfigDictionary, but I have moved
it into a new file such that class name and file name match.

Done.

http://codereview.chromium.org/6240013/diff/21001/chrome/browser/prefs/proxy_...
chrome/browser/prefs/proxy_prefs.h:86: const DictionaryValue* dict_;  // weak
pointer
On 2011/02/04 10:32:13, danno wrote:
> Would it not be better/safer to copy the dictionary and own the copy?

Done.

[danno, 2011-02-09 13:22:42]

lgtm