Merge {Device,User}ActiveDirectoryPolicyManager into a single class

Merge {Device,User}ActiveDirectoryPolicyManager into a single class

These two classes deviate in only a single flow and thus it seems
sensible to combine them.

BUG=677304
TBR=bauerb (rename carries over to webui)

Review-Url: https://codereview.chromium.org/2638623006
Cr-Commit-Position: refs/heads/master@{#444445}
Committed: https://chromium.googlesource.com/chromium/src/+/1e8e0ffaa542982e1e9c4ebdbdcccb47e6364f3b

[Thiemo Nagel, 2017-01-17 14:53:40]

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

[commit-bot: I haz the power, 2017-01-17 14:53:51]

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, 2017-01-17 15:46:00]

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

[commit-bot: I haz the power, 2017-01-17 15:46:01]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2017-01-17 16:26:50]

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

[commit-bot: I haz the power, 2017-01-17 16:26:57]

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

[Thiemo Nagel, 2017-01-17 16:31:08]

Description was changed from

==========
Merge {Device,User}ActiveDirectoryPolicyManager into a single class

BUG=677304
==========

to

==========
Merge {Device,User}ActiveDirectoryPolicyManager into a single class

These two classes deviate in only a single flow and thus it seems
sensible to combine them.

BUG=677304
==========

[Thiemo Nagel, 2017-01-17 16:31:41]

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

[Thiemo Nagel, 2017-01-17 16:31:42]

Hi Maksim,

may I ask you to take a look?

Thank you,
Thiemo

[Thiemo Nagel, 2017-01-17 16:33:41]

Description was changed from

==========
Merge {Device,User}ActiveDirectoryPolicyManager into a single class

These two classes deviate in only a single flow and thus it seems
sensible to combine them.

BUG=677304
==========

to

==========
Merge {Device,User}ActiveDirectoryPolicyManager into a single class

These two classes deviate in only a single flow and thus it seems
sensible to combine them.

BUG=677304
TBR=bauerb (rename carries over to webui)
==========

[commit-bot: I haz the power, 2017-01-17 17:16:17]

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

[commit-bot: I haz the power, 2017-01-17 17:16:18]

Dry run: This issue passed the CQ dry run.

[emaxx, 2017-01-18 01:19:44]

https://codereview.chromium.org/2638623006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right):

https://codereview.chromium.org/2638623006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_policy_manager.h:27: explicit
ActiveDirectoryPolicyManager(
The intent could be made more explicit by exposing the factory methods (e.g.
CreateForDevicePolicy) instead of the constructors. This would also help to
understand the code that uses them.

[Thiemo Nagel, 2017-01-18 17:53:22]

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

[commit-bot: I haz the power, 2017-01-18 17:53: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, 2017-01-18 18:29:41]

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

[Thiemo Nagel, 2017-01-18 18:29:44]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-01-18 18:29:58]

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

[Thiemo Nagel, 2017-01-18 18:30:31]

Thank you!  Could you please take another look?

https://codereview.chromium.org/2638623006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right):

https://codereview.chromium.org/2638623006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_policy_manager.h:27: explicit
ActiveDirectoryPolicyManager(
On 2017/01/18 01:19:44, emaxx wrote:
> The intent could be made more explicit by exposing the factory methods (e.g.
> CreateForDevicePolicy) instead of the constructors. This would also help to
> understand the code that uses them.

Good idea!  Done.

[emaxx, 2017-01-18 18:35:55]

lgtm with a suggestion

https://codereview.chromium.org/2638623006/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right):

https://codereview.chromium.org/2638623006/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_policy_manager.h:29: static
ActiveDirectoryPolicyManager* CreateForDevicePolicy(
Why not returning through a std::unique_ptr here? This would make the fact of
the ownership passing obvious.

[Thiemo Nagel, 2017-01-18 18:51:58]

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

[commit-bot: I haz the power, 2017-01-18 18:52:40]

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

[Thiemo Nagel, 2017-01-18 18:54:21]

Sending this again in case you'd like to take another look.  (Otherwise I'll
commit tomorrow morning.)

https://codereview.chromium.org/2638623006/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/active_directory_policy_manager.h (right):

https://codereview.chromium.org/2638623006/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_policy_manager.h:29: static
ActiveDirectoryPolicyManager* CreateForDevicePolicy(
On 2017/01/18 18:35:55, emaxx wrote:
> Why not returning through a std::unique_ptr here? This would make the fact of
> the ownership passing obvious.

Thanks!  Done.

[emaxx, 2017-01-18 19:10:00]

LGTM with a nit (sorry for hair-splitting!)

Thanks!

https://codereview.chromium.org/2638623006/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right):

https://codereview.chromium.org/2638623006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_policy_manager.cc:25: return
base::WrapUnique(
nit: base::MakeUnique?

[Thiemo Nagel, 2017-01-18 19:18:27]

On 2017/01/18 19:10:00, emaxx wrote:
> LGTM with a nit (sorry for hair-splitting!)

I mostly always enjoy having my hairs split.  ;)

[Thiemo Nagel, 2017-01-18 19:18:37]

https://codereview.chromium.org/2638623006/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/active_directory_policy_manager.cc (right):

https://codereview.chromium.org/2638623006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_policy_manager.cc:25: return
base::WrapUnique(
On 2017/01/18 19:09:59, emaxx wrote:
> nit: base::MakeUnique?

MakeUnique won't work because the constructor is private.

[Thiemo Nagel, 2017-01-18 19:45:19]

The CQ bit was unchecked by tnagel@chromium.org

[Thiemo Nagel, 2017-01-18 19:45:24]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2017-01-18 19:45:52]

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, 2017-01-18 19:59:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484768724333940,
"parent_rev": "e30959a32e573cc2264bafe4ab57fc93c6a234af", "commit_rev":
"1e8e0ffaa542982e1e9c4ebdbdcccb47e6364f3b"}

[commit-bot: I haz the power, 2017-01-18 20:00:08]

Description was changed from

==========
Merge {Device,User}ActiveDirectoryPolicyManager into a single class

These two classes deviate in only a single flow and thus it seems
sensible to combine them.

BUG=677304
TBR=bauerb (rename carries over to webui)
==========

to

==========
Merge {Device,User}ActiveDirectoryPolicyManager into a single class

These two classes deviate in only a single flow and thus it seems
sensible to combine them.

BUG=677304
TBR=bauerb (rename carries over to webui)

Review-Url: https://codereview.chromium.org/2638623006
Cr-Commit-Position: refs/heads/master@{#444445}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e8e0ffaa542982e1e9c4ebdbdcc...
==========

[commit-bot: I haz the power, 2017-01-18 20:00:10]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/1e8e0ffaa542982e1e9c4ebdbdcc...