Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2079853002:
Add Edit/Create Address Dialog to MD Settings. (Closed)
|
Created:
4 years, 6 months ago by hcarmona Modified:
4 years, 5 months ago Reviewers:
michaelpg CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-polymer_chromium.org, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, jlklein+watch-closure_chromium.org, michaelpg+watch-polymer_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, oshima+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, vitalyp+closure_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Edit/Create Address Dialog to MD Settings.
Change includes lots of tests.
Screenshots of dialog in bug.
BUG=607347
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/7b42531c9e81e485dadc15ee7320134c9aac202a
Cr-Commit-Position: refs/heads/master@{#402580}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Feedback #Patch Set 3 : Fix chromium_presubmit #
Total comments: 52
Patch Set 4 : spacing and rebase #
Total comments: 2
Patch Set 5 : feedback #
Total comments: 26
Patch Set 6 : Feedback #
Total comments: 1
Patch Set 7 : rebase and reset polymer_resources.grdp #Dependent Patchsets: Messages
Total messages: 22 (9 generated)
Description was changed from ========== Add Edit/Create Address Dialog to MD Settings. Change includes lots of tests. BUG=607347 ========== to ========== Add Edit/Create Address Dialog to MD Settings. Change includes lots of tests. BUG=607347 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Add Edit/Create Address Dialog to MD Settings. Change includes lots of tests. BUG=607347 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Add Edit/Create Address Dialog to MD Settings. Change includes lots of tests. BUG=607347 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
hcarmona@chromium.org changed reviewers: + michaelpg@chromium.org
PTAL, sorry it's a big CL. This is a complex dialog b/c each country has a different address format. About half of this CL is tests. I will add screenshots to the bug and update the description here once that's done. Thanks!
Description was changed from ========== Add Edit/Create Address Dialog to MD Settings. Change includes lots of tests. BUG=607347 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Add Edit/Create Address Dialog to MD Settings. Change includes lots of tests. Screenshots of dialog in bug. BUG=607347 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Will also update with some CSS fixes that Tom mentioned in the bug. https://codereview.chromium.org/2079853002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js (right): https://codereview.chromium.org/2079853002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:124: var resizable = /** @type {!{notifyResize: function()}} */(this.$.dialog); Spoke to Dan about these TODOs, I'll update with something better today.
Updated screenshots in bug. https://codereview.chromium.org/2079853002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js (right): https://codereview.chromium.org/2079853002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:124: var resizable = /** @type {!{notifyResize: function()}} */(this.$.dialog); On 2016/06/20 19:01:48, Hector Carmona wrote: > Spoke to Dan about these TODOs, I'll update with something better today. Per discussion w/ Dan, we updated this to ignore the missing properties and we want to update the closure compiler to better handle this.
Fix spacing between save/cancel and rebase to use cr-dialog.
https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js (right): https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:7: * saved password. nit: no indent https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:17: * @type {!chrome.autofillPrivate.AddressEntry} opt nit here & below: /** @private {foo} */ https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:127: (function() { indent? https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:200: * @param {!string} countryCode string is non-null by default, no need to specify ! but... should this be {string|undefined} given the property can be undefined? https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:230: cr.define('settings.AddressEditDialog', function() { maybe settings.address or something... settings.AddressEditDialog looks like a constructor https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:232: * Creates a wrapper against a single data member for an address. Sorry, there's just too much abstraction here for me to understand the point. I can't tell where the "value" for AddressSetting ultimately comes from; I see that it can get the value from its address, and set that value back to its address, but not how the value changes. IMO, AddressSetting is doing too many things: creating closures around getting/setting an address component, having its own value, describing what the UI for the field its value corresponds to should look like. Can I suggest breaking it out into: * a "SetAddressField" Command class to set the value on the address: https://sourcemaking.com/design_patterns/command * an "AddressComponentUI" to describe the UI given an AddressComponent (basically wrap AddressComponent plus isTextArea) Then each AddressComponentUI or maybe just the dialog itself can create a SetAddressField command. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:255: * @param {string} field c.aP.AddressField https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:262: return address.fullNames ? address.fullNames[0] : undefined; [comment on] why 0? https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:280: return undefined; assertNotReached? https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:287: * @param {string} field c.aP.AddressField https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:320: function AddressRow() { what's an AddressRow? why does it have settings? https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:335: /** @type {!Array<!settings.AddressEditDialog.AddressRow>} */ More descriptive name, please. Either way, I feel like this is supposed to be a 1:1 mapping of our dialog, which doesn't seem useful. If it's just a collection of descriptions of UI components, does it need to be its own class? https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:363: function CountryInformationHelper() {} s/InformationHelper/<something more descriptive> https://big.corp.google.com/~jmcmaster/testing/2016/06/episode-431-identifier... https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:367: * have a space between the default country and an alphabetized list of I don't understand this comment given that CountryEntry is just a single name and two-letter code. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:368: * countries. Country names should be internationalized. the IDL already specifies it's i18n'ed https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:369: * @param {function(!Array<!chrome.autofillPrivate.CountryEntry>)} callback maybe @return a Promise<!Array...> instead? getFoo implies to me you're returning something. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:398: chrome.autofillPrivate.getAddressComponents(countryCode || '', callback); countryCode is {string}, when is it falsy other than '' https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:86: this.$.addressSharedMenu.closeMenu(); menu. https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:29: {name: undefined, countryCode: undefined}, // Spacer. oh, I see. Should the browser/browser proxy really be responsible for this? Seems like a strictly UI concern. https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:61: editAddressTitle: 'add-title', edit-title? https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:312: test('verifyAddAddressDialog', function(done) { here & below: return the Promise instead of using a done callback, it's cleaner https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:314: FakeDataMaker.emptyAddressEntry()).then(function(dialog){ missing space before { https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:324: self.createAddressDialog_( return self.createAddressDialog_(... https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:325: FakeDataMaker.addressEntry()).then(function(dialog){ missing space before { https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:350: assertFalse(!!dialog.$.phoneInput.value); why not assertEqual('', dialog.$.phoneInput.value), isn't that what it should be? https://codereview.chromium.org/2079853002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2079853002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:417: this.ensureDisabledAndPopulateInput(); I think you've created a Turing-complete test case... Can we simplify this to not be recursive?
Patchset #5 (id:80001) has been deleted
Thanks for the great feedback! I've simplified things a bit. PTAL https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js (right): https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:7: * saved password. On 2016/06/22 00:25:13, michaelpg wrote: > nit: no indent Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:17: * @type {!chrome.autofillPrivate.AddressEntry} On 2016/06/22 00:25:13, michaelpg wrote: > opt nit here & below: /** @private {foo} */ Cool, done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:127: (function() { On 2016/06/22 00:25:13, michaelpg wrote: > indent? Left this un-indented so it stands out. I can indent if it makes more sense, but it's supposed to be analogous to an #ifdef. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:200: * @param {!string} countryCode On 2016/06/22 00:25:13, michaelpg wrote: > string is non-null by default, no need to specify ! > > but... should this be {string|undefined} given the property can be undefined? Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:230: cr.define('settings.AddressEditDialog', function() { On 2016/06/22 00:25:13, michaelpg wrote: > maybe settings.address or something... settings.AddressEditDialog looks like a > constructor Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:232: * Creates a wrapper against a single data member for an address. On 2016/06/22 00:25:14, michaelpg wrote: > Sorry, there's just too much abstraction here for me to understand the point. I > can't tell where the "value" for AddressSetting ultimately comes from; I see > that it can get the value from its address, and set that value back to its > address, but not how the value changes. > > IMO, AddressSetting is doing too many things: creating closures around > getting/setting an address component, having its own value, describing what the > UI for the field its value corresponds to should look like. Can I suggest > breaking it out into: > > * a "SetAddressField" Command class to set the value on the address: > https://sourcemaking.com/design_patterns/command > * an "AddressComponentUI" to describe the UI given an AddressComponent > (basically wrap AddressComponent plus isTextArea) > > Then each AddressComponentUI or maybe just the dialog itself can create a > SetAddressField command. Sounds good. Code looks simpler. Let me know what you think. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:255: * @param {string} field On 2016/06/22 00:25:13, michaelpg wrote: > c.aP.AddressField Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:262: return address.fullNames ? address.fullNames[0] : undefined; On 2016/06/22 00:25:13, michaelpg wrote: > [comment on] why 0? Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:280: return undefined; On 2016/06/22 00:25:13, michaelpg wrote: > assertNotReached? Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:287: * @param {string} field On 2016/06/22 00:25:13, michaelpg wrote: > c.aP.AddressField Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:320: function AddressRow() { On 2016/06/22 00:25:14, michaelpg wrote: > what's an AddressRow? why does it have settings? Acknowledged. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:335: /** @type {!Array<!settings.AddressEditDialog.AddressRow>} */ On 2016/06/22 00:25:13, michaelpg wrote: > More descriptive name, please. > > Either way, I feel like this is supposed to be a 1:1 mapping of our dialog, > which doesn't seem useful. If it's just a collection of descriptions of UI > components, does it need to be its own class? Acknowledged. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:363: function CountryInformationHelper() {} On 2016/06/22 00:25:14, michaelpg wrote: > s/InformationHelper/<something more descriptive> > https://big.corp.google.com/~jmcmaster/testing/2016/06/episode-431-identifier... Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:367: * have a space between the default country and an alphabetized list of On 2016/06/22 00:25:13, michaelpg wrote: > I don't understand this comment given that CountryEntry is just a single name > and two-letter code. Attempted to make comment clearer. WDYT? https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:368: * countries. Country names should be internationalized. On 2016/06/22 00:25:13, michaelpg wrote: > the IDL already specifies it's i18n'ed Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:369: * @param {function(!Array<!chrome.autofillPrivate.CountryEntry>)} callback On 2016/06/22 00:25:13, michaelpg wrote: > maybe @return a Promise<!Array...> instead? getFoo implies to me you're > returning something. Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:398: chrome.autofillPrivate.getAddressComponents(countryCode || '', callback); On 2016/06/22 00:25:14, michaelpg wrote: > countryCode is {string}, when is it falsy other than '' Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js (right): https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:86: this.$.addressSharedMenu.closeMenu(); On 2016/06/22 00:25:14, michaelpg wrote: > menu. Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:29: {name: undefined, countryCode: undefined}, // Spacer. On 2016/06/22 00:25:14, michaelpg wrote: > oh, I see. Should the browser/browser proxy really be responsible for this? > Seems like a strictly UI concern. Spacer is provided by API to keep the default separate from the alphabetized list of countries. I had it here to emulate the API return values, but the tests don't need the spacer. List is now shorter, but I kept US as first item b/c tests were assuming it as the default. Default is remembered per user, which is why the API provides it. https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:61: editAddressTitle: 'add-title', On 2016/06/22 00:25:14, michaelpg wrote: > edit-title? Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:312: test('verifyAddAddressDialog', function(done) { On 2016/06/22 00:25:14, michaelpg wrote: > here & below: return the Promise instead of using a done callback, it's cleaner Nice! Didn't know you could return a promise in the tests. https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:314: FakeDataMaker.emptyAddressEntry()).then(function(dialog){ On 2016/06/22 00:25:14, michaelpg wrote: > missing space before { Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:324: self.createAddressDialog_( On 2016/06/22 00:25:14, michaelpg wrote: > return self.createAddressDialog_(... Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:325: FakeDataMaker.addressEntry()).then(function(dialog){ On 2016/06/22 00:25:14, michaelpg wrote: > missing space before { Done. https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:350: assertFalse(!!dialog.$.phoneInput.value); On 2016/06/22 00:25:14, michaelpg wrote: > why not assertEqual('', dialog.$.phoneInput.value), isn't that what it should > be? Done. https://codereview.chromium.org/2079853002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2079853002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:417: this.ensureDisabledAndPopulateInput(); On 2016/06/22 00:25:14, michaelpg wrote: > I think you've created a Turing-complete test case... Can we simplify this to > not be recursive? Tests are now easier to read and not recursive :D
Just some small things. https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js (right): https://codereview.chromium.org/2079853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:127: (function() { On 2016/06/24 18:27:50, Hector Carmona wrote: > On 2016/06/22 00:25:13, michaelpg wrote: > > indent? > > Left this un-indented so it stands out. I can indent if it makes more > sense, but it's supposed to be analogous to an #ifdef. Acknowledged. https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2079853002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:312: test('verifyAddAddressDialog', function(done) { On 2016/06/24 18:27:51, Hector Carmona wrote: > On 2016/06/22 00:25:14, michaelpg wrote: > > here & below: return the Promise instead of using a done callback, it's > cleaner > > Nice! Didn't know you could return a promise in the tests. Yep. See last paragraph: https://www.chromium.org/developers/updating-webui-for-material-design/testin... https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html (right): https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html:4: <link rel="import" href="chrome://resources/html/i18n_behavior.html"> alphabetize https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js (right): https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:31: /** @type {!Array<!Array<!settings.address.AddressComponentUI>>} */ private https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:51: I18nBehavior, nit: behaviors before properties https://www.chromium.org/developers/web-development-style-guide https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:255: AddressComponentUI.getValue_ = function(address, field) { optional suggestion: make getValue_ and setValue_ members of the AddressComponentUI prototype so they can just use |this.address| and |this.component|, rather than passing address and field as parameters https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:282: * Gets a setter for the |field| value from the address. update comment (Sets...) https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:329: * @return {!Promise<!Array<!chrome.autofillPrivate.CountryEntry>>} callback nit: remove "callback" name https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:336: * @return {!Promise<!chrome.autofillPrivate.AddressComponents>} callback same https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:28: return { If this doesn't *have* to be synchronous, use a real Promise instead of faking it: return new Promise(function(resolve, reject) { resolve([ {name: ...}, ... ]); }); and if it does have to be synchronous, fix the test so it doesn't have to be synchronous :-P https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:50: * @param {!Array<object>} items capitalize Object https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:51: * @param {!function(!object):!Promise} loopBody capitalize Object https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:62: loopBody(item).then(next); i think this can just be then(loop) and we don't need a "next" variable https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:73: * Resolves the promise after the element throws the expected event. Will add nit: s/throws/fires, throws implies an error :-) https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:433: return asyncForEach(testElements, function(element) { Thank you! Much nicer.
https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html (right): https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.html:4: <link rel="import" href="chrome://resources/html/i18n_behavior.html"> On 2016/06/27 21:29:18, michaelpg wrote: > alphabetize Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js (right): https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:31: /** @type {!Array<!Array<!settings.address.AddressComponentUI>>} */ On 2016/06/27 21:29:18, michaelpg wrote: > private Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:51: I18nBehavior, On 2016/06/27 21:29:18, michaelpg wrote: > nit: behaviors before properties > https://www.chromium.org/developers/web-development-style-guide Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:255: AddressComponentUI.getValue_ = function(address, field) { On 2016/06/27 21:29:18, michaelpg wrote: > optional suggestion: make getValue_ and setValue_ members of the > AddressComponentUI prototype so they can just use |this.address| and > |this.component|, rather than passing address and field as parameters Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:282: * Gets a setter for the |field| value from the address. On 2016/06/27 21:29:18, michaelpg wrote: > update comment (Sets...) Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:329: * @return {!Promise<!Array<!chrome.autofillPrivate.CountryEntry>>} callback On 2016/06/27 21:29:18, michaelpg wrote: > nit: remove "callback" name Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:336: * @return {!Promise<!chrome.autofillPrivate.AddressComponents>} callback On 2016/06/27 21:29:18, michaelpg wrote: > same Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:28: return { On 2016/06/27 21:29:18, michaelpg wrote: > If this doesn't *have* to be synchronous, use a real Promise instead of faking > it: > > return new Promise(function(resolve, reject) { > resolve([ > {name: ...}, > ... > ]); > }); > > and if it does have to be synchronous, fix the test so it doesn't have to be > synchronous :-P This is now async. In order to make this async, I added a setTimeout(0) at time of creation to allow the countries' promise to resolve. https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:50: * @param {!Array<object>} items On 2016/06/27 21:29:18, michaelpg wrote: > capitalize Object Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:51: * @param {!function(!object):!Promise} loopBody On 2016/06/27 21:29:19, michaelpg wrote: > capitalize Object Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:62: loopBody(item).then(next); On 2016/06/27 21:29:18, michaelpg wrote: > i think this can just be then(loop) and we don't need a "next" variable Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:73: * Resolves the promise after the element throws the expected event. Will add On 2016/06/27 21:29:19, michaelpg wrote: > nit: s/throws/fires, throws implies an error :-) Done. https://codereview.chromium.org/2079853002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:433: return asyncForEach(testElements, function(element) { On 2016/06/27 21:29:18, michaelpg wrote: > Thank you! Much nicer. Thanks!
lgtm! https://codereview.chromium.org/2079853002/diff/120001/ui/webui/resources/pol... File ui/webui/resources/polymer_resources.grdp (right): https://codereview.chromium.org/2079853002/diff/120001/ui/webui/resources/pol... ui/webui/resources/polymer_resources.grdp:512: <structure name="IDR_POLYMER_1_0_PAPER_ICON_BUTTON_PAPER_ICON_BUTTON_HTML" this isn't wrong, but don't include in this CL
The CQ bit was checked by hcarmona@chromium.org
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/2079853002/#ps140001 (title: "rebase and reset polymer_resources.grdp")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add Edit/Create Address Dialog to MD Settings. Change includes lots of tests. Screenshots of dialog in bug. BUG=607347 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Add Edit/Create Address Dialog to MD Settings. Change includes lots of tests. Screenshots of dialog in bug. BUG=607347 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add Edit/Create Address Dialog to MD Settings. Change includes lots of tests. Screenshots of dialog in bug. BUG=607347 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Add Edit/Create Address Dialog to MD Settings. Change includes lots of tests. Screenshots of dialog in bug. BUG=607347 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/7b42531c9e81e485dadc15ee7320134c9aac202a Cr-Commit-Position: refs/heads/master@{#402580} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7b42531c9e81e485dadc15ee7320134c9aac202a Cr-Commit-Position: refs/heads/master@{#402580}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:140001) has been created in https://codereview.chromium.org/2107093002/ by shans@chromium.org. The reason for reverting is: New tests are failing on https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2.... |
