Add new version of enrollment screen supporting OAuth.

Add new version of enrollment screen supporting OAuth.

If switched on via a command line flag, the new version of the
enrollment screen is enabled. It uses the authentication extension,
which will fetch an OAuth token that is then used to register for device
policy.

BUG=chromium-os:18203
TEST=Enable flags and test enrollment.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96181

[Mattias Nissler (ping if slow), 2011-08-03 14:43:10]

Hi guys, here is a first stab at converting the enrollment screen to OAuth and
switch it over to the new-style UI. The original mocks include a descriptive
text on the right side of the the screen, which I haven't implemented yet. Also,
the warning icon is not the correct size yet, waiting for UI to provide one.
Other than that, the thing is functional and works for me.

Regarding the review:
Nikita: main reviewer, focus on OOBE
Erik: HTML+JS
Julian: policy bits (not much)
Alexey, Zel, Sumit: FYI.

Thanks,
Mattias

[pastarmovj, 2011-08-03 15:01:55]

For the policy part it is LGTM for me. I spotted only two tiny nits in there.

http://codereview.chromium.org/7562008/diff/1/chrome/browser/chromeos/login/e...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.h (right):

http://codereview.chromium.org/7562008/diff/1/chrome/browser/chromeos/login/e...
chrome/browser/chromeos/login/enterprise_enrollment_screen.h:80: // Kicks of the
policy infrastructure to register with the service. Either
s/of/off/

http://codereview.chromium.org/7562008/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/7562008/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe.css:206: #oobe.user-image
#ok-button ,
There seems to be one space too much before the ,

[Nikita (slow), 2011-08-03 16:00:12]

I'm OOO for 3 weeks, adding Anton instead.

[whywhat, 2011-08-03 17:48:03]

LGTM with a few nits and suggestions
BTW, could you tell me how to trigger this screen to show up for testing?

http://codereview.chromium.org/7562008/diff/1/chrome/browser/chromeos/login/e...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.h (right):

http://codereview.chromium.org/7562008/diff/1/chrome/browser/chromeos/login/e...
chrome/browser/chromeos/login/enterprise_enrollment_screen.h:83: void
RegisterForDevicePolicy(const std::string& gaia_auth_token,
optional: maybe instead pass one string token and one flag or enum that strictly
identifies what token is in the string. Or even better, have two methods and
refactor common code into private method?

http://codereview.chromium.org/7562008/diff/1/chrome/browser/policy/browser_p...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/7562008/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:65: // with this policy
connector.
Should the comment be updated about agreement on the |token| argument values
being exclusively empty?
optional: the same thing about passing two tokens at once as in higher-level
method

http://codereview.chromium.org/7562008/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/7562008/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe.css:207: #oobe.oauth-enrollment
#oauth-enroll-button-cancel,
nit: Could you move button to the end of the name to conform with the rest
button names?

http://codereview.chromium.org/7562008/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe.css:422: display: -webkit-box;
please, sort all properties for each selector alphabetically

http://codereview.chromium.org/7562008/diff/1/chrome/browser/ui/webui/chromeo...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

http://codereview.chromium.org/7562008/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:284:
base::StringValue(step));
This causes Clang bot to fail.

[Mattias Nissler (ping if slow), 2011-08-03 21:13:24]

Answers to Anton's questions, will fix the nits tomorrow.

On 2011/08/03 17:48:03, whywhat wrote:
> LGTM with a few nits and suggestions
> BTW, could you tell me how to trigger this screen to show up for testing?

