Chromad: Use DM server reply to determine enrollment type

Chromad: Use DM server reply to determine enrollment type

Now user have to press custom shortuct to enroll into Active Directory
management. This CL use DM server to determine that.
It also show Active Directory domain join UI just before locking the
device. So user has possibility to switch between internet and intranet.

BUG=668491
TEST=manual

Review-Url: https://codereview.chromium.org/2677563005
Cr-Commit-Position: refs/heads/master@{#450919}
Committed: https://chromium.googlesource.com/chromium/src/+/3c6d07134f1a93f57b6c0d191251deb3d9d9e887

[Roman Sorokin (ftl), 2017-02-03 15:02:42]

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

[Roman Sorokin (ftl), 2017-02-03 15:02:43]

Hey Achuith, Thiemo, could you PTAL? Thanks!

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:435:
->GetEnrollmentScreenActor()
Not sure if it's a good way to start showing AD ui.

[achuithb, 2017-02-07 20:27:17]

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:186: auth_code,
shark_controller_ != NULL /* fetch_additional_token */);
nullptr

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:56: using
OnAdJoinedCallback = base::OnceCallback<void(std::string realm)>;
const std::string&

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:104: //
TODO(rsorokin): Remove that once DM server does not require requisition.
Is there a tracking bug?

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:214: LOG(ERROR) <<
"Bad device mode " << device_mode_;
This error message is unclear. This device mode should not be seen in beta and
stable channel, right?

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:225: LOG(ERROR) <<
"Bad device mode " << device_mode_;
Make this more explicit/clear too.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:435:
->GetEnrollmentScreenActor()
On 2017/02/03 15:02:43, Roman Sorokin wrote:
> Not sure if it's a good way to start showing AD ui.

Pretty sure it's not.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:443: realm_ =
std::move(realm);
Why not realm_ = realm;

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

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:141: // Start
joining to Active Directory domain in case device is enrolling into
Start joining the Active Directory domain in case the device is enrolling into
Active Directory management mode.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:146: void
OnAdDomainJoined(std::string realm);
const std::string&

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:200: DeviceMode
device_mode_;
initialize this

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:203: bool
skip_robot_auth_;
initialize this.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:219: int
lockbox_init_duration_;
initialize this

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:584:
std::move(on_joined_callback_).Run(gaia::ExtractDomainName(user_name));
Is this your way of setting on_joined_callback_ to null? I don't think we should
use std::move here.

[achuithb, 2017-02-07 20:27:39]

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

[commit-bot: I haz the power, 2017-02-07 20:28: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-07 20:55:33]

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

[commit-bot: I haz the power, 2017-02-07 20:55:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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), 2017-02-07 21:41:06]

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:435:
->GetEnrollmentScreenActor()
On 2017/02/07 20:27:16, achuithb wrote:
> On 2017/02/03 15:02:43, Roman Sorokin wrote:
> > Not sure if it's a good way to start showing AD ui.
> 
> Pretty sure it's not.

You got any idea of a proper way to do that? Should we pass the screen actor
inside enrollmenthandler somehow?

[achuithb, 2017-02-07 21:50:51]

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:435:
->GetEnrollmentScreenActor()
On 2017/02/07 21:41:05, Roman Sorokin wrote:
> On 2017/02/07 20:27:16, achuithb wrote:
> > On 2017/02/03 15:02:43, Roman Sorokin wrote:
> > > Not sure if it's a good way to start showing AD ui.
> > 
> > Pretty sure it's not.
> 
> You got any idea of a proper way to do that? Should we pass the screen actor
> inside enrollmenthandler somehow?

Off the top of my head, I don't; I'd have to research this. Could you look
through the code base for perhaps another example of how this is done?

[Roman Sorokin (ftl), 2017-02-10 14:57:10]

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:186: auth_code,
shark_controller_ != NULL /* fetch_additional_token */);
On 2017/02/07 20:27:16, achuithb wrote:
> nullptr

Done.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:56: using
OnAdJoinedCallback = base::OnceCallback<void(std::string realm)>;
On 2017/02/07 20:27:16, achuithb wrote:
> const std::string&

Done.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:104: //
TODO(rsorokin): Remove that once DM server does not require requisition.
On 2017/02/07 20:27:16, achuithb wrote:
> Is there a tracking bug?

