Use the Windows MDM API to check if the machine is being managed.

Use the Windows MDM API to check if the machine is being managed.

BUG=690163
Review-Url: https://codereview.chromium.org/2581353002
Cr-Original-Commit-Position: refs/heads/master@{#452028}
Committed: https://chromium.googlesource.com/chromium/src/+/ee0c03ddb4a10c8c9d14244bdefc8034054e846e
Review-Url: https://codereview.chromium.org/2581353002
Cr-Commit-Position: refs/heads/master@{#452484}
Committed: https://chromium.googlesource.com/chromium/src/+/57aed38568117ea2787c4b7aa7e24ce27934c7e8

[Roger Tawa OOO till Jul 10th, 2016-12-16 20:35:41]

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

[commit-bot: I haz the power, 2016-12-16 20:37: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-12-16 20:44:30]

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

[commit-bot: I haz the power, 2016-12-16 20:44:31]

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

[Roger Tawa OOO till Jul 10th, 2016-12-16 21:00:54]

Hi Georges,

Please take a look.  This API is already available in windows10 pro version
10.0.14393 Build 14393 as well as windows10 enterprise version  10.0.10240 Build
12240.

https://codereview.chromium.org/2581353002/diff/20001/components/update_clien...
File components/update_client/updater_state.cc (right):

https://codereview.chromium.org/2581353002/diff/20001/components/update_clien...
components/update_client/updater_state.cc:17: const char
UpdaterState::kIsEnterpriseUser[] = "domainjoined";
I changed the name of the constant but not the value because I believe this is
persisted.  Let me know if you think the value should be changed too.

[Roger Tawa OOO till Jul 10th, 2016-12-16 21:00:57]

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

[commit-bot: I haz the power, 2016-12-16 21:03:39]

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-16 22:56:03]

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

[commit-bot: I haz the power, 2016-12-16 22:56:04]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2017-01-10 15:59:49]

Description was changed from

==========
Use the Windows MDM API to check if the machine is being managed.

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

to

==========
Use the Windows MDM API to check if the machine is being managed.

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

[Roger Tawa OOO till Jul 10th, 2017-01-10 15:59:50]

rogerta@chromium.org changed reviewers:
+ georgesak@chromium.org

[Roger Tawa OOO till Jul 10th, 2017-02-02 21:32:24]

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

[commit-bot: I haz the power, 2017-02-02 21:33:05]

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-02-02 22:49:21]

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

[commit-bot: I haz the power, 2017-02-02 22:49:21]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2017-02-08 21:12:46]

Patchset #7 (id:120001) has been deleted

[Roger Tawa OOO till Jul 10th, 2017-02-08 21:33:30]

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

[commit-bot: I haz the power, 2017-02-08 21:34:28]

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

[Roger Tawa OOO till Jul 10th, 2017-02-08 21:44:09]

Description was changed from

==========
Use the Windows MDM API to check if the machine is being managed.

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

to

==========
Use the Windows MDM API to check if the machine is being managed.

BUG=690163
==========

[Roger Tawa OOO till Jul 10th, 2017-02-08 21:47:53]

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

[Roger Tawa OOO till Jul 10th, 2017-02-08 21:47:54]

Hi Georges,

Please take a look.  I added UMA logging as we discussed today.  A few other
things to watch out for:

1/ You said you don't necessarily like the name IsEnterpriseUser().  Let me know
what you prefer.

2/ We discussed possibly collecting information about the new api before
actually enabling in chrome.  In that case, I would suggested changing the
implementation of IsEnterpriseUser() to only check for AD domain.  We can add
the || case in a follow up CL.

3/ Please answer question I left for you in patchset 2.

Thanks.

[grt (UTC plus 2), 2017-02-08 21:59:54]

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

[grt (UTC plus 2), 2017-02-08 21:59:56]

