Add dialog to edit and save credit cards.

Add dialog to edit and save credit cards.

Tests included for dialog.
Screenshots in bug.

BUG=607348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a02e2448d903564a3199edc570835c2eabdf12b7
Cr-Commit-Position: refs/heads/master@{#398133}

[hcarmona, 2016-05-25 19:29:16]

Description was changed from

==========
Add dialog to edit and save credit cards.

BUG=607348
==========

to

==========
Add dialog to edit and save credit cards.

BUG=607348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-05-25 23:05:03]

Patchset #1 (id:1) has been deleted

[hcarmona, 2016-05-25 23:05:53]

Description was changed from

==========
Add dialog to edit and save credit cards.

BUG=607348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add dialog to edit and save credit cards.

BUG=607348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-05-25 23:05:53]

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

[hcarmona, 2016-05-25 23:07:15]

Description was changed from

==========
Add dialog to edit and save credit cards.

BUG=607348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add dialog to edit and save credit cards.

Tests included for dialog.
Screenshots in bug.

BUG=607348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-05-25 23:07:54]

PTAL, thanks!

[michaelpg, 2016-05-27 17:57:08]

https://codereview.chromium.org/2015463003/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2015463003/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:234: <message
name="IDS_SETTINGS_NAME_ON_CREDIT_CARD" desc="">
description

https://codereview.chromium.org/2015463003/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:237: <message
name="IDS_SETTINGS_CREDIT_CARD_NUMBER" desc="">
description

https://codereview.chromium.org/2015463003/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:243: <message
name="IDS_SETTINGS_CREDIT_CARD_EXPIRATION_MONTH" desc="A11y label on the month
drop down to let users know the field corresponds to the expiration month">
nit: spell out a11y, dunno if translators know all our acronyms