Done.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:214: LOG(ERROR) <<
"Bad device mode " << device_mode_;
On 2017/02/07 20:27:16, achuithb wrote:
> This error message is unclear. This device mode should not be seen in beta and
> stable channel, right?

Done.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:225: LOG(ERROR) <<
"Bad device mode " << device_mode_;
On 2017/02/07 20:27:16, achuithb wrote:
> Make this more explicit/clear too.

Done.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:435:
->GetEnrollmentScreenActor()
On 2017/02/07 21:50:50, achuithb wrote:
> On 2017/02/07 21:41:05, Roman Sorokin wrote:
> > On 2017/02/07 20:27:16, achuithb wrote:
> > > On 2017/02/03 15:02:43, Roman Sorokin wrote:
> > > > Not sure if it's a good way to start showing AD ui.
> > > 
> > > Pretty sure it's not.
> > 
> > You got any idea of a proper way to do that? Should we pass the screen actor
> > inside enrollmenthandler somehow?
> 
> Off the top of my head, I don't; I'd have to research this. Could you look
> through the code base for perhaps another example of how this is done?

Created ActiveDirectoryJoinDelegate

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:443: realm_ =
std::move(realm);
On 2017/02/07 20:27:16, achuithb wrote:
> Why not realm_ = realm;

Done.

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

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:141: // Start
joining to Active Directory domain in case device is enrolling into
On 2017/02/07 20:27:17, achuithb wrote:
> Start joining the Active Directory domain in case the device is enrolling into
> Active Directory management mode.

Done.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:146: void
OnAdDomainJoined(std::string realm);
On 2017/02/07 20:27:16, achuithb wrote:
> const std::string&

Done.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:200: DeviceMode
device_mode_;
On 2017/02/07 20:27:17, achuithb wrote:
> initialize this

Done.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:203: bool
skip_robot_auth_;
On 2017/02/07 20:27:17, achuithb wrote:
> initialize this.

Done.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:219: int
lockbox_init_duration_;
On 2017/02/07 20:27:16, achuithb wrote:
> initialize this

Done.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:584:
std::move(on_joined_callback_).Run(gaia::ExtractDomainName(user_name));
On 2017/02/07 20:27:17, achuithb wrote:
> Is this your way of setting on_joined_callback_ to null? I don't think we
should
> use std::move here.

This is the way to consume OnceCallbacks according to 
https://cs.chromium.org/chromium/src/docs/callback.md

[achuithb, 2017-02-13 13:35:13]

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

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

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

[achuithb, 2017-02-13 13:39:03]

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/chromeos/pol...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:435:
->GetEnrollmentScreenActor()
On 2017/02/10 14:57:10, Roman Sorokin wrote:
> On 2017/02/07 21:50:50, achuithb wrote:
> > On 2017/02/07 21:41:05, Roman Sorokin wrote:
> > > On 2017/02/07 20:27:16, achuithb wrote:
> > > > On 2017/02/03 15:02:43, Roman Sorokin wrote:
> > > > > Not sure if it's a good way to start showing AD ui.
> > > > 
> > > > Pretty sure it's not.
> > > 
> > > You got any idea of a proper way to do that? Should we pass the screen
actor
> > > inside enrollmenthandler somehow?
> > 
> > Off the top of my head, I don't; I'd have to research this. Could you look
> > through the code base for perhaps another example of how this is done?
> 
> Created ActiveDirectoryJoinDelegate

Acknowledged.

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/ui/webui/chr...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2677563005/diff/1/chrome/browser/ui/webui/chr...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:584:
std::move(on_joined_callback_).Run(gaia::ExtractDomainName(user_name));
On 2017/02/10 14:57:10, Roman Sorokin wrote:
> On 2017/02/07 20:27:17, achuithb wrote:
> > Is this your way of setting on_joined_callback_ to null? I don't think we
> should
> > use std::move here.
> 
> This is the way to consume OnceCallbacks according to 
> https://cs.chromium.org/chromium/src/docs/callback.md

Acknowledged.

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

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.h:30:
ActiveDirectoryJoinDelegate* join_delegate,
ad_join_delegate here and everywhere else?

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.h:92:
ActiveDirectoryJoinDelegate* join_delegate_;
ad_join_delegate_?

