Add DeviceADPolicyManager to provide AD policy.

Add DeviceADPolicyManager to provide AD policy.

For Active Directory-managed devices, in BrowserPolicyConnectorChromeOS
replace the DeviceCloudPolicyManagerChromeOS ConfigurationPolicyProvider
by the new DeviceADPolicyManager ConfigurationPolicyProvider and disable
DeviceLocalAccountPolicyService and ServerBackedStateKeysBroker.  Note
that accessors to the disabled services may return nullptr now.

Policy refresh is not yet wired through to authpolicyd, instead it
triggers another load from session_manager.

BUG=656558
TBR=bartfab (enterprise_platform_keys_private_api.cc)
TBR=achuith (chrome/browser/ui/webui/options/chromeos)

Committed: https://crrev.com/8d2791ecec5187ab2188b123ce50710d14f9b9e4
Cr-Commit-Position: refs/heads/master@{#433171}

[Thiemo Nagel, 2016-11-08 16:34:43]

Patchset #1 (id:1) has been deleted

[Thiemo Nagel, 2016-11-09 14:27:46]

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

[commit-bot: I haz the power, 2016-11-09 14:28:02]

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-11-09 14:34:56]

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

[commit-bot: I haz the power, 2016-11-09 14:34:57]

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

[Thiemo Nagel, 2016-11-09 14:49:42]

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

[commit-bot: I haz the power, 2016-11-09 14:49:59]

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-11-09 15:03:54]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-09 15:04:20]

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

[commit-bot: I haz the power, 2016-11-09 15:04:21]

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-11-09 15:07:10]

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

[commit-bot: I haz the power, 2016-11-09 15:07:21]

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-11-09 15:44:03]

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

[commit-bot: I haz the power, 2016-11-09 15:44: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-11-09 21:08:37]

Description was changed from

==========
WORK IN PROGRESS: Add DeviceADPolicyManager.

Policy refresh does not yet go to authpolicyd, instead it triggers another load
from session_manager.  The code contains insane amounts of logging, cleanup
required before landing.

BUG=
==========

to

==========
WORK IN PROGRESS: Add DeviceADPolicyManager.

Policy refresh does not yet go to authpolicyd, instead it triggers another load
from session_manager.  The code contains insane amounts of logging, cleanup
required before landing.

For AD-managed devices, in BrowserPolicyConnectorChromeOS replace the
DeviceCloudPolicyManagerChromeOS ConfigurationPolicyProvider by the new
DeviceADPolicyManager ConfigurationPolicyProvider and disable
DeviceLocalAccountPolicyService and ServerBackedStateKeysBroker.  Note that
accessors to the disabled services may return nullptr now.

BUG=
==========

[Thiemo Nagel, 2016-11-09 21:19:54]

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

[commit-bot: I haz the power, 2016-11-09 21:20:32]

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-11-09 23:16:52]

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

[commit-bot: I haz the power, 2016-11-09 23:16:53]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-11-10 16:35:30]

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

[Thiemo Nagel, 2016-11-10 16:35:41]

Patchset #1 (id:40001) has been deleted

[Thiemo Nagel, 2016-11-10 16:35:52]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-11-10 16:35: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, 2016-11-10 16:36:03]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-11-10 16:57:27]

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

[commit-bot: I haz the power, 2016-11-10 16:57:29]

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

[Thiemo Nagel, 2016-11-10 17:39:24]

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

[commit-bot: I haz the power, 2016-11-10 17:40:01]

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-11-10 18:33:45]

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

[commit-bot: I haz the power, 2016-11-10 18:33:46]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2016-11-10 18:58:43]

Patchset #1 (id:100001) has been deleted

[Thiemo Nagel, 2016-11-10 18:58:52]

Patchset #1 (id:120001) has been deleted

[Thiemo Nagel, 2016-11-10 19:15:07]

Description was changed from

==========
WORK IN PROGRESS: Add DeviceADPolicyManager.

Policy refresh does not yet go to authpolicyd, instead it triggers another load
from session_manager.  The code contains insane amounts of logging, cleanup
required before landing.