base/win drive-by

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc#n...
base/win/win_util.cc:504: static IsDeviceRegisteredWithManagementFunction fn =
nullptr;
please initialize this in a lambda sorta like is_user32_and_gdi32_available
below so that the LoadLibrary is only attempted the first time through. as it is
now, a failure to load/resolve on the first call will attempt the load on every
subsequent call.

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc#n...
base/win/win_util.cc:509: if (!fn) {
nit: omit braces for one-liners like this

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc#n...
base/win/win_util.cc:519: bool IsEnterpriseUser() {
this is checking machine-level rather than user account-level state, no? if this
is the case, i think "user" is a misleading term.

[commit-bot: I haz the power, 2017-02-08 23:10:47]

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

[commit-bot: I haz the power, 2017-02-08 23:10:48]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2017-02-09 14:55:59]

Thanks Greg.  See question below.

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc#n...
base/win/win_util.cc:504: static IsDeviceRegisteredWithManagementFunction fn =
nullptr;
On 2017/02/08 21:59:55, grt (UTC plus 1) wrote:
> please initialize this in a lambda sorta like is_user32_and_gdi32_available
> below so that the LoadLibrary is only attempted the first time through. as it
is
> now, a failure to load/resolve on the first call will attempt the load on
every
> subsequent call.

Done.  I was debating whether to do this or not, but agree the state is unlikely
to change during a run of chrome.

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc#n...
base/win/win_util.cc:506: HMODULE mdm_dll =
::LoadLibrary(L"MDMRegistration.dll");
Should I call FreeLibrary() after calling through fn?  Not sure if its better to
keep this dll loaded or not.

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc#n...
base/win/win_util.cc:509: if (!fn) {
On 2017/02/08 21:59:55, grt (UTC plus 1) wrote:
> nit: omit braces for one-liners like this

Done.

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc#n...
base/win/win_util.cc:519: bool IsEnterpriseUser() {
On 2017/02/08 21:59:55, grt (UTC plus 1) wrote:
> this is checking machine-level rather than user account-level state, no? if
this
> is the case, i think "user" is a misleading term.

I'll wait for Georges' suggestion on the name.  However we use "user" in many
other places, both in code and comments.

[Georges Khalil, 2017-02-09 14:59:57]

Just minor comments.

I think we should not enable the MDM check just yet, until we have sufficient
metrics first. Just AD for now (original behavior).

https://codereview.chromium.org/2581353002/diff/20001/components/update_clien...
File components/update_client/updater_state.cc (right):

https://codereview.chromium.org/2581353002/diff/20001/components/update_clien...
components/update_client/updater_state.cc:17: const char
UpdaterState::kIsEnterpriseUser[] = "domainjoined";
On 2016/12/16 21:00:53, Roger Tawa wrote:
> I changed the name of the constant but not the value because I believe this is
> persisted.  Let me know if you think the value should be changed too.

I think you are right, that it is persisted. But I would double check this with
an owner (sorin@?).

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc#n...
base/win/win_util.cc:519: bool IsEnterpriseUser() {
On 2017/02/08 21:59:55, grt (UTC plus 1) wrote:
> this is checking machine-level rather than user account-level state, no? if
this
> is the case, i think "user" is a misleading term.

I will suggest IsEnterpriseManaged, which is what is used by ChromeOS:

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/policy/browser_p...

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.h
File base/win/win_util.h (right):

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.h#ne...
base/win/win_util.h:161: // The caller of this function must be running as an
elevated process.
No need for elevated permission after all, correct?

[Roger Tawa OOO till Jul 10th, 2017-02-09 16:06:44]

Thanks Georges.  I sent an email to Sorin about string value so let's see.

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.cc#n...
base/win/win_util.cc:519: bool IsEnterpriseUser() {
On 2017/02/09 14:59:57, Georges Khalil wrote:
> On 2017/02/08 21:59:55, grt (UTC plus 1) wrote:
> > this is checking machine-level rather than user account-level state, no? if
> this
> > is the case, i think "user" is a misleading term.
> 
> I will suggest IsEnterpriseManaged, which is what is used by ChromeOS:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/policy/browser_p...

Done.  Note that the UMA stat is called "IsEnterpriseUser" which I can't change.

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.h
File base/win/win_util.h (right):

https://codereview.chromium.org/2581353002/diff/140001/base/win/win_util.h#ne...
base/win/win_util.h:161: // The caller of this function must be running as an
elevated process.
On 2017/02/09 14:59:57, Georges Khalil wrote:
> No need for elevated permission after all, correct?

Good catch! Removed sentence.

[Roger Tawa OOO till Jul 10th, 2017-02-09 16:14:39]

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

[commit-bot: I haz the power, 2017-02-09 16:14:58]

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-02-09 17:35:03]

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

[commit-bot: I haz the power, 2017-02-09 17:35:04]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-02-10 12:12:26]

a smattering of comments

https://codereview.chromium.org/2581353002/diff/220001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/220001/base/win/win_util.cc#n...
base/win/win_util.cc:505: static IsDeviceRegisteredWithManagementFunction fn =
nullptr;
"Names should be descriptive; avoid abbreviation."
(https://google.github.io/styleguide/cppguide.html#General_Naming_Rules)

https://codereview.chromium.org/2581353002/diff/220001/base/win/win_util.cc#n...
base/win/win_util.cc:507: HMODULE mdm_dll =
::LoadLibrary(L"MDMRegistration.dll");
while presence of the library will almost certainly not change during the
lifetime of Chrome, is it possible that the machine's registration with MDM may?

https://codereview.chromium.org/2581353002/diff/220001/base/win/win_util.cc#n...
base/win/win_util.cc:526: // However, for now it is decided to collected some
UMA metrics about
nit: "collect"

https://codereview.chromium.org/2581353002/diff/220001/base/win/win_util.cc#n...
base/win/win_util.cc:527: // IsDeviceRegisteredWithMdm() before changing chrome
behavior.
nit: "Chrome's"

https://codereview.chromium.org/2581353002/diff/220001/components/update_clie...
File components/update_client/updater_state.cc (right):

https://codereview.chromium.org/2581353002/diff/220001/components/update_clie...
components/update_client/updater_state.cc:18: const char
UpdaterState::kIsEnterpriseManaged[] = "domainjoined";
nit: please drop a comment here explaining why the name and value don't match so
that someone like me doesn't come along and change the value 6 months from now.
:-)

https://codereview.chromium.org/2581353002/diff/220001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2581353002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:14290: +    either is a domain or is
managed via MDM. This check is performed once at
nit: "in a domain"?

[Roger Tawa OOO till Jul 10th, 2017-02-13 20:30:06]

Hi Greg, please take another look.  Thanks.

https://codereview.chromium.org/2581353002/diff/220001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/220001/base/win/win_util.cc#n...
base/win/win_util.cc:505: static IsDeviceRegisteredWithManagementFunction fn =
nullptr;
On 2017/02/10 12:12:26, grt (UTC plus 1) wrote:
> "Names should be descriptive; avoid abbreviation."
> (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules)

Done.

https://codereview.chromium.org/2581353002/diff/220001/base/win/win_util.cc#n...
base/win/win_util.cc:507: HMODULE mdm_dll =
::LoadLibrary(L"MDMRegistration.dll");
On 2017/02/10 12:12:26, grt (UTC plus 1) wrote:
> while presence of the library will almost certainly not change during the
> lifetime of Chrome, is it possible that the machine's registration with MDM
may?

Let me double check.  A larger question is whether chrome is capable of handling
that at runtime.  Today chrome assumes it won't change.

https://codereview.chromium.org/2581353002/diff/220001/base/win/win_util.cc#n...
base/win/win_util.cc:526: // However, for now it is decided to collected some
UMA metrics about
On 2017/02/10 12:12:26, grt (UTC plus 1) wrote:
> nit: "collect"

Done.

https://codereview.chromium.org/2581353002/diff/220001/base/win/win_util.cc#n...
base/win/win_util.cc:527: // IsDeviceRegisteredWithMdm() before changing chrome
behavior.
On 2017/02/10 12:12:26, grt (UTC plus 1) wrote:
> nit: "Chrome's"

Done.

https://codereview.chromium.org/2581353002/diff/220001/components/update_clie...
File components/update_client/updater_state.cc (right):

https://codereview.chromium.org/2581353002/diff/220001/components/update_clie...
components/update_client/updater_state.cc:18: const char
UpdaterState::kIsEnterpriseManaged[] = "domainjoined";
On 2017/02/10 12:12:26, grt (UTC plus 1) wrote:
> nit: please drop a comment here explaining why the name and value don't match
so
> that someone like me doesn't come along and change the value 6 months from
now.
> :-)

I'm currently discussing with omaha folks and I may just change it here right
away.  If not I'll add the comment.

https://codereview.chromium.org/2581353002/diff/220001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2581353002/diff/220001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:14290: +    either is a domain or is
managed via MDM. This check is performed once at
On 2017/02/10 12:12:26, grt (UTC plus 1) wrote:
> nit: "in a domain"?

Done.

[grt (UTC plus 2), 2017-02-13 21:06:25]

https://codereview.chromium.org/2581353002/diff/240001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/240001/base/win/win_util.cc#n...
base/win/win_util.cc:505: static IsDeviceRegisteredWithManagementFunction fn =
nullptr;
my remark about avoiding abbreviation was in regards to |fn|.

also, please remove "static" here -- that isn't needed since this whole code
block in only evaluated once.

[Roger Tawa OOO till Jul 10th, 2017-02-14 15:19:39]

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

[commit-bot: I haz the power, 2017-02-14 15:19:54]

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-02-14 15:28:59]

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

[commit-bot: I haz the power, 2017-02-14 15:29:00]

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

[Roger Tawa OOO till Jul 10th, 2017-02-14 18:14:39]

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

[commit-bot: I haz the power, 2017-02-14 18:15:04]

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

[Roger Tawa OOO till Jul 10th, 2017-02-14 19:23:44]

Also added comment to explain why value of string string with omaha is not
changing.  Please take another look.

https://codereview.chromium.org/2581353002/diff/240001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/240001/base/win/win_util.cc#n...
base/win/win_util.cc:505: static IsDeviceRegisteredWithManagementFunction fn =
nullptr;
On 2017/02/13 21:06:25, grt (UTC plus 1) wrote:
> my remark about avoiding abbreviation was in regards to |fn|.
> 
> also, please remove "static" here -- that isn't needed since this whole code
> block in only evaluated once.

Done.

[commit-bot: I haz the power, 2017-02-14 19:37:24]

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

[commit-bot: I haz the power, 2017-02-14 19:37:26]

Dry run: This issue passed the CQ dry run.

[grt (UTC plus 2), 2017-02-14 21:20:13]

apologies for missing these before. base/win lgtm with these changes.

https://codereview.chromium.org/2581353002/diff/300001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/300001/base/win/win_util.cc#n...
base/win/win_util.cc:501: static auto is_device_registered_with_management =
[]() {
auto -> bool

https://codereview.chromium.org/2581353002/diff/300001/base/win/win_util.cc#n...
base/win/win_util.cc:507: if (!is_device_registered_with_management_function) {
remove this check and move the definition of
is_device_registered_with_management_function down to its point of assignment on
line 509

https://codereview.chromium.org/2581353002/diff/300001/base/win/win_util.cc#n...
base/win/win_util.cc:508: HMODULE mdm_dll =
::LoadLibrary(L"MDMRegistration.dll");
?
if (!mdm_dll)
  return false;

[Roger Tawa OOO till Jul 10th, 2017-02-15 19:11:05]

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

[Roger Tawa OOO till Jul 10th, 2017-02-15 19:11:27]

Thanks Greg.

https://codereview.chromium.org/2581353002/diff/300001/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/300001/base/win/win_util.cc#n...
base/win/win_util.cc:501: static auto is_device_registered_with_management =
[]() {
On 2017/02/14 21:20:13, grt (UTC plus 1) wrote:
> auto -> bool

Done.

https://codereview.chromium.org/2581353002/diff/300001/base/win/win_util.cc#n...
base/win/win_util.cc:507: if (!is_device_registered_with_management_function) {
On 2017/02/14 21:20:13, grt (UTC plus 1) wrote:
> remove this check and move the definition of
> is_device_registered_with_management_function down to its point of assignment
on
> line 509

Doh!  Good catch, not sure how that line got there.

https://codereview.chromium.org/2581353002/diff/300001/base/win/win_util.cc#n...
base/win/win_util.cc:508: HMODULE mdm_dll =
::LoadLibrary(L"MDMRegistration.dll");
On 2017/02/14 21:20:13, grt (UTC plus 1) wrote:
> ?
> if (!mdm_dll)
>   return false;

Done.

[commit-bot: I haz the power, 2017-02-15 19:12:29]

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

[Georges Khalil, 2017-02-15 19:14:40]

LGTM as well.

[Roger Tawa OOO till Jul 10th, 2017-02-15 20:36:39]

rogerta@chromium.org changed reviewers:
+ asvitkine@chromium.org, sky@chromium.org, waffles@chromium.org

[Roger Tawa OOO till Jul 10th, 2017-02-15 20:36:40]

Hi Alexei, Julian, Joshua, Scott,

Can you please do owner reviews?  I basically changed the name of a helper
function so I had to update in several places.  However, this CL introduces no
new behaviour (except for logging a new UMA).

Alexei:
tools/metrics/histograms/*
(logged in components/policy/core/common/policy_loader_win.cc)

Julian:
components/policy/core/common/policy_loader_win.cc

Joshua:
components/update_client/*

Scott:
chrome/browser/*

Thanks.

[waffles, 2017-02-15 20:44:16]

component_updater lgtm

[commit-bot: I haz the power, 2017-02-15 21:14:31]

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

[commit-bot: I haz the power, 2017-02-15 21:14:32]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2017-02-15 21:16:28]

lgtm for histograms.xml

[sky, 2017-02-15 21:27:15]

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:314: // Record whether the machine is
domain joined in a crash key. This will be
Update comment to say enterprise managed?

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:317: crash_keys::kEnrolledToDomain,
Should this key be renamed to match?

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/compone...
File chrome/browser/component_updater/chrome_component_updater_configurator.cc
(right):

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/compone...
chrome/browser/component_updater/chrome_component_updater_configurator.cc:156: :
std::string("");
std::string("") -> std::string()

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/compone...
chrome/browser/component_updater/chrome_component_updater_configurator.cc:158:
return std::string("");
Same comment here.

[pastarmovj, 2017-02-17 16:21:17]

policy lgtm.

https://codereview.chromium.org/2581353002/diff/320001/components/policy/core...
File components/policy/core/common/policy_loader_win.cc (right):

https://codereview.chromium.org/2581353002/diff/320001/components/policy/core...
components/policy/core/common/policy_loader_win.cc:93: bool
ShouldHonorPolicies() {
nit: I presume the indirection here is to be able to swap out what
ShouldHonorPolicies mean? Can you please document that in a comment above this
function.

[Roger Tawa OOO till Jul 10th, 2017-02-17 18:32:43]

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

[commit-bot: I haz the power, 2017-02-17 18:33:40]

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

[Roger Tawa OOO till Jul 10th, 2017-02-17 19:20:13]

Thanks Julian, Scott.  Comments addressed, please take a look.

Scott: currently the enterprise constant is declared in two places.  I chatted
with Ananta and this cannot be refactored until scottmg@ lands some changes.  So
should be addresses then.

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:314: // Record whether the machine is
domain joined in a crash key. This will be
On 2017/02/15 21:27:15, sky wrote:
> Update comment to say enterprise managed?

Done.

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main_win.cc:317: crash_keys::kEnrolledToDomain,
On 2017/02/15 21:27:15, sky wrote:
> Should this key be renamed to match?

Done.  Chatted with crash folks and they are good with this.  (It's even already
implemented :-) they will handle both old and new name).

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/compone...
File chrome/browser/component_updater/chrome_component_updater_configurator.cc
(right):

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/compone...
chrome/browser/component_updater/chrome_component_updater_configurator.cc:156: :
std::string("");
On 2017/02/15 21:27:15, sky wrote:
> std::string("") -> std::string()

Done.

https://codereview.chromium.org/2581353002/diff/320001/chrome/browser/compone...
chrome/browser/component_updater/chrome_component_updater_configurator.cc:158:
return std::string("");
On 2017/02/15 21:27:15, sky wrote:
> Same comment here.

Done.

https://codereview.chromium.org/2581353002/diff/320001/components/policy/core...
File components/policy/core/common/policy_loader_win.cc (right):

https://codereview.chromium.org/2581353002/diff/320001/components/policy/core...
components/policy/core/common/policy_loader_win.cc:93: bool
ShouldHonorPolicies() {
On 2017/02/17 16:21:17, pastarmovj wrote:
> nit: I presume the indirection here is to be able to swap out what
> ShouldHonorPolicies mean? Can you please document that in a comment above this
> function.

Done.

[commit-bot: I haz the power, 2017-02-17 20:41:14]

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

[commit-bot: I haz the power, 2017-02-17 20:41:16]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2017-02-17 20:53:57]

LGTM

[Roger Tawa OOO till Jul 10th, 2017-02-17 21:01:29]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2017-02-17 21:01:30]

The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org,
waffles@chromium.org, georgesak@chromium.org, asvitkine@chromium.org,
pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2581353002/#ps360001
(title: "rebased")

[commit-bot: I haz the power, 2017-02-17 21:02:45]

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-02-17 23:06:52]

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

[commit-bot: I haz the power, 2017-02-17 23:06:54]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Roger Tawa OOO till Jul 10th, 2017-02-18 17:16:57]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2017-02-18 17:17:08]

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-02-18 21:01:52]

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

[commit-bot: I haz the power, 2017-02-18 21:01:54]

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

[Roger Tawa OOO till Jul 10th, 2017-02-21 16:06:00]

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

[commit-bot: I haz the power, 2017-02-21 16:06:29]

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-02-21 16:24:34]

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

[commit-bot: I haz the power, 2017-02-21 16:24:35]

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

[Roger Tawa OOO till Jul 10th, 2017-02-21 18:32:41]

rogerta@chromium.org changed reviewers:
+ avi@chromium.org

[Roger Tawa OOO till Jul 10th, 2017-02-21 18:32:42]

Hi Avi,

Could you please do an owner review for content/renderer/render_process_impl.cc?
 Turns out this string is hard coded here, and scottmg@ is looking into
refactoring so that this is not necessary.  But for the moment it is.

Thanks.

[Will Harris, 2017-02-21 18:34:50]

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

[Will Harris, 2017-02-21 18:34:52]

https://codereview.chromium.org/2581353002/diff/380001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2581353002/diff/380001/content/renderer/rende...
content/renderer/render_process_impl.cc:142: // Record whether the machine is
domain joined in a crash key. This will be
drive by: seems this comment needs updating?

[Roger Tawa OOO till Jul 10th, 2017-02-21 19:03:32]

https://codereview.chromium.org/2581353002/diff/380001/content/renderer/rende...
File content/renderer/render_process_impl.cc (right):

https://codereview.chromium.org/2581353002/diff/380001/content/renderer/rende...
content/renderer/render_process_impl.cc:142: // Record whether the machine is
domain joined in a crash key. This will be
On 2017/02/21 18:34:51, Will Harris wrote:
> drive by: seems this comment needs updating?

Good catch, done.  I also fixed long line below.

[Roger Tawa OOO till Jul 10th, 2017-02-21 19:39:47]

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

[commit-bot: I haz the power, 2017-02-21 19:41:01]

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

[Avi (use Gerrit), 2017-02-21 20:32:34]

render process impl lgtm

[commit-bot: I haz the power, 2017-02-21 21:43:23]

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

[commit-bot: I haz the power, 2017-02-21 21:43:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Roger Tawa OOO till Jul 10th, 2017-02-21 22:50:21]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2017-02-21 22:50:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, sky@chromium.org, georgesak@chromium.org,
asvitkine@chromium.org, pastarmovj@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2581353002/#ps400001
(title: "Fix long line and comment")

[commit-bot: I haz the power, 2017-02-21 22:51:59]

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-02-22 00:54:41]

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

[commit-bot: I haz the power, 2017-02-22 00:54:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[Roger Tawa OOO till Jul 10th, 2017-02-22 12:58:53]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2017-02-22 12:59:14]

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-02-22 14:02:09]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1487768333044230,
"parent_rev": "ec69818af043485daccd4d4f43718390e9ddcb08", "commit_rev":
"ee0c03ddb4a10c8c9d14244bdefc8034054e846e"}

[commit-bot: I haz the power, 2017-02-22 14:03:42]

Description was changed from

==========
Use the Windows MDM API to check if the machine is being managed.

BUG=690163
==========

to

==========
Use the Windows MDM API to check if the machine is being managed.

BUG=690163

Review-Url: https://codereview.chromium.org/2581353002
Cr-Commit-Position: refs/heads/master@{#452028}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee0c03ddb4a10c8c9d14244bdefc...
==========

[commit-bot: I haz the power, 2017-02-22 14:03:44]

Committed patchset #20 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/ee0c03ddb4a10c8c9d14244bdefc...

[fdoray, 2017-02-22 14:55:00]

Revert: https://codereview.chromium.org/2710713004/

Didn't use the "Revert Patchset" button because:

Revert failed: File too large.
This sometimes happens when a file's contents are not uploaded because it is too
large. Please revert manually.
Sorry for the inconvenience.

[Roger Tawa OOO till Jul 10th, 2017-02-22 15:53:58]

Description was changed from

==========
Use the Windows MDM API to check if the machine is being managed.

BUG=690163

Review-Url: https://codereview.chromium.org/2581353002
Cr-Commit-Position: refs/heads/master@{#452028}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee0c03ddb4a10c8c9d14244bdefc...
==========

to

==========
Use the Windows MDM API to check if the machine is being managed.

BUG=690163

Review-Url: https://codereview.chromium.org/2581353002
Cr-Commit-Position: refs/heads/master@{#452028}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee0c03ddb4a10c8c9d14244bdefc...
==========

[Roger Tawa OOO till Jul 10th, 2017-02-22 18:10:23]

Patchset #21 (id:420001) has been deleted

[Roger Tawa OOO till Jul 10th, 2017-02-22 20:07:51]

On 2017/02/22 14:55:00, fdoray wrote:
> Revert: https://codereview.chromium.org/2710713004/
> 
> Didn't use the "Revert Patchset" button because:
> 
> Revert failed: File too large.
> This sometimes happens when a file's contents are not uploaded because it is
too
> large. Please revert manually.
> Sorry for the inconvenience.

This CL was reverted due to a build break on official builders, which is not
caught by the CQ.  The problem was a typo: I forgot the letter k in a constant
name.  See patchset #22 for details.  I verified with a local official cros
build on my machine that this fixes the compile.  I will commit this again.

[Roger Tawa OOO till Jul 10th, 2017-02-22 20:08:42]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2017-02-22 20:08:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, avi@chromium.org, sky@chromium.org,
georgesak@chromium.org, asvitkine@chromium.org, grt@chromium.org,
pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2581353002/#ps460001
(title: "Fix typo in official build")

[commit-bot: I haz the power, 2017-02-22 20:09:45]

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-02-22 23:36:11]

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

[commit-bot: I haz the power, 2017-02-22 23:36:13]

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

[Roger Tawa OOO till Jul 10th, 2017-02-23 13:09:21]

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

[commit-bot: I haz the power, 2017-02-23 13:09:30]

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-02-23 14:04:21]

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

[commit-bot: I haz the power, 2017-02-23 14:04:23]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2017-02-23 14:46:14]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2017-02-23 14:46:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
waffles@chromium.org, avi@chromium.org, sky@chromium.org,
georgesak@chromium.org, asvitkine@chromium.org, grt@chromium.org,
pastarmovj@chromium.org
Link to the patchset: https://codereview.chromium.org/2581353002/#ps420002
(title: "rebased")

[commit-bot: I haz the power, 2017-02-23 14:46:32]

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-02-23 14:50:51]

CQ is committing da patch.

Bot data: {"patchset_id": 420002, "attempt_start_ts": 1487861174122400,
"parent_rev": "bef2a02e166ad014ddab90d3be30bb94c755b8cf", "commit_rev":
"57aed38568117ea2787c4b7aa7e24ce27934c7e8"}

[commit-bot: I haz the power, 2017-02-23 14:51:30]

Description was changed from

==========
Use the Windows MDM API to check if the machine is being managed.

BUG=690163

Review-Url: https://codereview.chromium.org/2581353002
Cr-Commit-Position: refs/heads/master@{#452028}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee0c03ddb4a10c8c9d14244bdefc...
==========

to

==========
Use the Windows MDM API to check if the machine is being managed.

BUG=690163

Review-Url: https://codereview.chromium.org/2581353002
Cr-Original-Commit-Position: refs/heads/master@{#452028}
Committed:
https://chromium.googlesource.com/chromium/src/+/ee0c03ddb4a10c8c9d14244bdefc...
Review-Url: https://codereview.chromium.org/2581353002
Cr-Commit-Position: refs/heads/master@{#452484}
Committed:
https://chromium.googlesource.com/chromium/src/+/57aed38568117ea2787c4b7aa7e2...
==========

[commit-bot: I haz the power, 2017-02-23 14:51:32]

Committed patchset #24 (id:420002) as
https://chromium.googlesource.com/chromium/src/+/57aed38568117ea2787c4b7aa7e2...

[S. Ganesh, 2017-03-01 03:04:00]

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

[S. Ganesh, 2017-03-01 03:04:01]

https://codereview.chromium.org/2581353002/diff/420002/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/420002/base/win/win_util.cc#n...
base/win/win_util.cc:502: HMODULE mdm_dll =
::LoadLibrary(L"MDMRegistration.dll");
I noticed here that the DLL is not being freed. Intentional?

[Roger Tawa OOO till Jul 10th, 2017-03-01 13:29:12]

On 2017/03/01 03:04:01, S. Ganesh wrote:
> https://codereview.chromium.org/2581353002/diff/420002/base/win/win_util.cc
> File base/win/win_util.cc (right):
> 
>
https://codereview.chromium.org/2581353002/diff/420002/base/win/win_util.cc#n...
> base/win/win_util.cc:502: HMODULE mdm_dll =
> ::LoadLibrary(L"MDMRegistration.dll");
> I noticed here that the DLL is not being freed. Intentional?

I had asked the same question myself, see comment #26 above.  Nobody commented,
so I left it as is.  Do you think I should free it right away?

[S. Ganesh, 2017-03-01 17:21:22]

On 2017/03/01 13:29:12, Roger Tawa wrote:
> On 2017/03/01 03:04:01, S. Ganesh wrote:
> > https://codereview.chromium.org/2581353002/diff/420002/base/win/win_util.cc
> > File base/win/win_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/2581353002/diff/420002/base/win/win_util.cc#n...
> > base/win/win_util.cc:502: HMODULE mdm_dll =
> > ::LoadLibrary(L"MDMRegistration.dll");
> > I noticed here that the DLL is not being freed. Intentional?
> 
> I had asked the same question myself, see comment #26 above.  Nobody
commented,
> so I left it as is.  Do you think I should free it right away?

Ah ok. Yes, freeing would be a good idea.

[Roger Tawa OOO till Jul 10th, 2017-03-01 17:52:25]

On 2017/03/01 17:21:22, S. Ganesh wrote:
> On 2017/03/01 13:29:12, Roger Tawa wrote:
> > On 2017/03/01 03:04:01, S. Ganesh wrote:
> > >
https://codereview.chromium.org/2581353002/diff/420002/base/win/win_util.cc
> > > File base/win/win_util.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2581353002/diff/420002/base/win/win_util.cc#n...
> > > base/win/win_util.cc:502: HMODULE mdm_dll =
> > > ::LoadLibrary(L"MDMRegistration.dll");
> > > I noticed here that the DLL is not being freed. Intentional?
> > 
> > I had asked the same question myself, see comment #26 above.  Nobody
> commented,
> > so I left it as is.  Do you think I should free it right away?
> 
> Ah ok. Yes, freeing would be a good idea.

OK.  I'll write up a CL for that.  Thanks.

[Will Harris, 2017-03-01 18:01:09]

https://codereview.chromium.org/2581353002/diff/420002/base/win/win_util.cc
File base/win/win_util.cc (right):

https://codereview.chromium.org/2581353002/diff/420002/base/win/win_util.cc#n...
base/win/win_util.cc:502: HMODULE mdm_dll =
::LoadLibrary(L"MDMRegistration.dll");
On 2017/03/01 03:04:01, S. Ganesh wrote:
> I noticed here that the DLL is not being freed. Intentional?

perhaps just use base::ScopedNativeLibrary here