Handle the button presses in the password edit dialog.

Handle the button presses in the password edit dialog.

CL includes tests for this element.

BUG=595538
Committed: https://crrev.com/64fa78f7076e976ac4b4e72104af0b3e3680e599
Cr-Commit-Position: refs/heads/master@{#385331}

[hcarmona, 2016-03-22 01:33:53]

hcarmona@chromium.org changed reviewers:
+ dbeam@chromium.org

[hcarmona, 2016-03-22 01:33:54]

PTAL, this CL makes the buttons do work.

[Dan Beam, 2016-03-25 03:12:56]

https://codereview.chromium.org/1822913003/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js
(right):

https://codereview.chromium.org/1822913003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js:52:
* @param {!string} password
you can remove the ! from these types: for built-in values like string, number,
or boolean, the ! is implied (they're not "nullable")

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js
(right):

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:163:
},
i don't think this is better than just calling Polymer.dom.flush() directly
because it's the same number of lines and less commonly used (I'd have to look
up what flushPolymer_ does, whereas I'm more familiar with what
Polymer.dom.flush() does without having to look it up)

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:363:
var blankPassword = ' '.repeat(7);
nit: BLANK_PASSWORD

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:363:
var blankPassword = ' '.repeat(7);
nit: why 7?

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:363:
var blankPassword = ' '.repeat(7);
nit: put closer to use

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:404:
var password = 'bAn@n@5';
nit: put closer to use

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:432:
function(event) {
nit: doesn't this fit on one line?

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:438:
passwordDialog.$.showPasswordButton.click();
can you use MockInteractions.tap() instead?

[hcarmona, 2016-03-25 19:08:21]

https://codereview.chromium.org/1822913003/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js
(right):

https://codereview.chromium.org/1822913003/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js:52:
* @param {!string} password
On 2016/03/25 03:12:55, Dan Beam wrote:
> you can remove the ! from these types: for built-in values like string,
number,
> or boolean, the ! is implied (they're not "nullable")

Done.

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
File chrome/test/data/webui/settings/settings_passwords_section_browsertest.js
(right):

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:163:
},
On 2016/03/25 03:12:55, Dan Beam wrote:
> i don't think this is better than just calling Polymer.dom.flush() directly
> because it's the same number of lines and less commonly used (I'd have to look
> up what flushPolymer_ does, whereas I'm more familiar with what
> Polymer.dom.flush() does without having to look it up)

Sounds good. Removed.

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:363:
var blankPassword = ' '.repeat(7);
On 2016/03/25 03:12:55, Dan Beam wrote:
> nit: BLANK_PASSWORD

Done.

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:363:
var blankPassword = ' '.repeat(7);
On 2016/03/25 03:12:55, Dan Beam wrote:
> nit: why 7?

Arbitrary. I replaced it with '       ' for simplicity.

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:363:
var blankPassword = ' '.repeat(7);
On 2016/03/25 03:12:55, Dan Beam wrote:
> nit: put closer to use

Used on next line.

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:404:
var password = 'bAn@n@5';
On 2016/03/25 03:12:55, Dan Beam wrote:
> nit: put closer to use

Used on next line.

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:432:
function(event) {
On 2016/03/25 03:12:55, Dan Beam wrote:
> nit: doesn't this fit on one line?

Done.

https://codereview.chromium.org/1822913003/diff/1/chrome/test/data/webui/sett...
chrome/test/data/webui/settings/settings_passwords_section_browsertest.js:438:
passwordDialog.$.showPasswordButton.click();
On 2016/03/25 03:12:55, Dan Beam wrote:
> can you use MockInteractions.tap() instead?

Yes! Replaced all |click| calls.

[Dan Beam, 2016-03-29 00:27:22]

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html
(right):

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:42:
on-tap="onTapShowPasswordButton_"></paper-icon-button>
why is this visible/tappable if there's no password or we can't show it?

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:42:
on-tap="onTapShowPasswordButton_"></paper-icon-button>
right now you're doing onTap*_, but it should be on*Tap_

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js
(right):

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js:55:
canShowPassword_: function(showPassword, password) {
should this be a computed property instead?

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js:61:
*     and 'password' when it's not.
don't indent continuations of these types of comments ^

  * Gets ...
  * and 'password ...

[hcarmona, 2016-03-29 02:01:22]

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html
(right):

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:42:
on-tap="onTapShowPasswordButton_"></paper-icon-button>
On 2016/03/29 00:27:22, Dan Beam wrote:
> right now you're doing onTap*_, but it should be on*Tap_

Done.

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.html:42:
on-tap="onTapShowPasswordButton_"></paper-icon-button>
On 2016/03/29 00:27:22, Dan Beam wrote:
> why is this visible/tappable if there's no password or we can't show it?

The button toggles visibility and |password| is populated by firing the
"show-password" event.

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js
(right):

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js:55:
canShowPassword_: function(showPassword, password) {
On 2016/03/29 00:27:22, Dan Beam wrote:
> should this be a computed property instead?

Looks cleaner. Done.

https://codereview.chromium.org/1822913003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js:61:
*     and 'password' when it's not.
On 2016/03/29 00:27:22, Dan Beam wrote:
> don't indent continuations of these types of comments ^
> 
>   * Gets ...
>   * and 'password ...

Done.

[hcarmona, 2016-04-05 00:36:51]

Friendly ping on this CL.

[Dan Beam, 2016-04-05 17:07:13]

why do you need password, showPassword, and canShowPassword?

[hcarmona, 2016-04-05 21:01:13]

On 2016/04/05 17:07:13, Dan Beam wrote:
> why do you need password, showPassword, and canShowPassword?

I've simplified this and only using |password| now.

[Dan Beam, 2016-04-05 21:10:02]

much better, lgtm

[hcarmona, 2016-04-05 21:11:46]

The CQ bit was checked by hcarmona@chromium.org

[commit-bot: I haz the power, 2016-04-05 21:12:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822913003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822913003/80001

[commit-bot: I haz the power, 2016-04-06 00:03:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-06 00:04:32]

Description was changed from

==========
Handle the button presses in the password edit dialog.

CL includes tests for this element.

BUG=595538
==========

to

==========
Handle the button presses in the password edit dialog.

CL includes tests for this element.

BUG=595538

Committed: https://crrev.com/64fa78f7076e976ac4b4e72104af0b3e3680e599
Cr-Commit-Position: refs/heads/master@{#385331}
==========

[commit-bot: I haz the power, 2016-04-06 00:04:33]

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