For AD-managed devices, in BrowserPolicyConnectorChromeOS replace the
DeviceCloudPolicyManagerChromeOS ConfigurationPolicyProvider by the new
DeviceADPolicyManager ConfigurationPolicyProvider and disable
DeviceLocalAccountPolicyService and ServerBackedStateKeysBroker.  Note that
accessors to the disabled services may return nullptr now.

BUG=
==========

to

==========
Add DeviceADPolicyManager to provide AD policy.

For Active Directory-managed devices, in BrowserPolicyConnectorChromeOS
replace the DeviceCloudPolicyManagerChromeOS ConfigurationPolicyProvider
by the new DeviceADPolicyManager ConfigurationPolicyProvider and disable
DeviceLocalAccountPolicyService and ServerBackedStateKeysBroker.  Note
that accessors to the disabled services may return nullptr now.

Policy refresh is not yet wired through to authpolicyd, instead it
triggers another load from session_manager.

BUG=656558
==========

[Thiemo Nagel, 2016-11-10 19:27:44]

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

[Thiemo Nagel, 2016-11-10 19:27:45]

Hi Maksim, following your comments on my user policy plumbing CL, I've tried to
design the device policy flow in a better way.

Could you maybe take a look?
Thank you,
Thiemo

[emaxx, 2016-11-11 15:25:09]

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc
(right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:117:
AddPolicyProvider(std::unique_ptr<ConfigurationPolicyProvider>(
nit: Use base::WrapUnique?

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:100: // May
be nullptr, eg. for AD-managed devices.
nit: I think either "eg" or "e.g." should be used. But maybe I'm wrong.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_ad_policy_manager.cc (right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.cc:47:
DCHECK_EQ(store_.get(), cloud_policy_store);
nit: #include "base/logging.h"

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.cc:63:
std::unique_ptr<PolicyBundle> bundle = base::MakeUnique<PolicyBundle>();
nit: #include "components/policy/core/common/policy_bundle.h"

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.cc:65:
bundle->Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()));
nit: #include <string>

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.cc:69: // TODO(tnagel):
Rename CloudPolicyStore to PolicyStore and make the source
Just a point from my perspective: there are other (non-cloud) policy providers,
data from which is not stored in CloudPolicyStore. So there's some value in
keeping this "cloud" word in CloudPolicyStore. And, maybe, having this thing in
mind, the DeviceADPolicyManager also should have the word "cloud" in it?
Not sure though.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.cc:71:
policy_map.SetSourceForAll(POLICY_SOURCE_ACTIVE_DIRECTORY);
nit: #include "components/policy/core/common/policy_types.h"

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_ad_policy_manager.h (right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.h:19: class
DeviceADPolicyManager : public ConfigurationPolicyProvider,
I understand that unabbreviated "AD" will be quite long, but I think that this
acronym will be unclear to most of the Chromium committers.
Also quoting the style guide:
> Do not use abbreviations that are ambiguous or unfamiliar
> to readers outside your project, and do not abbreviate by
> deleting letters within a word.


The same concern applies to other parts of the CL that use this "ad" acronym.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_service.cc (left):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:253: if
(new_key.get()) {
Are these checks for new_key.get() and new_key->is_loaded() not applicable
anymore?

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:84:
device_mode_(policy::DEVICE_MODE_PENDING),
nit: Move initializers to the class definition?

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:120: device_mode_ =
device_mode;
Can we add some DCHECK/CHECK regarding allowed device mode transitions?
For instance, that the device mode can only change when it was
DEVICE_MODE_PENDING.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:230: force_key_load
= false;
That's a bit unexpected that the "force" value is actually not respected in some
cases.
I'd suggest either to move this condition to the calling place, or to rename the
method argument.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:338: for (auto&
callback : callbacks) {
const auto& should be probably enough.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.h:81: // Indicates
device ownership status changes.  Not called for the transition
Is this special behavior for the AD case really necessary?
Can't there be a client code that would really want to observe status changes in
the AD case too?

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/session_manager_operation_unittest.cc
(right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/session_manager_operation_unittest.cc:110:
false,  // force_key_load
Not insisting, but I saw the following style of commenting more often:
some_value /* argument_name */,

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

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:310: class
DeviceADPolicyStatusProvider : public PolicyStatusProvider {
Add a short comment for this class?

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:313:
~DeviceADPolicyStatusProvider() override = default;
Just curious: is this due to some compiler complaint?

[Thiemo Nagel, 2016-11-15 17:29:29]

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

[commit-bot: I haz the power, 2016-11-15 17:30:28]

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-11-15 17:34:12]

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

[commit-bot: I haz the power, 2016-11-15 17:34:12]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Thiemo Nagel, 2016-11-15 17:40:00]

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

[commit-bot: I haz the power, 2016-11-15 17:40:59]

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-11-15 18:48:06]

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

[commit-bot: I haz the power, 2016-11-15 18:48:07]

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

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

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

[commit-bot: I haz the power, 2016-11-16 10:16:02]

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-11-16 10:19:02]

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

[commit-bot: I haz the power, 2016-11-16 10:19:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Thiemo Nagel, 2016-11-16 10:41:01]

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

[commit-bot: I haz the power, 2016-11-16 10:41:37]

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-11-16 10:59:08]

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

[commit-bot: I haz the power, 2016-11-16 10:59:09]

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

[Thiemo Nagel, 2016-11-16 11:02:28]

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

[commit-bot: I haz the power, 2016-11-16 11:02:59]

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-11-16 12:25:03]

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

