Setup Chromad user policy plumbing

Setup Chromad user policy plumbing

Add UserActiveDirectoryPolicyManager configuration policy provider as a
browser context keyed service for Active Directory user policy.  To
avoid code duplication, the corresponding factory is shared with
UserCloudPolicyManager.  (Only one of the two managers can exist at the
same time.)  To clarify the factory's new role, the "Cloud" is dropped
from its name ("UserCloudPolicyManagerFactoryChromeOS").

As user and device Active Directory policy managers are very similar,
they probably should be merged in a future CL.

BUG=655971
TBR=sky (function rename in managed_bookmark_service_factory.cc)
TBR=achuith (a few renames in chrome/browser/chromeos/login)
TBR=anthonyvd (renames, type generalization in chrome/browser/profiles)
TBR=dbeam (renames propagated to policy_ui_handler.cc)

Committed: https://crrev.com/57bb531048630ec894f1c90676ed05457939359e
Cr-Commit-Position: refs/heads/master@{#440967}

[Thiemo Nagel, 2016-12-28 09:34:40]

Patchset #4 (id:60001) has been deleted

[Thiemo Nagel, 2016-12-28 16:19:21]

Patchset #7 (id:140001) has been deleted

[Thiemo Nagel, 2016-12-28 16:19:28]

Patchset #7 (id:160001) has been deleted

[Thiemo Nagel, 2016-12-28 16:20:18]

Patchset #4 (id:80001) has been deleted

[Thiemo Nagel, 2016-12-28 16:20:25]

Patchset #1 (id:1) has been deleted

[Thiemo Nagel, 2016-12-28 16:20:33]

Patchset #1 (id:20001) has been deleted

[Thiemo Nagel, 2016-12-28 16:28:37]

Description was changed from

==========
Setup Chromad user policy plumbing

WORK IN PROGRESS -- DO NOT REVIEW

BUG=655971
==========

to

==========
Setup Chromad user policy plumbing

Add UserActiveDirectoryPolicyManager configuration policy provider as a
browser context keyed service for Active Directory user policy.  To
avoid code duplication, the corresponding factory is shared with
UserCloudPolicyManager.  (Only one of the two managers can exist at the
same time.)

As user and device Active Directory policy managers are very similar,
they probably should be merged in a future CL.

BUG=655971
==========

[Thiemo Nagel, 2016-12-28 17:14:29]

Patchset #5 (id:200001) has been deleted

[Thiemo Nagel, 2016-12-28 17:14:42]

Patchset #5 (id:220001) has been deleted

[Thiemo Nagel, 2016-12-28 17:15:02]

Patchset #5 (id:240001) has been deleted

[Thiemo Nagel, 2016-12-28 17:15:12]

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

[commit-bot: I haz the power, 2016-12-28 17:15:16]

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-12-28 17:17:50]

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

[Thiemo Nagel, 2016-12-28 17:17:51]

Hi Maksim,

may I ask you to take a quick look?

Thank you,
Thiemo

[Thiemo Nagel, 2016-12-28 17:18:39]

Patchset #2 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-12-28 17:47:25]

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

[commit-bot: I haz the power, 2016-12-28 17:47:25]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Thiemo Nagel, 2016-12-28 18:04:35]

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

[commit-bot: I haz the power, 2016-12-28 18:04:45]

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

