ChromeOS Gaia: SAML password confirmation dialog

ChromeOS Gaia: SAML password confirmation dialog

Known issues:
-Cancel confirmation dialog and backdrop style should be according to crbug/482962
-Cancel confirmation dialog doesn't cover "account creation" case
-Fatal error does not use 'notification-card' element.

BUG=467119, 469428
TEST=Manual
TBR=grt@chromium.org

Committed: https://crrev.com/3d1921c4804a40cdab73cda3a22fb4d76f926230
Cr-Commit-Position: refs/heads/master@{#329243}

[Roman Sorokin (ftl), 2015-04-30 10:38:55]

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

[Roman Sorokin (ftl), 2015-04-30 10:38:56]

Hi Bartosz, please take a look

[Roman Sorokin (ftl), 2015-04-30 11:01:57]

Patchset #2 (id:20001) has been deleted

[Roman Sorokin (ftl), 2015-04-30 11:02:05]

Patchset #1 (id:1) has been deleted

[Roman Sorokin (ftl), 2015-04-30 11:24:47]

Patchset #2 (id:60001) has been deleted

[bartfab (slow), 2015-04-30 12:39:49]

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

https://codereview.chromium.org/1115993002/diff/80001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:1618: NO
Nit: Are you sure we want to use an upper case string here? We could use regular
capitalization and upper-case using CSS instead.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/saml_confirm_password.html (right):

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:17: 'email' -
displayed email in header.
Nit: Here and below: You have sentences or sentence-like structures with a full
stop at the end, but you begin them with a lowercase letter.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:22: 'cancel' 
      - fired when user press X-button and then press YES in
Nit 1: s/press/presses/g
Nit 2: s/X-button/the X-button/

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:27: 'reset'  
   - reset element, sets in on the first screen and enables
Nit: What does "sets in" mean?

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:29: 'focus'  
   - if current card is the first one it focuses password input.
Nit 1: s/if/if the/
Nit 2: s/ it/,/

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:67: <section
flex vertical layout center-justified>
When does this get triggered?

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/saml_confirm_password.js (right):

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.js:27: focus:
function() {
Where is this triggered?

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.js:40: function()
{
Nit: Indentation.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.js:41:
this.disabled = true;
Doesn't this mean that while the dialog is in the process of showing, the dialog
is still enabled and if you are quick enough, you can still interact with it?

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.js:44: function()
{
Nit: Indentation.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.js:48: var
inputPassword = this.$.passwordInput.inputValue;
Nit: Should this block not be indented 2 spaces, not 4?

[bartfab (slow), 2015-04-30 12:43:12]

One more nit: You should add 469428 to the BUG line as you are fixing that bug
as well now (thanks for that!).

[Roman Sorokin (ftl), 2015-04-30 15:03:04]

Hi Bartosz, please review

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

https://codereview.chromium.org/1115993002/diff/80001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:1618: NO
On 2015/04/30 12:39:48, bartfab wrote:
> Nit: Are you sure we want to use an upper case string here? We could use
regular
> capitalization and upper-case using CSS instead.

Done.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/saml_confirm_password.html (right):

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:17: 'email' -
displayed email in header.
On 2015/04/30 12:39:48, bartfab wrote:
> Nit: Here and below: You have sentences or sentence-like structures with a
full
> stop at the end, but you begin them with a lowercase letter.

Done.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:22: 'cancel' 
      - fired when user press X-button and then press YES in
On 2015/04/30 12:39:48, bartfab wrote:
> Nit 1: s/press/presses/g
> Nit 2: s/X-button/the X-button/

Done.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:27: 'reset'  
   - reset element, sets in on the first screen and enables
On 2015/04/30 12:39:48, bartfab wrote:
> Nit: What does "sets in" mean?

Changed to "switches to"

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:29: 'focus'  
   - if current card is the first one it focuses password input.
On 2015/04/30 12:39:48, bartfab wrote:
> Nit 1: s/if/if the/
> Nit 2: s/ it/,/

Done.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:67: <section
flex vertical layout center-justified>
On 2015/04/30 12:39:48, bartfab wrote:
> When does this get triggered?

When user inputs password and presses next.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/saml_confirm_password.js (right):

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.js:27: focus:
function() {
On 2015/04/30 12:39:49, bartfab wrote:
> Where is this triggered?

in ConfirmPasswordScreen.onAfterShow

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.js:40: function()
{
On 2015/04/30 12:39:49, bartfab wrote:
> Nit: Indentation.

Done.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.js:41:
this.disabled = true;
On 2015/04/30 12:39:48, bartfab wrote:
> Doesn't this mean that while the dialog is in the process of showing, the
dialog
> is still enabled and if you are quick enough, you can still interact with it?

Moved to onClose

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.js:44: function()
{
On 2015/04/30 12:39:48, bartfab wrote:
> Nit: Indentation.

Done.

https://codereview.chromium.org/1115993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/saml_confirm_password.js:48: var
inputPassword = this.$.passwordInput.inputValue;
On 2015/04/30 12:39:49, bartfab wrote:
> Nit: Should this block not be indented 2 spaces, not 4?

Done.

[Roman Sorokin (ftl), 2015-04-30 17:06:05]

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

[Roman Sorokin (ftl), 2015-04-30 17:06:06]

Hi Pavel, please review

[dzhioev (left Google), 2015-05-05 18:55:19]

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/gaia_password_changed.html (right):

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/gaia_password_changed.html:72: <div
horizontal layout justified center>
How this content is injected to <gaia-card>. I only see <content
select=".header"> and <content select=".footer"> in element's declaration.

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/screen_confirm_password.js (right):

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/screen_confirm_password.js:41: get
isNewGaiaFlow() {
I believe I already saw exactly same function somewhere. Can you please create a
single function 'isNewGaiaFlow' (for example in Oobe) and use it.

[dzhioev (left Google), 2015-05-05 21:43:37]

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/screen_confirm_password.js (right):

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/screen_confirm_password.js:41: get
isNewGaiaFlow() {
On 2015/05/05 18:55:19, dzhioev wrote:
> I believe I already saw exactly same function somewhere. Can you please create
a
> single function 'isNewGaiaFlow' (for example in Oobe) and use it.

Here it
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

[dzhioev (left Google), 2015-05-05 22:18:15]

On 2015/04/30 12:43:12, bartfab wrote:
> One more nit: You should add 469428 to the BUG line as you are fixing that bug
> as well now (thanks for that!).

Note: this CL doesn't cover "account creation" case

[bartfab (slow), 2015-05-07 15:05:11]

lgtm

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/saml_confirm_password.html (right):

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:28: the
cancel confirm dialog, displays the close button,
Nit: s/cancel //

[bartfab (slow), 2015-05-07 15:07:26]

On 2015/05/05 22:18:15, dzhioev wrote:
> On 2015/04/30 12:43:12, bartfab wrote:
> > One more nit: You should add 469428 to the BUG line as you are fixing that
bug
> > as well now (thanks for that!).
> 
> Note: this CL doesn't cover "account creation" case

Good observation. The CL still partially covers 469428, so referencing that bug
number is a good idea, but we should not mark the bug as fixed when the CL
lands.

[Roman Sorokin (ftl), 2015-05-08 09:22:45]

Hi Pavel, please review

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/gaia_password_changed.html (right):

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/gaia_password_changed.html:72: <div
horizontal layout justified center>
On 2015/05/05 18:55:19, dzhioev wrote:
> How this content is injected to <gaia-card>. I only see <content
> select=".header"> and <content select=".footer"> in element's declaration.

It is inside <div class="footer"> (Row 66)

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/saml_confirm_password.html (right):

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/saml_confirm_password.html:28: the
cancel confirm dialog, displays the close button,
On 2015/05/07 15:05:11, bartfab wrote:
> Nit: s/cancel //

Done.

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/login/screen_confirm_password.js (right):

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/screen_confirm_password.js:41: get
isNewGaiaFlow() {
On 2015/05/05 18:55:19, dzhioev wrote:
> I believe I already saw exactly same function somewhere. Can you please create
a
> single function 'isNewGaiaFlow' (for example in Oobe) and use it.

Done.

https://codereview.chromium.org/1115993002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/login/screen_confirm_password.js:41: get
isNewGaiaFlow() {
On 2015/05/05 21:43:37, dzhioev wrote:
> On 2015/05/05 18:55:19, dzhioev wrote:
> > I believe I already saw exactly same function somewhere. Can you please
create
> a
> > single function 'isNewGaiaFlow' (for example in Oobe) and use it.
> 
> Here it
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

Done.

[Nikita (slow), 2015-05-08 11:05:40]

nkostylev@chromium.org changed reviewers:
+ nkostylev@chromium.org

[Nikita (slow), 2015-05-08 11:05:41]

Please rebase first.

[Roman Sorokin (ftl), 2015-05-08 12:11:00]

Rebased, please review

[dzhioev (left Google), 2015-05-11 18:01:58]

On 2015/05/08 12:11:00, Roman Sorokin (OOO 1.05-18.05) wrote:
> Rebased, please review

LGTM

[dzhioev (left Google), 2015-05-11 18:06:52]

The CQ bit was checked by dzhioev@chromium.org

[dzhioev (left Google), 2015-05-11 18:06:53]

The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org
Link to the patchset: https://codereview.chromium.org/1115993002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-11 18:11:37]

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

[commit-bot: I haz the power, 2015-05-11 18:27:52]

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

[commit-bot: I haz the power, 2015-05-11 18:27:54]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Roman Sorokin (ftl), 2015-05-11 21:42:15]

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

[Roman Sorokin (ftl), 2015-05-11 21:42:16]

Hi Xiyuan, please make owner review for    
chrome/browser/resources/gaia_auth_host/authenticator.js
chrome/browser/resources/gaia_auth_host/gaia_auth_host.js

[xiyuan, 2015-05-11 21:45:42]

lgtm

[Roman Sorokin (ftl), 2015-05-11 21:48:14]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2015-05-11 21:49:00]

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

[commit-bot: I haz the power, 2015-05-11 21:57:14]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-05-11 21:58:08]

Patchset 5 (id:??) landed as
https://crrev.com/3d1921c4804a40cdab73cda3a22fb4d76f926230
Cr-Commit-Position: refs/heads/master@{#329243}