Mop up incorrect uses of IsEnterpriseManaged()

Mop up incorrect uses of IsEnterpriseManaged()

Adding Active Directory support has created ambiguity as to whether
IsEnterpriseManaged() means "cloud management" or just "any kind of
management" which used to be conflated in the past.  This CL aims to
eliminate the remaining incorrect calls.

BUG=639295
Committed: https://crrev.com/9faf37792aadb357b5895b318f69a1a84a62bbe9
Cr-Commit-Position: refs/heads/master@{#434509}

[Thiemo Nagel, 2016-11-24 16:44:27]

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

[commit-bot: I haz the power, 2016-11-24 16:44:45]

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-24 17:12:43]

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

[Thiemo Nagel, 2016-11-24 17:12:44]

Roman, could you please take a look?

Thank you,
Thiemo

[Thiemo Nagel, 2016-11-24 17:18:41]

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

[Thiemo Nagel, 2016-11-24 17:18:42]

Hi Achuith,

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

Thank you,
Thiemo

[commit-bot: I haz the power, 2016-11-24 17:22:13]

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

[commit-bot: I haz the power, 2016-11-24 17:22:14]

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

[Roman Sorokin (ftl), 2016-11-25 09:40:17]

https://codereview.chromium.org/2534433002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right):

https://codereview.chromium.org/2534433002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:74: bool
IsEnterpriseManaged() const;
What's up with that method? Should we delete it? Or rewrite comment?

[Thiemo Nagel, 2016-11-25 09:58:10]

PTAL

https://codereview.chromium.org/2534433002/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right):

https://codereview.chromium.org/2534433002/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:74: bool
IsEnterpriseManaged() const;
On 2016/11/25 09:40:16, Roman Sorokin wrote:
> What's up with that method? Should we delete it? Or rewrite comment?

We need to keep it, there's many legit uses of it.  Still I've synchronized the
comments with those in InstallAttributes for maximal clarity.

[achuithb, 2016-11-25 10:10:48]

lgtm for login/. I'm not sure the new comments are helpful?

https://codereview.chromium.org/2534433002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2534433002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:149:
// Re-enrollment not (yet?) supported for Active Directory.
Is this intended to be a TODO? If so, we should add a bug. Otherwise, is it
useful?

https://codereview.chromium.org/2534433002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/2534433002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:77: // Active
Directory would require a different treatment, but it doesn't
I think this comment may be more confusing than clarifying?

[Thiemo Nagel, 2016-11-25 14:26:10]

Many thanks, Achuith!

Roman, could you please take a look at everything except
chrome/browser/chromeos/login?

Thank you,
Thiemo

https://codereview.chromium.org/2534433002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
(right):

https://codereview.chromium.org/2534433002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc:149:
// Re-enrollment not (yet?) supported for Active Directory.
On 2016/11/25 10:10:48, achuithb wrote:
> Is this intended to be a TODO? If so, we should add a bug. Otherwise, is it
> useful?

Good point.  It's not intended as a TODO.  I've changed the text to "not
implemented" to clarify that re-enrollment for AD is not disabled for
fundamental reasons but only due to low priority. ;)

https://codereview.chromium.org/2534433002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_selection_screen.cc (right):

https://codereview.chromium.org/2534433002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_selection_screen.cc:77: // Active
Directory would require a different treatment, but it doesn't
On 2016/11/25 10:10:48, achuithb wrote:
> I think this comment may be more confusing than clarifying?

OK.  I've removed that comment.

[Roman Sorokin (ftl), 2016-11-25 14:30:12]

lgtm with nit

https://codereview.chromium.org/2534433002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2534433002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:151: //
Enrollment recovery is not (yet?) supported for Active Directory.
nit: Also change to "not implemented"

[Thiemo Nagel, 2016-11-25 15:06:30]

https://codereview.chromium.org/2534433002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2534433002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:151: //
Enrollment recovery is not (yet?) supported for Active Directory.
On 2016/11/25 14:30:12, Roman Sorokin wrote:
> nit: Also change to "not implemented"

Thanks.  Done.

[Thiemo Nagel, 2016-11-25 15:06:36]

The CQ bit was checked by tnagel@chromium.org

[Thiemo Nagel, 2016-11-25 15:06:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
achuith@chromium.org, rsorokin@chromium.org
Link to the patchset: https://codereview.chromium.org/2534433002/#ps60001
(title: "Address Roman's comment")

[commit-bot: I haz the power, 2016-11-25 15:06:48]

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-25 15:36:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480086396842530,
"parent_rev": "f9011641f9d968d1bd7a1ee8d5e258d8ff83fb8e", "commit_rev":
"13519c92477e9e194ccf4b8952f71bbfa4e06d1e"}

[commit-bot: I haz the power, 2016-11-25 15:37:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-25 15:39:13]

Description was changed from

==========
Mop up incorrect uses of IsEnterpriseManaged()

Adding Active Directory support has created ambiguity as to whether
IsEnterpriseManaged() means "cloud management" or just "any kind of
management" which used to be conflated in the past.  This CL aims to
eliminate the remaining incorrect calls.

BUG=639295
==========

to

==========
Mop up incorrect uses of IsEnterpriseManaged()

Adding Active Directory support has created ambiguity as to whether
IsEnterpriseManaged() means "cloud management" or just "any kind of
management" which used to be conflated in the past.  This CL aims to
eliminate the remaining incorrect calls.

BUG=639295

Committed: https://crrev.com/9faf37792aadb357b5895b318f69a1a84a62bbe9
Cr-Commit-Position: refs/heads/master@{#434509}
==========

[commit-bot: I haz the power, 2016-11-25 15:39:17]

Patchset 4 (id:??) landed as
https://crrev.com/9faf37792aadb357b5895b318f69a1a84a62bbe9
Cr-Commit-Position: refs/heads/master@{#434509}