Add source column to chrome://policy showing the origins of policies.

Add source column to chrome://policy showing the origins of policies.

These are the possible sources for a policy:
Cloud - Policy values received from the cloud.
Platform - Local configured policies.
Enterprise Default - Defaults for policies in enterprise environments
                     on Chrome OS. Can be overridden.
Enterprise Override - Policies enforced in enterprise environments on
                      Chrome OS. Can NOT be overridden.

The source of a policy is determined when it's first set. For cloud
policies, this would be the decoding of the protocol buffers.
Platform policies are usually set by loading platform-specific local
configurations like entries in the Windows registry.
Enterprise defaults are automatically set if the device is active in
an enterprise environment.
Enterprise override policies are set within the code to enforce
specific behaviors (like ash tray visibility). They cannot be
overridden.

BUG=506123
TBR=jam@chromium.org

Committed: https://crrev.com/92e7960f06c2ac2ec0113bb88df3d322f0eb97fe
Cr-Commit-Position: refs/heads/master@{#349654}

Committed: https://crrev.com/2c4ce2a1988fdde3456e49112eba6c7855ca4528
Cr-Commit-Position: refs/heads/master@{#349749}

Committed: https://crrev.com/64834b71e60e7861a80f68f80d5cde8a56107084
Cr-Commit-Position: refs/heads/master@{#349927}

[fhorschig, 2015-08-31 16:31:34]

fhorschig@chromium.org changed reviewers:
+ tnagel@chromium.org

[Thiemo Nagel, 2015-09-01 17:40:38]

I've added comments.

In my point of view, it makes for cleaner and more legible code to make source a
member of the policy provider.

Also, in general please don't change the whitespace in the code that you modify,
it makes the changes much harder to read.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:59:
key::kLidCloseAction, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,
Please try to avoid reformatting existing code.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:62: NULL,
POLICY_SOURCE_PLATFORM);
I'd consider defining a different source for this "injected" policy.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:68:
POLICY_SOURCE_PLATFORM);
see above

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:74:
POLICY_SOURCE_PLATFORM);
see above

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:79: new
base::FundamentalValue(false), NULL, POLICY_SOURCE_PLATFORM);
see above

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:112:
PolicySource DeviceLocalAccountPolicyProvider::source() const {
Remove.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.h
(right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.h:49:
PolicySource source() const override;
Remove.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:808:
expected_policy_map_.Set(
Please don't reformat existing code.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:743:
DecodeIntegerValue(container.extension_cache_size()).release(), NULL);
Don't convert code from nullptr to NULL (only the other way round).

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/login_profile_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:119:
PolicySource LoginProfilePolicyProvider::source() const {
Remove.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/login_profile_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_profile_policy_provider.h:30: PolicySource
source() const override;
Remove.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:115:
policy_map_.Set(key::kHomepageLocation, POLICY_LEVEL_MANDATORY,
Don't reformat!

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/policy/c...
File chrome/browser/policy/cloud/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/policy/c...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:148:
POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
Don't reformat!

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/policy.html (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/policy.html:111: <td class="source-column">
I'd suggest to insert before "name-column".

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/policy.js:524: var tableHeadings = ['Scope', 'Level',
'Name', 'Value', 'Source',
I'd suggest to insert before 'Name'.  That seems more logical to me (and also
matches the order of PolicyMap::Entry members).

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/policy_ui.cc:15: #include "base/prefs/pref_service.h"
What is this required for?

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/policy_ui_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/policy_ui_browsertest.cc:44: const std::string& source,
I don't see a point in adding the source here.  It seems unused, doesn't it?

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/policy_ui_browsertest.cc:215:
values.Set(policy::key::kRestoreOnStartupURLs, policy::POLICY_LEVEL_MANDATORY,
Don't change existing formatting!

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/async_policy_loader.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_loader.h:13: #include
"components/policy/core/common/policy_types.h"
Don't include header files when it can be prevented by a forward declaration.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_loader.h:54: virtual PolicySource
source() const = 0;
Please add a source_ member and make this a non-abstract member function.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/async_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_provider.cc:51: PolicySource
AsyncPolicyProvider::source() const {
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/async_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_provider.h:43: //
ConfigurationPolicyProvider::source() now provided by internal loader.
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/async_policy_provider_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_provider_unittest.cc:50: PolicySource
source() const override;
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_provider_unittest.cc:76: PolicySource
MockPolicyLoader::source() const {
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.cc:93: PolicySource
CloudPolicyManager::source() const {
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.h:67: PolicySource
source() const override;
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager_unittest.cc:88:
store_.policy_map_.Set(policy_name, policy_level(), policy_scope(),
Don't reformat.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager_unittest.cc:117:
store_.policy_map_.Set(policy_name, policy_level(), policy_scope(),
Except here I would actually reformat in the other direction to make the file
more consistent.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc:35:
policy_map_.Set("key", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
Don't reformat.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/config_dir_policy_loader.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/config_dir_policy_loader.cc:87: PolicySource
ConfigDirPolicyLoader::source() const {
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/config_dir_policy_loader.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/config_dir_policy_loader.h:36: //
ConfigurationPolicyProvider::source() now indicates provided sources are
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/configuration_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/configuration_policy_provider.cc:48:
SetSources(source());
Not sure whether that makes any difference (can the policy be accessed
concurrently?) but it would seem more conservative to me to change the sources
first and swap second.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/configuration_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/configuration_policy_provider.h:77: // Template
Method used to inject a providers source into |UpdatePolicy|.
Could you please add a source_ member and make that a non-abstract getter
method?

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/configuration_policy_provider.h:93: void
SetSources(PolicySource source);
Again I think it would be clearer to call this "SetSource".

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/configuration_policy_provider_test.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/configuration_policy_provider_test.cc:343:
.Set(test_keys::kKeyString, test_harness_->policy_level(),
Don't reformat...

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/mock_configuration_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/mock_configuration_policy_provider.cc:32:
PolicySource MockConfigurationPolicyProvider::source() const {
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/mock_configuration_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/mock_configuration_policy_provider.h:49:
PolicySource source() const override;
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/policy_map.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.cc:52: source == other.source &&
base::Value::Equals(value, other.value) &&
I'm somehow split about this.  Since the source is only an annotation, I'm not
sure how useful it is to include it in comparison.  Either way, please add a
comment to motivate your decision.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.cc:76: if (source !=
POLICY_SOURCE_PLATFORM &&
This deserves a comment!

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/policy_map.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.h:28: PolicySource source;
Please annotate that this is for debugging/display only.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.h:63: void SetSources(PolicySource
source);
SetSource would seem easier understandable to me.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.h:74: void Set(const std::string&
policy,
Function overloading is frowned upon, cf.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over....

The combination of the old Set() and the new SetSource() should be sufficient
for all uses, shouldn't it?

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.h:103: // Loads the values in
|policies| into this PolicyMap. All policies loaded
Again, please don't overload.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_types.h:34: POLICY_SOURCE_DEFAULT,
POLICY_SOURCE_UNKNOWN would seem like a more descriptive name to me.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/proxy_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/proxy_policy_provider.cc:55: PolicySource
ProxyPolicyProvider::source() const {
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/proxy_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/proxy_policy_provider.h:52: //
ConfigurationPolicyProvider::source() now provided by delegate.
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/schema_registry_tracking_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/schema_registry_tracking_policy_provider.cc:76:
PolicySource SchemaRegistryTrackingPolicyProvider::source() const {
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/schema_registry_tracking_policy_provider.h
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/schema_registry_tracking_policy_provider.h:73: //
ConfigurationPolicyProvider::source() now provided by delegate.
Remove.

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
components/policy_strings.grdp:294: <message name="IDS_POLICY_HEADER_SOURCE"
desc="Table header for the column in policy table that contains the policy
source (managed, set by custodian, ...).">
The bracket "managed, set by custodian, ..." seems wrong.

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
components/policy_strings.grdp:325: <message name="IDS_POLICY_SOURCE_ENTERPRISE"
desc="Indicates that a policy has a default value
Please don't wrap lines: Stay consistent with the rest of the file.

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
components/policy_strings.grdp:329: <message name="IDS_POLICY_SOURCE_CLOUD"
desc="Indicates that the source of the policy originates
Either "the policy originates from the cloud" or "the source of the policy is
the cloud".

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
components/policy_strings.grdp:333: <message name="IDS_POLICY_SOURCE_PLATFORM"
desc="Indicates that the policy's value is read from
It's not just the "value", also the key is read.

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
components/policy_strings.grdp:334: the platform's local file system.">
"local file system" isn't entirely correct, under some circumstances the policy
can be read from the Windows registry, for example.  I'd make that more generic
and write that policy is "obtained" or "received" or "read from" the local OS.

[fhorschig, 2015-09-04 06:53:55]

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:59:
key::kLidCloseAction, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_MACHINE,
On 2015/09/01 17:40:35, Thiemo Nagel wrote:
> Please try to avoid reformatting existing code.

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:62: NULL,
POLICY_SOURCE_PLATFORM);
On 2015/09/01 17:40:35, Thiemo Nagel wrote:
> I'd consider defining a different source for this "injected" policy.

Done. Enforced policies are now of type ENFORCED.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:68:
POLICY_SOURCE_PLATFORM);
On 2015/09/01 17:40:35, Thiemo Nagel wrote:
> see above

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:74:
POLICY_SOURCE_PLATFORM);
On 2015/09/01 17:40:35, Thiemo Nagel wrote:
> see above

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:79: new
base::FundamentalValue(false), NULL, POLICY_SOURCE_PLATFORM);
On 2015/09/01 17:40:35, Thiemo Nagel wrote:
> see above

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:112:
PolicySource DeviceLocalAccountPolicyProvider::source() const {
On 2015/09/01 17:40:35, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.h
(right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_provider.h:49:
PolicySource source() const override;
On 2015/09/01 17:40:35, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:808:
expected_policy_map_.Set(
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Please don't reformat existing code.

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:743:
DecodeIntegerValue(container.extension_cache_size()).release(), NULL);
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Don't convert code from nullptr to NULL (only the other way round).

Done. (FYI: These two calls of "policies->Set" are now again the only 2 of all
55 which use nullptr instead of NULL)

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/login_profile_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:119:
PolicySource LoginProfilePolicyProvider::source() const {
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/login_profile_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/login_profile_policy_provider.h:30: PolicySource
source() const override;
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:115:
policy_map_.Set(key::kHomepageLocation, POLICY_LEVEL_MANDATORY,
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Don't reformat!

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/policy/c...
File chrome/browser/policy/cloud/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/policy/c...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:148:
POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Don't reformat!

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/policy.html (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/policy.html:111: <td class="source-column">
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> I'd suggest to insert before "name-column".

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/resource...
chrome/browser/resources/policy.js:524: var tableHeadings = ['Scope', 'Level',
'Name', 'Value', 'Source',
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> I'd suggest to insert before 'Name'.  That seems more logical to me (and also
> matches the order of PolicyMap::Entry members).

Done.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/policy_ui.cc:15: #include "base/prefs/pref_service.h"
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> What is this required for?

Deprecated. Removed.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/policy_ui_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/policy_ui_browsertest.cc:44: const std::string& source,
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> I don't see a point in adding the source here.  It seems unused, doesn't it?

Equals compares for sources. This is a good thing as this ensures that the
displayed source is in fact based on the actually set source.

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/policy_ui_browsertest.cc:215:
values.Set(policy::key::kRestoreOnStartupURLs, policy::POLICY_LEVEL_MANDATORY,
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Don't change existing formatting!

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/async_policy_loader.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_loader.h:13: #include
"components/policy/core/common/policy_types.h"
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Don't include header files when it can be prevented by a forward declaration.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_loader.h:54: virtual PolicySource
source() const = 0;
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Please add a source_ member and make this a non-abstract member function.

Done. (Not even necessary. Loaders are always of platform type.)

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/async_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_provider.cc:51: PolicySource
AsyncPolicyProvider::source() const {
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/async_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_provider.h:43: //
ConfigurationPolicyProvider::source() now provided by internal loader.
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/async_policy_provider_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_provider_unittest.cc:50: PolicySource
source() const override;
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/async_policy_provider_unittest.cc:76: PolicySource
MockPolicyLoader::source() const {
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.cc:93: PolicySource
CloudPolicyManager::source() const {
On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager.h:67: PolicySource
source() const override;
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_manager_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager_unittest.cc:88:
store_.policy_map_.Set(policy_name, policy_level(), policy_scope(),
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Don't reformat.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_manager_unittest.cc:117:
store_.policy_map_.Set(policy_name, policy_level(), policy_scope(),
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Except here I would actually reformat in the other direction to make the file
> more consistent.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc:35:
policy_map_.Set("key", POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Don't reformat.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/config_dir_policy_loader.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/config_dir_policy_loader.cc:87: PolicySource
ConfigDirPolicyLoader::source() const {
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/config_dir_policy_loader.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/config_dir_policy_loader.h:36: //
ConfigurationPolicyProvider::source() now indicates provided sources are
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/configuration_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/configuration_policy_provider.cc:48:
SetSources(source());
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Not sure whether that makes any difference (can the policy be accessed
> concurrently?) but it would seem more conservative to me to change the sources
> first and swap second.

The poorly named function SetSources sets the sources of all entries in
policy_bundle_.
Therefore, setting the sources first would not work.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/configuration_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/configuration_policy_provider.h:77: // Template
Method used to inject a providers source into |UpdatePolicy|.
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Could you please add a source_ member and make that a non-abstract getter
> method?

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/configuration_policy_provider.h:93: void
SetSources(PolicySource source);
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Again I think it would be clearer to call this "SetSource".

The name is misleading. This function actually sets the sources of the Policies,
not of this provider.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/configuration_policy_provider_test.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/configuration_policy_provider_test.cc:343:
.Set(test_keys::kKeyString, test_harness_->policy_level(),
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Don't reformat...

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/mock_configuration_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/mock_configuration_policy_provider.cc:32:
PolicySource MockConfigurationPolicyProvider::source() const {
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/mock_configuration_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/mock_configuration_policy_provider.h:49:
PolicySource source() const override;
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/policy_map.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.cc:52: source == other.source &&
base::Value::Equals(value, other.value) &&
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> I'm somehow split about this.  Since the source is only an annotation, I'm not
> sure how useful it is to include it in comparison.  Either way, please add a
> comment to motivate your decision.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.cc:76: if (source !=
POLICY_SOURCE_PLATFORM &&
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> This deserves a comment!

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/policy_map.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.h:28: PolicySource source;
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Please annotate that this is for debugging/display only.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.h:63: void SetSources(PolicySource
source);
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> SetSource would seem easier understandable to me.

Obviously misleading name. It sets the sources of all Entries, not the map, so
SetEntrySources is more appropriate.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.h:74: void Set(const std::string&
policy,
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Function overloading is frowned upon, cf.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over....
> 
> The combination of the old Set() and the new SetSource() should be sufficient
> for all uses, shouldn't it?

SetSources manipulates ALL entries. We don't want to do this every time we add a
policy. The function gets a new name.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_map.h:103: // Loads the values in
|policies| into this PolicyMap. All policies loaded
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Again, please don't overload.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/policy_types.h:34: POLICY_SOURCE_DEFAULT,
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> POLICY_SOURCE_UNKNOWN would seem like a more descriptive name to me.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/proxy_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/proxy_policy_provider.cc:55: PolicySource
ProxyPolicyProvider::source() const {
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/proxy_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/proxy_policy_provider.h:52: //
ConfigurationPolicyProvider::source() now provided by delegate.
On 2015/09/01 17:40:37, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/schema_registry_tracking_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/schema_registry_tracking_policy_provider.cc:76:
PolicySource SchemaRegistryTrackingPolicyProvider::source() const {
On 2015/09/01 17:40:38, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
File components/policy/core/common/schema_registry_tracking_policy_provider.h
(right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy/core/...
components/policy/core/common/schema_registry_tracking_policy_provider.h:73: //
ConfigurationPolicyProvider::source() now provided by delegate.
On 2015/09/01 17:40:38, Thiemo Nagel wrote:
> Remove.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
components/policy_strings.grdp:294: <message name="IDS_POLICY_HEADER_SOURCE"
desc="Table header for the column in policy table that contains the policy
source (managed, set by custodian, ...).">
On 2015/09/01 17:40:38, Thiemo Nagel wrote:
> The bracket "managed, set by custodian, ..." seems wrong.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
components/policy_strings.grdp:325: <message name="IDS_POLICY_SOURCE_ENTERPRISE"
desc="Indicates that a policy has a default value
On 2015/09/01 17:40:38, Thiemo Nagel wrote:
> Please don't wrap lines: Stay consistent with the rest of the file.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
components/policy_strings.grdp:329: <message name="IDS_POLICY_SOURCE_CLOUD"
desc="Indicates that the source of the policy originates
On 2015/09/01 17:40:38, Thiemo Nagel wrote:
> Either "the policy originates from the cloud" or "the source of the policy is
> the cloud".

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
components/policy_strings.grdp:333: <message name="IDS_POLICY_SOURCE_PLATFORM"
desc="Indicates that the policy's value is read from
On 2015/09/01 17:40:38, Thiemo Nagel wrote:
> It's not just the "value", also the key is read.

Done.

https://codereview.chromium.org/1304843004/diff/40001/components/policy_strin...
components/policy_strings.grdp:334: the platform's local file system.">
On 2015/09/01 17:40:38, Thiemo Nagel wrote:
> "local file system" isn't entirely correct, under some circumstances the
policy
> can be read from the Windows registry, for example.  I'd make that more
generic
> and write that policy is "obtained" or "received" or "read from" the local OS.

Done.

[Thiemo Nagel, 2015-09-04 20:15:54]

Getting there...

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/1304843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc:743:
DecodeIntegerValue(container.extension_cache_size()).release(), NULL);
On 2015/09/04 06:53:53, fhorschig wrote:
> On 2015/09/01 17:40:36, Thiemo Nagel wrote:
> > Don't convert code from nullptr to NULL (only the other way round).
> 
> Done. (FYI: These two calls of "policies->Set" are now again the only 2 of all
> 55 which use nullptr instead of NULL)

Feel free to submit a CL to convert the remaining 53.  Cf.
http://chromium-cpp.appspot.com/ and
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/4mijeJHzxLg.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:32:
source_ = POLICY_SOURCE_CLOUD;
I'd suggest to add a comment why this always is cloud policy.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:203:
expected_policy_map_.Set(key::kDisableSpdy,
This seems obsolete.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:898:
std::string()))
Please restore the formatting of this line.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:899:
.SetWithSource(key::kDisableSpdy,
Please indent this and the following lines by 4 characters.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/login_profile_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:93: source_ =
POLICY_SOURCE_CLOUD;
I'd suggest to add a comment why this always is cloud policy.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:410: // Asks the profile's Pref_Service for
the store that finally set the policy.
That comment seems bogus.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:771: std::string
PolicyUIHandler::GetSourceName(
Take a look at

https://codereview.chromium.org/385983007/diff/130001/chrome/browser/ui/webui...

for a more concise version of that code.  (Possibly also more efficient if the
std::string return type is replaced by char*.)

In any case, I'd suggest to move the function out of the class and into the
anonymous namespace at the top since it doesn't seem to be related to the class
at all.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:782: default:
Don't use a default case.  This prevents the compiler from warning about a
forgotten case once another entry is added to the enum.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:75: 
Remove blank line.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:193:
expected_policies.push_back(PopulateExpectedPolicy(
Don't reformat!

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:226: 
Remove blank line.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:325:
expected_policies.push_back(PopulateExpectedPolicy(
Don't reformat.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/async_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/async_policy_provider.cc:26: source_ =
POLICY_SOURCE_PLATFORM;
Add a comment why this always is platform policy.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/async_policy_provider_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/async_policy_provider_unittest.cc:39: void
SetPlatformPolicy(PolicyBundle* bundle,
Is it really necessary to have a separate function?  Why not change the existing
one?

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/configuration_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/configuration_policy_provider.cc:59: for
(PolicyBundle::iterator it = policy_bundle_.begin();
I'd suggest a range-based loop to improve readability.  C++11 FTW.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/configuration_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/configuration_policy_provider.h:77: // Returns
where the provided policies originate from.
In my opinion, a 1-line getter isn't worth a comment.

Don't document the obvious. ;)

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/configuration_policy_provider.h:90: // This
provider provides a policies only from one source at a time. And this
"a policies" --> "policies"

". And this source is stored in this value" --> ""

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/configuration_policy_provider.h:95: // Used to tag
all policies provided by this provider as coming from a
Not from "a specific source" but from "|source_|".

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/policy_map.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_map.cc:53: source == other.source &&  //
Necessary for observer updates.
It would be nice to mention which observer benefits from this.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_map.cc:77: for (PolicyMapType::iterator it
= map_.begin(); it != map_.end(); ++it) {
Please use range loop.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_map.cc:79: (it->second.source ==
POLICY_SOURCE_ENTERPRISE_DEFAULT ||
I still don't get what's happening here.  It seems wrong to have such
specialized functionality to be part of the very generic PolicyMap class.

Would it be possible to just ignore all entries that have (it->second.source !=
POLICY_SOURCE_UNKNOWN)?

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_types.h:33: // The Default value of a
policy source.
"Default" --> "default"

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_types.h:39: // If the policy was set
programmatically set to an unchangeable value (e.g.
"set programmatically set" --> "programmatically set"

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_types.h:40: // to enforce ash tray
visibility in public sessions)
Missing period at end of sentence.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_types.h:41: POLICY_SOURCE_ENFORCED,
ENFORCED seems unclear because all mandatory policies are "enforced" in some
sense, after all.  How about PROGRAMMATIC or OVERRIDE?  Any other ideas?

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_types.h:43: // If the policy is set by a
source specifically for this platform.
"source specifically for this platform" --> "platform source"

https://codereview.chromium.org/1304843004/diff/100001/components/policy_stri...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy_stri...
components/policy_strings.grdp:325: <message name="IDS_POLICY_SOURCE_ENTERPRISE"
desc="Indicates that a policy has a default value that is enforced in an
enterprise system (like Chrome OS).">
It is not enforced in an enterprise system but in an enterprise environment, not
sure what the criterion for that is (device enrolled?).

Thus I'd suggest:
"enterprise system (like Chrome OS)" --> "enterprise environment (for Chrome OS
only)"

https://codereview.chromium.org/1304843004/diff/100001/components/policy_stri...
components/policy_strings.grdp:331: <message name="IDS_POLICY_SOURCE_PLATFORM"
desc="Indicates that the policy is obtained from the  local OS.">
duplicate blank

https://codereview.chromium.org/1304843004/diff/100001/components/policy_stri...
components/policy_strings.grdp:335: Enforced
"Enforced" sounds somewhat negative.  Wouldn't "Programmatic" be nicer?

[fhorschig, 2015-09-07 14:09:35]

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:32:
source_ = POLICY_SOURCE_CLOUD;
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> I'd suggest to add a comment why this always is cloud policy.

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:203:
expected_policy_map_.Set(key::kDisableSpdy,
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> This seems obsolete.

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:898:
std::string()))
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> Please restore the formatting of this line.

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:899:
.SetWithSource(key::kDisableSpdy,
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> Please indent this and the following lines by 4 characters.

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/login_profile_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:93: source_ =
POLICY_SOURCE_CLOUD;
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> I'd suggest to add a comment why this always is cloud policy.

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:410: // Asks the profile's Pref_Service for
the store that finally set the policy.
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> That comment seems bogus.

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:771: std::string
PolicyUIHandler::GetSourceName(
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> Take a look at
> 
>
https://codereview.chromium.org/385983007/diff/130001/chrome/browser/ui/webui...
> 
> for a more concise version of that code.  (Possibly also more efficient if the
> std::string return type is replaced by char*.)
> 
> In any case, I'd suggest to move the function out of the class and into the
> anonymous namespace at the top since it doesn't seem to be related to the
class
> at all.

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:782: default:
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> Don't use a default case.  This prevents the compiler from warning about a
> forgotten case once another entry is added to the enum.

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:75: 
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> Remove blank line.

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:193:
expected_policies.push_back(PopulateExpectedPolicy(
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> Don't reformat!

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:226: 
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> Remove blank line.

Done.

https://codereview.chromium.org/1304843004/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:325:
expected_policies.push_back(PopulateExpectedPolicy(
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> Don't reformat.

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/async_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/async_policy_provider.cc:26: source_ =
POLICY_SOURCE_PLATFORM;
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> Add a comment why this always is platform policy.

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/async_policy_provider_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/async_policy_provider_unittest.cc:39: void
SetPlatformPolicy(PolicyBundle* bundle,
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> Is it really necessary to have a separate function?  Why not change the
existing
> one?

No. Removed.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/configuration_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/configuration_policy_provider.cc:59: for
(PolicyBundle::iterator it = policy_bundle_.begin();
On 2015/09/04 20:15:53, Thiemo Nagel wrote:
> I'd suggest a range-based loop to improve readability.  C++11 FTW.

Wow, it works !? Awesome! Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/configuration_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/configuration_policy_provider.h:77: // Returns
where the provided policies originate from.
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> In my opinion, a 1-line getter isn't worth a comment.
> 
> Don't document the obvious. ;)

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/configuration_policy_provider.h:90: // This
provider provides a policies only from one source at a time. And this
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> "a policies" --> "policies"
> 
> ". And this source is stored in this value" --> ""

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/configuration_policy_provider.h:95: // Used to tag
all policies provided by this provider as coming from a
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> Not from "a specific source" but from "|source_|".

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/policy_map.cc (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_map.cc:53: source == other.source &&  //
Necessary for observer updates.
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> It would be nice to mention which observer benefits from this.

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_map.cc:77: for (PolicyMapType::iterator it
= map_.begin(); it != map_.end(); ++it) {
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> Please use range loop.

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_map.cc:79: (it->second.source ==
POLICY_SOURCE_ENTERPRISE_DEFAULT ||
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> I still don't get what's happening here.  It seems wrong to have such
> specialized functionality to be part of the very generic PolicyMap class.
> 
> Would it be possible to just ignore all entries that have (it->second.source
!=
> POLICY_SOURCE_UNKNOWN)?

Currently, Yes. (If policies were set multiple time, this could be a problem,
but I can't find a place where this might occur, so I think the simpler way is
better.)

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_types.h:33: // The Default value of a
policy source.
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> "Default" --> "default"

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_types.h:39: // If the policy was set
programmatically set to an unchangeable value (e.g.
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> "set programmatically set" --> "programmatically set"

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_types.h:40: // to enforce ash tray
visibility in public sessions)
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> Missing period at end of sentence.

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_types.h:41: POLICY_SOURCE_ENFORCED,
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> ENFORCED seems unclear because all mandatory policies are "enforced" in some
> sense, after all.  How about PROGRAMMATIC or OVERRIDE?  Any other ideas?

All policies are set programmatically in some way, too. But PROGRAMMATIC is
definitely closer than ENFORCED.

https://codereview.chromium.org/1304843004/diff/100001/components/policy/core...
components/policy/core/common/policy_types.h:43: // If the policy is set by a
source specifically for this platform.
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> "source specifically for this platform" --> "platform source"

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy_stri...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/1304843004/diff/100001/components/policy_stri...
components/policy_strings.grdp:325: <message name="IDS_POLICY_SOURCE_ENTERPRISE"
desc="Indicates that a policy has a default value that is enforced in an
enterprise system (like Chrome OS).">
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> It is not enforced in an enterprise system but in an enterprise environment,
not
> sure what the criterion for that is (device enrolled?).
> 
> Thus I'd suggest:
> "enterprise system (like Chrome OS)" --> "enterprise environment (for Chrome
OS
> only)"

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy_stri...
components/policy_strings.grdp:331: <message name="IDS_POLICY_SOURCE_PLATFORM"
desc="Indicates that the policy is obtained from the  local OS.">
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> duplicate blank

Done.

https://codereview.chromium.org/1304843004/diff/100001/components/policy_stri...
components/policy_strings.grdp:335: Enforced
On 2015/09/04 20:15:54, Thiemo Nagel wrote:
> "Enforced" sounds somewhat negative.  Wouldn't "Programmatic" be nicer?

Was my first thought, too. Originally, I considered it slightly misleading (as
everything is set by the program) and thought we should indicate that it was a
special case. If you think programmatic is good, I am happy to go with that.

[Thiemo Nagel, 2015-09-07 15:18:57]

Very nice, you're getting there...

About the commit message:
Please wrap the message at 70 characters, this makes it easier to read in a
terminal and allows some leeway in case your CL is quoted (eg. during revert or
merge).

Also, please describe the fourth possible source.

Also there's a duplicate space in the description and in general there is some
further polish required.

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:804:
expected_policy_map_.SetWithSource(key::kDisableSpdy,
This is already setup in TestBase.  I don't think it's necessary here, either.

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/login_profile_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:93: // Policies
for profiles must be set by an admin and thus come from the cloud.
How about "All Chrome OS policies are coming from the cloud."?

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:81: const char* const kSourceNameStrings[5]
= {"",
Don't use bare numbers.  (Gasp!)

Please add a 6th (last) value to the PolicySource enum, calling it
POLICY_SOURCE_SIZE or POLICY_SOURCE_COUNT (not sure which is more common) and
use that here.

https://codereview.chromium.org/1304843004/diff/120001/components/policy/core...
File components/policy/core/common/policy_map.cc (right):

https://codereview.chromium.org/1304843004/diff/120001/components/policy/core...
components/policy/core/common/policy_map.cc:78: for (auto& policy : map_) {
This is not really a "policy", it's more a "map_entry" or somesuch.

https://codereview.chromium.org/1304843004/diff/120001/components/policy/core...
components/policy/core/common/policy_map.cc:79: if (policy.second.source !=
POLICY_SOURCE_UNKNOWN)
You can save one more line of code by inverting the if condition.

[fhorschig, 2015-09-07 15:59:35]

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:804:
expected_policy_map_.SetWithSource(key::kDisableSpdy,
On 2015/09/07 15:18:57, Thiemo Nagel wrote:
> This is already setup in TestBase.  I don't think it's necessary here, either.

The setup in base doesn't have the cloud source. This file contains tests for
the service and the provider. The first one can't set sources and the second one
has to. I think I can't spare this command.

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/login_profile_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/login_profile_policy_provider.cc:93: // Policies
for profiles must be set by an admin and thus come from the cloud.
On 2015/09/07 15:18:57, Thiemo Nagel wrote:
> How about "All Chrome OS policies are coming from the cloud."?

Much easier. I like it.

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:81: const char* const kSourceNameStrings[5]
= {"",
On 2015/09/07 15:18:57, Thiemo Nagel wrote:
> Don't use bare numbers.  (Gasp!)
> 
> Please add a 6th (last) value to the PolicySource enum, calling it
> POLICY_SOURCE_SIZE or POLICY_SOURCE_COUNT (not sure which is more common) and
> use that here.

Didn't find any convention, so I go with COUNT as stackoverflow proposed it...

https://codereview.chromium.org/1304843004/diff/120001/components/policy/core...
File components/policy/core/common/policy_map.cc (right):

https://codereview.chromium.org/1304843004/diff/120001/components/policy/core...
components/policy/core/common/policy_map.cc:78: for (auto& policy : map_) {
On 2015/09/07 15:18:57, Thiemo Nagel wrote:
> This is not really a "policy", it's more a "map_entry" or somesuch.

Done.

https://codereview.chromium.org/1304843004/diff/120001/components/policy/core...
components/policy/core/common/policy_map.cc:79: if (policy.second.source !=
POLICY_SOURCE_UNKNOWN)
On 2015/09/07 15:18:57, Thiemo Nagel wrote:
> You can save one more line of code by inverting the if condition.

Done.

[Thiemo Nagel, 2015-09-07 17:48:55]

You've overlooked the part about the commit message:

Please wrap the message at 70 characters, this makes it easier to read in a
terminal and allows some leeway in case your CL is quoted (eg. during revert or
merge).

Also, please describe the fourth possible source.

Also there's a duplicate space in the description and in general there is some
further polish required.

https://codereview.chromium.org/1304843004/diff/140001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/140001/components/policy/core...
components/policy/core/common/policy_types.h:49: // Number of sources types. Has
to be the last element.
s/sources/source/

[Thiemo Nagel, 2015-09-08 08:09:16]

https://codereview.chromium.org/1304843004/diff/140001/components/policy/core...
File components/policy/core/common/policy_map.h (right):

https://codereview.chromium.org/1304843004/diff/140001/components/policy/core...
components/policy/core/common/policy_map.h:64: // Sets the source of every Entry
in the Map to the passed |source|.
Please also mention the conditional.  This might confuse some readers otherwise.

[fhorschig, 2015-09-08 09:12:29]

https://codereview.chromium.org/1304843004/diff/140001/components/policy/core...
File components/policy/core/common/policy_map.h (right):

https://codereview.chromium.org/1304843004/diff/140001/components/policy/core...
components/policy/core/common/policy_map.h:64: // Sets the source of every Entry
in the Map to the passed |source|.
On 2015/09/08 08:09:16, Thiemo Nagel wrote:
> Please also mention the conditional.  This might confuse some readers
otherwise.

Done.

https://codereview.chromium.org/1304843004/diff/140001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/140001/components/policy/core...
components/policy/core/common/policy_types.h:49: // Number of sources types. Has
to be the last element.
On 2015/09/07 17:48:55, Thiemo Nagel wrote:
> s/sources/source/

Done.

[fhorschig, 2015-09-09 09:38:28]

fhorschig@chromium.org changed reviewers:
+ bartfab@chromium.org

[fhorschig, 2015-09-09 09:38:29]

Could you please take a look Bartosz?

[fhorschig, 2015-09-09 09:44:26]

fhorschig@chromium.org changed reviewers:
+ dbeam@chromium.org

[fhorschig, 2015-09-09 09:44:27]

Hi Dan, could you please check the files in chrome/browser/resources/?

[fhorschig, 2015-09-09 09:49:33]

On 2015/09/09 09:44:27, fhorschig wrote:
> Hi Dan, could you please check the files in chrome/browser/resources/?

Sorry, I missed something: could you please have a look into the
chrome/browser/ui/webui/ files?

[Thiemo Nagel, 2015-09-09 09:52:18]

LGTM.

Further nits about the commit message:

> Unknown - Source for unset policies.

It's not the policy that is unset but the source.

> Cloud - Policy values synced with a server.

... received from the cloud.  "Synced" could imply bidirectionality.

> Platform - Local configuration as used for debugging.

Debugging is not the main purpose of platform policy.

> Programmatic - Logically enforced by the program.

"Set by program code to achieve a specific functionality."?

> If a value is set and provided by any provider, the respective 

s/value/policy/

> If the default is a special case for an Chrome OS device in an 
> enterprise environment, the source for the initial value will be set 
> to "Enterprise Default".

You might want to mention that this is the "default_for_enterprise_users"
mechanism.

[Dan Beam, 2015-09-09 21:50:16]

https://codereview.chromium.org/1304843004/diff/220001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/220001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:81: const char* const
kSourceNameStrings[policy::POLICY_SOURCE_COUNT] = {"",
eh, this is kinda weird.  why do we have to duplicate the key names?  i'd say
this would be better as:

struct PolicySourceMap {
  policy::PolicySource source;
  const char* key;
  int string_id;
};

const PolicySourceMap kPolicySources[] = {
  {policy::POLICY_SOURCE_ENTERPRISE,
   "sourceEnterprise",
   IDS_POLICY_SOURCE_ENTERPRISE},
  ...
};

https://codereview.chromium.org/1304843004/diff/220001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:128:
source->AddLocalizedString("sourceCloud", IDS_POLICY_SOURCE_CLOUD);
and use kPolicySources here

https://codereview.chromium.org/1304843004/diff/220001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:765: value->SetString("source",
kSourceNameStrings[entry->second.source]);
and here

[fhorschig, 2015-09-10 11:36:31]

Removed duplication of i18n keys. Issue description updated. Only failing try
bots fail at things I didn't even remotely touch.

https://codereview.chromium.org/1304843004/diff/220001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/220001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:81: const char* const
kSourceNameStrings[policy::POLICY_SOURCE_COUNT] = {"",
On 2015/09/09 21:50:16, Dan Beam wrote:
> eh, this is kinda weird.  why do we have to duplicate the key names?  i'd say
> this would be better as:
> 
> struct PolicySourceMap {
>   policy::PolicySource source;
>   const char* key;
>   int string_id;
> };
> 
> const PolicySourceMap kPolicySources[] = {
>   {policy::POLICY_SOURCE_ENTERPRISE,
>    "sourceEnterprise",
>    IDS_POLICY_SOURCE_ENTERPRISE},
>   ...
> };

Good idea! Was there a special reason to include the source itself into the
struct? Never used, so I spared it.

https://codereview.chromium.org/1304843004/diff/220001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:128:
source->AddLocalizedString("sourceCloud", IDS_POLICY_SOURCE_CLOUD);
On 2015/09/09 21:50:16, Dan Beam wrote:
> and use kPolicySources here

Done.

https://codereview.chromium.org/1304843004/diff/220001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:765: value->SetString("source",
kSourceNameStrings[entry->second.source]);
On 2015/09/09 21:50:16, Dan Beam wrote:
> and here

Done.

[Thiemo Nagel, 2015-09-10 13:08:54]

Patchset #14 (id:260001) has been deleted

[Dan Beam, 2015-09-10 17:39:29]

lgtm

https://codereview.chromium.org/1304843004/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:87: {"", -1},  //
policy::POLICY_SOURCE_UNKNOWN
i originally included a soure member in PolicySourceMap so you might not need
this ...

https://codereview.chromium.org/1304843004/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:778: value->SetString("source",
kPolicySources[entry->second.source].key);
... but this could be worse

[fhorschig, 2015-09-11 07:50:23]

Patchset #5 (id:80001) has been deleted

[fhorschig, 2015-09-11 07:50:47]

Patchset #2 (id:20001) has been deleted

[fhorschig, 2015-09-11 07:51:15]

Patchset #8 (id:180001) has been deleted

[fhorschig, 2015-09-11 07:51:27]

Patchset #8 (id:200001) has been deleted

[fhorschig, 2015-09-11 07:51:51]

Patchset #9 (id:240001) has been deleted

[fhorschig, 2015-09-11 12:22:34]

https://codereview.chromium.org/1304843004/diff/280001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:87: {"", -1},  //
policy::POLICY_SOURCE_UNKNOWN
On 2015/09/10 17:39:28, Dan Beam wrote:
> i originally included a soure member in PolicySourceMap so you might not need
> this ...

Acknowledged.

https://codereview.chromium.org/1304843004/diff/280001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:778: value->SetString("source",
kPolicySources[entry->second.source].key);
On 2015/09/10 17:39:28, Dan Beam wrote:
> ... but this could be worse

Acknowledged.

[bartfab (slow), 2015-09-14 14:42:28]

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc:272:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:188:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:189:
POLICY_SOURCE_CLOUD);
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:33:
source_ = POLICY_SOURCE_CLOUD;
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:179:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(left):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:148:

Nit: Why remove the line?

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:808:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:809:
POLICY_SOURCE_CLOUD);
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:818:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:824:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:830:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:836:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:903:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:119:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:120:
POLICY_SOURCE_CLOUD);
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:125:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:131:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:137:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
File chrome/browser/policy/cloud/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:151: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:152:
POLICY_SOURCE_ENTERPRISE_DEFAULT);
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:157: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:163: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:173: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:179: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:188: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:194: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:200: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:207: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:213: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:219: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/resourc...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/resourc...
chrome/browser/resources/policy.js:137:
this.querySelector('.source').textContent = value.source ?
How can |source| be not set?

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:87: {"", -1},  //
policy::POLICY_SOURCE_UNKNOWN
Will the UI handle ID -1 gracefully? Should there not be a separate "unknown"
string?

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:194: it.key(), std::string(),
std::string(), NULL, false));
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:245: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/async_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/async_policy_provider.cc:25: source_ =
POLICY_SOURCE_PLATFORM;
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/async_policy_provider_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/async_policy_provider_unittest.cc:36: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/async_policy_provider_unittest.cc:37:
POLICY_SOURCE_PLATFORM);
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_manager.cc:37: source_ =
POLICY_SOURCE_CLOUD;
Nit 1: #include "components/policy/core/common/policy_types.h"
Nit 2: Finish setting up |this| before becoming a store observer. Otherwise, if
the store triggers an immediate notification, |this| may not be set up
completely yet.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_manager_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_manager_unittest.cc:65: :
PolicyProviderTestHarness(level, POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD) {
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc:39:
NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc:40:
POLICY_SOURCE_CLOUD);
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/config_dir_policy_loader_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/config_dir_policy_loader_unittest.cc:73:
POLICY_SOURCE_PLATFORM),
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/configuration_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider.h:77: PolicySource
source() const { return source_; }
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/configuration_policy_provider_test.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.cc:233: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.cc:350: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.cc:403: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.cc:412:
expected_policy.ApplySourceToEntries(test_harness_->policy_source());
If LoadFrom() is able to set the level and scope, could it not set the source as
well, removing the need to call ApplySourceToEntries()?

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/configuration_policy_provider_test.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.h:69: // the
providers from CreateProvider().
Nit: Document |source|.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.h:83: //
Returns the policy level and scope set by the policy provider.
Nit: Update to document the source as well.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/mock_configuration_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/mock_configuration_policy_provider.h:51:
MOCK_METHOD0(SetPolicySources, void());
I do not understand what you are doing here. SetPolicySources is private and not
virtual How would adding a mock method here change anything?

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_loader_ios_unittest.mm (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_ios_unittest.mm:72:
POLICY_SOURCE_PLATFORM),
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_loader_mac_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_mac_unittest.cc:63:
POLICY_SOURCE_PLATFORM) {}
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_mac_unittest.cc:195: NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:337:
POLICY_SOURCE_PLATFORM),
Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:338: hive_(hive) {}
Nit: Put the closing brace on a separate line (what was here before was not
compliant with the agreed style).

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:461:
POLICY_SOURCE_PLATFORM) {}
Nit: Put the closing brace on a separate line.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_map.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.cc:53: source == other.source &&  //
Necessary for PolicyUIHandler observers.
You can leave the comment here if you want but in general, I see no real need
for it. Equals() should obviously only return |true| if the two objects really
are... equal.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.cc:127: : NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.cc:147: : NULL,
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_map.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.h:64: // Sets the source of every Entry
with unknown source to the passed |source|.
Nit: s/passed/given/ ("passed" is used with scoped_ptrs only).

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.h:67: // Calls |SetWithSource| with
default source.
Nit 1: The most common style is to use |pipes| for variables, constants and
classes but empty brackets() instead for methods.
Nit 2: What is the default policy source?
Nit 3: Retain the documentation that this takes ownership of |value| and
|external_data_fetcher| (it is easy to miss if the comment is attached to the
method below only). Of course it would be even better if we used scoped_ptrs
here.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.h:68: void Set(const std::string&
policy,
Would it not be possible to make the code that calls Set() aware of the policy
source? That code already provides the |level| and |scope|. If it also set the
|source|, we could do away with the separate SetWithSource() method and with
ApplySourcesToEntries().

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_types.h:36: // If the policy's default was
determined by enterprise default settings.
Policies do not have defaults. They either are set or not.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_types.h:41: POLICY_SOURCE_PROGRAMMATIC,
How about something like BUILT_IN instead?

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_types.h:46: // If the policy originates
from a cloud source.
Nit: Unlike German, English actually appreciates repetitiveness. If you write
"is set by" and then "originates from" next, you are implying that there are
semantics hiding in the change of terms. In your case, the semantics should be
identical throughout. Please simplifying the comments to something more
repetitive, e.g.:

// Value automatically set by Chrome.
// Value set by platform policy.
// Value set by cloud policy.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/proxy_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/proxy_policy_provider.cc:39: source_ =
POLICY_SOURCE_UNKNOWN;
Nit 1: #include "components/policy/core/common/policy_types.h"
Nit 2: Do we really need to clear the source here? The provider is now shut down
and its policy list is empty, but the source that policies came from is still
the same.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/schema_registry_tracking_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/schema_registry_tracking_policy_provider.cc:15:
state_(WAITING_FOR_REGISTRY_READY) {
Nit: Why did you change the formatting of this line?

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/schema_registry_tracking_policy_provider.cc:16:
source_ = delegate->source();
Here and in all the other classes and tests: Why can |source_| not be set via
the initializer list?

https://codereview.chromium.org/1304843004/diff/300001/components/policy/tool...
File components/policy/tools/generate_policy_source.py (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/tool...
components/policy/tools/generate_policy_source.py:723: '                        
     NULL,\n'
Nit: s/NULL/nullptr/

https://codereview.chromium.org/1304843004/diff/300001/components/policy/tool...
components/policy/tools/generate_policy_source.py:724: '                        
     '
Nit: Remove blank line.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/tool...
components/policy/tools/generate_policy_source.py:725:
'POLICY_SOURCE_ENTERPRISE_DEFAULT);\n'
1: Where is |POLICY_SOURCE_ENTERPRISE_DEFAULT| coming from? policy_types.h does
not define it.
2: Nit: If you indent four spaces after |policy_map| above, there should be no
need for this odd line break.
3: Nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/1304843004/diff/300001/components/policy_stri...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy_stri...
components/policy_strings.grdp:334: <message
name="IDS_POLICY_SOURCE_PROGRAMMATIC" desc="Indicates that the policy is
programmatically set to an unchangeable value (e.g. to enforce ash tray
visibility in public sessions).">
Would this not be the same thing as "enterprise default" in the end?

[fhorschig, 2015-09-16 13:46:21]

Patchset #11 (id:320001) has been deleted

[fhorschig, 2015-09-16 13:52:06]

TBR'd jam@chromium.org because I changed PolicyMap::Set to have a source
parameter. Most of the changes are in tests where the source isn't actually
important but a policy must have a source.

Updated the CL description.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc:272:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:188:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:189:
POLICY_SOURCE_CLOUD);
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_local_account_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:33:
source_ = POLICY_SOURCE_CLOUD;
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_provider.cc:179:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(left):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:148:

On 2015/09/14 14:42:25, bartfab wrote:
> Nit: Why remove the line?

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:808:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:809:
POLICY_SOURCE_CLOUD);
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:818:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:824:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:830:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:836:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:903:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:119:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:120:
POLICY_SOURCE_CLOUD);
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:125:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:131:
NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:137:
NULL,
On 2015/09/14 14:42:25, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
File chrome/browser/policy/cloud/cloud_policy_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:151: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:152:
POLICY_SOURCE_ENTERPRISE_DEFAULT);
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:157: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:163: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:173: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:179: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:188: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:194: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:200: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:207: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:213: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/policy/...
chrome/browser/policy/cloud/cloud_policy_browsertest.cc:219: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/resourc...
File chrome/browser/resources/policy.js (right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/resourc...
chrome/browser/resources/policy.js:137:
this.querySelector('.source').textContent = value.source ?
On 2015/09/14 14:42:26, bartfab wrote:
> How can |source| be not set?

An empty string evaluates to false. Checking for empty string now to make it
more explicit.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:87: {"", -1},  //
policy::POLICY_SOURCE_UNKNOWN
On 2015/09/14 14:42:26, bartfab wrote:
> Will the UI handle ID -1 gracefully? Should there not be a separate "unknown"
> string?

The value won't be used as it is mapped to an actual key. the unknown string
would most likely map to an empty string (as discussed offline). Unknown will be
a thing of the past once a source is required for setting a policy.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:194: it.key(), std::string(),
std::string(), NULL, false));
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:245: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/async_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/async_policy_provider.cc:25: source_ =
POLICY_SOURCE_PLATFORM;
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/async_policy_provider_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/async_policy_provider_unittest.cc:36: NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/async_policy_provider_unittest.cc:37:
POLICY_SOURCE_PLATFORM);
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_manager.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_manager.cc:37: source_ =
POLICY_SOURCE_CLOUD;
On 2015/09/14 14:42:26, bartfab wrote:
> Nit 1: #include "components/policy/core/common/policy_types.h"
> Nit 2: Finish setting up |this| before becoming a store observer. Otherwise,
if
> the store triggers an immediate notification, |this| may not be set up
> completely yet.

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/cloud/cloud_policy_manager_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/cloud/cloud_policy_manager_unittest.cc:65: :
PolicyProviderTestHarness(level, POLICY_SCOPE_USER, POLICY_SOURCE_CLOUD) {
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc:39:
NULL,
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc:40:
POLICY_SOURCE_CLOUD);
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/config_dir_policy_loader_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/config_dir_policy_loader_unittest.cc:73:
POLICY_SOURCE_PLATFORM),
On 2015/09/14 14:42:26, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/configuration_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider.h:77: PolicySource
source() const { return source_; }
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/configuration_policy_provider_test.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.cc:233: NULL,
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.cc:350: NULL,
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.cc:403: NULL,
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.cc:412:
expected_policy.ApplySourceToEntries(test_harness_->policy_source());
On 2015/09/14 14:42:27, bartfab wrote:
> If LoadFrom() is able to set the level and scope, could it not set the source
as
> well, removing the need to call ApplySourceToEntries()?

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/configuration_policy_provider_test.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.h:69: // the
providers from CreateProvider().
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: Document |source|.

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/configuration_policy_provider_test.h:83: //
Returns the policy level and scope set by the policy provider.
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: Update to document the source as well.

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/mock_configuration_policy_provider.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/mock_configuration_policy_provider.h:51:
MOCK_METHOD0(SetPolicySources, void());
On 2015/09/14 14:42:27, bartfab wrote:
> I do not understand what you are doing here. SetPolicySources is private and
not
> virtual How would adding a mock method here change anything?

You are right, this change should not be relevant. In the latest version, the
method won't appear at all.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_loader_ios_unittest.mm (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_ios_unittest.mm:72:
POLICY_SOURCE_PLATFORM),
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_loader_mac_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_mac_unittest.cc:63:
POLICY_SOURCE_PLATFORM) {}
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_mac_unittest.cc:195: NULL,
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:337:
POLICY_SOURCE_PLATFORM),
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:338: hive_(hive) {}
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: Put the closing brace on a separate line (what was here before was not
> compliant with the agreed style).

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:461:
POLICY_SOURCE_PLATFORM) {}
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: Put the closing brace on a separate line.

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_map.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.cc:53: source == other.source &&  //
Necessary for PolicyUIHandler observers.
On 2015/09/14 14:42:27, bartfab wrote:
> You can leave the comment here if you want but in general, I see no real need
> for it. Equals() should obviously only return |true| if the two objects really
> are... equal.

This value is far less important for the Equality. It is tempting to remove it
because "hey, it doesn't matter where the same configuration comes from!'. So
this comment is just to prevent this thought.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.cc:127: : NULL,
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.cc:147: : NULL,
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_map.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.h:64: // Sets the source of every Entry
with unknown source to the passed |source|.
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: s/passed/given/ ("passed" is used with scoped_ptrs only).

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.h:67: // Calls |SetWithSource| with
default source.
On 2015/09/14 14:42:27, bartfab wrote:
> Nit 1: The most common style is to use |pipes| for variables, constants and
> classes but empty brackets() instead for methods.
> Nit 2: What is the default policy source?
> Nit 3: Retain the documentation that this takes ownership of |value| and
> |external_data_fetcher| (it is easy to miss if the comment is attached to the
> method below only). Of course it would be even better if we used scoped_ptrs
> here.

1 & 2 Done. Nit 3: This change is certainly useful but should get an own issue.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.h:68: void Set(const std::string&
policy,
On 2015/09/14 14:42:27, bartfab wrote:
> Would it not be possible to make the code that calls Set() aware of the policy
> source? That code already provides the |level| and |scope|. If it also set the
> |source|, we could do away with the separate SetWithSource() method and with
> ApplySourcesToEntries().

It is possible. Let me just update these 531 references.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_types.h:36: // If the policy's default was
determined by enterprise default settings.
On 2015/09/14 14:42:27, bartfab wrote:
> Policies do not have defaults. They either are set or not.

Removed in the comment. The enum value should still be named ENTERPRISE_DEFAULT
as it's a key in the policy_templates.json.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_types.h:41: POLICY_SOURCE_PROGRAMMATIC,
On 2015/09/14 14:42:27, bartfab wrote:
> How about something like BUILT_IN instead?

Good! We've been struggling with a good name.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_types.h:46: // If the policy originates
from a cloud source.
On 2015/09/14 14:42:27, bartfab wrote:
> Nit: Unlike German, English actually appreciates repetitiveness. If you write
> "is set by" and then "originates from" next, you are implying that there are
> semantics hiding in the change of terms. In your case, the semantics should be
> identical throughout. Please simplifying the comments to something more
> repetitive, e.g.:
> 
> // Value automatically set by Chrome.
> // Value set by platform policy.
> // Value set by cloud policy.

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/proxy_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/proxy_policy_provider.cc:39: source_ =
POLICY_SOURCE_UNKNOWN;
On 2015/09/14 14:42:27, bartfab wrote:
> Nit 1: #include "components/policy/core/common/policy_types.h"
> Nit 2: Do we really need to clear the source here? The provider is now shut
down
> and its policy list is empty, but the source that policies came from is still
> the same.

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/schema_registry_tracking_policy_provider.cc
(right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/schema_registry_tracking_policy_provider.cc:15:
state_(WAITING_FOR_REGISTRY_READY) {
On 2015/09/14 14:42:28, bartfab wrote:
> Nit: Why did you change the formatting of this line?

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/schema_registry_tracking_policy_provider.cc:16:
source_ = delegate->source();
On 2015/09/14 14:42:28, bartfab wrote:
> Here and in all the other classes and tests: Why can |source_| not be set via
> the initializer list?

In this case, I didn't want to call a function in an initializer list. Due to
its const nature, it shouldn't be a problem.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/tool...
File components/policy/tools/generate_policy_source.py (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/tool...
components/policy/tools/generate_policy_source.py:723: '                        
     NULL,\n'
On 2015/09/14 14:42:28, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/tool...
components/policy/tools/generate_policy_source.py:724: '                        
     '
On 2015/09/14 14:42:28, bartfab wrote:
> Nit: Remove blank line.

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy/tool...
components/policy/tools/generate_policy_source.py:725:
'POLICY_SOURCE_ENTERPRISE_DEFAULT);\n'
On 2015/09/14 14:42:28, bartfab wrote:
> 1: Where is |POLICY_SOURCE_ENTERPRISE_DEFAULT| coming from? policy_types.h
does
> not define it.
> 2: Nit: If you indent four spaces after |policy_map| above, there should be no
> need for this odd line break.
> 3: Nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/1304843004/diff/300001/components/policy_stri...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy_stri...
components/policy_strings.grdp:334: <message
name="IDS_POLICY_SOURCE_PROGRAMMATIC" desc="Indicates that the policy is
programmatically set to an unchangeable value (e.g. to enforce ash tray
visibility in public sessions).">
On 2015/09/14 14:42:28, bartfab wrote:
> Would this not be the same thing as "enterprise default" in the end?

There might be cases where this setting is different from a default (mutual
exclusions, fix of deprecated policies as in
https://code.google.com/p/chromium/codesearch#chromium/src/components/policy/...
)

[bartfab (slow), 2015-09-16 15:58:41]

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_map.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.h:68: void Set(const std::string&
policy,
On 2015/09/16 13:52:05, fhorschig wrote:
> On 2015/09/14 14:42:27, bartfab wrote:
> > Would it not be possible to make the code that calls Set() aware of the
policy
> > source? That code already provides the |level| and |scope|. If it also set
the
> > |source|, we could do away with the separate SetWithSource() method and with
> > ApplySourcesToEntries().
> 
> It is possible. Let me just update these 531 references. 

Thanks. This looks much better now.

https://codereview.chromium.org/1304843004/diff/300001/components/policy_stri...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy_stri...
components/policy_strings.grdp:334: <message
name="IDS_POLICY_SOURCE_PROGRAMMATIC" desc="Indicates that the policy is
programmatically set to an unchangeable value (e.g. to enforce ash tray
visibility in public sessions).">
On 2015/09/16 13:52:06, fhorschig wrote:
> On 2015/09/14 14:42:28, bartfab wrote:
> > Would this not be the same thing as "enterprise default" in the end?
> 
> There might be cases where this setting is different from a default (mutual
> exclusions, fix of deprecated policies as in
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/policy/...
> )

As discussed offline, let us adjust this as follows:

* IDS_POLICY_SOURCE_ENTERPRISE_DEFAULT stays
* This constant becomes IDS_POLICY_SOURCE_ENTERPRISE_OVERRIDE
* Policies whose value is programmatically derived from another policy (e.g.
proxy) inherit the source parameter.

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:807:
expected_policy_map_.Set(key::kDisableSpdy,
How come you expect a policy to be set that was not set before? The test setup
has not changed.

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:132:
POLICY_LEVEL_MANDATORY,POLICY_SCOPE_USER,
Nit: s/,/, /

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:94: for (int i = 0; i <
policy::POLICY_SOURCE_COUNT; ++i) {
Nit: arraysize() would be safer.

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:218:
policy::POLICY_SOURCE_CLOUD,
Nit: Could you verify at least one other source, to ensure that it is plumbed
through to the UI correctly?

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/async_policy_provider_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/async_policy_provider_unittest.cc:16: #include
"components/policy/core/common/policy_types.h"
Nit: Duplicate include.

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc:41:
nullptr);
Nit: You did not touch this line other than s/NULL/nullptr/ :).

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/configuration_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/configuration_policy_provider.cc:46: if
(bundle.get()) {
Nit: You do not need curly braces for single-line conditionals.

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/policy_map_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/policy_map_unittest.cc:80: 
Nit: Please include one or more of the other sources in the tests, to verify
that Set(), LoadFrom() and MergeFrom() set/copy sources as expected.

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/policy_service_impl.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/policy_service_impl.cc:68: POLICY_SOURCE_BUILT_IN,
Nit: As discussed offline, it is probably better to inherit the source here.

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/policy_service_impl_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/policy_service_impl_unittest.cc:74: 
Can you make one of the providers provide platform policy, so that we can check
the source is relfected correctly?

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/policy_types.h:33: // If the policy was set by
because session is an enterprise environment.
Nit: "by" whom?

[fhorschig, 2015-09-17 11:15:04]

Patchset #12 (id:360001) has been deleted

[fhorschig, 2015-09-17 11:20:42]

Issue description updated (especially for "Enterprise Override" which is the new
"Built-In").

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
File components/policy/core/common/policy_map.h (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy/core...
components/policy/core/common/policy_map.h:68: void Set(const std::string&
policy,
On 2015/09/16 15:58:40, bartfab wrote:
> On 2015/09/16 13:52:05, fhorschig wrote:
> > On 2015/09/14 14:42:27, bartfab wrote:
> > > Would it not be possible to make the code that calls Set() aware of the
> policy
> > > source? That code already provides the |level| and |scope|. If it also set
> the
> > > |source|, we could do away with the separate SetWithSource() method and
with
> > > ApplySourcesToEntries().
> > 
> > It is possible. Let me just update these 531 references. 
> 
> Thanks. This looks much better now.

Thank you :D

https://codereview.chromium.org/1304843004/diff/300001/components/policy_stri...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/1304843004/diff/300001/components/policy_stri...
components/policy_strings.grdp:334: <message
name="IDS_POLICY_SOURCE_PROGRAMMATIC" desc="Indicates that the policy is
programmatically set to an unchangeable value (e.g. to enforce ash tray
visibility in public sessions).">
On 2015/09/16 15:58:40, bartfab wrote:
> On 2015/09/16 13:52:06, fhorschig wrote:
> > On 2015/09/14 14:42:28, bartfab wrote:
> > > Would this not be the same thing as "enterprise default" in the end?
> > 
> > There might be cases where this setting is different from a default (mutual
> > exclusions, fix of deprecated policies as in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/policy/...
> > )
> 
> As discussed offline, let us adjust this as follows:
> 
> * IDS_POLICY_SOURCE_ENTERPRISE_DEFAULT stays
> * This constant becomes IDS_POLICY_SOURCE_ENTERPRISE_OVERRIDE
> * Policies whose value is programmatically derived from another policy (e.g.
> proxy) inherit the source parameter.

Done.

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_local_account_policy_service_unittest.cc:807:
expected_policy_map_.Set(key::kDisableSpdy,
On 2015/09/16 15:58:41, bartfab wrote:
> How come you expect a policy to be set that was not set before? The test setup
> has not changed.

Removed. It's exactly what was set in line 144. I had set up two different tests
and later merged them (my history told me).

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc:132:
POLICY_LEVEL_MANDATORY,POLICY_SCOPE_USER,
On 2015/09/16 15:58:41, bartfab wrote:
> Nit: s/,/, /

Done.

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:94: for (int i = 0; i <
policy::POLICY_SOURCE_COUNT; ++i) {
On 2015/09/16 15:58:41, bartfab wrote:
> Nit: arraysize() would be safer.

Done.

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui_browsertest.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_browsertest.cc:218:
policy::POLICY_SOURCE_CLOUD,
On 2015/09/16 15:58:41, bartfab wrote:
> Nit: Could you verify at least one other source, to ensure that it is plumbed
> through to the UI correctly?

Yes. Done.

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/async_policy_provider_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/async_policy_provider_unittest.cc:16: #include
"components/policy/core/common/policy_types.h"
On 2015/09/16 15:58:41, bartfab wrote:
> Nit: Duplicate include.

Done. (Arrggh!)

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc
(right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/cloud/user_cloud_policy_manager_unittest.cc:41:
nullptr);
On 2015/09/16 15:58:41, bartfab wrote:
> Nit: You did not touch this line other than s/NULL/nullptr/ :).

Done.

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/configuration_policy_provider.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/configuration_policy_provider.cc:46: if
(bundle.get()) {
On 2015/09/16 15:58:41, bartfab wrote:
> Nit: You do not need curly braces for single-line conditionals.

Done.

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/policy_map_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/policy_map_unittest.cc:80: 
On 2015/09/16 15:58:41, bartfab wrote:
> Nit: Please include one or more of the other sources in the tests, to verify
> that Set(), LoadFrom() and MergeFrom() set/copy sources as expected.

Done.

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/policy_service_impl.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/policy_service_impl.cc:68: POLICY_SOURCE_BUILT_IN,
On 2015/09/16 15:58:41, bartfab wrote:
> Nit: As discussed offline, it is probably better to inherit the source here.

Done.

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/policy_service_impl_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/policy_service_impl_unittest.cc:74: 
On 2015/09/16 15:58:41, bartfab wrote:
> Can you make one of the providers provide platform policy, so that we can
check
> the source is relfected correctly?

Done.

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/340001/components/policy/core...
components/policy/core/common/policy_types.h:33: // If the policy was set by
because session is an enterprise environment.
On 2015/09/16 15:58:41, bartfab wrote:
> Nit: "by" whom?

Done.

[fhorschig, 2015-09-17 11:26:45]

Patchset #12 (id:380001) has been deleted

[fhorschig, 2015-09-17 12:18:41]

Patchset #12 (id:400001) has been deleted

[bartfab (slow), 2015-09-17 15:45:51]

https://codereview.chromium.org/1304843004/diff/440001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/440001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:87: {"sourceENterpriseOverride",
IDS_POLICY_SOURCE_ENTERPRISE_OVERRIDE},
Nit: s/EN/En/

https://codereview.chromium.org/1304843004/diff/440001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:93: DCHECK(policy::POLICY_SOURCE_COUNT ==
arraysize(kPolicySources));
Nit 1: Use DCHECK_EQ()
Nit 2: #include "base/macros.h"

https://codereview.chromium.org/1304843004/diff/440001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:94: for (size_t i = 0; i <
arraysize(kPolicySources); ++i) {
Nit: #include <stddef.h>

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
File components/policy/core/common/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:767:
.LoadFrom(&expected_policy, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER
Nit: Add a comma at the end of the line.

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:945:
expected.Get(ns).LoadFrom(&policy, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER
Nit: Add a comma at the end of the line.

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:978:
expected.Get(ns).LoadFrom(&policy, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER
Nit: Add a comma at the end of the line.

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
File components/policy/core/common/policy_service_impl.cc (right):

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_service_impl.cc:53: inherited_source =
entry->source;  // Source of last existing entry.
Nit: Policy sources actually have a precedence order (enterprise_override >
platform > cloud > enterprise_default). Why not choose the highest-precendence
source instead of the (randomly) last one?

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_types.h:33: // The policy was set because
session is an enterprise environment.
Nit 1: s/because/because the/
Nit 2: It would be nice to avoid "session" here because the description will
then still make sense if we decide to use this source for a device policy at
some point.

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_types.h:36: // The policy was overridden
because session is an enterprise environment.
Nit 1: s/because/because the/
Nit 2: It would be nice to avoid "session" here because the description will
then still make sense if we decide to use this source for a device policy at
some point.

https://codereview.chromium.org/1304843004/diff/440001/components/policy_stri...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/1304843004/diff/440001/components/policy_stri...
components/policy_strings.grdp:334: <message
name="IDS_POLICY_SOURCE_ENTERPRISE_OVERRIDE" desc="Indicates that a policy is
enforced  in an enterprise environment and can not be overridden .">
Nit: I think this isv used on Chrome OS only as well.

[fhorschig, 2015-09-18 08:29:29]

Patchset #15 (id:480001) has been deleted

[fhorschig, 2015-09-18 08:29:40]

Patchset #14 (id:460001) has been deleted

[fhorschig, 2015-09-18 08:30:22]

https://codereview.chromium.org/1304843004/diff/440001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui.cc (right):

https://codereview.chromium.org/1304843004/diff/440001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:87: {"sourceENterpriseOverride",
IDS_POLICY_SOURCE_ENTERPRISE_OVERRIDE},
On 2015/09/17 15:45:51, bartfab wrote:
> Nit: s/EN/En/

Done.

https://codereview.chromium.org/1304843004/diff/440001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:93: DCHECK(policy::POLICY_SOURCE_COUNT ==
arraysize(kPolicySources));
On 2015/09/17 15:45:51, bartfab wrote:
> Nit 1: Use DCHECK_EQ()
> Nit 2: #include "base/macros.h"

Done.

https://codereview.chromium.org/1304843004/diff/440001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui.cc:94: for (size_t i = 0; i <
arraysize(kPolicySources); ++i) {
On 2015/09/17 15:45:51, bartfab wrote:
> Nit: #include <stddef.h>

Done.

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
File components/policy/core/common/policy_loader_win_unittest.cc (right):

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:767:
.LoadFrom(&expected_policy, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER
On 2015/09/17 15:45:51, bartfab wrote:
> Nit: Add a comma at the end of the line.

Done.

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:945:
expected.Get(ns).LoadFrom(&policy, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER
On 2015/09/17 15:45:51, bartfab wrote:
> Nit: Add a comma at the end of the line.

Done.

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_loader_win_unittest.cc:978:
expected.Get(ns).LoadFrom(&policy, POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER
On 2015/09/17 15:45:51, bartfab wrote:
> Nit: Add a comma at the end of the line.

Done.

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
File components/policy/core/common/policy_service_impl.cc (right):

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_service_impl.cc:53: inherited_source =
entry->source;  // Source of last existing entry.
On 2015/09/17 15:45:51, bartfab wrote:
> Nit: Policy sources actually have a precedence order (enterprise_override >
> platform > cloud > enterprise_default). Why not choose the highest-precendence
> source instead of the (randomly) last one?

Agreed. Platform policies can override (all known) enterprise-overrides, at
least in debug mode. I think as this is an debug view, platform should have
higher priority.

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_types.h:33: // The policy was set because
session is an enterprise environment.
On 2015/09/17 15:45:51, bartfab wrote:
> Nit 1: s/because/because the/
> Nit 2: It would be nice to avoid "session" here because the description will
> then still make sense if we decide to use this source for a device policy at
> some point.

Done.

https://codereview.chromium.org/1304843004/diff/440001/components/policy/core...
components/policy/core/common/policy_types.h:36: // The policy was overridden
because session is an enterprise environment.
On 2015/09/17 15:45:51, bartfab wrote:
> Nit 1: s/because/because the/
> Nit 2: It would be nice to avoid "session" here because the description will
> then still make sense if we decide to use this source for a device policy at
> some point.

Done.

https://codereview.chromium.org/1304843004/diff/440001/components/policy_stri...
File components/policy_strings.grdp (right):

https://codereview.chromium.org/1304843004/diff/440001/components/policy_stri...
components/policy_strings.grdp:334: <message
name="IDS_POLICY_SOURCE_ENTERPRISE_OVERRIDE" desc="Indicates that a policy is
enforced  in an enterprise environment and can not be overridden .">
On 2015/09/17 15:45:51, bartfab wrote:
> Nit: I think this isv used on Chrome OS only as well.

Done.

[bartfab (slow), 2015-09-18 08:58:01]

lgtm

https://codereview.chromium.org/1304843004/diff/500001/components/policy/core...
File components/policy/core/common/policy_service_impl.cc (right):

https://codereview.chromium.org/1304843004/diff/500001/components/policy/core...
components/policy/core/common/policy_service_impl.cc:53: if (entry->source >
inherited_source)  // Has entry a higher priority?
Nit: I can haz cheesburger? This is broken English :).

https://codereview.chromium.org/1304843004/diff/500001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/500001/components/policy/core...
components/policy/core/common/policy_types.h:34: // The policy was set because
the system is running in an enterprise
Nit: s/the system is/we are/ ("the system" would sound like the entire device on
CrOS, which is not true in case of user policy)

https://codereview.chromium.org/1304843004/diff/500001/components/policy/core...
components/policy/core/common/policy_types.h:41: // Any non-platform policy was
overridden because the system is running in an
Nit: s/the system is/we are/

[fhorschig, 2015-09-18 09:02:59]

https://codereview.chromium.org/1304843004/diff/500001/components/policy/core...
File components/policy/core/common/policy_service_impl.cc (right):

https://codereview.chromium.org/1304843004/diff/500001/components/policy/core...
components/policy/core/common/policy_service_impl.cc:53: if (entry->source >
inherited_source)  // Has entry a higher priority?
On 2015/09/18 08:58:01, bartfab wrote:
> Nit: I can haz cheesburger? This is broken English :).

Done.

https://codereview.chromium.org/1304843004/diff/500001/components/policy/core...
File components/policy/core/common/policy_types.h (right):

https://codereview.chromium.org/1304843004/diff/500001/components/policy/core...
components/policy/core/common/policy_types.h:34: // The policy was set because
the system is running in an enterprise
On 2015/09/18 08:58:01, bartfab wrote:
> Nit: s/the system is/we are/ ("the system" would sound like the entire device
on
> CrOS, which is not true in case of user policy)

Done.

https://codereview.chromium.org/1304843004/diff/500001/components/policy/core...
components/policy/core/common/policy_types.h:41: // Any non-platform policy was
overridden because the system is running in an
On 2015/09/18 08:58:01, bartfab wrote:
> Nit: s/the system is/we are/

Done.

[fhorschig, 2015-09-18 09:06:06]

The CQ bit was checked by fhorschig@chromium.org

[fhorschig, 2015-09-18 09:06:06]

The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org,
dbeam@chromium.org, bartfab@chromium.org
Link to the patchset: https://codereview.chromium.org/1304843004/#ps520001
(title: "Nits.")

[commit-bot: I haz the power, 2015-09-18 09:06:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304843004/520001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304843004/520001

[commit-bot: I haz the power, 2015-09-18 09:45:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-18 09:45:20]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[fhorschig, 2015-09-18 11:19:14]

Patchset #15 (id:520001) has been deleted

[fhorschig, 2015-09-18 11:19:27]

The CQ bit was checked by fhorschig@chromium.org

[fhorschig, 2015-09-18 11:19:28]

The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org,
bartfab@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1304843004/#ps540001
(title: " ")

[commit-bot: I haz the power, 2015-09-18 11:19:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304843004/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304843004/540001

[commit-bot: I haz the power, 2015-09-18 12:40:21]

Committed patchset #15 (id:540001)

[commit-bot: I haz the power, 2015-09-18 12:40:51]

Patchset 15 (id:??) landed as
https://crrev.com/92e7960f06c2ac2ec0113bb88df3d322f0eb97fe
Cr-Commit-Position: refs/heads/master@{#349654}

[jochen (gone - plz use gerrit), 2015-09-18 12:48:43]

please don't TBR someone without actually adding them as reviewer.

[jochen (gone - plz use gerrit), 2015-09-18 12:48:45]

please don't TBR someone without actually adding them as reviewer.

[jochen (gone - plz use gerrit), 2015-09-18 12:49:33]

A revert of this CL (patchset #15 id:540001) has been created in
https://codereview.chromium.org/1348903007/ by jochen@chromium.org.

The reason for reverting is: breaks compilation on iOS Device (ninja).

[vabr (Chromium), 2015-09-18 12:53:07]

A revert of this CL (patchset #15 id:540001) has been created in
https://codereview.chromium.org/1342283003/ by vabr@chromium.org.

The reason for reverting is: This broke
http://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/buil...,
will add more info later..

[vabr (Chromium), 2015-09-18 12:54:31]

On 2015/09/18 12:53:07, vabr (Chromium) wrote:
> A revert of this CL (patchset #15 id:540001) has been created in
> https://codereview.chromium.org/1342283003/ by mailto:vabr@chromium.org.
> 
> The reason for reverting is: This broke
>
http://build.chromium.org/p/chromium.mac/builders/iOS_Device_%28ninja%29/buil...,
> will add more info later..

Sorry, missed the revert message. Second revert aborted. :)

[fhorschig, 2015-09-18 13:15:52]

fhorschig@chromium.org changed reviewers:
+ jam@chromium.org

[fhorschig, 2015-09-18 13:15:53]

jam@chromium.org: included you as reviewer in case TBR wasn't enough to notify
you.

[fhorschig, 2015-09-18 16:02:34]

Patchset #17 (id:580001) has been deleted

[fhorschig, 2015-09-18 19:56:50]

The CQ bit was checked by fhorschig@chromium.org

[fhorschig, 2015-09-18 19:56:50]

The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org,
bartfab@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1304843004/#ps620001
(title: "... and removed Landmine. But added last ID.")

[commit-bot: I haz the power, 2015-09-18 19:57:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304843004/620001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304843004/620001

[commit-bot: I haz the power, 2015-09-18 20:04:16]

Committed patchset #18 (id:620001)

[commit-bot: I haz the power, 2015-09-18 20:05:04]

Patchset 18 (id:??) landed as
https://crrev.com/2c4ce2a1988fdde3456e49112eba6c7855ca4528
Cr-Commit-Position: refs/heads/master@{#349749}

[dgrogan, 2015-09-18 21:20:44]

dgrogan@chromium.org changed reviewers:
+ dgrogan@chromium.org

[dgrogan, 2015-09-18 21:20:45]

I think this broke Google Chrome Linux x64:

http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux%...

../../chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1235:23:error:
too few arguments to function call, expected 6, have 5
                  NULL);
                      ^
../../components/policy/core/common/policy_map.h:66:3: note: 'Set' declared here
  void Set(const std::string& policy,
  ^
1 error generated.

[dgrogan, 2015-09-18 21:27:57]

A revert of this CL (patchset #18 id:620001) has been created in
https://codereview.chromium.org/1350913006/ by dgrogan@chromium.org.

The reason for reverting is: Fix google chrome compile.
http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux%...


../../chrome/browser/ui/startup/startup_browser_creator_browsertest.cc:1235:23:error:
too few arguments to function call, expected 6, have 5
                  NULL);
                      ^
../../components/policy/core/common/policy_map.h:66:3: note: 'Set' declared here
  void Set(const std::string& policy,
  ^.

[fhorschig, 2015-09-21 13:51:11]

The CQ bit was checked by fhorschig@chromium.org

[fhorschig, 2015-09-21 13:51:12]

The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org,
bartfab@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1304843004/#ps640001
(title: "Fixed another test.")

[commit-bot: I haz the power, 2015-09-21 13:51:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1304843004/640001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1304843004/640001

[commit-bot: I haz the power, 2015-09-21 14:20:31]

Committed patchset #19 (id:640001)

[commit-bot: I haz the power, 2015-09-21 14:21:12]

Patchset 19 (id:??) landed as
https://crrev.com/64834b71e60e7861a80f68f80d5cde8a56107084
Cr-Commit-Position: refs/heads/master@{#349927}