[emaxx, 2016-12-28 19:09:20]

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:124:
auto const it = cloud_managers_.find(profile->GetOriginalProfile());
nit: The existing code in this file uses the style with "const" being put first.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:172:
bool is_active_directory;
nit: I think usually in Chrome source code it's preferred to initialize all
variables immediately. The false value would be suitable here.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:175:
return std::unique_ptr<ConfigurationPolicyProvider>();
nit: Not insisting, but C++11 allows to shorten this to:
  return {};

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:176:
break;
nit: May be removed.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:236:
new UserCloudPolicyStoreChromeOS(
nit: Use base::MakeUnique here too while you're here?

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:295:
DCHECK(cloud_managers_.find(profile) == cloud_managers_.end());
You could move these two DCHECKS out of the condition to rid of duplicating
them.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h
(right):

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h:47:
// TODO(tnagel): Rename to make clear that the factory also provides AD
Don't you want to do the renaming straight away?

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h:56:
// |profile|. Depending on the type of management either a
nit: s/management either/management, either/

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h:57:
// UserCloudPolicyManagerChromeOS or a UserActiveDirectoryPolicyManager or a
nit: s/a nullptr/nullptr/

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:235:
EnsurePolicyKeyLoaded(
Before the refactoring is done to separate out the independent blocks in this
class, maybe add more DCHECK(!is_active_directory_) - into EnsurePolicyKeyLoaded
and related?

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right):

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:36: class
UserCloudPolicyStoreChromeOS : public UserCloudPolicyStoreBase {
nit: Is there a TODO somewhere to rename this class too? If not, then worth
adding.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:46: const
base::FilePath& user_policy_key_dir,
This argument is probably unused for the Active Directory case. Same is for a
bunch of private stuff related to the policy key.

I know it's not the best time currently to suggest a bigger refactoring (like
separating AD and non-AD cases into two different classes, with a separate one
containing the shared functionality).
But if you agree that this interface is more like a temporary solution, then at
least a TODO may be added for now.

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_active_directory_policy_manager.h
(right):

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_active_directory_policy_manager.h:26:
explicit UserActiveDirectoryPolicyManager(
nit: Remove explicit.

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
File chrome/browser/policy/profile_policy_connector.cc (right):

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector.cc:159: return policy_store_ &&
policy_store_->is_managed();
You are changing the behavior here, when special_user_policy_provider_ is not
null. Are you sure this change is safe?

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector.cc:193: return (provider ==
configuration_policy_provider_);
nit: The parentheses here don't seem to be helpful. See:
https://google.github.io/styleguide/cppguide.html#Return_Values

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
File chrome/browser/policy/profile_policy_connector.h (right):

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector.h:39: const CloudPolicyStore*
policy_store);
nit: Document that policy_store may be null? (and maybe explain in which cases)

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector.h:59: std::string
GetDisplayDomainForManagedBookmarks() const;
This method looks alien to this class.
I don't feel that hiding the methods that allow to calculate this for external
clients has improved the interface.

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
File chrome/browser/policy/profile_policy_connector_unittest.cc (right):

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector_unittest.cc:66:
connector.Init(nullptr, &schema_registry_, cloud_policy_manager_.get(),
nit: "nullptr /* user */" ?

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui_handler.cc (right):

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:312: // Provides status for any
Active Directory policy manager.
nit: Add comment to explain explicitly that it's both for device and user
policy? Unless I misunderstood this.

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:559: if (active_directory_policy) {
Could we transform this into a more readable sequence of conditions like
if ...
else if ...
else if ...
?

https://codereview.chromium.org/2606773002/diff/260001/chromeos/dbus/fake_aut...
File chromeos/dbus/fake_auth_policy_client.cc (right):

https://codereview.chromium.org/2606773002/diff/260001/chromeos/dbus/fake_aut...
chromeos/dbus/fake_auth_policy_client.cc:119: // DELETE_ME
Wrong change?

[commit-bot: I haz the power, 2016-12-28 19:41:08]

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

[commit-bot: I haz the power, 2016-12-28 19:41:08]

Dry run: Try jobs failed on following builders:
  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-12-29 11:35:45]

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

[commit-bot: I haz the power, 2016-12-29 11:36:01]

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-12-29 11:56:16]

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

[commit-bot: I haz the power, 2016-12-29 11:56:25]

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-12-29 12:49:04]

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

[commit-bot: I haz the power, 2016-12-29 12:49:05]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-12-29 14:28:31]

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

[commit-bot: I haz the power, 2016-12-29 14:28:36]

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-12-29 15:02:12]

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

[commit-bot: I haz the power, 2016-12-29 15:02:17]

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-12-29 15:05:36]

Patchset #10 (id:360001) has been deleted

[Thiemo Nagel, 2016-12-29 15:06:24]

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

[commit-bot: I haz the power, 2016-12-29 15:06:28]

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-12-29 15:08:28]

Thanks a lot for the fast review!  I've addressed your comments and done some
minor improvements, could you please take another look?

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
File
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:124:
auto const it = cloud_managers_.find(profile->GetOriginalProfile());
On 2016/12/28 19:09:19, emaxx wrote:
> nit: The existing code in this file uses the style with "const" being put
first.

Done.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:172:
bool is_active_directory;
On 2016/12/28 19:09:19, emaxx wrote:
> nit: I think usually in Chrome source code it's preferred to initialize all
> variables immediately. The false value would be suitable here.

Thanks, that seems prudent.  I'd assume that the compiler most probably will
optimize away duplicate assignments anyways.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:175:
return std::unique_ptr<ConfigurationPolicyProvider>();
On 2016/12/28 19:09:19, emaxx wrote:
> nit: Not insisting, but C++11 allows to shorten this to:
>   return {};