Currently, you need to pass --login-screen=enroll. The intended way for getting
to the screen is to hit a keyboard shortcut (Ctrl-Alt-E) on the main signin
screen. Alexey (cc'ed) was going to implement that as part of the original
enrollment screen migration. Alexey?

Also note that you'll only get past the first screen if you're on a VM or a
physical device.

[Mattias Nissler (ping if slow), 2011-08-04 09:58:59]

Addressed your comments, new version is up for review.

http://codereview.chromium.org/7562008/diff/1/chrome/browser/chromeos/login/e...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.h (right):

http://codereview.chromium.org/7562008/diff/1/chrome/browser/chromeos/login/e...
chrome/browser/chromeos/login/enterprise_enrollment_screen.h:80: // Kicks of the
policy infrastructure to register with the service. Either
On 2011/08/03 15:01:56, pastarmovj wrote:
> s/of/off/

Done.

http://codereview.chromium.org/7562008/diff/1/chrome/browser/chromeos/login/e...
chrome/browser/chromeos/login/enterprise_enrollment_screen.h:83: void
RegisterForDevicePolicy(const std::string& gaia_auth_token,
On 2011/08/03 17:48:03, whywhat wrote:
> optional: maybe instead pass one string token and one flag or enum that
strictly
> identifies what token is in the string. Or even better, have two methods and
> refactor common code into private method?

Done.

http://codereview.chromium.org/7562008/diff/1/chrome/browser/policy/browser_p...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/7562008/diff/1/chrome/browser/policy/browser_p...
chrome/browser/policy/browser_policy_connector.h:65: // with this policy
connector.
On 2011/08/03 17:48:03, whywhat wrote:
> Should the comment be updated about agreement on the |token| argument values
> being exclusively empty?
> optional: the same thing about passing two tokens at once as in higher-level
> method

Done.

http://codereview.chromium.org/7562008/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/7562008/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe.css:206: #oobe.user-image
#ok-button ,
On 2011/08/03 15:01:56, pastarmovj wrote:
> There seems to be one space too much before the ,

Done.

http://codereview.chromium.org/7562008/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe.css:207: #oobe.oauth-enrollment
#oauth-enroll-button-cancel,
On 2011/08/03 17:48:03, whywhat wrote:
> nit: Could you move button to the end of the name to conform with the rest
> button names?

Done.

http://codereview.chromium.org/7562008/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/oobe.css:422: display: -webkit-box;
On 2011/08/03 17:48:03, whywhat wrote:
> please, sort all properties for each selector alphabetically

Done.

http://codereview.chromium.org/7562008/diff/1/chrome/browser/ui/webui/chromeo...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

http://codereview.chromium.org/7562008/diff/1/chrome/browser/ui/webui/chromeo...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:284:
base::StringValue(step));
On 2011/08/03 17:48:03, whywhat wrote:
> This causes Clang bot to fail.

Fixed.

[whywhat, 2011-08-04 10:47:51]

Thanks for the fixes!

http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:266:
new GaiaOAuthFetcher(this,
This ctor also has one more parameter now.

http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.h
(right):

http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.h:61:
virtual void OnOAuthWrapBridgeSuccess(
This method now has service_name parameter. Please, sync and check that Clang
trybot actually pass before committing.

[Mattias Nissler (ping if slow), 2011-08-04 14:38:44]

Addressed comments (but note that Rick has another CL up that will change
GaiaOAuthFetcher again...).

arv: Friendly ping.

http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
(right):

http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:266:
new GaiaOAuthFetcher(this,
On 2011/08/04 10:47:52, whywhat wrote:
> This ctor also has one more parameter now.

Done.

http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
File
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.h
(right):

http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.h:61:
virtual void OnOAuthWrapBridgeSuccess(
On 2011/08/04 10:47:52, whywhat wrote:
> This method now has service_name parameter. Please, sync and check that Clang
> trybot actually pass before committing. 

Done.

[whywhat, 2011-08-05 04:52:41]

On 2011/08/04 14:38:44, Mattias Nissler wrote:
> Addressed comments (but note that Rick has another CL up that will change
> GaiaOAuthFetcher again...).
> 
> arv: Friendly ping.

I guess Erik is n/a according to his calendar, adding James instead.
James, could you, please, review HTML/JS part of the change?

> 
>
http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
> File
>
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc
> (right):
> 
>
http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
>
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.cc:266:
> new GaiaOAuthFetcher(this,
> On 2011/08/04 10:47:52, whywhat wrote:
> > This ctor also has one more parameter now.
> 
> Done.
> 
>
http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
> File
>
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.h
> (right):
> 
>
http://codereview.chromium.org/7562008/diff/6001/chrome/browser/ui/webui/chro...
>
chrome/browser/ui/webui/chromeos/login/enterprise_oauth_enrollment_screen_handler.h:61:
> virtual void OnOAuthWrapBridgeSuccess(
> On 2011/08/04 10:47:52, whywhat wrote:
> > This method now has service_name parameter. Please, sync and check that
Clang
> > trybot actually pass before committing. 
> 
> Done.

[whywhat, 2011-08-05 07:24:58]

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:29:
onclick="chrome.send('oauthEnrollRetry', []);"
nit: indent got off by one space here and on the next line

[Mattias Nissler (ping if slow), 2011-08-05 07:33:15]

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:29:
onclick="chrome.send('oauthEnrollRetry', []);"
On 2011/08/05 07:24:58, whywhat wrote:
> nit: indent got off by one space here and on the next line 

Actually, it's indented 4 spaces after a line break. Other oobe pages do the
same thing, but I don't know what the style rule for breaking long elements
is/should be though.

[whywhat, 2011-08-05 07:38:52]

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:29:
onclick="chrome.send('oauthEnrollRetry', []);"
Ah, ok. I thought you wanted to align attributes like with iframe tag above :)

On 2011/08/05 07:33:16, Mattias Nissler wrote:
> On 2011/08/05 07:24:58, whywhat wrote:
> > nit: indent got off by one space here and on the next line 
> 
> Actually, it's indented 4 spaces after a line break. Other oobe pages do the
> same thing, but I don't know what the style rule for breaking long elements
> is/should be though.

[Mattias Nissler (ping if slow), 2011-08-05 07:40:58]

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:29:
onclick="chrome.send('oauthEnrollRetry', []);"
On 2011/08/05 07:38:53, whywhat wrote:
> Ah, ok. I thought you wanted to align attributes like with iframe tag above :)
> 
> On 2011/08/05 07:33:16, Mattias Nissler wrote:
> > On 2011/08/05 07:24:58, whywhat wrote:
> > > nit: indent got off by one space here and on the next line 
> > 
> > Actually, it's indented 4 spaces after a line break. Other oobe pages do the
> > same thing, but I don't know what the style rule for breaking long elements
> > is/should be though. 
> 

You are right, that's inconsistent. I'll fix the iframe then :)

[James Hawkins, 2011-08-05 17:01:01]

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe.css:455: top: 0.5em;
Why are you using em here?

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:1:
<div class="step right hidden" id="oauth-enrollment">
id should be specified first.

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:5:
src="about:blank"
Indentation is 4 spaces from the start of the tag.  Try to fit as many
attributes on a line as possible though instead of one per line.

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:29:
onclick="chrome.send('oauthEnrollRetry', []);"
Move the JS to a .js file.

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:131: if
(active) {
No braces for single line block.

[Mattias Nissler (ping if slow), 2011-08-08 16:38:47]

Here's the next iteration on this. I addressed James' comments. James, please
take another look.

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe.css (right):

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe.css:455: top: 0.5em;
On 2011/08/05 17:01:01, James Hawkins wrote:
> Why are you using em here?

That's for aligning the vertical center of the icon with the vertical center of
the description text. To do that, I move the oauth-enroll-step-icon div
downwards 0.5 times line height here, and then the icon back up half the icon
height through top: -11px in the block below.

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:1:
<div class="step right hidden" id="oauth-enrollment">
On 2011/08/05 17:01:01, James Hawkins wrote:
> id should be specified first.

Done.

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:5:
src="about:blank"
On 2011/08/05 17:01:01, James Hawkins wrote:
> Indentation is 4 spaces from the start of the tag.  Try to fit as many
> attributes on a line as possible though instead of one per line.

Done.

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:29:
onclick="chrome.send('oauthEnrollRetry', []);"
On 2011/08/05 17:01:01, James Hawkins wrote:
> Move the JS to a .js file.

Done.

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js
(right):

http://codereview.chromium.org/7562008/diff/6002/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.js:131: if
(active) {
On 2011/08/05 17:01:01, James Hawkins wrote:
> No braces for single line block.

Done.

[James Hawkins, 2011-08-08 19:00:10]

LGTM with nit.

http://codereview.chromium.org/7562008/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

http://codereview.chromium.org/7562008/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:4:
<iframe id="oauth-enroll-signin-frame"
Please fill up the 80 cols and wrap only when necessary.

[Mattias Nissler (ping if slow), 2011-08-10 12:37:24]

I plan to land this later today.

http://codereview.chromium.org/7562008/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html
(right):

http://codereview.chromium.org/7562008/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/oobe_screen_oauth_enrollment.html:4:
<iframe id="oauth-enroll-signin-frame"
On 2011/08/08 19:00:10, James Hawkins wrote:
> Please fill up the 80 cols and wrap only when necessary.

Done.