Removed ConfigurationPolicyType and extended PolicyMap.

Removed ConfigurationPolicyType and extended PolicyMap.

PolicyMap was strongly coupled to specific Chrome policies, through the
ConfigurationPolicyType. That enum has been removed and replaced by
strings.

Also introduced new PolicyMap-related enums: PolicyScope and PolicyLevel.
Started removing mandatory/recommended policy_maps around. Currently there
are still mandatory/recommended providers, but that'll go away after this.

TBR=jhawkins@chromium.org
BUG=108993
TEST=All works as before

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=118648

[Joao da Silva, 2012-01-05 20:01:42]

DON'T PANIC

It's not as bad as it seems :-) Most of the changes are because
ConfigurationPolicyType went away.

Please review, thanks!

[Mattias Nissler (ping if slow), 2012-01-16 18:23:33]

I got about half way through, but my brain refuses to go any further, so I'll
send the half-finished review for now and continue tomorrow morning.

Two meta-comments regarding reviewability:

Since this is a fairly large review, it would have been really helpful to make
the two main aspects of the change into separate reviews (i.e. the
ConfigurationPolicyType -> string constants part and the PolicyMap introduction
part).

I wonder whether it would have been easier to leave the old PolicyMap definition
in place for now so the upper policy layers wouldn't need modifications,
introduce a new PolicyMap type (maybe in a future:: namespace) and then convert
the pieces one-by-one starting at the providers and working towards the higher
layers?

