Policy: Add ProxyMode and deprecate ProxyServerMode.

Policy: Add ProxyMode and deprecate ProxyServerMode.

- Add support for 'deprecated' attribute in template generator
- Add support for both int- and string- based enums in the template generator
- Add logic to handle ProxyMode and fall back to ProxyServerMode

BUG=68134
TEST=ConfigurationPolicyPrefStore*, new policy template generator tests

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

[danno, 2010-12-29 11:14:43]

Please review. Given the open discussion about the "new" proxy mode, I'm not
sure if this is done, but it's certainly a start.

[gfeher, 2011-01-03 10:30:10]

I think the template generator changes would be nicer in a separate CL. (They
look overall good.)

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.grd (left):

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:864:
http://www.chromium.org/administrators/policy-list-3/serious-problem</ex></ph>
for more details.
Please don't remove this. There is still code in doc_writer.py that uses this
text, it's just currently not triggered from the JSON file. I'd be more happy if
you would create a separate bug for cleaning this up.

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:513: // If there's a
server mode, convert it info a mode.
Not: into

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:610: std::string
string_mode;
Why not use ProxyPrefs::StringToProxyMode here?

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/policy_template_generator.py
(right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/policy_template_generator.py:160: elif
policy['type'] == 'string-enum' or policy['type'] == 'int-enum':
Nit:
policy['type'] in ('string-enum', 'int-enum')

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/adm_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/adm_writer.py:87: if
policy['type'] == 'string-enum':
Here and at all the similar places:

I suggest using policy['type'] in ('int-enum', 'string-enum') as a condition and
 %s ... str(item['value']) would work for both cases.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/adml_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/adml_writer.py:90: elif
policy_type == 'int-enum':
Please merge the handling of 'int-enum' and 'string-enum'.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/admx_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/admx_writer.py:161: def
_AddIntEnumPolicy(self, parent, name, items):
Merge with the next function?

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/admx_writer_unittest.py
(right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/admx_writer_unittest.py:246:
{'name': 'item 1', 'value': 'one'},
I know this was already here, but please rename to 'item_1' and 'item_2'. There
should be no spaces here, because in real life these strings are also appearing
in grd message ids.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/doc_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/doc_writer.py:119: if
policy['type'] == 'int-enum':
Please merge handling of 'int-enum' and 'string-enum'.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/doc_writer.py:418:
self.AddText(h2, policy['name'] + " (DEPRECATED)")
Do we want to localize the word 'deprecated'?

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/json_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/json_writer.py:42:
example_value_str = example_value
Please make int-to-string conversion explicit.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/plist_strings_writer.py
(right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/plist_strings_writer.py:48: if
(policy['type'] == 'int-enum'):
Please merge handling of 'int-enum' and 'string-enum'.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/plist_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/plist_writer.py:89: if
(policy_type == 'int-enum'):
Please merge handling of 'int-enum' and 'string-enum'.

[battre, 2011-01-03 13:19:16]

Some comments from my side.

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:301: Use a fixed proxy server
There can be more than one proxy server (one for http, different one for https,
etc.)

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:333: <message
name="IDS_POLICY_PROXYMODE_DESC" desc="The description of the 'proxy bypass
rules' policy when displayed in OSX Workgroup Manager.">
update the desc="..."?

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:349: <message
name="IDS_POLICY_PROXYSERVERMODE_DESC" desc="The description of the 'proxy
mode">
nit: missing ' after proxy mode

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:281: 'deprecated' : True,
should we add more details:
- since when is it deprecated?
- when will it be removed?
- in API docs I like a comment like "use ProxyMode instead"

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:501: bool mode =
HasProxyPolicy(kPolicyProxyMode);
Add comment "deprecated"?

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:588: << " fixed proxy
server should be used but also"
servers (plural)?

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:159:
provider->AddPolicy(policy::kPolicyProxyServerMode, mode_value);
given that kPolicyProxyServerMode is deprecated, it might be batter to use the
new version. (also below)

[danno, 2011-01-07 12:24:25]

please take a look again

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.grd (right):

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:301: Use a fixed proxy server
On 2011/01/03 13:19:16, battre wrote:
> There can be more than one proxy server (one for http, different one for
https,
> etc.)

Done.

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:301: Use a fixed proxy server
On 2011/01/03 13:19:16, battre wrote:
> There can be more than one proxy server (one for http, different one for
https,
> etc.)

Done.

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:333: <message
name="IDS_POLICY_PROXYMODE_DESC" desc="The description of the 'proxy bypass
rules' policy when displayed in OSX Workgroup Manager.">
On 2011/01/03 13:19:16, battre wrote:
> update the desc="..."?

Done.

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.grd:349: <message
name="IDS_POLICY_PROXYSERVERMODE_DESC" desc="The description of the 'proxy
mode">
On 2011/01/03 13:19:16, battre wrote:
> nit: missing ' after proxy mode

Done.

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/5958014/diff/10001/chrome/app/policy/policy_te...
chrome/app/policy/policy_templates.json:281: 'deprecated' : True,
On 2011/01/03 13:19:16, battre wrote:
> should we add more details:
> - since when is it deprecated?
> - when will it be removed?
> - in API docs I like a comment like "use ProxyMode instead"
I would argue that we can add more complexity to the mechanism later. We
actually don't know when this option will be removed, and I'm not sure how
important the "deprecated since" information is, since it's still supported in
all versions. There already is an additional comment in the API docs for
ProxyServerMode that ProxyMode should be used.

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:501: bool mode =
HasProxyPolicy(kPolicyProxyMode);
On 2011/01/03 13:19:16, battre wrote:
> Add comment "deprecated"?

Done.

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:513: // If there's a
server mode, convert it info a mode.
On 2011/01/03 10:30:10, gfeher wrote:
> Not: into

Done.

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:588: << " fixed proxy
server should be used but also"
On 2011/01/03 13:19:16, battre wrote:
> servers (plural)?

Done.

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:610: std::string
string_mode;
On 2011/01/03 10:30:10, gfeher wrote:
> Why not use ProxyPrefs::StringToProxyMode here?

Done.

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/prefs/pref_s...
File chrome/browser/prefs/pref_service_unittest.cc (right):

http://codereview.chromium.org/5958014/diff/10001/chrome/browser/prefs/pref_s...
chrome/browser/prefs/pref_service_unittest.cc:159:
provider->AddPolicy(policy::kPolicyProxyServerMode, mode_value);
On 2011/01/03 13:19:16, battre wrote:
> given that kPolicyProxyServerMode is deprecated, it might be batter to use the
> new version. (also below)

Done.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/policy_template_generator.py
(right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/policy_template_generator.py:160: elif
policy['type'] == 'string-enum' or policy['type'] == 'int-enum':
On 2011/01/03 10:30:10, gfeher wrote:
> Nit:
> policy['type'] in ('string-enum', 'int-enum')

Done.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/adm_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/adm_writer.py:87: if
policy['type'] == 'string-enum':
On 2011/01/03 10:30:10, gfeher wrote:
> Here and at all the similar places:
> 
> I suggest using policy['type'] in ('int-enum', 'string-enum') as a condition
and
>  %s ... str(item['value']) would work for both cases.

Done.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/adml_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/adml_writer.py:90: elif
policy_type == 'int-enum':
On 2011/01/03 10:30:10, gfeher wrote:
> Please merge the handling of 'int-enum' and 'string-enum'.

Done.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/admx_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/admx_writer.py:161: def
_AddIntEnumPolicy(self, parent, name, items):
On 2011/01/03 10:30:10, gfeher wrote:
> Merge with the next function?

Done.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/admx_writer_unittest.py
(right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/admx_writer_unittest.py:246:
{'name': 'item 1', 'value': 'one'},
On 2011/01/03 10:30:10, gfeher wrote:
> I know this was already here, but please rename to 'item_1' and 'item_2'.
There
> should be no spaces here, because in real life these strings are also
appearing
> in grd message ids.

Done.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/doc_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/doc_writer.py:119: if
policy['type'] == 'int-enum':
On 2011/01/03 10:30:10, gfeher wrote:
> Please merge handling of 'int-enum' and 'string-enum'.

Done.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/doc_writer.py:418:
self.AddText(h2, policy['name'] + " (DEPRECATED)")
On 2011/01/03 10:30:10, gfeher wrote:
> Do we want to localize the word 'deprecated'?

Done.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/json_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/json_writer.py:42:
example_value_str = example_value
On 2011/01/03 10:30:10, gfeher wrote:
> Please make int-to-string conversion explicit.

Done.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/plist_strings_writer.py
(right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/plist_strings_writer.py:48: if
(policy['type'] == 'int-enum'):
On 2011/01/03 10:30:10, gfeher wrote:
> Please merge handling of 'int-enum' and 'string-enum'.

Done.

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/plist_writer.py (right):

http://codereview.chromium.org/5958014/diff/10001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/plist_writer.py:89: if
(policy_type == 'int-enum'):
On 2011/01/03 10:30:10, gfeher wrote:
> Please merge handling of 'int-enum' and 'string-enum'.

Done.

[battre, 2011-01-10 11:42:20]

two more nits

http://codereview.chromium.org/5958014/diff/36001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5958014/diff/36001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:495: bool server_mode =
HasProxyPolicy(kPolicyProxyServerMode);
actually, the ProxyServerMode is deprecated. (sorry for my imprecise comment
before)

Nit: whitespaces around //

http://codereview.chromium.org/5958014/diff/36001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:538: << " proxy servers
or a .pac url, but specifies neither.";
Nit: 80 characters

[gfeher, 2011-01-10 14:20:33]

http://codereview.chromium.org/5958014/diff/36001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5958014/diff/36001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:605:
ProxyPrefs::StringToProxyMode(string_mode, &mode);
A warning in case the supplied string literal is unknown and a default is
applied? if (!ProxyPrefs::StringToProx...)

http://codereview.chromium.org/5958014/diff/36001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/admx_writer.py (right):

http://codereview.chromium.org/5958014/diff/36001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/admx_writer.py:161: def
_AddEnumPolicy(self, parent, name, items, integer_enum):
I'd prefer to pass policy_type here, or better, why not pass the whole policy
and get rid of name, items and integer_enum?

http://codereview.chromium.org/5958014/diff/36001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/reg_writer.py (right):

http://codereview.chromium.org/5958014/diff/36001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/reg_writer.py:60:
example_value_str = 'dword:%d' % example_value
FYI: I've filed http://crbug.com/69054 for this line. It's okay to fix that in
another CL.

http://codereview.chromium.org/5958014/diff/36001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/template_writer.py (right):

http://codereview.chromium.org/5958014/diff/36001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/template_writer.py:61: return
self.IsDeprecatedPolicySupported(policy)
It would be better to still respect platform filtering for deprecated policies.
I am thinking of something like this:

if 'deprecated' in policy and policy['deprecated'] is True and not
self.IsDeprecatedPolicySupported(policy):
  return False
...

[danno, 2011-01-11 08:46:46]

Addressed feedback, please have a (final?) look.

http://codereview.chromium.org/5958014/diff/36001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5958014/diff/36001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:495: bool server_mode =
HasProxyPolicy(kPolicyProxyServerMode);
On 2011/01/10 11:42:20, battre wrote:
> actually, the ProxyServerMode is deprecated. (sorry for my imprecise comment
> before)
> 
> Nit: whitespaces around //

Done.

http://codereview.chromium.org/5958014/diff/36001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:538: << " proxy servers
or a .pac url, but specifies neither.";
On 2011/01/10 11:42:20, battre wrote:
> Nit: 80 characters

Done.

http://codereview.chromium.org/5958014/diff/36001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:605:
ProxyPrefs::StringToProxyMode(string_mode, &mode);
On 2011/01/10 14:20:33, gfeher wrote:
> A warning in case the supplied string literal is unknown and a default is
> applied? if (!ProxyPrefs::StringToProx...)

Done.

http://codereview.chromium.org/5958014/diff/36001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/admx_writer.py (right):

http://codereview.chromium.org/5958014/diff/36001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/admx_writer.py:161: def
_AddEnumPolicy(self, parent, name, items, integer_enum):
On 2011/01/10 14:20:33, gfeher wrote:
> I'd prefer to pass policy_type here, or better, why not pass the whole policy
> and get rid of name, items and integer_enum?

Done.

http://codereview.chromium.org/5958014/diff/36001/tools/grit/grit/format/poli...
File tools/grit/grit/format/policy_templates/writers/template_writer.py (right):

http://codereview.chromium.org/5958014/diff/36001/tools/grit/grit/format/poli...
tools/grit/grit/format/policy_templates/writers/template_writer.py:61: return
self.IsDeprecatedPolicySupported(policy)
On 2011/01/10 14:20:33, gfeher wrote:
> It would be better to still respect platform filtering for deprecated
policies.
> I am thinking of something like this:
> 
> if 'deprecated' in policy and policy['deprecated'] is True and not
> self.IsDeprecatedPolicySupported(policy):
>   return False
> ...

Done.

[gfeher, 2011-01-11 16:46:51]

LGTM for the Python part.
Some more comments for the C++ part.

http://codereview.chromium.org/5958014/diff/49001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5958014/diff/49001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:512: }
Warning if both mode and server_mode is defined?

http://codereview.chromium.org/5958014/diff/49001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:555: if
(!mode_value.empty()) {
Please add a comment to explain what does an empty mode_value mean. Maybe an
early exit in that case?

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

http://codereview.chromium.org/5958014/diff/49001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:266: 
Are you also testing the following stuff somewhere:
-ProxyMode?
-ProxyMode overrides ProxyServerMode?

[danno, 2011-01-12 09:32:33]

Make the changes you requested, there were a few non-trivial changes, so maybe
you want to take another look?

http://codereview.chromium.org/5958014/diff/49001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_pref_store.cc (right):

http://codereview.chromium.org/5958014/diff/49001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:512: }
On 2011/01/11 16:46:51, gfeher wrote:
> Warning if both mode and server_mode is defined?

Done.

http://codereview.chromium.org/5958014/diff/49001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store.cc:555: if
(!mode_value.empty()) {
On 2011/01/11 16:46:51, gfeher wrote:
> Please add a comment to explain what does an empty mode_value mean. Maybe an
> early exit in that case?

Done.

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

http://codereview.chromium.org/5958014/diff/49001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:266: 
For all modes, there should now be tests for both ProxyMode and ProxyServerMode
(e.g. see UseSystemProxyServerMode and UseSystemProxyMode).

I added a test to test the overrides.
On 2011/01/11 16:46:51, gfeher wrote:
> Are you also testing the following stuff somewhere:
> -ProxyMode?
> -ProxyMode overrides ProxyServerMode?

[gfeher, 2011-01-12 09:51:38]

LGTM