Also, initialize to nullptr for consistency

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/host_pairing_screen.h (right):

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:82: // Implements
ActiveDirectoryJoinDelegate.
Use the same comment form as above for consistency:
// Overridden from ActiveDirectoryJoinDelegate:

Also, move this to right after the EnrollmentStatusConsumer overrides.

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:85: Delegate*
delegate_;
= nullptr

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:87:
HostPairingScreenActor* actor_;
= nullptr

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:90:
pairing_chromeos::HostPairingController* remora_controller_;
= nullptr

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:95: Stage
current_stage_;
= STAGE_NONE

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

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_join_delegate.h:20: void virtual
JoinDomain(OnDomainJoinedCallback on_joined_callback) = 0;
DISALLOW_COPY_AND_ASSIGN

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

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:191:
chromeos::ActiveDirectoryJoinDelegate* join_delegate_;
ad_join_delegate_ = nullptr

[commit-bot: I haz the power, 2017-02-13 13:56:29]

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

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

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

[Roman Sorokin (ftl), 2017-02-14 12:44:26]

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

[commit-bot: I haz the power, 2017-02-14 12:44:47]

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

[Roman Sorokin (ftl), 2017-02-14 12:47:10]

Thanks, Achuith! Please take another look.

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

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.h:30:
ActiveDirectoryJoinDelegate* join_delegate,
On 2017/02/13 13:39:02, achuithb wrote:
> ad_join_delegate here and everywhere else?

Done.

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.h:92:
ActiveDirectoryJoinDelegate* join_delegate_;
On 2017/02/13 13:39:02, achuithb wrote:
> ad_join_delegate_?
> 
> Also, initialize to nullptr for consistency

Done.

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/host_pairing_screen.h (right):

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:82: // Implements
ActiveDirectoryJoinDelegate.
On 2017/02/13 13:39:02, achuithb wrote:
> Use the same comment form as above for consistency:
> // Overridden from ActiveDirectoryJoinDelegate:
> 
> Also, move this to right after the EnrollmentStatusConsumer overrides.

Done.

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:85: Delegate*
delegate_;
On 2017/02/13 13:39:02, achuithb wrote:
> = nullptr

Done.

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:87:
HostPairingScreenActor* actor_;
On 2017/02/13 13:39:02, achuithb wrote:
> = nullptr

Done.

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:90:
pairing_chromeos::HostPairingController* remora_controller_;
On 2017/02/13 13:39:02, achuithb wrote:
> = nullptr

Done.

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:95: Stage
current_stage_;
On 2017/02/13 13:39:02, achuithb wrote:
> = STAGE_NONE

Done.

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

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_join_delegate.h:20: void virtual
JoinDomain(OnDomainJoinedCallback on_joined_callback) = 0;
On 2017/02/13 13:39:03, achuithb wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

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

https://codereview.chromium.org/2677563005/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.h:191:
chromeos::ActiveDirectoryJoinDelegate* join_delegate_;
On 2017/02/13 13:39:03, achuithb wrote:
> ad_join_delegate_ = nullptr

Done.

[commit-bot: I haz the power, 2017-02-14 13:08:10]

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

[commit-bot: I haz the power, 2017-02-14 13:08:10]

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

[achuithb, 2017-02-14 13:11:11]

lgtm

https://codereview.chromium.org/2677563005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/active_directory_join_delegate.h (right):

https://codereview.chromium.org/2677563005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_join_delegate.h:19:
ActiveDirectoryJoinDelegate() {}
Does =Default not work here?

Also, I think we can add a protected dtor?

[Roman Sorokin (ftl), 2017-02-14 13:54:39]

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

[commit-bot: I haz the power, 2017-02-14 13:54:53]

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

[Roman Sorokin (ftl), 2017-02-14 13:55:04]

https://codereview.chromium.org/2677563005/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/active_directory_join_delegate.h (right):

https://codereview.chromium.org/2677563005/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_join_delegate.h:19:
ActiveDirectoryJoinDelegate() {}
On 2017/02/14 13:11:11, achuithb wrote:
> Does =Default not work here?
> 
> Also, I think we can add a protected dtor?

Done.

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

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