Very interesting, thanks.  Done.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:176:
break;
On 2016/12/28 19:09:19, emaxx wrote:
> nit: May be removed.

If you don't mind, I'd suggest to keep it for symmetry (and maybe also to
prevent havoc in case the return statement is ever replaced by something else).

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:236:
new UserCloudPolicyStoreChromeOS(
On 2016/12/28 19:09:19, emaxx wrote:
> nit: Use base::MakeUnique here too while you're here?

Done.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc:295:
DCHECK(cloud_managers_.find(profile) == cloud_managers_.end());
On 2016/12/28 19:09:19, emaxx wrote:
> You could move these two DCHECKS out of the condition to rid of duplicating
> them.

Good point, they look much better at the top of the method.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h
(right):

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h:47:
// TODO(tnagel): Rename to make clear that the factory also provides AD
On 2016/12/28 19:09:19, emaxx wrote:
> Don't you want to do the renaming straight away?

Done.  (As discussed offline.)

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h:56:
// |profile|. Depending on the type of management either a
On 2016/12/28 19:09:19, emaxx wrote:
> nit: s/management either/management, either/

Done.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.h:57:
// UserCloudPolicyManagerChromeOS or a UserActiveDirectoryPolicyManager or a
On 2016/12/28 19:09:19, emaxx wrote:
> nit: s/a nullptr/nullptr/

Done.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:235:
EnsurePolicyKeyLoaded(
On 2016/12/28 19:09:20, emaxx wrote:
> Before the refactoring is done to separate out the independent blocks in this
> class, maybe add more DCHECK(!is_active_directory_) - into
EnsurePolicyKeyLoaded
> and related?

Done.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h (right):

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:36: class
UserCloudPolicyStoreChromeOS : public UserCloudPolicyStoreBase {
On 2016/12/28 19:09:20, emaxx wrote:
> nit: Is there a TODO somewhere to rename this class too? If not, then worth
> adding.

Done.

https://codereview.chromium.org/2606773002/diff/150021/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h:46: const
base::FilePath& user_policy_key_dir,
> I know it's not the best time currently to suggest a bigger refactoring (like
> separating AD and non-AD cases into two different classes, with a separate one
> containing the shared functionality).

I've added this thought to the TODO above.  (I haven't thought deeply about that
yet thus I'm not sure what would be the best approach.)

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_active_directory_policy_manager.h
(right):

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_active_directory_policy_manager.h:26:
explicit UserActiveDirectoryPolicyManager(
On 2016/12/28 19:09:20, emaxx wrote:
> nit: Remove explicit.

Done.

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
File chrome/browser/policy/profile_policy_connector.cc (right):

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector.cc:159: return policy_store_ &&
policy_store_->is_managed();
On 2016/12/28 19:09:20, emaxx wrote:
> You are changing the behavior here, when special_user_policy_provider_ is not
> null. Are you sure this change is safe?

Good point.  Maybe not.  I'm reverting this part.

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector.cc:193: return (provider ==
configuration_policy_provider_);
On 2016/12/28 19:09:20, emaxx wrote:
> nit: The parentheses here don't seem to be helpful. See:
> https://google.github.io/styleguide/cppguide.html#Return_Values

Sorry, that was not intentional.

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
File chrome/browser/policy/profile_policy_connector.h (right):

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector.h:39: const CloudPolicyStore*
policy_store);
On 2016/12/28 19:09:20, emaxx wrote:
> nit: Document that policy_store may be null? (and maybe explain in which
cases)

Done.

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector.h:59: std::string
GetDisplayDomainForManagedBookmarks() const;
On 2016/12/28 19:09:20, emaxx wrote:
> This method looks alien to this class.
> I don't feel that hiding the methods that allow to calculate this for external
> clients has improved the interface.

OK, reverting this part.  (I don't understand the purpose of the
IsProfilePolicy() check anyways.)

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
File chrome/browser/policy/profile_policy_connector_unittest.cc (right):

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector_unittest.cc:66:
connector.Init(nullptr, &schema_registry_, cloud_policy_manager_.get(),
On 2016/12/28 19:09:20, emaxx wrote:
> nit: "nullptr /* user */" ?

Done.

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/policy_ui_handler.cc (right):

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:312: // Provides status for any
Active Directory policy manager.
On 2016/12/28 19:09:20, emaxx wrote:
> nit: Add comment to explain explicitly that it's both for device and user
> policy? Unless I misunderstood this.

Actually it's "either/or" not "both".  I've updated the comment to (hopefully)
make that more clear.  (Not visible here because I've moved the webui changes
out to a dependent CL: https://codereview.chromium.org/2608573002/.)

https://codereview.chromium.org/2606773002/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:559: if (active_directory_policy) {
On 2016/12/28 19:09:20, emaxx wrote:
> Could we transform this into a more readable sequence of conditions like
> if ...
> else if ...
> else if ...
> ?

Done.  (Also moved to separate CL.)

https://codereview.chromium.org/2606773002/diff/260001/chromeos/dbus/fake_aut...
File chromeos/dbus/fake_auth_policy_client.cc (right):

https://codereview.chromium.org/2606773002/diff/260001/chromeos/dbus/fake_aut...
chromeos/dbus/fake_auth_policy_client.cc:119: // DELETE_ME
On 2016/12/28 19:09:20, emaxx wrote:
> Wrong change?

Thanks.  Deleted.  ;)

https://codereview.chromium.org/2606773002/diff/340001/chrome/browser/policy/...
File chrome/browser/policy/profile_policy_connector_factory.cc (right):

https://codereview.chromium.org/2606773002/diff/340001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector_factory.cc:137: } else if
(active_directory_manager) {
I've flattened the code nesting in the same way as you had proposed for webui.

[Thiemo Nagel, 2016-12-29 15:21:27]

Description was changed from

==========
Setup Chromad user policy plumbing

Add UserActiveDirectoryPolicyManager configuration policy provider as a
browser context keyed service for Active Directory user policy.  To
avoid code duplication, the corresponding factory is shared with
UserCloudPolicyManager.  (Only one of the two managers can exist at the
same time.)

As user and device Active Directory policy managers are very similar,
they probably should be merged in a future CL.

BUG=655971
==========

to

==========
Setup Chromad user policy plumbing

Add UserActiveDirectoryPolicyManager configuration policy provider as a
browser context keyed service for Active Directory user policy.  To
avoid code duplication, the corresponding factory is shared with
UserCloudPolicyManager.  (Only one of the two managers can exist at the
same time.)  To clarify the factory's new role, the "Cloud" is dropped
from its name ("UserCloudPolicyManagerFactoryChromeOS").

As user and device Active Directory policy managers are very similar,
they probably should be merged in a future CL.

BUG=655971
TBR=sky (function rename in managed_bookmark_service_factory.cc)
TBR=achuith (a few renames in chrome/browser/chromeos/login)
TBR=anthonyvd (renames, type generalization in chrome/browser/profiles)
TBR=dbeam (renames propagated to policy_ui_handler.cc)
==========

[emaxx, 2016-12-29 15:38:01]

lgtm

https://codereview.chromium.org/2606773002/diff/380001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/2606773002/diff/380001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:368:
validator->ValidateTimestamp(
Is there any point in calling this method with the NOT_VALIDATED flag?
Actually, the same question is for ValidateDMToken and ValidateDeviceId - it
looks like they have no effect with the given parameters.

https://codereview.chromium.org/2606773002/diff/380001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/2606773002/diff/380001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc:271:
return std::move(manager);
nit: It's up to you, but usually std::move is not required in such constructs.
However, it seems that the compiler is not obliged to understand this.

[Thiemo Nagel, 2016-12-29 15:50:42]

https://codereview.chromium.org/2606773002/diff/380001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc (right):

https://codereview.chromium.org/2606773002/diff/380001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.cc:368:
validator->ValidateTimestamp(
On 2016/12/29 15:38:01, emaxx wrote:
> Is there any point in calling this method with the NOT_VALIDATED flag?
> Actually, the same question is for ValidateDMToken and ValidateDeviceId - it
> looks like they have no effect with the given parameters.

The purpose is to overwrite the validation settings that have been configured by
CreateValidator() above.  (This pattern is used in some other places as well, I
haven't invented it.)

https://codereview.chromium.org/2606773002/diff/380001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc
(right):

https://codereview.chromium.org/2606773002/diff/380001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc:271:
return std::move(manager);
On 2016/12/29 15:38:01, emaxx wrote:
> nit: It's up to you, but usually std::move is not required in such constructs.
> However, it seems that the compiler is not obliged to understand this.

Very true.  Without std::move() compilation fails on some of the bots, iirc.

[Thiemo Nagel, 2016-12-29 16:14:19]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2016-12-29 16:14:20]

The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2606773002/#ps420001
(title: "Rebase")

[commit-bot: I haz the power, 2016-12-29 16:14:37]

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-12-29 17:46:10]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1483028059527460,
"parent_rev": "d1026bbd01619d5e59924011ae5a866d89a359aa", "commit_rev":
"055a2c6bc0ca34a54b82883072149814c5e21e53"}

[commit-bot: I haz the power, 2016-12-29 17:46:37]

Description was changed from

==========
Setup Chromad user policy plumbing

Add UserActiveDirectoryPolicyManager configuration policy provider as a
browser context keyed service for Active Directory user policy.  To
avoid code duplication, the corresponding factory is shared with
UserCloudPolicyManager.  (Only one of the two managers can exist at the
same time.)  To clarify the factory's new role, the "Cloud" is dropped
from its name ("UserCloudPolicyManagerFactoryChromeOS").

As user and device Active Directory policy managers are very similar,
they probably should be merged in a future CL.

BUG=655971
TBR=sky (function rename in managed_bookmark_service_factory.cc)
TBR=achuith (a few renames in chrome/browser/chromeos/login)
TBR=anthonyvd (renames, type generalization in chrome/browser/profiles)
TBR=dbeam (renames propagated to policy_ui_handler.cc)
==========

to

==========
Setup Chromad user policy plumbing

Add UserActiveDirectoryPolicyManager configuration policy provider as a
browser context keyed service for Active Directory user policy.  To
avoid code duplication, the corresponding factory is shared with
UserCloudPolicyManager.  (Only one of the two managers can exist at the
same time.)  To clarify the factory's new role, the "Cloud" is dropped
from its name ("UserCloudPolicyManagerFactoryChromeOS").

As user and device Active Directory policy managers are very similar,
they probably should be merged in a future CL.

BUG=655971
TBR=sky (function rename in managed_bookmark_service_factory.cc)
TBR=achuith (a few renames in chrome/browser/chromeos/login)
TBR=anthonyvd (renames, type generalization in chrome/browser/profiles)
TBR=dbeam (renames propagated to policy_ui_handler.cc)

Review-Url: https://codereview.chromium.org/2606773002
==========

[commit-bot: I haz the power, 2016-12-29 17:46:39]

Committed patchset #12 (id:420001)

[commit-bot: I haz the power, 2017-01-02 15:52:35]

Description was changed from

==========
Setup Chromad user policy plumbing

Add UserActiveDirectoryPolicyManager configuration policy provider as a
browser context keyed service for Active Directory user policy.  To
avoid code duplication, the corresponding factory is shared with
UserCloudPolicyManager.  (Only one of the two managers can exist at the
same time.)  To clarify the factory's new role, the "Cloud" is dropped
from its name ("UserCloudPolicyManagerFactoryChromeOS").

As user and device Active Directory policy managers are very similar,
they probably should be merged in a future CL.

BUG=655971
TBR=sky (function rename in managed_bookmark_service_factory.cc)
TBR=achuith (a few renames in chrome/browser/chromeos/login)
TBR=anthonyvd (renames, type generalization in chrome/browser/profiles)
TBR=dbeam (renames propagated to policy_ui_handler.cc)

Review-Url: https://codereview.chromium.org/2606773002
==========

to

==========
Setup Chromad user policy plumbing

Add UserActiveDirectoryPolicyManager configuration policy provider as a
browser context keyed service for Active Directory user policy.  To
avoid code duplication, the corresponding factory is shared with
UserCloudPolicyManager.  (Only one of the two managers can exist at the
same time.)  To clarify the factory's new role, the "Cloud" is dropped
from its name ("UserCloudPolicyManagerFactoryChromeOS").

As user and device Active Directory policy managers are very similar,
they probably should be merged in a future CL.

BUG=655971
TBR=sky (function rename in managed_bookmark_service_factory.cc)
TBR=achuith (a few renames in chrome/browser/chromeos/login)
TBR=anthonyvd (renames, type generalization in chrome/browser/profiles)
TBR=dbeam (renames propagated to policy_ui_handler.cc)

Committed: https://crrev.com/57bb531048630ec894f1c90676ed05457939359e
Cr-Commit-Position: refs/heads/master@{#440967}
==========

[commit-bot: I haz the power, 2017-01-02 15:52:37]

Patchset 12 (id:??) landed as
https://crrev.com/57bb531048630ec894f1c90676ed05457939359e
Cr-Commit-Position: refs/heads/master@{#440967}