Fix log spew for "_comment..." policies.

Fix log spew for "_comment..." policies.

Following popular demand revert the bad parts of "Drop non-user policy in
ConfigDirPolicyLoader on Chrome OS." (7cf59c2689f31) and move the filtering
logic to ConfigurationPolicyHandlerList.

BUG=644571
Committed: https://crrev.com/f3cb017aa1b4a7e2239ca2abfc62672508fead70
Cr-Commit-Position: refs/heads/master@{#425995}

[Thiemo Nagel, 2016-10-12 11:08:36]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 11:08:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2016-10-12 11:17:32]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 11:17:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2016-10-12 11:18:44]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-10-12 11:26:51]

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

[commit-bot: I haz the power, 2016-10-12 11:26:52]

Dry run: Exceeded global retry quota

[Thiemo Nagel, 2016-10-12 12:17:02]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 12:17:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Thiemo Nagel, 2016-10-12 12:18:20]

Mattias cc fyi since you were opinionated about the CL that I'm (partially)
reverting now.

[Thiemo Nagel, 2016-10-12 12:19:17]

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

[Thiemo Nagel, 2016-10-12 12:19:19]

Hi Julian,

could you please take a look?

Thank you,
Thiemo

[pastarmovj, 2016-10-12 12:30:56]

https://codereview.chromium.org/2414663002/diff/40001/components/policy/core/...
File components/policy/core/browser/configuration_policy_handler_list.cc
(right):

https://codereview.chromium.org/2414663002/diff/40001/components/policy/core/...
components/policy/core/browser/configuration_policy_handler_list.cc:55: for
(auto handler : handlers_) {
I think it is more efficient the way it used to be. With the if(prefs) outside.
I think prefs can be null only in tests but still test performance is important
for the healthy dev infra of Chrome.

https://codereview.chromium.org/2414663002/diff/40001/components/policy/core/...
File components/policy/core/common/config_dir_policy_loader_unittest.cc (left):

https://codereview.chromium.org/2414663002/diff/40001/components/policy/core/...
components/policy/core/common/config_dir_policy_loader_unittest.cc:24: // Use
real existing policies as samples because ConfigDirPolicyLoader does
Why are those not needed anymore?

[Thiemo Nagel, 2016-10-12 12:46:37]

Thanks a lot!  CIL

https://codereview.chromium.org/2414663002/diff/40001/components/policy/core/...
File components/policy/core/browser/configuration_policy_handler_list.cc
(right):

https://codereview.chromium.org/2414663002/diff/40001/components/policy/core/...
components/policy/core/browser/configuration_policy_handler_list.cc:55: for
(auto handler : handlers_) {
On 2016/10/12 12:30:56, pastarmovj wrote:
> I think it is more efficient the way it used to be. With the if(prefs)
outside.

That's exactly the mistake I made which caused errors to disappear from
chrome://policy.  Apparently, ApplyPolicySettings() is called with prefs=nullptr
when generating the data for chrome://policy.

https://codereview.chromium.org/2414663002/diff/40001/components/policy/core/...
File components/policy/core/common/config_dir_policy_loader_unittest.cc (left):

https://codereview.chromium.org/2414663002/diff/40001/components/policy/core/...
components/policy/core/common/config_dir_policy_loader_unittest.cc:24: // Use
real existing policies as samples because ConfigDirPolicyLoader does
On 2016/10/12 12:30:56, pastarmovj wrote:
> Why are those not needed anymore?

This is part of the revert.  I had to do some refactoring in
https://codereview.chromium.org/1824743002/ so that I could do the filtering in
ConfigDirLoader, but now that the code has moved out of ConfigDirLoader, that
refactoring is obsolete.

[commit-bot: I haz the power, 2016-10-12 12:52:58]

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

[commit-bot: I haz the power, 2016-10-12 12:52:59]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pastarmovj, 2016-10-12 12:59:51]

lgtm

[Thiemo Nagel, 2016-10-18 12:48:33]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 12:48:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 13:22:30]

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

[commit-bot: I haz the power, 2016-10-18 13:22:31]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Thiemo Nagel, 2016-10-18 13:43:38]

The CQ bit was checked by tnagel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 13:43:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 14:44:11]

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

[commit-bot: I haz the power, 2016-10-18 14:44:12]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-10-18 15:14:17]

Hi Julian,

may I ask you to take a look at the delta from PS#3 to PS#4?  I've fixed a
nullptr dereference (blame on me for not running tests before requesting review
:( ) and I've changed the factoring to use EraseMatching(IsPlatformDevicePolicy)
instead of EraseNonmatching(IsNotPlatformDevicePolicy) which seems a lot more
readable to me.

Kind regards,
Thiemo

[pastarmovj, 2016-10-18 16:20:11]

still lgtm.

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

https://codereview.chromium.org/2414663002/diff/80001/components/policy/core/...
components/policy/core/common/policy_map.h:132: void EraseGeneric(const
base::Callback<bool(const const_iterator)>& filter,
nit: I don't like this name much but I can't come up with anything better either
so I will just leave it here as a comment if you can come up with something less
"generic" :)

[Thiemo Nagel, 2016-10-18 16:31:50]

Thanks a lot, Julian!

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

https://codereview.chromium.org/2414663002/diff/80001/components/policy/core/...
components/policy/core/common/policy_map.h:132: void EraseGeneric(const
base::Callback<bool(const const_iterator)>& filter,
I wasn't happy with FilterGeneric() either. Using FilterErase() instead.

[Thiemo Nagel, 2016-10-18 16:32:14]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2016-10-18 16:32:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2414663002/#ps100001
(title: "Rename EraseGeneric() to FilterErase().")

[commit-bot: I haz the power, 2016-10-18 16:32:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-18 17:13:08]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-10-18 17:15:17]

Description was changed from

==========
Fix log spew for "_comment..." policies.

Following popular demand revert the bad parts of "Drop non-user policy in
ConfigDirPolicyLoader on Chrome OS." (7cf59c2689f31) and move the filtering
logic to ConfigurationPolicyHandlerList.

BUG=644571
==========

to

==========
Fix log spew for "_comment..." policies.

Following popular demand revert the bad parts of "Drop non-user policy in
ConfigDirPolicyLoader on Chrome OS." (7cf59c2689f31) and move the filtering
logic to ConfigurationPolicyHandlerList.

BUG=644571

Committed: https://crrev.com/f3cb017aa1b4a7e2239ca2abfc62672508fead70
Cr-Commit-Position: refs/heads/master@{#425995}
==========

[commit-bot: I haz the power, 2016-10-18 17:15:19]

Patchset 5 (id:??) landed as
https://crrev.com/f3cb017aa1b4a7e2239ca2abfc62672508fead70
Cr-Commit-Position: refs/heads/master@{#425995}