[commit-bot: I haz the power, 2017-02-14 14:27:13]

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2017-02-14 14:45:01]

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:203: NOTREACHED();
Why do you inherit the Delegate and then implement it NOTREACHED()?  That seems
questionable design to me.

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/host_pairing_screen.h (right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:78: // Overriden
from ActiveDirectoryJoinDelegate:
Nit: Overridden

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

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_join_delegate.h:17: class
ActiveDirectoryJoinDelegate {
Your ActiveDirectoryJoinDelegate frightens and confuses me.  Please add a class
comment to explain its purpose.

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:256:
// Overriden from ActiveDirectoryJoinDelegate:
Nit: Overridden

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:208: if
(device_mode_ == DEVICE_MODE_ENTERPRISE_AD) {
Nit: That section might be more readable if factored as:

if (device_mode_ == ENTERPRISE) {
  // do nothing
} else if (device_mode_ == ENTERPRISE_AD) {
  // do AD stuff
} else {
  // supplied device mode is not supported
}

Or maybe use a switch statement?

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:583:
controller_->OnAdJoined(gaia::ExtractDomainName(user_name));
The email splitting in Chrome is different from email splitting in authpolicy
(though I don't know whether that difference matters).

In the long run, imho it would be good to make the realm a separate parameter to
the JoinAdDomain() D-Bus call.  Wdyt?

[Roman Sorokin (ftl), 2017-02-14 16:01:33]

Thanks, Thiemo! Please take another look

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:203: NOTREACHED();
On 2017/02/14 14:45:00, Thiemo Nagel (slow) wrote:
> Why do you inherit the Delegate and then implement it NOTREACHED()?  That
seems
> questionable design to me.

Created FakeActiveDirectoryJoinDelegate

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/host_pairing_screen.h (right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.h:78: // Overriden
from ActiveDirectoryJoinDelegate:
On 2017/02/14 14:45:00, Thiemo Nagel (slow) wrote:
> Nit: Overridden

Done.

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

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_join_delegate.h:17: class
ActiveDirectoryJoinDelegate {
On 2017/02/14 14:45:00, Thiemo Nagel (slow) wrote:
> Your ActiveDirectoryJoinDelegate frightens and confuses me.  Please add a
class
> comment to explain its purpose.

Done.

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:256:
// Overriden from ActiveDirectoryJoinDelegate:
On 2017/02/14 14:45:00, Thiemo Nagel (slow) wrote:
> Nit: Overridden

Done.

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:208: if
(device_mode_ == DEVICE_MODE_ENTERPRISE_AD) {
On 2017/02/14 14:45:00, Thiemo Nagel (slow) wrote:
> Nit: That section might be more readable if factored as:
> 
> if (device_mode_ == ENTERPRISE) {
>   // do nothing
> } else if (device_mode_ == ENTERPRISE_AD) {
>   // do AD stuff
> } else {
>   // supplied device mode is not supported
> }
> 
> Or maybe use a switch statement?

Done.

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:583:
controller_->OnAdJoined(gaia::ExtractDomainName(user_name));
On 2017/02/14 14:45:01, Thiemo Nagel (slow) wrote:
> The email splitting in Chrome is different from email splitting in authpolicy
> (though I don't know whether that difference matters).
> 
> In the long run, imho it would be good to make the realm a separate parameter
to
> the JoinAdDomain() D-Bus call.  Wdyt?

You mean output param from JoinAdDomain? Make sense

[Thiemo Nagel, 2017-02-14 16:43:06]

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc
(right):

https://codereview.chromium.org/2677563005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:583:
controller_->OnAdJoined(gaia::ExtractDomainName(user_name));
> You mean output param from JoinAdDomain? Make sense

I think it could be either an input or an output parameter.  Anything that
ensures that authpolicy and Chrome agree on the realm.

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:150: this,
fake_ad_join_delegate_.get(), enrollment_config, std::string());
Maybe I'm missing something, but couldn't you just pass in a nullptr?

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/active_directory_join_delegate.h (right):

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_join_delegate.h:17: // Delegate
being used during Enterprise Enrollment to start Active Directory
Nit: enterprise enrollment

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_join_delegate.h:18: // domain
join flow.
Nit: For cavemen like me, maybe explain the motivation that starting AD requires
access to the actor which is not available inside enrollmenthandler?

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

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:513:
&device_management_service_, fake_ad_join_delegate_.get(),
nullptr?

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

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:206:
SetStep(STEP_POLICY_FETCH);
Nit: It would seem more readable to me to move SetStep() below, above
client_->FetchPolicy();

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:223: // Do
nothing. Go to FetchPolicy.
Nit: I'd suggest to drop the "Go to FetchPolicy" part from the comment because
it could become out of date easily.  Also, I'd suggest to sort the cases by
their numeric value and thus have ENTERPRISE above ENTERPRISE_AD.

[Roman Sorokin (ftl), 2017-02-15 14:01:45]

Thanks, Thiemo! PTAL.

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/host_pairing_screen.cc (right):

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/host_pairing_screen.cc:150: this,
fake_ad_join_delegate_.get(), enrollment_config, std::string());
On 2017/02/14 16:43:06, Thiemo Nagel (slow) wrote:
> Maybe I'm missing something, but couldn't you just pass in a nullptr?

Done.

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/active_directory_join_delegate.h (right):

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_join_delegate.h:17: // Delegate
being used during Enterprise Enrollment to start Active Directory
On 2017/02/14 16:43:06, Thiemo Nagel (slow) wrote:
> Nit: enterprise enrollment

Done.

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/active_directory_join_delegate.h:18: // domain
join flow.
On 2017/02/14 16:43:06, Thiemo Nagel (slow) wrote:
> Nit: For cavemen like me, maybe explain the motivation that starting AD
requires
> access to the actor which is not available inside enrollmenthandler?

Done.

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

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc:513:
&device_management_service_, fake_ad_join_delegate_.get(),
On 2017/02/14 16:43:06, Thiemo Nagel (slow) wrote:
> nullptr?

Done.

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

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:206:
SetStep(STEP_POLICY_FETCH);
On 2017/02/14 16:43:06, Thiemo Nagel (slow) wrote:
> Nit: It would seem more readable to me to move SetStep() below, above
> client_->FetchPolicy();

Done.

https://codereview.chromium.org/2677563005/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:223: // Do
nothing. Go to FetchPolicy.
On 2017/02/14 16:43:06, Thiemo Nagel (slow) wrote:
> Nit: I'd suggest to drop the "Go to FetchPolicy" part from the comment because
> it could become out of date easily.  Also, I'd suggest to sort the cases by
> their numeric value and thus have ENTERPRISE above ENTERPRISE_AD.

Done.

[Roman Sorokin (ftl), 2017-02-15 14:01:56]

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

[commit-bot: I haz the power, 2017-02-15 14:02: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-15 14:31:03]

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

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

Dry run: This issue passed the CQ dry run.

[Thiemo Nagel, 2017-02-15 17:44:43]

Lgtm.

[Roman Sorokin (ftl), 2017-02-16 09:50:02]

The CQ bit was checked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2017-02-16 09:50:03]

The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2677563005/#ps100001
(title: "More comments")

[commit-bot: I haz the power, 2017-02-16 09:50:37]

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

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

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487238602574560,
"parent_rev": "1397281ab2c5d5e47437363822b7439196851ea3", "commit_rev":
"3c6d07134f1a93f57b6c0d191251deb3d9d9e887"}

[commit-bot: I haz the power, 2017-02-16 09:57:47]

Description was changed from

==========
Chromad: Use DM server reply to determine enrollment type

Now user have to press custom shortuct to enroll into Active Directory
management. This CL use DM server to determine that.
It also show Active Directory domain join UI just before locking the
device. So user has possibility to switch between internet and intranet.

BUG=668491
TEST=manual
==========

to

==========
Chromad: Use DM server reply to determine enrollment type

Now user have to press custom shortuct to enroll into Active Directory
management. This CL use DM server to determine that.
It also show Active Directory domain join UI just before locking the
device. So user has possibility to switch between internet and intranet.

BUG=668491
TEST=manual

Review-Url: https://codereview.chromium.org/2677563005
Cr-Commit-Position: refs/heads/master@{#450919}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c6d07134f1a93f57b6c0d191251...
==========

[commit-bot: I haz the power, 2017-02-16 09:57:48]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/3c6d07134f1a93f57b6c0d191251...