Consumer management enrollment signin screen change:

Consumer management enrollment signin screen change:

- Show Gaia signin page instead of account picker.
- Pre-fill owner email in Gaia signin page.
- Show only the enrollment message in Gaia screen right panel.
- Show only a cancel button (in addition to Shutdown) at the bottom.

BUG=chromium:353050
TEST=manually test the signin page.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287143

[davidyu, 2014-07-30 09:22:30]

Hi Nikita,

I've made changes to the sign-in screen. I did my best to understand the code
structure, but I am not very confident about the change. Please let me know if
you have a better way to make the change.

Also, the modification on error screen is still in progress. I'll upload another
patch set once it is ready.

Thanks!

[Nikita (slow), 2014-07-31 10:03:43]

Were you planning to extend enrollment_screen_handler instead? It also supports
showing its own GAIA in iframe.

It seems to be a better place for consumer enrollment and let signin_handler
only handle regular user sign in.

https://codereview.chromium.org/426153003/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/426153003/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:310:
CrosSettings::Get()->GetString(kDeviceOwner, &owner_email_);
CrosSettings need to be verified first like here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

However for your particular case it maybe fine to use cached version.
You should add additional checks after owner signs in that it's indeed owner
(since user may change prefilled value) and that CrosSettings values are
trusted.

[davidyu, 2014-07-31 11:28:03]

On 2014/07/31 10:03:43, Nikita Kostylev wrote:
> Were you planning to extend enrollment_screen_handler instead? It also
supports
> showing its own GAIA in iframe.
> 
> It seems to be a better place for consumer enrollment and let signin_handler
> only handle regular user sign in.

Hmm....sounds reasonable. However, unlike the enterprise enrollment screen, we
should continue to log the user in instead of going back to the normal login
screen. Will that be an issue?

[davidyu, 2014-08-01 07:07:23]

Regarding enrollment screen, it seems to be inadequate to use it as it is a
wizard screen, not a normal signin page. Although we probably can bring out the
wizard controller and do some modifications to make it look like a normal signin
page, the change may not look better than modifying the normal signin screen. I
can try to move my code to enrollment screen if you still think that it's
better. Otherwise, I will leave it as it is. Thanks.

https://codereview.chromium.org/426153003/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/426153003/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:310:
CrosSettings::Get()->GetString(kDeviceOwner, &owner_email_);
On 2014/07/31 10:03:43, Nikita Kostylev wrote:
> CrosSettings need to be verified first like here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> 
> However for your particular case it maybe fine to use cached version.
> You should add additional checks after owner signs in that it's indeed owner
> (since user may change prefilled value) and that CrosSettings values are
> trusted.

I updated the code so that the untrusted value is not saved, and added a comment
explaining why it is good enough to use the untrusted value. After signing in, I
will use the value from UserManager to verify the authenticated email.

[Nikita (slow), 2014-08-01 10:51:30]

lgtm

[davidyu, 2014-08-01 16:35:09]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-08-01 16:37:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidyu@chromium.org/426153003/40001

[commit-bot: I haz the power, 2014-08-02 01:32:46]

Change committed as 287143

[bartfab (slow), 2014-08-04 17:40:52]

I am a bit late to the party (I took some time off and was away for a long
weekend) but I have some nits/comments.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/header_bar.html (right):

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/header_bar.html:33: </button>
You have two </button>, one here and one at the end of the previous line.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/header_bar.js (right):

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/header_bar.js:49:
.addEventListener('click',
Nit: This indentation is incorrect. Either indent all arguments by four spaces
or align them all with the opening brace. Do not mix the two styles.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right):

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:223:
SIGNIN_UI_STATE.CONSUMER_MANAGEMENT_ENROLLMENT :
Nit: Indent this line and the next by four more spaces. They are parts of a
ternary operator that starts a line above.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:327:
this.isEnrollingConsumerManagement_ = data.isEnrollingConsumerManagement;
Nit: You use |data.isEnrollingConsumerManagement| in two places. Why not store
it in a temporary variable to make it easier to check the code for consistency?

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:39: void
UpdateAuthParams(base::DictionaryValue* params, bool has_users,
Nit: Per the style guide, either put all arguments on one line or each on a
separate line. Do not mix the two styles.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:146:
context.is_enrolling_consumer_management);
Nit: You use |context.is_enrolling_consumer_managementt| in two places. Why not
store it in a temporary |const bool| variable to make it easier to check the
code for consistency?

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:292: std::string
owner_email = UserManager::Get()->GetOwnerEmail();
Nit: const.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:294: // Show Gaia
signin page again since we only allow the owner to sign in.
Nit 1: s/signin/sign-in/
Nit 2: s/again/again, /

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:302:
CommandLine* command_line = CommandLine::ForCurrentProcess();
Nit: const.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:303:
PrefService* prefs = g_browser_process->local_state();
Nit: const.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:434: // We don't
check if the value of the owner email is trusted because it is
Nit: s/email/e-mail/

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:435: // only
used to pre-fill the email field in Gaia sign-in page and a cached
Nit: s/email/e-mail/

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1348:
PrefService* prefs = g_browser_process->local_state();
Nit: const.

[davidyu, 2014-08-05 07:24:06]

Still, thanks for the comments. All of them are addressed in
https://codereview.chromium.org/438493002/

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/header_bar.html (right):

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/header_bar.html:33: </button>
On 2014/08/04 17:40:51, bartfab wrote:
> You have two </button>, one here and one at the end of the previous line.

Oops. Fixed.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/header_bar.js (right):

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/header_bar.js:49:
.addEventListener('click',
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: This indentation is incorrect. Either indent all arguments by four spaces
> or align them all with the opening brace. Do not mix the two styles.

Done.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/screen_gaia_signin.js (right):

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:223:
SIGNIN_UI_STATE.CONSUMER_MANAGEMENT_ENROLLMENT :
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: Indent this line and the next by four more spaces. They are parts of a
> ternary operator that starts a line above.

Done.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/screen_gaia_signin.js:327:
this.isEnrollingConsumerManagement_ = data.isEnrollingConsumerManagement;
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: You use |data.isEnrollingConsumerManagement| in two places. Why not store
> it in a temporary variable to make it easier to check the code for
consistency?

Done.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right):

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:39: void
UpdateAuthParams(base::DictionaryValue* params, bool has_users,
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: Per the style guide, either put all arguments on one line or each on a
> separate line. Do not mix the two styles.

Done.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:146:
context.is_enrolling_consumer_management);
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: You use |context.is_enrolling_consumer_managementt| in two places. Why
not
> store it in a temporary |const bool| variable to make it easier to check the
> code for consistency?

Done.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:292: std::string
owner_email = UserManager::Get()->GetOwnerEmail();
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:294: // Show Gaia
signin page again since we only allow the owner to sign in.
On 2014/08/04 17:40:52, bartfab wrote:
> Nit 1: s/signin/sign-in/
> Nit 2: s/again/again, /

Done.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:302:
CommandLine* command_line = CommandLine::ForCurrentProcess();
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: const.

Done.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:303:
PrefService* prefs = g_browser_process->local_state();
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: const.

This line is removed.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:434: // We don't
check if the value of the owner email is trusted because it is
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: s/email/e-mail/

Done.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:435: // only
used to pre-fill the email field in Gaia sign-in page and a cached
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: s/email/e-mail/

Done.

https://codereview.chromium.org/426153003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1348:
PrefService* prefs = g_browser_process->local_state();
On 2014/08/04 17:40:52, bartfab wrote:
> Nit: const.

This line is removed.