https://codereview.chromium.org/2015463003/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:246: <message
name="IDS_SETTINGS_CREDIT_CARD_EXPIRATION_YEAR" desc="A11y label on the year
drop down to let users know the field corresponds to the expiration year">
same

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

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:7:
<link rel="import"
href="chrome://md-settings/passwords_and_forms_page/credit_card_edit_dialog.html">
alphabetize

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

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:127:
expirationMonth: date.getMonth() + 1,  // Months are 0 based.
open() takes a CreditCardEntry, in which these are strings, not numbers

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

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:7:
<link rel="import" href="chrome://md-settings/settings_dialog.html">
remember to make these relative to / after rebasing

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:42:
<paper-input id="numberInput" label="$i18n{creditCardNumber}"
indent off

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

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:6:
* @fileoverview 'password-edit-dialog' is the dialog that allows showing a
fix

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:21:
newTitle: {
nit: use shorthand "newTitle: String," for this and the below props

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:29:
editTitle: {
Do these really have to be separate variables? It looks like they always map to
hard-coded i18n strings.

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:43:
* |newTitle| or |editTitle|
nit: end with period

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:83:
this.title_ = item.guid ? this.editTitle : this.newTitle;
this.title_ = this.i18n(item.guid ? 'editTitle' : 'newTitle');

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:85:
this.selectedMonthIndex_ = parseInt(item.expirationMonth, 10) - 1;
it's weird that the expiration dates are Strings in the API, but whatever

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:139:
this.item.expirationMonth = '' + (monthIndex + 1);
it may be clearer to just give the paper-items string values "01" thru "12" and
use that as the attribute for selection, so you don't have to keep converting
between 0-indexed numbers and 1-indexed strings

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:159:
yearList.push('' + i);
i would prefer i.toString() so it's clear why you're doing this

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

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js:21:
item: {
same shorthand nits

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js
(right):

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:76:
ret.expirationMonth = now.getMonth() + 1;
strings?

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js
(right):

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:39:
newTitle_: 'new-title',
you shouldn't have to set this in a test

[hcarmona, 2016-05-31 21:13:17]

Patchset #2 (id:40001) has been deleted

[hcarmona, 2016-05-31 21:14:07]

https://codereview.chromium.org/2015463003/diff/20001/chrome/app/settings_str...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2015463003/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:234: <message
name="IDS_SETTINGS_NAME_ON_CREDIT_CARD" desc="">
On 2016/05/27 17:57:07, michaelpg wrote:
> description

Done.

https://codereview.chromium.org/2015463003/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:237: <message
name="IDS_SETTINGS_CREDIT_CARD_NUMBER" desc="">
On 2016/05/27 17:57:07, michaelpg wrote:
> description

Done.

https://codereview.chromium.org/2015463003/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:243: <message
name="IDS_SETTINGS_CREDIT_CARD_EXPIRATION_MONTH" desc="A11y label on the month
drop down to let users know the field corresponds to the expiration month">
On 2016/05/27 17:57:07, michaelpg wrote:
> nit: spell out a11y, dunno if translators know all our acronyms

Done.

https://codereview.chromium.org/2015463003/diff/20001/chrome/app/settings_str...
chrome/app/settings_strings.grdp:246: <message
name="IDS_SETTINGS_CREDIT_CARD_EXPIRATION_YEAR" desc="A11y label on the year
drop down to let users know the field corresponds to the expiration year">
On 2016/05/27 17:57:07, michaelpg wrote:
> same

Done.

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

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.html:7:
<link rel="import"
href="chrome://md-settings/passwords_and_forms_page/credit_card_edit_dialog.html">
On 2016/05/27 17:57:07, michaelpg wrote:
> alphabetize

Done.

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

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:127:
expirationMonth: date.getMonth() + 1,  // Months are 0 based.
On 2016/05/27 17:57:07, michaelpg wrote:
> open() takes a CreditCardEntry, in which these are strings, not numbers

Done.

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

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:7:
<link rel="import" href="chrome://md-settings/settings_dialog.html">
On 2016/05/27 17:57:07, michaelpg wrote:
> remember to make these relative to / after rebasing

Done.

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:42:
<paper-input id="numberInput" label="$i18n{creditCardNumber}"
On 2016/05/27 17:57:07, michaelpg wrote:
> indent off

Good eye. Fixed.

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

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:6:
* @fileoverview 'password-edit-dialog' is the dialog that allows showing a
On 2016/05/27 17:57:08, michaelpg wrote:
> fix

Done.

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:21:
newTitle: {
On 2016/05/27 17:57:08, michaelpg wrote:
> nit: use shorthand "newTitle: String," for this and the below props

Didn't know this shorthand existed. Will use more now. Thanks!

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:29:
editTitle: {
On 2016/05/27 17:57:08, michaelpg wrote:
> Do these really have to be separate variables? It looks like they always map
to
> hard-coded i18n strings.

Not anymore. I18nBehavior'd.

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:43:
* |newTitle| or |editTitle|
On 2016/05/27 17:57:08, michaelpg wrote:
> nit: end with period

Done.

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:83:
this.title_ = item.guid ? this.editTitle : this.newTitle;
On 2016/05/27 17:57:08, michaelpg wrote:
> this.title_ = this.i18n(item.guid ? 'editTitle' : 'newTitle');

Done.

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:85:
this.selectedMonthIndex_ = parseInt(item.expirationMonth, 10) - 1;
On 2016/05/27 17:57:07, michaelpg wrote:
> it's weird that the expiration dates are Strings in the API, but whatever

Acknowledged.

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:139:
this.item.expirationMonth = '' + (monthIndex + 1);
On 2016/05/27 17:57:08, michaelpg wrote:
> it may be clearer to just give the paper-items string values "01" thru "12"
and
> use that as the attribute for selection, so you don't have to keep converting
> between 0-indexed numbers and 1-indexed strings

Done. Did same for year.

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:159:
yearList.push('' + i);
On 2016/05/27 17:57:08, michaelpg wrote:
> i would prefer i.toString() so it's clear why you're doing this

Done. Everywhere.

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

https://codereview.chromium.org/2015463003/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/password_edit_dialog.js:21:
item: {
On 2016/05/27 17:57:08, michaelpg wrote:
> same shorthand nits

Done.

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js
(right):

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js:76:
ret.expirationMonth = now.getMonth() + 1;
On 2016/05/27 17:57:08, michaelpg wrote:
> strings?

Done.

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js
(right):

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:39:
newTitle_: 'new-title',
On 2016/05/27 17:57:08, michaelpg wrote:
> you shouldn't have to set this in a test

Removed, but I had to change |browserPreload| because my element is
dependent on |loadTimeData|. Is there any way to create a fake one for
the test? The only test I could find that was using mocha and
loatTimeData was loading chrome://md-settings

[dschuyler, 2016-06-01 23:19:06]

Just some sideline requests.

https://codereview.chromium.org/2015463003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html
(right):

https://codereview.chromium.org/2015463003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:31:
color: #737373;
Please use a var(--some-color); here.

https://codereview.chromium.org/2015463003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js
(right):

https://codereview.chromium.org/2015463003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:26:
* @type {string}
This @type is redundant since properties
get their type from polymer and it's already
a string.

(The {!Array<string>}} below is still good
because it adds information about what is
in the array).

[michaelpg, 2016-06-03 13:51:13]

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

[michaelpg, 2016-06-03 13:51:14]

michaelpg@chromium.org changed required reviewers:
+ michaelpg@chromium.org

[michaelpg, 2016-06-03 13:51:15]

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js
(right):

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:39:
newTitle_: 'new-title',
On 2016/05/31 21:14:06, Hector Carmona wrote:
> On 2016/05/27 17:57:08, michaelpg wrote:
> > you shouldn't have to set this in a test
> 
> Removed, but I had to change |browserPreload| because my element is
> dependent on |loadTimeData|. Is there any way to create a fake one for
> the test? The only test I could find that was using mocha and
> loatTimeData was loading chrome://md-settings

dbeam, want to weigh in? Question is how to make conditional i18n work inside a
test: stub out loadTimeData, load the full WebUI instead of just the element
under test, or make the UI take strings as variables instead of doing the i18n
itself?

Compare credit_card_edit_dialog.js versions 1 and 3.

[Dan Beam, 2016-06-03 21:44:44]

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js
(right):

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:39:
newTitle_: 'new-title',
On 2016/06/03 13:51:14, michaelpg wrote:
> On 2016/05/31 21:14:06, Hector Carmona wrote:
> > On 2016/05/27 17:57:08, michaelpg wrote:
> > > you shouldn't have to set this in a test
> > 
> > Removed, but I had to change |browserPreload| because my element is
> > dependent on |loadTimeData|. Is there any way to create a fake one for
> > the test? The only test I could find that was using mocha and
> > loatTimeData was loading chrome://md-settings
> 
> dbeam, want to weigh in? Question is how to make conditional i18n work inside
a
> test: stub out loadTimeData, load the full WebUI instead of just the element
> under test, or make the UI take strings as variables instead of doing the i18n
> itself?
> 
> Compare credit_card_edit_dialog.js versions 1 and 3.

I'm confused: why can't the element just do:

<link rel=import href=chrome://resources/js/load_time_data.html>
<link rel=import href=/strings.html>

if it depend on using loadTimeData?

[hcarmona, 2016-06-04 00:36:48]

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js
(right):

https://codereview.chromium.org/2015463003/diff/20001/chrome/test/data/webui/...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:39:
newTitle_: 'new-title',
On 2016/06/03 21:44:43, Dan Beam wrote:
> On 2016/06/03 13:51:14, michaelpg wrote:
> > On 2016/05/31 21:14:06, Hector Carmona wrote:
> > > On 2016/05/27 17:57:08, michaelpg wrote:
> > > > you shouldn't have to set this in a test
> > > 
> > > Removed, but I had to change |browserPreload| because my element is
> > > dependent on |loadTimeData|. Is there any way to create a fake one for
> > > the test? The only test I could find that was using mocha and
> > > loatTimeData was loading chrome://md-settings
> > 
> > dbeam, want to weigh in? Question is how to make conditional i18n work
inside
> a
> > test: stub out loadTimeData, load the full WebUI instead of just the element
> > under test, or make the UI take strings as variables instead of doing the
i18n
> > itself?
> > 
> > Compare credit_card_edit_dialog.js versions 1 and 3.
> 
> I'm confused: why can't the element just do:
> 
> <link rel=import href=chrome://resources/js/load_time_data.html>
> <link rel=import href=/strings.html>
> 
> if it depend on using loadTimeData?

Thanks guys, this helped me find a solution:
  Included load_time_data.js in the test.
  Added strings I care about to test (/strings.html doesn't exist)

https://codereview.chromium.org/2015463003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html
(right):

https://codereview.chromium.org/2015463003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.html:31:
color: #737373;
On 2016/06/01 23:19:06, dschuyler wrote:
> Please use a var(--some-color); here.

Done.

https://codereview.chromium.org/2015463003/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js
(right):

https://codereview.chromium.org/2015463003/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:26:
* @type {string}
On 2016/06/01 23:19:06, dschuyler wrote:
> This @type is redundant since properties
> get their type from polymer and it's already
> a string.
> 
> (The {!Array<string>}} below is still good
> because it adds information about what is
> in the array).

Good to know. Fixed.

[Dan Beam, 2016-06-04 01:00:26]

https://codereview.chromium.org/2015463003/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js
(right):

https://codereview.chromium.org/2015463003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:32:
* @type {!Array<string>}}
typo, why do these need to be strings?

https://codereview.chromium.org/2015463003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:71:
}
no curlies

https://codereview.chromium.org/2015463003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right):

https://codereview.chromium.org/2015463003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/passwords_and_forms_browsertest.js:222:
browsePreload: 'chrome://md-settings',
wait, this seems like a step in the wrong direction...

https://codereview.chromium.org/2015463003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js
(right):

https://codereview.chromium.org/2015463003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:45: };
this seems fine to me

[hcarmona, 2016-06-04 01:19:10]

https://codereview.chromium.org/2015463003/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js
(right):

https://codereview.chromium.org/2015463003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:32:
* @type {!Array<string>}}
On 2016/06/04 01:00:26, Dan Beam wrote:
> typo, why do these need to be strings?

API uses strings, I can refactor the API to use ints and fix this in another CL.
Fixed typo.

https://codereview.chromium.org/2015463003/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:71:
}
On 2016/06/04 01:00:26, Dan Beam wrote:
> no curlies

Done.

https://codereview.chromium.org/2015463003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/passwords_and_forms_browsertest.js (right):

https://codereview.chromium.org/2015463003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/passwords_and_forms_browsertest.js:222:
browsePreload: 'chrome://md-settings',
On 2016/06/04 01:00:26, Dan Beam wrote:
> wait, this seems like a step in the wrong direction...

Fixed the other test, forgot to update this URL. Fixed.

https://codereview.chromium.org/2015463003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js
(right):

https://codereview.chromium.org/2015463003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:45: };
On 2016/06/04 01:00:26, Dan Beam wrote:
> this seems fine to me

Acknowledged.

[michaelpg, 2016-06-06 17:00:47]

not thrilled with the amount of string manip here either, but overall lgtm

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js
(right):

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:173:
firstSelectableYear.textContent);
4-space indent

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:192:
firstSelectableYear.textContent);
same

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:206:
creditCardDialog.$.saveButton.click();
MockInteractions.tap(foo)

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:222:
done();
i'm being kind of mean, but save-credit-card could still fire *after*
iron-overlay-close, and the test will succeed. mind putting done inside
setTimeout with a comment that this ensures an invalid event is handled by the
test?

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:225:
creditCardDialog.$.cancelButton.click();
MockInteractions.tap(foo)