http://codereview.chromium.org/9111022/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider.cc:6012: policies, list,
providers[i].level, policy::POLICY_SCOPE_USER);
nitty nit: Putting the first 3 parameters after ( and breaking for the last one
seems more readable to me. Also, is hardcoding POLICY_SCOPE_USER appropriate
here, or should we also make that configurable through the parameter dictionary?

http://codereview.chromium.org/9111022/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:198:
dict->Set(i->first), i->second->value->DeepCopy());
I think this won't compile.

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/asynchron...
File chrome/browser/policy/asynchronous_policy_provider.h (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/asynchron...
chrome/browser/policy/asynchronous_policy_provider.h:61: PolicyScope scope_;
having the scope per-provider here might not be appropriate for the windows
provider (see other comment)

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_provider_impl.cc (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_provider_impl.cc:98:
combined_.FilterLevel(level_);
shouldn't that rather be FilterLevel first and then Swap? Doesn't really make a
difference here, but the idea of the swap in the first place is to construct the
fully-built replacement first before swapping.

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_provider_unittest.cc:27: void Set(PolicyMap*
map, const char* policy, Value* value) {
Maybe call this SetPolicy? Just Set is asking for name collisions.

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_handler.h (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_handler.h:260: kNoProxyServerMode =
0,
If you touch the enum, you might as well make the constants UPPER_CASE.

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

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_provider_win.cc:27:
POLICY_SCOPE_USER,
that should be POLICY_SCOPE_DEVICE for HKLM, no?

[Mattias Nissler (ping if slow), 2012-01-17 09:52:45]

I made it through! :)

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider.cc:90: POLICY_SCOPE_USER,
should be POLICY_SCOPE_DEVICE

btw., should we rename POLICY_SCOPE_DEVICE to POLICY_SCOPE_MACHINE? That would
be more in line with the group policy terms and "machine" is also not quite as
overloaded as "device" is.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:74: const char*
preference_path;
I see several random declaration order changes. Any reason?

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

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:130:
prefs::kApplicationLocale),
this seems to be a dupe.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:221:
prefs::kSavingBrowserHistoryDisabled),
another dupe?

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_mac.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_mac.cc:141:
POLICY_SCOPE_USER,
It looks like there is a way to figure out whether a setting on the mac is
machine-scope vs. user-scope after all:
http://developer.apple.com/library/mac/documentation/CoreFoundation/Reference...

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_win.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_win.cc:27:
POLICY_SCOPE_USER,
HKLM should be POLICY_SCOPE_MACHINE

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_reader.h (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_reader.h:65: GetPolicyStatus(const
char* policy) const;
fits the previous line?

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_reader.h:120: ListValue* list) const;
fits the previous line?

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:330: policies->Set(key::kProxyMode,
maybe we should create a proper proxy config dictionary here instead of doing
the legacy thing and relying on the fixup logic?

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider_unittest.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:48:
POLICY_SCOPE_USER,
SCOPE_MACHINE for consistency?

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/mock_...
chrome/browser/policy/mock_configuration_policy_provider.h:23: void
AddRecommendedPolicy(const std::string& policy, Value* value);
Hm, this looks weird. Either rename to AddMandatoryPolicy() or scrap
AddRecommendedPolicy and pass in the level as a parameter.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_map.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.cc:52: delete entry->value;
If Entry has a default constructor that NULLs value, you can just do:

Entry& entry = map_[policy];
delete entry.value;
// assignments.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.cc:101: void PolicyMap::GetDifferingKeys(const
PolicyMap& other,
I can't see who is calling this.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_map.h (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:24: POLICY_LEVEL_RECOMMENDED = 0,
don't need the = 0

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:32: // be applied. The values are listed in
increasing order of priority.
I don't think that description matches what the policy scope really is. The
browser vs. device split is really an intrinsic property of the policy itself
and not variable. I rather think of this as flag saying whether the policy is
being applied to the user or the machine. Of course, the device-level policies
we have on CrOS can never be POLICY_SCOPE_USER, since there is no fixed active
user for the machine.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:35: POLICY_SCOPE_USER = 0,
ditto

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:39: 
stray newline

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:56: bool equals(const Entry& other) const;
Should be Equals?

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:116: // temporary helper method, until
providers aren't split into mandatory and
had to re-read the sentence. Perhaps: "until mandatory and recommended levels
are served by a single provider"?

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_map_unittest.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map_unittest.cc:19: void Set(PolicyMap* map, const
char* name, Value* value) {
Asking for name clashes.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_status_info.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_status_info.cc:75: static const char* strings[] = {
"User", "Device" };
will about:policy now show "User" and "Mandatory" for policies that are not set?
That'd be unfortunate.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_cache.h (left):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_cache.h:76: // this field is decoded into
|mandatory|.
Please keep this comment, updating it to reflect the |policies| change.

[Joao da Silva, 2012-01-17 13:09:29]

Thanks for going through this monster :-) Please have a look at the changes.

http://codereview.chromium.org/9111022/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider.cc:6012: policies, list,
providers[i].level, policy::POLICY_SCOPE_USER);
On 2012/01/16 18:23:33, Mattias Nissler wrote:
> nitty nit: Putting the first 3 parameters after ( and breaking for the last
one
> seems more readable to me. Also, is hardcoding POLICY_SCOPE_USER appropriate
> here, or should we also make that configurable through the parameter
dictionary?

The scope should indeed be configurable. Added an arg for that, and incidentally
it also makes the LoadFrom call fit in one line.

http://codereview.chromium.org/9111022/diff/1/chrome/browser/automation/testi...
File chrome/browser/automation/testing_automation_provider_chromeos.cc (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/automation/testi...
chrome/browser/automation/testing_automation_provider_chromeos.cc:198:
dict->Set(i->first), i->second->value->DeepCopy());
On 2012/01/16 18:23:33, Mattias Nissler wrote:
> I think this won't compile.

Done.

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/asynchron...
File chrome/browser/policy/asynchronous_policy_provider.h (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/asynchron...
chrome/browser/policy/asynchronous_policy_provider.h:61: PolicyScope scope_;
On 2012/01/16 18:23:33, Mattias Nissler wrote:
> having the scope per-provider here might not be appropriate for the windows
> provider (see other comment)

This is meant to be removed once all the providers can load both user/device and
mandatory/recommended policies. It was introduced because each provider is
currently loading only one combination of scope+level.

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_provider_impl.cc (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_provider_impl.cc:98:
combined_.FilterLevel(level_);
On 2012/01/16 18:23:33, Mattias Nissler wrote:
> shouldn't that rather be FilterLevel first and then Swap? Doesn't really make
a
> difference here, but the idea of the swap in the first place is to construct
the
> fully-built replacement first before swapping.

Done.

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/cloud_pol...
File chrome/browser/policy/cloud_policy_provider_unittest.cc (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/cloud_pol...
chrome/browser/policy/cloud_policy_provider_unittest.cc:27: void Set(PolicyMap*
map, const char* policy, Value* value) {
On 2012/01/16 18:23:33, Mattias Nissler wrote:
> Maybe call this SetPolicy? Just Set is asking for name collisions.

Done.

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/configura...
File chrome/browser/policy/configuration_policy_handler.h (right):

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_handler.h:260: kNoProxyServerMode =
0,
On 2012/01/16 18:23:33, Mattias Nissler wrote:
> If you touch the enum, you might as well make the constants UPPER_CASE.

Done.

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

http://codereview.chromium.org/9111022/diff/1/chrome/browser/policy/configura...
chrome/browser/policy/configuration_policy_provider_win.cc:27:
POLICY_SCOPE_USER,
On 2012/01/16 18:23:33, Mattias Nissler wrote:
> that should be POLICY_SCOPE_DEVICE for HKLM, no?

The provider should set the scope as DEVICE or USER at each policy individually
depending on whether it comes from HKLM or HKCU. That requires more substantial
changes to AsynchronousPolicyProvider (such as directly getting a PolicyMap from
its loader, or more than one DictionaryValue, so that each policy can have its
own flags together with the Value). Will be done in a subsequent CL.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/config_dir_policy_provider.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/config_dir_policy_provider.cc:90: POLICY_SCOPE_USER,
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> should be POLICY_SCOPE_DEVICE
> 

Done.

> btw., should we rename POLICY_SCOPE_DEVICE to POLICY_SCOPE_MACHINE? That would
> be more in line with the group policy terms and "machine" is also not quite as
> overloaded as "device" is.

Agree, done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_handler.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_handler.cc:74: const char*
preference_path;
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> I see several random declaration order changes. Any reason?

Personal preference :-)

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

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:130:
prefs::kApplicationLocale),
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> this seems to be a dupe.

Yep, nice catch.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:221:
prefs::kSavingBrowserHistoryDisabled),
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> another dupe?

Done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_mac.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_mac.cc:141:
POLICY_SCOPE_USER,
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> It looks like there is a way to figure out whether a setting on the mac is
> machine-scope vs. user-scope after all:
>
http://developer.apple.com/library/mac/documentation/CoreFoundation/Reference...

Will use that when refactoring the mac provider!

Looking at that API, I'd guess SCOPE_MACHINE is when the function is invoked
with AnyUser and CurrentHost, while SCOPE_USER is CurrentUser and AnyHost. We'll
have to check if this does what we expect.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_win.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_win.cc:27:
POLICY_SCOPE_USER,
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> HKLM should be POLICY_SCOPE_MACHINE

Yes, that is planned for a subsequent CL. It requires changing the
AsynchronousPolicyProvider to expect a PolicyMap from the loader, instead of a
DictionaryValue, so that the loader can set the right policy level and scope at
each policy.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_reader.h (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_reader.h:65: GetPolicyStatus(const
char* policy) const;
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> fits the previous line?

Done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_reader.h:120: ListValue* list) const;
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> fits the previous line?

Done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/devic...
File chrome/browser/policy/device_policy_cache.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/devic...
chrome/browser/policy/device_policy_cache.cc:330: policies->Set(key::kProxyMode,
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> maybe we should create a proper proxy config dictionary here instead of doing
> the legacy thing and relying on the fixup logic?

Done.

The keys for the dictionary are the old policy names, but the 'dict' policy type
should really have a list of expected keys and their types in the template. This
would be used to generate documentation, and string constants for places like
this.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/file_...
File chrome/browser/policy/file_based_policy_provider_unittest.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/file_...
chrome/browser/policy/file_based_policy_provider_unittest.cc:48:
POLICY_SCOPE_USER,
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> SCOPE_MACHINE for consistency?

Done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/mock_...
File chrome/browser/policy/mock_configuration_policy_provider.h (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/mock_...
chrome/browser/policy/mock_configuration_policy_provider.h:23: void
AddRecommendedPolicy(const std::string& policy, Value* value);
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> Hm, this looks weird. Either rename to AddMandatoryPolicy() or scrap
> AddRecommendedPolicy and pass in the level as a parameter.

Renamed to AddMandatoryPolicy.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_map.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.cc:52: delete entry->value;
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> If Entry has a default constructor that NULLs value, you can just do:
> 
> Entry& entry = map_[policy];
> delete entry.value;
> // assignments.

Done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.cc:101: void PolicyMap::GetDifferingKeys(const
PolicyMap& other,
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> I can't see who is calling this.

This is meant to be used by clients of the future PolicyService, so that they
can figure which policies have been modified since the last update. wdyt?

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_map.h (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:24: POLICY_LEVEL_RECOMMENDED = 0,
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> don't need the = 0

Done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:32: // be applied. The values are listed in
increasing order of priority.
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> I don't think that description matches what the policy scope really is. The
> browser vs. device split is really an intrinsic property of the policy itself
> and not variable. I rather think of this as flag saying whether the policy is
> being applied to the user or the machine. Of course, the device-level policies
> we have on CrOS can never be POLICY_SCOPE_USER, since there is no fixed active
> user for the machine.

Rewrote the comments. Renaming SCOPE_DEVICE to SCOPE_MACHINE also helps clearing
this up.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:35: POLICY_SCOPE_USER = 0,
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> ditto

Done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:39: 
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> stray newline

Done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:56: bool equals(const Entry& other) const;
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> Should be Equals?

Done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:116: // temporary helper method, until
providers aren't split into mandatory and
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> had to re-read the sentence. Perhaps: "until mandatory and recommended levels
> are served by a single provider"?

Done.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_map_unittest.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map_unittest.cc:19: void Set(PolicyMap* map, const
char* name, Value* value) {
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> Asking for name clashes.

Renamed to SetPolicy.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_status_info.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_status_info.cc:75: static const char* strings[] = {
"User", "Device" };
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> will about:policy now show "User" and "Mandatory" for policies that are not
set?
> That'd be unfortunate.

about:policy will have a clean after this CL :-) There's this issue, and also
the scope field that has been disabled.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/user_...
File chrome/browser/policy/user_policy_cache.h (left):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/user_...
chrome/browser/policy/user_policy_cache.h:76: // this field is decoded into
|mandatory|.
On 2012/01/17 09:52:45, Mattias Nissler wrote:
> Please keep this comment, updating it to reflect the |policies| change.

Done.

[Mattias Nissler (ping if slow), 2012-01-18 15:07:07]

Made it through a second time! \o/

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider_mac.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider_mac.cc:141:
POLICY_SCOPE_USER,
On 2012/01/17 13:09:29, Joao da Silva wrote:
> On 2012/01/17 09:52:45, Mattias Nissler wrote:
> > It looks like there is a way to figure out whether a setting on the mac is
> > machine-scope vs. user-scope after all:
> >
>
http://developer.apple.com/library/mac/documentation/CoreFoundation/Reference...
> 
> Will use that when refactoring the mac provider!
> 
> Looking at that API, I'd guess SCOPE_MACHINE is when the function is invoked
> with AnyUser and CurrentHost, while SCOPE_USER is CurrentUser and AnyHost.
We'll
> have to check if this does what we expect.

I think user-level should rather be CurrentUser and CurrentHost, as otherwise we
might miss settings, otherwise I agree.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_map.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.cc:101: void PolicyMap::GetDifferingKeys(const
PolicyMap& other,
On 2012/01/17 13:09:29, Joao da Silva wrote:
> On 2012/01/17 09:52:45, Mattias Nissler wrote:
> > I can't see who is calling this.
> 
> This is meant to be used by clients of the future PolicyService, so that they
> can figure which policies have been modified since the last update. wdyt?

I can see what it's good for, but I still think should add it when you actually
need it and not introduce dead code.

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
File chrome/browser/policy/policy_status_info.cc (right):

http://codereview.chromium.org/9111022/diff/10007/chrome/browser/policy/polic...
chrome/browser/policy/policy_status_info.cc:75: static const char* strings[] = {
"User", "Device" };
On 2012/01/17 13:09:29, Joao da Silva wrote:
> On 2012/01/17 09:52:45, Mattias Nissler wrote:
> > will about:policy now show "User" and "Mandatory" for policies that are not
> set?
> > That'd be unfortunate.
> 
> about:policy will have a clean after this CL :-) There's this issue, and also
> the scope field that has been disabled.

I prefer not to commit stuff that regresses existing functionality.

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:6086: if
(args->GetBoolean("machine_scope", &machine_scope) && machine_scope)
shouldn't we rather name this "scope" with possible values of "machine" and
"user"? Also, this is a bit inconsistent, since the rest of the parameters allow
arbitrary combination, while you cannot set some policies to SCOPE_MACHINE and
some to SCOPE_USER with this code. I think we should rather change the format of
the argument to the call something like this:

{ "HomepageIsNewTabPage": {
     "value": "http://google.com",
     "scope": "machine",
     "level": "recommended"
} }

WDYT? If we choose to do this, I'm fine with not making scope configurable in
this CL but putting a TODO and doing it in a subsequent CL (that'll also have to
change existing tests).

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider.cc (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider.cc:94: // individual
deprecated policies.
I think this is actually incorrect. Assume you have proxy config set bot
recommended and mandatory. If mandatory specifies PAC but recommended specifies
fixed server, you'll combine them here. You should rather figure out the
highest-priority level/scope pair you have and only consider settings that have
that exact level/scope pair configured.

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_reader_unittest.cc (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_reader_unittest.cc:61: ListValue
recommended_value;
if this is called recommend_value, then we should rename in_value to
mandatory_value.

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_reader_unittest.cc:86: StringValue
string1("http://chromium.org");
maybe call these mandatory_value and recommended_value? string1 and string2 seem
bad choices.

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_map.h (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:50: Entry() :
level(POLICY_LEVEL_RECOMMENDED),
should break before :

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_status_info.cc (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/polic...
chrome/browser/policy/policy_status_info.cc:75: static const char* strings[] = {
"User", "Device" };
Shouldn't this now be "Machine"?

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/resources/po...
File chrome/browser/resources/policy.js (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/resources/po...
chrome/browser/resources/policy.js:21: 'scope': 'Device',
Shouldn't this now be 'Machine'?

http://codereview.chromium.org/9111022/diff/22001/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:414: '            do_set =
false;\n'
Hm, maybe we want to force the compiler warning here if a case handler is
missing by initializing do_set = false, putting do_set = true; into the
MANDATORY and RECOMMENDED branches and removing the default branch?

[Joao da Silva, 2012-01-18 16:49:29]

Thanks a lot! Only 8 files modified this time :-)

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:6086: if
(args->GetBoolean("machine_scope", &machine_scope) && machine_scope)
On 2012/01/18 15:07:07, Mattias Nissler wrote:
> shouldn't we rather name this "scope" with possible values of "machine" and
> "user"? Also, this is a bit inconsistent, since the rest of the parameters
allow
> arbitrary combination, while you cannot set some policies to SCOPE_MACHINE and
> some to SCOPE_USER with this code. I think we should rather change the format
of
> the argument to the call something like this:
> 
> { "HomepageIsNewTabPage": {
>      "value": "http://google.com",
>      "scope": "machine",
>      "level": "recommended"
> } }
> 
> WDYT? If we choose to do this, I'm fine with not making scope configurable in
> this CL but putting a TODO and doing it in a subsequent CL (that'll also have
to
> change existing tests). 

Agreed! The level is determined by the provider and the scope is hardcoded to
USER for now. Left a TODO to improve this interface once the PolicyService is
ready.

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider.cc (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider.cc:94: // individual
deprecated policies.
On 2012/01/18 15:07:07, Mattias Nissler wrote:
> I think this is actually incorrect. Assume you have proxy config set bot
> recommended and mandatory. If mandatory specifies PAC but recommended
specifies
> fixed server, you'll combine them here. You should rather figure out the
> highest-priority level/scope pair you have and only consider settings that
have
> that exact level/scope pair configured.

Thanks for catching this, that is exactly what should be done here. Fixed and
added a test for this in configuration_policy_pref_store_unittest.cc.

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_reader_unittest.cc (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_reader_unittest.cc:61: ListValue
recommended_value;
On 2012/01/18 15:07:07, Mattias Nissler wrote:
> if this is called recommend_value, then we should rename in_value to
> mandatory_value.

Done.

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_reader_unittest.cc:86: StringValue
string1("http://chromium.org");
On 2012/01/18 15:07:07, Mattias Nissler wrote:
> maybe call these mandatory_value and recommended_value? string1 and string2
seem
> bad choices.

Done.

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_map.h (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/polic...
chrome/browser/policy/policy_map.h:50: Entry() :
level(POLICY_LEVEL_RECOMMENDED),
On 2012/01/18 15:07:07, Mattias Nissler wrote:
> should break before :
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

Done.

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/polic...
File chrome/browser/policy/policy_status_info.cc (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/policy/polic...
chrome/browser/policy/policy_status_info.cc:75: static const char* strings[] = {
"User", "Device" };
On 2012/01/18 15:07:07, Mattias Nissler wrote:
> Shouldn't this now be "Machine"?

Yes, done. "Mandatory" and "Recommended" below have also swapped positions in
the new enum :-) Found it while fixing about:policy in another CL. I think these
strings should be translated anyway, so I'll fix that too later.

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/resources/po...
File chrome/browser/resources/policy.js (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/browser/resources/po...
chrome/browser/resources/policy.js:21: 'scope': 'Device',
On 2012/01/18 15:07:07, Mattias Nissler wrote:
> Shouldn't this now be 'Machine'?

Done.

http://codereview.chromium.org/9111022/diff/22001/chrome/tools/build/generate...
File chrome/tools/build/generate_policy_source.py (right):

http://codereview.chromium.org/9111022/diff/22001/chrome/tools/build/generate...
chrome/tools/build/generate_policy_source.py:414: '            do_set =
false;\n'
On 2012/01/18 15:07:07, Mattias Nissler wrote:
> Hm, maybe we want to force the compiler warning here if a case handler is
> missing by initializing do_set = false, putting do_set = true; into the
> MANDATORY and RECOMMENDED branches and removing the default branch?

Good idea, done.

[Mattias Nissler (ping if slow), 2012-01-19 12:58:36]

Getting close.

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

http://codereview.chromium.org/9111022/diff/24002/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:580: new
ConfigurationPolicyPrefStore(&provider));
Hm, this does a lot more than just testing the merging and I think the test
would be more focused if we just constructed two policy maps and called
FixDeprecatedPolicies on it. If we do that, we might as well move
FixDeprecatedPolicies to PolicyMap and make it a non-static member function.
WDYT?

http://codereview.chromium.org/9111022/diff/24002/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider.cc (right):

http://codereview.chromium.org/9111022/diff/24002/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider.cc:100:
!current_priority.has_higher_priority_than(*entry))
need curlies.

http://codereview.chromium.org/9111022/diff/24002/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider.cc:110:
policies->Erase(kProxyPolicies[i]);
Shouldn't we be doing this unconditionally?

[Joao da Silva, 2012-01-19 16:21:40]

One more pass :-)

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

http://codereview.chromium.org/9111022/diff/24002/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:580: new
ConfigurationPolicyPrefStore(&provider));
On 2012/01/19 12:58:36, Mattias Nissler wrote:
> Hm, this does a lot more than just testing the merging and I think the test
> would be more focused if we just constructed two policy maps and called
> FixDeprecatedPolicies on it. If we do that, we might as well move
> FixDeprecatedPolicies to PolicyMap and make it a non-static member function.
> WDYT?

PolicyMap is meant to be more generic, as discussed offline. Moved the test to
configuration_policy_provider_test.cc instead, and it's now directly using
FixDeprecatedPolicies.

http://codereview.chromium.org/9111022/diff/24002/chrome/browser/policy/confi...
File chrome/browser/policy/configuration_policy_provider.cc (right):

http://codereview.chromium.org/9111022/diff/24002/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider.cc:100:
!current_priority.has_higher_priority_than(*entry))
On 2012/01/19 12:58:36, Mattias Nissler wrote:
> need curlies.

Done.

http://codereview.chromium.org/9111022/diff/24002/chrome/browser/policy/confi...
chrome/browser/policy/configuration_policy_provider.cc:110:
policies->Erase(kProxyPolicies[i]);
On 2012/01/19 12:58:36, Mattias Nissler wrote:
> Shouldn't we be doing this unconditionally?

Yes. The only case where it matters is when there are both deprecated and
kProxySettings in the map, and in that case the deprecated policies should be
ignored anyway. Done.

[Mattias Nissler (ping if slow), 2012-01-20 11:36:32]

LGTM finally ;)

[commit-bot: I haz the power, 2012-01-20 13:25:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/9111022/13015

[commit-bot: I haz the power, 2012-01-20 13:26:08]

Presubmit check for 9111022-13015 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/resources/policy.js,chrome/browser/resources/policy.html

Presubmit checks took 5.9s to calculate.

[Joao da Silva, 2012-01-20 13:29:55]

@jhawkins: can you stamp this? A subsequent CL will cleanup about:policy a bit
and I'll have you reviewing that one :-)

[commit-bot: I haz the power, 2012-01-20 13:30:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/9111022/13015

[commit-bot: I haz the power, 2012-01-20 16:56:31]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 118453, job name
was 9111022-13015 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2012-01-22 11:48:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/9111022/45001

[commit-bot: I haz the power, 2012-01-22 14:16:04]

Try job failure for 9111022-45001 (retry) on win_rel for step "base_unittests".
It's a second try, previously, step "base_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...