Added OOBE Asset / Naming prompt

Added OOBE Asset / Attribute prompt

Added new attribute prompt step to enrollment screen.

BUG=464184
Committed: https://crrev.com/7cbf3af29b13f373c3cefa04f8ce929792230b84
Cr-Commit-Position: refs/heads/master@{#325434}

[Polina Bondarenko, 2015-03-04 11:53:14]

Patchset #1 (id:1) has been deleted

[Polina Bondarenko, 2015-03-04 13:23:42]

Patchset #1 (id:20001) has been deleted

[Polina Bondarenko, 2015-03-04 13:30:44]

Patchset #1 (id:40001) has been deleted

[Polina Bondarenko, 2015-03-04 13:50:17]

pbond@chromium.org changed reviewers:
+ atwilson@chromium.org, bartfab@chromium.org, mnissler@chromium.org

[Polina Bondarenko, 2015-03-04 13:50:18]

Hi,

I have published a review of the device naming UI.

It refers to "OOBE Asset / Naming prompt" section of Design Doc
https://docs.google.com/document/d/18Uo4yx43wtDGyehNkYnX2F-H0LMIjXz3X2NRUm__5...

There are some issues:
1. Cancel/Done-button workaround on oauth-enroll-error-step after
oauth-enroll-naming-step. (I'm not sure, it is done correctly).
2. Concern about showing existing asset id and location. (I don't know how to
send them on oauth-enroll-naming-step to oobe. Now they are sent before showing
the enrollment screen)

[Polina Bondarenko, 2015-03-09 13:19:28]

Patchset #1 (id:60001) has been deleted

[Polina Bondarenko, 2015-03-09 13:24:24]

Patchset #1 (id:80001) has been deleted

[Andrew T Wilson (Slow), 2015-03-12 17:07:17]

A few nits - it looks like you are setting the existing name/location fields in
the right place, so I wasn't sure about your comment on this CL - can you
clarify your second comment?

https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5830: <message
name="IDS_ENTERPRISE_ENROLLMENT_DEVICE_NAMING_ERROR" desc="Error message shown
when successfully enrolled, but the device naming is failed.">
nit: is failed -> has failed.

https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5860: <message
name="IDS_ENTERPRISE_ENROLLMENT_NAMING" desc="Device naming message to be shown
once enterprise naming device.">
nit: once enterprise naming device -> when editing the name and location field
for a device.

https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5861: <ph name="BEGIN_BOLD">&lt;b&gt;</ph>Your
account is enabled to provide an Asset ID and Location for this device
(optional).<ph name="END_BOLD">&lt;/b&gt;</ph> <ph
name="BEGIN_ITALIC">&lt;i&gt;</ph>You may provide a device identifier and
location in the fields below. These details will be displayed for this device on
the chrome:policy page in your management console.<ph
name="END_ITALIC">&lt;/i&gt;</ph>
nit: most places in this file use <strong>, not <b> - let's use <strong>
instead.

https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:182: 
nit:remove blank line

https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:212:
hideCancel: function(hide) {
nit: name this hideCancelShowDone() for clarity?

https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:231: if
(this.isNamingError(step)) {
Explain what you are doing here in a comment?

[Polina Bondarenko, 2015-03-13 12:46:54]

On 2015/03/12 17:07:17, Andrew T Wilson wrote:
> A few nits - it looks like you are setting the existing name/location fields
in
> the right place, so I wasn't sure about your comment on this CL - can you
> clarify your second comment?
> 
>
https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
> File chrome/app/chromeos_strings.grdp (right):
> 
>
https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
> chrome/app/chromeos_strings.grdp:5830: <message
> name="IDS_ENTERPRISE_ENROLLMENT_DEVICE_NAMING_ERROR" desc="Error message shown
> when successfully enrolled, but the device naming is failed.">
> nit: is failed -> has failed.
> 
>
https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
> chrome/app/chromeos_strings.grdp:5860: <message
> name="IDS_ENTERPRISE_ENROLLMENT_NAMING" desc="Device naming message to be
shown
> once enterprise naming device.">
> nit: once enterprise naming device -> when editing the name and location field
> for a device.
> 
>
https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
> chrome/app/chromeos_strings.grdp:5861: <ph
name="BEGIN_BOLD">&lt;b&gt;</ph>Your
> account is enabled to provide an Asset ID and Location for this device
> (optional).<ph name="END_BOLD">&lt;/b&gt;</ph> <ph
> name="BEGIN_ITALIC">&lt;i&gt;</ph>You may provide a device identifier and
> location in the fields below. These details will be displayed for this device
on
> the chrome:policy page in your management console.<ph
> name="END_ITALIC">&lt;/i&gt;</ph>
> nit: most places in this file use <strong>, not <b> - let's use <strong>
> instead.
> 
>
https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
> File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
> (right):
> 
>
https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
> chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:182: 
> nit:remove blank line
> 
>
https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
> chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:212:
> hideCancel: function(hide) {
> nit: name this hideCancelShowDone() for clarity?
> 
>
https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
> chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:231:
if
> (this.isNamingError(step)) {
> Explain what you are doing here in a comment?

Sorry for second concern, it has been already resolved.

[Polina Bondarenko, 2015-03-13 12:56:01]

Fixed comments

https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5830: <message
name="IDS_ENTERPRISE_ENROLLMENT_DEVICE_NAMING_ERROR" desc="Error message shown
when successfully enrolled, but the device naming is failed.">
On 2015/03/12 17:07:17, Andrew T Wilson wrote:
> nit: is failed -> has failed.

Done.

https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5860: <message
name="IDS_ENTERPRISE_ENROLLMENT_NAMING" desc="Device naming message to be shown
once enterprise naming device.">
On 2015/03/12 17:07:17, Andrew T Wilson wrote:
> nit: once enterprise naming device -> when editing the name and location field
> for a device.

Done.

https://codereview.chromium.org/975353002/diff/100001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5861: <ph name="BEGIN_BOLD">&lt;b&gt;</ph>Your
account is enabled to provide an Asset ID and Location for this device
(optional).<ph name="END_BOLD">&lt;/b&gt;</ph> <ph
name="BEGIN_ITALIC">&lt;i&gt;</ph>You may provide a device identifier and
location in the fields below. These details will be displayed for this device on
the chrome:policy page in your management console.<ph
name="END_ITALIC">&lt;/i&gt;</ph>
On 2015/03/12 17:07:17, Andrew T Wilson wrote:
> nit: most places in this file use <strong>, not <b> - let's use <strong>
> instead.

Done.

https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:182: 
On 2015/03/12 17:07:17, Andrew T Wilson wrote:
> nit:remove blank line

Done.

https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:212:
hideCancel: function(hide) {
On 2015/03/12 17:07:17, Andrew T Wilson wrote:
> nit: name this hideCancelShowDone() for clarity?

Done.

https://codereview.chromium.org/975353002/diff/100001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:231: if
(this.isNamingError(step)) {
On 2015/03/12 17:07:17, Andrew T Wilson wrote:
> Explain what you are doing here in a comment?

Done.

[Polina Bondarenko, 2015-03-31 19:56:47]

Patchset #4 (id:160001) has been deleted

[Polina Bondarenko, 2015-04-09 16:40:04]

pbond@chromium.org changed reviewers:
+ dzhioev@chromium.org

[Polina Bondarenko, 2015-04-09 16:40:05]

dzhioev@chromium.org: Please review changes in
 chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.css
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.h
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc

[Mattias Nissler (ping if slow), 2015-04-10 12:22:29]

https://codereview.chromium.org/975353002/diff/180001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/975353002/diff/180001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5952: <ph
name="BEGIN_BOLD">&lt;strong&gt;</ph>Your account is enabled to provide an Asset
ID and Location for this device (optional).<ph
name="END_BOLD">&lt;/strong&gt;</ph> <ph name="BEGIN_ITALIC">&lt;i&gt;</ph>You
may provide a device identifier and location in the fields below. These details
will be displayed for this device on the chrome:policy page in your management
console.<ph name="END_ITALIC">&lt;/i&gt;</ph>
What is the "chrome:policy page in your management console"?

What's the origin of these strings? mdrasner@? If not, let's loop him in.

https://codereview.chromium.org/975353002/diff/180001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5954: <message
name="IDS_ENTERPRISE_ENROLLMENT_ASSET_ID_LABEL" desc="Label for asset id textBox
on the device naming screen in the enterprise enrollment dialog.">
nit: text box

https://codereview.chromium.org/975353002/diff/180001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5957: <message
name="IDS_ENTERPRISE_ENROLLMENT_LOCATION_LABEL" desc="Label for location textBox
on the device naming screen in the enterprise enrollment dialog.">
nit: text box

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:231:
policy::BrowserPolicyConnectorChromeOS* connector =
This logic should probably all be moved to the enrollment helper.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:243: if
(remora_controller_) {
This should not be run if there's still a Naming permission check pending.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:251:
policy::BrowserPolicyConnectorChromeOS* connector =
This stuff should also move to the enrollment helper.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:278: void
EnrollmentScreen::OnDeviceNamed(bool status) {
This should probably be renamed to OnDeviceAttributeUploadCompleted or somesuch,
to make it clear that this runs to handle the server response.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:287: void
EnrollmentScreen::ShowDeviceNamingScreen() {
nit: Rename to ShowAttributePromptScreen?

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h (right):

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:36: virtual
void OnDeviceNaming(const std::string& asset_id,
Let's call this OnDeviceAttributesProvided, so we're not locking our language on
naming and we also to make clear that this gets called at the point where the
user has provided their input.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:59: virtual
void ShowDeviceNamingScreen() = 0;
Maybe ShowAttributePromptScreen() would be a more appropriate name?

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

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:154:
AddCallback("oauthEnrollNaming",
Let's change the language from naming -> attributes here as well.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:195:
policy::BrowserPolicyConnectorChromeOS* connector =
This should not grab the values from the connector, instead it should receive
these values as parameters. The idea is that this is the "view" part of the UI,
which shouldn't be concerned with pulling data from elsewhere.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:213:
ShowStep(kEnrollmentStepNaming);
These two should be replaced by a single JS call that takes the attributes as
parameters, i.e. ShowAttributePromptStep(asset_id, location)

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:557: void
EnrollmentScreenHandler::HandleDeviceNaming(const std::string& asset_id,
nit: Let's improve the naming here, i.e. rename to
HandleDeviceAttributesProvided or similar to make it clear that this handles the
input received from the user.

https://codereview.chromium.org/975353002/diff/180001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/975353002/diff/180001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_client.cc:328: void
CloudPolicyClient::GetDeviceAttributeUpdatePermission(
Don't you have a separate code change that adds these? Should probably land that
change first and only then commit this CL.

https://codereview.chromium.org/975353002/diff/180001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/975353002/diff/180001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_client.h:158: const
RemoteCommandCallback& callback);
nit: blank line before comment.

[Polina Bondarenko, 2015-04-10 13:22:04]

https://codereview.chromium.org/975353002/diff/180001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/975353002/diff/180001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5952: <ph
name="BEGIN_BOLD">&lt;strong&gt;</ph>Your account is enabled to provide an Asset
ID and Location for this device (optional).<ph
name="END_BOLD">&lt;/strong&gt;</ph> <ph name="BEGIN_ITALIC">&lt;i&gt;</ph>You
may provide a device identifier and location in the fields below. These details
will be displayed for this device on the chrome:policy page in your management
console.<ph name="END_ITALIC">&lt;/i&gt;</ph>
On 2015/04/10 12:22:28, Mattias Nissler wrote:
> What is the "chrome:policy page in your management console"?
> 
> What's the origin of these strings? mdrasner@? If not, let's loop him in.

Yes, PRD and mdrasner@

[dzhioev (left Google), 2015-04-10 13:23:11]

Polina, you'll need to repeat changes that you made in
oobe_screen_oauth_enrollment.* files for the new version implemented in
oobe_screen_oauth_enrollment_webview.*, and make sure that your UI works fine in
the new version. All this need to be done in this CL, otherwise the new version
will be broken. To activate the new version press Ctrl + Alt + Shift + W when
you are on enrollment screen.

We didn't expect big changes in enrollment screen anytime soon, that is why
creating a fork sounded like a good idea =(
Sorry for that.

[Nikita (slow), 2015-04-13 06:35:58]

On 2015/04/10 13:23:11, dzhioev wrote:
> Polina, you'll need to repeat changes that you made in
> oobe_screen_oauth_enrollment.* files for the new version implemented in
> oobe_screen_oauth_enrollment_webview.*, and make sure that your UI works fine
in
> the new version. All this need to be done in this CL, otherwise the new
version
> will be broken. To activate the new version press Ctrl + Alt + Shift + W when
> you are on enrollment screen.
> 
> We didn't expect big changes in enrollment screen anytime soon, that is why
> creating a fork sounded like a good idea =(
> Sorry for that.

Please attach screenshots to the bug.

[Polina Bondarenko, 2015-04-13 19:09:12]

On 2015/04/10 13:23:11, dzhioev wrote:
> Polina, you'll need to repeat changes that you made in
> oobe_screen_oauth_enrollment.* files for the new version implemented in
> oobe_screen_oauth_enrollment_webview.*, and make sure that your UI works fine
in
> the new version. All this need to be done in this CL, otherwise the new
version
> will be broken. To activate the new version press Ctrl + Alt + Shift + W when
> you are on enrollment screen.
> 
> We didn't expect big changes in enrollment screen anytime soon, that is why
> creating a fork sounded like a good idea =(
> Sorry for that.

Hi Pavel,

I try to activate new OOBE by Ctrl + Alt + Shift + W from enrollment screen on
chromium master build with chromeos=1,
but system crashes. Is it ok?

Or should it be tested only on normal chromiumos build? (I haven't tried it yet)

[bartfab (slow), 2015-04-14 08:03:33]

On 2015/04/13 19:09:12, Polina Bondarenko wrote:
> On 2015/04/10 13:23:11, dzhioev wrote:
> > Polina, you'll need to repeat changes that you made in
> > oobe_screen_oauth_enrollment.* files for the new version implemented in
> > oobe_screen_oauth_enrollment_webview.*, and make sure that your UI works
fine
> in
> > the new version. All this need to be done in this CL, otherwise the new
> version
> > will be broken. To activate the new version press Ctrl + Alt + Shift + W
when
> > you are on enrollment screen.
> > 
> > We didn't expect big changes in enrollment screen anytime soon, that is why
> > creating a fork sounded like a good idea =(
> > Sorry for that.
> 
> Hi Pavel,
> 
> I try to activate new OOBE by Ctrl + Alt + Shift + W from enrollment screen on
> chromium master build with chromeos=1,
> but system crashes. Is it ok?
> 
> Or should it be tested only on normal chromiumos build? (I haven't tried it
yet)

It doesn't crash. Ctrl-Alt-Shift-W restarts the browser. On a chromeos=1 build,
the browser just quits at that point and you have to restart it manually. Once
you start it up again, you will see the new flow.

[Polina Bondarenko, 2015-04-14 08:24:26]

Patchset #5 (id:200001) has been deleted

[Polina Bondarenko, 2015-04-14 14:14:53]

On 2015/04/14 08:03:33, bartfab wrote:
> On 2015/04/13 19:09:12, Polina Bondarenko wrote:
> > On 2015/04/10 13:23:11, dzhioev wrote:
> > > Polina, you'll need to repeat changes that you made in
> > > oobe_screen_oauth_enrollment.* files for the new version implemented in
> > > oobe_screen_oauth_enrollment_webview.*, and make sure that your UI works
> fine
> > in
> > > the new version. All this need to be done in this CL, otherwise the new
> > version
> > > will be broken. To activate the new version press Ctrl + Alt + Shift + W
> when
> > > you are on enrollment screen.
> > > 
> > > We didn't expect big changes in enrollment screen anytime soon, that is
why
> > > creating a fork sounded like a good idea =(
> > > Sorry for that.
> > 
> > Hi Pavel,
> > 
> > I try to activate new OOBE by Ctrl + Alt + Shift + W from enrollment screen
on
> > chromium master build with chromeos=1,
> > but system crashes. Is it ok?
> > 
> > Or should it be tested only on normal chromiumos build? (I haven't tried it
> yet)
> 
> It doesn't crash. Ctrl-Alt-Shift-W restarts the browser. On a chromeos=1
build,
> the browser just quits at that point and you have to restart it manually. Once
> you start it up again, you will see the new flow.

Thanks a lot, it works for me now.

[Polina Bondarenko, 2015-04-14 14:15:48]

Fixed comments, added changes to _webview files

https://codereview.chromium.org/975353002/diff/180001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/975353002/diff/180001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5954: <message
name="IDS_ENTERPRISE_ENROLLMENT_ASSET_ID_LABEL" desc="Label for asset id textBox
on the device naming screen in the enterprise enrollment dialog.">
On 2015/04/10 12:22:28, Mattias Nissler wrote:
> nit: text box

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:5957: <message
name="IDS_ENTERPRISE_ENROLLMENT_LOCATION_LABEL" desc="Label for location textBox
on the device naming screen in the enterprise enrollment dialog.">
On 2015/04/10 12:22:28, Mattias Nissler wrote:
> nit: text box

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:231:
policy::BrowserPolicyConnectorChromeOS* connector =
On 2015/04/10 12:22:28, Mattias Nissler wrote:
> This logic should probably all be moved to the enrollment helper.

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:243: if
(remora_controller_) {
On 2015/04/10 12:22:28, Mattias Nissler wrote:
> This should not be run if there's still a Naming permission check pending.

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:251:
policy::BrowserPolicyConnectorChromeOS* connector =
On 2015/04/10 12:22:28, Mattias Nissler wrote:
> This stuff should also move to the enrollment helper.

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:278: void
EnrollmentScreen::OnDeviceNamed(bool status) {
On 2015/04/10 12:22:28, Mattias Nissler wrote:
> This should probably be renamed to OnDeviceAttributeUploadCompleted or
somesuch,
> to make it clear that this runs to handle the server response.

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:287: void
EnrollmentScreen::ShowDeviceNamingScreen() {
On 2015/04/10 12:22:28, Mattias Nissler wrote:
> nit: Rename to ShowAttributePromptScreen?

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h (right):

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:36: virtual
void OnDeviceNaming(const std::string& asset_id,
On 2015/04/10 12:22:29, Mattias Nissler wrote:
> Let's call this OnDeviceAttributesProvided, so we're not locking our language
on
> naming and we also to make clear that this gets called at the point where the
> user has provided their input.

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enrollment_screen_actor.h:59: virtual
void ShowDeviceNamingScreen() = 0;
On 2015/04/10 12:22:29, Mattias Nissler wrote:
> Maybe ShowAttributePromptScreen() would be a more appropriate name?

Done.

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

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:154:
AddCallback("oauthEnrollNaming",
On 2015/04/10 12:22:29, Mattias Nissler wrote:
> Let's change the language from naming -> attributes here as well.

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:195:
policy::BrowserPolicyConnectorChromeOS* connector =
On 2015/04/10 12:22:29, Mattias Nissler wrote:
> This should not grab the values from the connector, instead it should receive
> these values as parameters. The idea is that this is the "view" part of the
UI,
> which shouldn't be concerned with pulling data from elsewhere.

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:213:
ShowStep(kEnrollmentStepNaming);
On 2015/04/10 12:22:29, Mattias Nissler wrote:
> These two should be replaced by a single JS call that takes the attributes as
> parameters, i.e. ShowAttributePromptStep(asset_id, location)

Done.

https://codereview.chromium.org/975353002/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/enrollment_screen_handler.cc:557: void
EnrollmentScreenHandler::HandleDeviceNaming(const std::string& asset_id,
On 2015/04/10 12:22:29, Mattias Nissler wrote:
> nit: Let's improve the naming here, i.e. rename to
> HandleDeviceAttributesProvided or similar to make it clear that this handles
the
> input received from the user.

Done.

https://codereview.chromium.org/975353002/diff/180001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_client.cc (right):

https://codereview.chromium.org/975353002/diff/180001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_client.cc:328: void
CloudPolicyClient::GetDeviceAttributeUpdatePermission(
On 2015/04/10 12:22:29, Mattias Nissler wrote:
> Don't you have a separate code change that adds these? Should probably land
that
> change first and only then commit this CL.

Done.

https://codereview.chromium.org/975353002/diff/180001/components/policy/core/...
File components/policy/core/common/cloud/cloud_policy_client.h (right):

https://codereview.chromium.org/975353002/diff/180001/components/policy/core/...
components/policy/core/common/cloud/cloud_policy_client.h:158: const
RemoteCommandCallback& callback);
On 2015/04/10 12:22:29, Mattias Nissler wrote:
> nit: blank line before comment.

Done.

[Polina Bondarenko, 2015-04-14 14:15:49]

Fixed comments, added changes to _webview files

[dzhioev (left Google), 2015-04-14 15:48:19]

https://codereview.chromium.org/975353002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h
(right):

https://codereview.chromium.org/975353002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:129: //
Gets oauth token, used during enterprise enrollment.
As far as I can see, you don't need to declare this method in the interface.

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

https://codereview.chromium.org/975353002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.h:48:
const std::string& GetOAuthToken() override;
This method is declared private in EnterpriseEnrollmentHelper. How it can be
public here? That is a compile error, isn't it?

https://codereview.chromium.org/975353002/diff/220001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.js
(right):

https://codereview.chromium.org/975353002/diff/220001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.js:242:
var focusSTep = step;
Misprint, should be 'focusStep'.
Anyway, is this variable needed? You only set it and never read, as far as I can
see.

[Polina Bondarenko, 2015-04-14 16:14:32]

Fixed comments.

https://codereview.chromium.org/975353002/diff/220001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h
(right):

https://codereview.chromium.org/975353002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:129: //
Gets oauth token, used during enterprise enrollment.
On 2015/04/14 15:48:19, dzhioev wrote:
> As far as I can see, you don't need to declare this method in the interface.

Done.

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

https://codereview.chromium.org/975353002/diff/220001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.h:48:
const std::string& GetOAuthToken() override;
On 2015/04/14 15:48:19, dzhioev wrote:
> This method is declared private in EnterpriseEnrollmentHelper. How it can be
> public here? That is a compile error, isn't it?

I didn't get a compilation error, anyway fixed.

https://codereview.chromium.org/975353002/diff/220001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.js
(right):

https://codereview.chromium.org/975353002/diff/220001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.js:242:
var focusSTep = step;
On 2015/04/14 15:48:19, dzhioev wrote:
> Misprint, should be 'focusStep'.
> Anyway, is this variable needed? You only set it and never read, as far as I
can
> see.

Thanks, it is also a bug. It should be used instead of step while focusElements
are selected. Fixed.

[dzhioev (left Google), 2015-04-14 18:24:47]

LGTM with nits

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h
(right):

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:62:
virtual void OnDeviceAttributeUpdatePermission(bool status) = 0;
nit: I suggest to rename |status| to |granted| (or something similar)

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:66:
virtual void OnDeviceAttributeUploadCompleted(bool status) = 0;
nit: I suggest to rename |status| to |success| (or something similar)

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:59:
<div class="oauth-enroll-attribute-label"
nit: I think "<label for="oauth-enroll-asset-id">" will be more appropriate than
<div>

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:67:
<div class="oauth-enroll-attribute-label"
nit: I think "<label for="oauth-enroll-attribute-field">" will be more
appropriate than <div>

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.js
(right):

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.js:198:
$('oauth-enroll-attribute-prompt-message').innerHTML =
Please add a comment on why we use 'innerHTML' and not 'textContent' here.

[Polina Bondarenko, 2015-04-14 21:15:40]

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:59:
<div class="oauth-enroll-attribute-label"
On 2015/04/14 18:24:47, dzhioev wrote:
> nit: I think "<label for="oauth-enroll-asset-id">" will be more appropriate
than
> <div>

This is the error message that I got when tried to upload new patch. What do you
think?

  line 58: Avoid 'for' attribute on <label>. Place the input within the <label>,
or use aria-labelledby for <select>.
            <label for="oauth-enroll-asset-id"
                   ^^^^

[Polina Bondarenko, 2015-04-15 08:14:13]

Thanks, fixed comments.

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h
(right):

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:62:
virtual void OnDeviceAttributeUpdatePermission(bool status) = 0;
On 2015/04/14 18:24:47, dzhioev wrote:
> nit: I suggest to rename |status| to |granted| (or something similar)

Done.

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:66:
virtual void OnDeviceAttributeUploadCompleted(bool status) = 0;
On 2015/04/14 18:24:47, dzhioev wrote:
> nit: I suggest to rename |status| to |success| (or something similar)

Done.

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.js
(right):

https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment_webview.js:198:
$('oauth-enroll-attribute-prompt-message').innerHTML =
On 2015/04/14 18:24:47, dzhioev wrote:
> Please add a comment on why we use 'innerHTML' and not 'textContent' here.

Done.

[Mattias Nissler (ping if slow), 2015-04-15 12:15:38]

LGTM from my side. I've not looked in detail at the CSS/HTML/JS changes as these
are already covered by dzhioev@

https://codereview.chromium.org/975353002/diff/260001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h
(right):

https://codereview.chromium.org/975353002/diff/260001/chrome/browser/chromeos...
chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.h:65: //
whether it is successful or not
nit: missing period

[dzhioev (left Google), 2015-04-15 12:51:50]

On 2015/04/14 21:15:40, Polina Bondarenko wrote:
>
https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
> File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
> (right):
> 
>
https://codereview.chromium.org/975353002/diff/240001/chrome/browser/resource...
> chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:59:
> <div class="oauth-enroll-attribute-label"
> On 2015/04/14 18:24:47, dzhioev wrote:
> > nit: I think "<label for="oauth-enroll-asset-id">" will be more appropriate
> than
> > <div>
> 
> This is the error message that I got when tried to upload new patch. What do
you
> think?
> 
>   line 58: Avoid 'for' attribute on <label>. Place the input within the
<label>,
> or use aria-labelledby for <select>.
>             <label for="oauth-enroll-asset-id"
>                    ^^^^

Just follow the advice, put <input> inside <label>:

<label>
  <span id="label"> Hello </span>
  <input type="text"></input>
</label>

I don't know what's wrong with "<label for=" though.

[Polina Bondarenko, 2015-04-15 14:48:14]

Patchset #8 (id:280001) has been deleted

[Polina Bondarenko, 2015-04-16 08:33:47]

The CQ bit was checked by pbond@chromium.org

[Polina Bondarenko, 2015-04-16 08:33:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dzhioev@chromium.org, mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/975353002/#ps320001
(title: "Fixed Mock error")

[Polina Bondarenko, 2015-04-16 08:34:04]

The CQ bit was unchecked by pbond@chromium.org

[Polina Bondarenko, 2015-04-16 08:37:03]

The CQ bit was checked by pbond@chromium.org

[commit-bot: I haz the power, 2015-04-16 08:37:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/975353002/320001

[commit-bot: I haz the power, 2015-04-16 10:00:46]

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

[commit-bot: I haz the power, 2015-04-16 10:00:47]

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

[Polina Bondarenko, 2015-04-16 10:14:03]

Patchset #10 (id:340001) has been deleted

[Polina Bondarenko, 2015-04-16 10:16:02]

The CQ bit was checked by pbond@chromium.org

[Polina Bondarenko, 2015-04-16 10:16:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
dzhioev@chromium.org, mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/975353002/#ps360001
(title: "Merged with master")

[commit-bot: I haz the power, 2015-04-16 10:16:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/975353002/360001

[Polina Bondarenko, 2015-04-16 13:31:35]

The CQ bit was unchecked by pbond@chromium.org

[Polina Bondarenko, 2015-04-16 13:31:36]

The CQ bit was checked by pbond@chromium.org

[commit-bot: I haz the power, 2015-04-16 13:32:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/975353002/360001

[commit-bot: I haz the power, 2015-04-16 13:33:53]

Committed patchset #10 (id:360001)

[commit-bot: I haz the power, 2015-04-16 13:34:52]

Patchset 10 (id:??) landed as
https://crrev.com/7cbf3af29b13f373c3cefa04f8ce929792230b84
Cr-Commit-Position: refs/heads/master@{#325434}