[hcarmona, 2016-06-06 19:03:07]

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js
(right):

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:173:
firstSelectableYear.textContent);
On 2016/06/06 17:00:47, michaelpg wrote:
> 4-space indent

Done.

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:192:
firstSelectableYear.textContent);
On 2016/06/06 17:00:47, michaelpg wrote:
> same

Done.

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:206:
creditCardDialog.$.saveButton.click();
On 2016/06/06 17:00:47, michaelpg wrote:
> MockInteractions.tap(foo)

Done.

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:222:
done();
On 2016/06/06 17:00:47, michaelpg wrote:
> i'm being kind of mean, but save-credit-card could still fire *after*
> iron-overlay-close, and the test will succeed. mind putting done inside
> setTimeout with a comment that this ensures an invalid event is handled by the
> test?

Sounds good, I've set the timeout to 100ms. Let me know if you want a
bigger timeout or if I should make this 0ms. I went with 100 so it can
run after anything else here.

https://codereview.chromium.org/2015463003/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:225:
creditCardDialog.$.cancelButton.click();
On 2016/06/06 17:00:47, michaelpg wrote:
> MockInteractions.tap(foo)

Done.

[hcarmona, 2016-06-06 21:01:15]

The CQ bit was checked by hcarmona@chromium.org

[hcarmona, 2016-06-06 21:01:16]

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

[commit-bot: I haz the power, 2016-06-06 21:01:51]

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

[commit-bot: I haz the power, 2016-06-06 21:37:17]

Description was changed from

==========
Add dialog to edit and save credit cards.

Tests included for dialog.
Screenshots in bug.

BUG=607348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add dialog to edit and save credit cards.

Tests included for dialog.
Screenshots in bug.

BUG=607348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-06-06 21:37:18]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-06-06 21:40:06]

Description was changed from

==========
Add dialog to edit and save credit cards.

Tests included for dialog.
Screenshots in bug.

BUG=607348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Add dialog to edit and save credit cards.

Tests included for dialog.
Screenshots in bug.

BUG=607348
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a02e2448d903564a3199edc570835c2eabdf12b7
Cr-Commit-Position: refs/heads/master@{#398133}
==========

[commit-bot: I haz the power, 2016-06-06 21:40:07]

Patchset 6 (id:??) landed as
https://crrev.com/a02e2448d903564a3199edc570835c2eabdf12b7
Cr-Commit-Position: refs/heads/master@{#398133}