[commit-bot: I haz the power, 2016-11-16 12:25:03]

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-11-16 16:25:51]

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

[commit-bot: I haz the power, 2016-11-16 16:26:39]

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-11-16 18:43:04]

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

[commit-bot: I haz the power, 2016-11-16 18:44:03]

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-11-16 19:01:40]

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

[commit-bot: I haz the power, 2016-11-16 19:04:03]

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-11-16 19:11:01]

Thank you Maksim, I've tried to address all of your comments.  Could you please
take another look?

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc
(right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:117:
AddPolicyProvider(std::unique_ptr<ConfigurationPolicyProvider>(
On 2016/11/11 15:25:08, emaxx wrote:
> nit: Use base::WrapUnique?

Done.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:100: // May
be nullptr, eg. for AD-managed devices.
On 2016/11/11 15:25:08, emaxx wrote:
> nit: I think either "eg" or "e.g." should be used. But maybe I'm wrong.

Good point, I wasn't aware of that!  I guess I'm guilty of writing "eg." in many
many places...

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_ad_policy_manager.cc (right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.cc:47:
DCHECK_EQ(store_.get(), cloud_policy_store);
On 2016/11/11 15:25:08, emaxx wrote:
> nit: #include "base/logging.h"

Done.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.cc:63:
std::unique_ptr<PolicyBundle> bundle = base::MakeUnique<PolicyBundle>();
On 2016/11/11 15:25:08, emaxx wrote:
> nit: #include "components/policy/core/common/policy_bundle.h"

Done.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.cc:65:
bundle->Get(PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()));
On 2016/11/11 15:25:08, emaxx wrote:
> nit: #include <string>

Done.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.cc:69: // TODO(tnagel):
Rename CloudPolicyStore to PolicyStore and make the source
On 2016/11/11 15:25:08, emaxx wrote:
> Just a point from my perspective: there are other (non-cloud) policy
providers,
> data from which is not stored in CloudPolicyStore. So there's some value in
> keeping this "cloud" word in CloudPolicyStore. And, maybe, having this thing
in
> mind, the DeviceADPolicyManager also should have the word "cloud" in it?
> Not sure though.

AD is not cloud, very often it's on-premise servers.  Thus I don't think it
would be correct to call it cloud.  And since the "CloudPolicyStore" is now used
by something that's non-cloud, I guess we should drop the "Cloud" from the name.
 (Also makes it shorter.)

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.cc:71:
policy_map.SetSourceForAll(POLICY_SOURCE_ACTIVE_DIRECTORY);
On 2016/11/11 15:25:08, emaxx wrote:
> nit: #include "components/policy/core/common/policy_types.h"

Done.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/device_ad_policy_manager.h (right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/device_ad_policy_manager.h:19: class
DeviceADPolicyManager : public ConfigurationPolicyProvider,
On 2016/11/11 15:25:08, emaxx wrote:
> I understand that unabbreviated "AD" will be quite long, but I think that this
> acronym will be unclear to most of the Chromium committers.
> Also quoting the style guide:
> > Do not use abbreviations that are ambiguous or unfamiliar
> > to readers outside your project, and do not abbreviate by
> > deleting letters within a word.
> 
> 
> The same concern applies to other parts of the CL that use this "ad" acronym.

I guess you're right.  I'm converting it all, please let me know if I've
overlooked it somewhere.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_service.cc (left):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:253: if
(new_key.get()) {
On 2016/11/11 15:25:08, emaxx wrote:
> Are these checks for new_key.get() and new_key->is_loaded() not applicable
> anymore?

RunPendingOwnershipStatusCallbacks() below calls GetOwnershipStatus() which does
the same check on public_key_ which has the same value as new_key.

But I've re-added the NOTREACHED(), it's now a DCHECK().

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:84:
device_mode_(policy::DEVICE_MODE_PENDING),
On 2016/11/11 15:25:09, emaxx wrote:
> nit: Move initializers to the class definition?

Done.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:120: device_mode_ =
device_mode;
On 2016/11/11 15:25:09, emaxx wrote:
> Can we add some DCHECK/CHECK regarding allowed device mode transitions?
> For instance, that the device mode can only change when it was
> DEVICE_MODE_PENDING.

Sounds like a good idea.  Done.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:230: force_key_load
= false;
I've renamed to "request_key_load" since there are multiple call sites and I
wouldn't want to have the condition at all of them.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:338: for (auto&
callback : callbacks) {
On 2016/11/11 15:25:09, emaxx wrote:
> const auto& should be probably enough.

Done.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_service.h (right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.h:81: // Indicates
device ownership status changes.  Not called for the transition
I agree that this isn't pretty.  Fixed.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/session_manager_operation_unittest.cc
(right):

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/session_manager_operation_unittest.cc:110:
false,  // force_key_load
On 2016/11/11 15:25:09, emaxx wrote:
> Not insisting, but I saw the following style of commenting more often:
> some_value /* argument_name */,

Done.

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

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:310: class
DeviceADPolicyStatusProvider : public PolicyStatusProvider {
On 2016/11/11 15:25:09, emaxx wrote:
> Add a short comment for this class?

Done.

https://codereview.chromium.org/2486813002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:313:
~DeviceADPolicyStatusProvider() override = default;
On 2016/11/11 15:25:09, emaxx wrote:
> Just curious: is this due to some compiler complaint?

No.  It's probably bad form because it creates an inline destructor.  Fixed.

[Thiemo Nagel, 2016-11-16 20:40:44]

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

[Thiemo Nagel, 2016-11-16 20:40:45]

Hi alemate,

could you please take a look at chrome/browser/chromeos/login/*?

Thank you,
Thiemo

[Thiemo Nagel, 2016-11-16 20:43:26]

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

[Thiemo Nagel, 2016-11-16 20:43:27]

Hi Dan,

could you please take a look at chrome/browser/ui/webui/*?

Kind regards,
Thiemo

[commit-bot: I haz the power, 2016-11-16 21:16:59]

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

[commit-bot: I haz the power, 2016-11-16 21:17:00]

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_...)

[emaxx, 2016-11-17 01:14:32]

LGTM

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:260:
DCHECK((device_mode_ == policy::DEVICE_MODE_ENTERPRISE_AD) !=
nit: Use DCHECK_NE or DCHECK_EQ? This will print the values of both
sub-expressions, which will help for debugging.

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/session_manager_operation.cc:181:
validator->ValidateAgainstCurrentPolicy(
There's some contradiction between the flag variable name (verify_signature_)
and what is actually done here. It seems that the flag triggers the validation
of signature, DM token, device ID.
Maybe something like "verify_credentials_and_signature_" will be more precise?
(Though I agree that's a lengthy name. Maybe you'll have a better idea.)

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

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/policy/...
chrome/browser/policy/profile_policy_connector_factory.cc:117: // TODO(tnagel):
Do we need to do something for Active Directory management?
This piece was solely for supporting cloud policy for login screen apps.
So, unless there are potential plans for supporting login screen apps managed by
Active Directory, this TODO may be omitted.

[Alexander Alekseev, 2016-11-17 11:21:37]

chrome/browser/chromeos/login/* lgtm

[Alexander Alekseev, 2016-11-17 11:24:19]

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:226: if
(!device_local_account_policy_service) {
Just a question:

Are you are modifying the lifecycle of DeviceLocalAccountPolicyService?
I.e. previously we would crash here.

[Thiemo Nagel, 2016-11-17 12:47:11]

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

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

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-11-17 13:51:47]

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

[commit-bot: I haz the power, 2016-11-17 13:52:00]

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-11-17 14:15:32]

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

[commit-bot: I haz the power, 2016-11-17 14:15: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-11-17 14:19:07]

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right):

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:226: if
(!device_local_account_policy_service) {
On 2016/11/17 11:24:19, Alexander Alekseev wrote:
> Just a question:
> 
> Are you are modifying the lifecycle of DeviceLocalAccountPolicyService?
> I.e. previously we would crash here.

Yes, Chromad management currently does not support device local accounts and
thus the service isn't started.

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/device_settings_service.cc:260:
DCHECK((device_mode_ == policy::DEVICE_MODE_ENTERPRISE_AD) !=
On 2016/11/17 01:14:32, emaxx wrote:
> nit: Use DCHECK_NE or DCHECK_EQ? This will print the values of both
> sub-expressions, which will help for debugging.

Good point.  Done.

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/2486813002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/settings/session_manager_operation.cc:181:
validator->ValidateAgainstCurrentPolicy(
On 2016/11/17 01:14:32, emaxx wrote:
> There's some contradiction between the flag variable name (verify_signature_)
> and what is actually done here. It seems that the flag triggers the validation
> of signature, DM token, device ID.
> Maybe something like "verify_credentials_and_signature_" will be more precise?
> (Though I agree that's a lengthy name. Maybe you'll have a better idea.)

Done.

[commit-bot: I haz the power, 2016-11-17 15:23:41]

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

[commit-bot: I haz the power, 2016-11-17 15:23:43]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-11-17 18:35:31]

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

[Bernhard Bauer, 2016-11-17 18:35:33]

https://codereview.chromium.org/2486813002/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc
(right):

https://codereview.chromium.org/2486813002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:139:
EXPECT_NE(nullptr, user);
Is something else going to depend on |user| not being null? If so, ASSERT might
be better so you won't crash. And then you might want to wrap the call to
methods that use ASSERT in ASSERT_NO_FATAL_FAILURE to propagate any failures.

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

https://codereview.chromium.org/2486813002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:443: // TODO(tnagel, 664747):
Provide more details and/or remove unused fields from
Nit: If you are going to have an LDAP in there, just use the regular TODO(ldap),
as that is the most common (and grepped for), then reference the bug (ideally
with a full URL for clickability) in the comment.

https://codereview.chromium.org/2486813002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:547:
user_status_provider_.reset(new PolicyStatusProvider());
Nit: If you're here already, you could replace this with base::MakeUnique<>().

https://codereview.chromium.org/2486813002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:780: if (manager) {
This check wasn't there before. Can that really happen? If so, at least document
under which circumstances?

[Thiemo Nagel, 2016-11-17 18:50:48]

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

[Thiemo Nagel, 2016-11-17 18:51:26]

Thanks a lot, Bernhard, I've addressed your comments.  Could you maybe take
another look?

https://codereview.chromium.org/2486813002/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc
(right):

https://codereview.chromium.org/2486813002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/shared_options_browsertest.cc:139:
EXPECT_NE(nullptr, user);
On 2016/11/17 18:35:33, Bernhard Bauer wrote:
> Is something else going to depend on |user| not being null? If so, ASSERT
might
> be better so you won't crash. And then you might want to wrap the call to
> methods that use ASSERT in ASSERT_NO_FATAL_FAILURE to propagate any failures.

If the user is nullptr, there's a crash in line 149, so I guess you're right and
ASSERT is the better choice here.  Thanks!

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

https://codereview.chromium.org/2486813002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:443: // TODO(tnagel, 664747):
Provide more details and/or remove unused fields from
On 2016/11/17 18:35:33, Bernhard Bauer wrote:
> Nit: If you are going to have an LDAP in there, just use the regular
TODO(ldap),
> as that is the most common (and grepped for), then reference the bug (ideally
> with a full URL for clickability) in the comment.

Done.

https://codereview.chromium.org/2486813002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:547:
user_status_provider_.reset(new PolicyStatusProvider());
On 2016/11/17 18:35:33, Bernhard Bauer wrote:
> Nit: If you're here already, you could replace this with base::MakeUnique<>().

Replaced it everwhere.

https://codereview.chromium.org/2486813002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:780: if (manager) {
On 2016/11/17 18:35:33, Bernhard Bauer wrote:
> This check wasn't there before. Can that really happen? If so, at least
document
> under which circumstances?

Done.

[commit-bot: I haz the power, 2016-11-17 18:51:31]

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

[Bernhard Bauer, 2016-11-17 18:52:51]

LGTM!

[Thiemo Nagel, 2016-11-17 19:01:11]

Description was changed from

==========
Add DeviceADPolicyManager to provide AD policy.

For Active Directory-managed devices, in BrowserPolicyConnectorChromeOS
replace the DeviceCloudPolicyManagerChromeOS ConfigurationPolicyProvider
by the new DeviceADPolicyManager ConfigurationPolicyProvider and disable
DeviceLocalAccountPolicyService and ServerBackedStateKeysBroker.  Note
that accessors to the disabled services may return nullptr now.

Policy refresh is not yet wired through to authpolicyd, instead it
triggers another load from session_manager.

BUG=656558
==========

to

==========
Add DeviceADPolicyManager to provide AD policy.

For Active Directory-managed devices, in BrowserPolicyConnectorChromeOS
replace the DeviceCloudPolicyManagerChromeOS ConfigurationPolicyProvider
by the new DeviceADPolicyManager ConfigurationPolicyProvider and disable
DeviceLocalAccountPolicyService and ServerBackedStateKeysBroker.  Note
that accessors to the disabled services may return nullptr now.

Policy refresh is not yet wired through to authpolicyd, instead it
triggers another load from session_manager.

BUG=656558
TBR=bartfab (enterprise_platform_keys_private_api.cc)
TBR=achuith (chrome/browser/ui/webui/options/chromeos)
==========

[Thiemo Nagel, 2016-11-17 19:01:29]

The CQ bit was unchecked by tnagel@chromium.org

[Thiemo Nagel, 2016-11-17 19:01:42]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2016-11-17 19:01:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
alemate@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2486813002/#ps380001
(title: "Address Bernhard's comments")

[commit-bot: I haz the power, 2016-11-17 19:03:48]

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

[Dan Beam, 2016-11-17 19:09:10]

https://codereview.chromium.org/2486813002/diff/380001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc
(right):

https://codereview.chromium.org/2486813002/diff/380001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc:66:
content::NotificationService::current()->Notify(
the notification service is deprecated (as is code in options/).  why are we
adding more uses of it?

[commit-bot: I haz the power, 2016-11-17 20:31:01]

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

[commit-bot: I haz the power, 2016-11-17 20:31:03]

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

[Thiemo Nagel, 2016-11-18 09:46:51]

PicasaDataProviderNoDatabaseGetListTest.NoDatabaseGetList looks like a flake to
me.

https://codereview.chromium.org/2486813002/diff/380001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc
(right):

https://codereview.chromium.org/2486813002/diff/380001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/accounts_options_browsertest.cc:66:
content::NotificationService::current()->Notify(
On 2016/11/17 19:09:10, Dan Beam wrote:
> the notification service is deprecated (as is code in options/).  why are we
> adding more uses of it?

As discussed offline: The CL fixes some useless firings of that very same
notification (in DeviceSettingsService::NotifyOwnershipStatusChanged()), but
apparently the tests had depended on just that to work.  To fix the tests, I've
re-added the notification there.  I'll look whether I can find a better approach
later.

[Thiemo Nagel, 2016-11-18 09:47:00]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2016-11-18 09:47:24]

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-11-18 10:46:07]

Description was changed from

==========
Add DeviceADPolicyManager to provide AD policy.

For Active Directory-managed devices, in BrowserPolicyConnectorChromeOS
replace the DeviceCloudPolicyManagerChromeOS ConfigurationPolicyProvider
by the new DeviceADPolicyManager ConfigurationPolicyProvider and disable
DeviceLocalAccountPolicyService and ServerBackedStateKeysBroker.  Note
that accessors to the disabled services may return nullptr now.

Policy refresh is not yet wired through to authpolicyd, instead it
triggers another load from session_manager.

BUG=656558
TBR=bartfab (enterprise_platform_keys_private_api.cc)
TBR=achuith (chrome/browser/ui/webui/options/chromeos)
==========

to

==========
Add DeviceADPolicyManager to provide AD policy.

For Active Directory-managed devices, in BrowserPolicyConnectorChromeOS
replace the DeviceCloudPolicyManagerChromeOS ConfigurationPolicyProvider
by the new DeviceADPolicyManager ConfigurationPolicyProvider and disable
DeviceLocalAccountPolicyService and ServerBackedStateKeysBroker.  Note
that accessors to the disabled services may return nullptr now.

Policy refresh is not yet wired through to authpolicyd, instead it
triggers another load from session_manager.

BUG=656558
TBR=bartfab (enterprise_platform_keys_private_api.cc)
TBR=achuith (chrome/browser/ui/webui/options/chromeos)
==========

[commit-bot: I haz the power, 2016-11-18 10:46:09]

Committed patchset #13 (id:380001)

[commit-bot: I haz the power, 2016-11-18 10:49:21]

Description was changed from

==========
Add DeviceADPolicyManager to provide AD policy.

For Active Directory-managed devices, in BrowserPolicyConnectorChromeOS
replace the DeviceCloudPolicyManagerChromeOS ConfigurationPolicyProvider
by the new DeviceADPolicyManager ConfigurationPolicyProvider and disable
DeviceLocalAccountPolicyService and ServerBackedStateKeysBroker.  Note
that accessors to the disabled services may return nullptr now.

Policy refresh is not yet wired through to authpolicyd, instead it
triggers another load from session_manager.

BUG=656558
TBR=bartfab (enterprise_platform_keys_private_api.cc)
TBR=achuith (chrome/browser/ui/webui/options/chromeos)
==========

to

==========
Add DeviceADPolicyManager to provide AD policy.

For Active Directory-managed devices, in BrowserPolicyConnectorChromeOS
replace the DeviceCloudPolicyManagerChromeOS ConfigurationPolicyProvider
by the new DeviceADPolicyManager ConfigurationPolicyProvider and disable
DeviceLocalAccountPolicyService and ServerBackedStateKeysBroker.  Note
that accessors to the disabled services may return nullptr now.

Policy refresh is not yet wired through to authpolicyd, instead it
triggers another load from session_manager.

BUG=656558
TBR=bartfab (enterprise_platform_keys_private_api.cc)
TBR=achuith (chrome/browser/ui/webui/options/chromeos)

Committed: https://crrev.com/8d2791ecec5187ab2188b123ce50710d14f9b9e4
Cr-Commit-Position: refs/heads/master@{#433171}
==========

[commit-bot: I haz the power, 2016-11-18 10:49:23]

Patchset 13 (id:??) landed as
https://crrev.com/8d2791ecec5187ab2188b123ce50710d14f9b9e4
Cr-Commit-Position: refs/heads/master@{#433171}

[Dan Beam, 2016-11-18 16:45:06]

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

https://codereview.chromium.org/2486813002/diff/380001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:87: {"sourceActiveDirectory",
IDS_POLICY_SOURCE_ACTIVE_DIRECTORY},
alphabetize

https://codereview.chromium.org/2486813002/diff/380001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:322: const
policy::CloudPolicyStore* store_;
DISALLOW

[Thiemo Nagel, 2016-11-21 13:31:22]

Thank you Dan!  I've created a follow-up CL here:
https://codereview.chromium.org/2519023002

Achuith, in case there's anything you'd like me to change in
chrome/browser/ui/webui/options/chromeos, please comment on the new CL above.

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

https://codereview.chromium.org/2486813002/diff/380001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:87: {"sourceActiveDirectory",
IDS_POLICY_SOURCE_ACTIVE_DIRECTORY},
On 2016/11/18 16:45:05, Dan Beam wrote:
> alphabetize

Done.

https://codereview.chromium.org/2486813002/diff/380001/chrome/browser/ui/webu...
chrome/browser/ui/webui/policy_ui_handler.cc:322: const
policy::CloudPolicyStore* store_;
On 2016/11/18 16:45:05, Dan Beam wrote:
> DISALLOW

Done.