[MD Settings] Lazy create dialogs in Passwords and Autofill section.

[MD Settings] Lazy create dialogs in Passwords and Autofill section.

BUG=628833
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[hcarmona, 2016-07-18 20:53:50]

Description was changed from

==========
[MD Settings] Lazy create dialogs in Passwords and Autofill section.

BUG=628833
==========

to

==========
[MD Settings] Lazy create dialogs in Passwords and Autofill section.

BUG=628833
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[hcarmona, 2016-07-18 20:53:54]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 20:55:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hcarmona, 2016-07-18 20:57:03]

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

[hcarmona, 2016-07-18 20:57:04]

PTAL

[dpapad, 2016-07-18 21:20:23]

Just a few comments to make the lazy showing/hiding more aligned with other
places where we already do that.

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

https://codereview.chromium.org/2154263004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:31:
addressEditDialogShown_: Boolean,
Nit: Since, this boolean is causing the dialog to be shown/hidden, it seems a
bit more intuitive to name it in a way that reflects that fact. How about
renaming
showingAddreesEditDialog_ or shouldShowAddressEditDialog_?

Current naming does not make it clear that this boolean controls the visibility
of the dialog (same for editCreditCardDialogShown_).

https://codereview.chromium.org/2154263004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:86:
this.$$('settings-address-edit-dialog').open({});
When we lazily create dialogs, it means that they are only attached to the DOM
when they should be showing. This means that you can move the open() call inside
the dialog's attached() method and remove lines 85-87 from here (see example
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search...).

https://codereview.chromium.org/2154263004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:102:
this.$$('settings-address-edit-dialog').open(address);
Same comment as above. Let the attached() method do the open() call. Also since
you need to pass data to the dialog before opening, the more Polymer-y way to do
this is by assigning a Polymer property prior to flipping the boolean, see
example at [1] and [2]. This eliminates the need to have to override the open()
method to accept a parameter.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif...
[2]
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif...

[commit-bot: I haz the power, 2016-07-18 22:49:29]

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

[commit-bot: I haz the power, 2016-07-18 22:49:29]

Dry run: This issue passed the CQ dry run.

[hcarmona, 2016-07-19 01:14:20]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 01:15:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hcarmona, 2016-07-19 01:16:04]

Thanks for the feedback! The CL is a bit larger now, but things look
much cleaner. Had to delete a test case that no longer made sense.

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

https://codereview.chromium.org/2154263004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:31:
addressEditDialogShown_: Boolean,
On 2016/07/18 21:20:23, dpapad wrote:
> Nit: Since, this boolean is causing the dialog to be shown/hidden, it seems a
> bit more intuitive to name it in a way that reflects that fact. How about
> renaming
> showingAddreesEditDialog_ or shouldShowAddressEditDialog_?
> 
> Current naming does not make it clear that this boolean controls the
visibility
> of the dialog (same for editCreditCardDialogShown_).

Changed slightly since the dialog is only shown when data is bound.

https://codereview.chromium.org/2154263004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:86:
this.$$('settings-address-edit-dialog').open({});
On 2016/07/18 21:20:23, dpapad wrote:
> When we lazily create dialogs, it means that they are only attached to the DOM
> when they should be showing. This means that you can move the open() call
inside
> the dialog's attached() method and remove lines 85-87 from here (see example
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/search...).

Done.

https://codereview.chromium.org/2154263004/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:102:
this.$$('settings-address-edit-dialog').open(address);
On 2016/07/18 21:20:23, dpapad wrote:
> Same comment as above. Let the attached() method do the open() call. Also
since
> you need to pass data to the dialog before opening, the more Polymer-y way to
do
> this is by assigning a Polymer property prior to flipping the boolean, see
> example at [1] and [2]. This eliminates the need to have to override the
open()
> method to accept a parameter.
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif...
> [2]
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif...

Done.

[commit-bot: I haz the power, 2016-07-19 03:19:26]

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

[commit-bot: I haz the power, 2016-07-19 03:19:27]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-07-19 17:42:09]

Indeed it looks much cleaner than before! Few more questions/comments.

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

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:77:
this.countryCode_ = this.address.countryCode;
The way the dialog is opened currently is fairly indirect. Either
updateAddressWrapper_() calls open(), or onUpdateCountryCode_() is triggered
here which calls updateAddressWrapper_().

Can we make this more explicit by moving the open() call here instead?

if (this.countryCode_ == this.address.countryCode)
  this.updateAddressWrapper_();
else
  this.countryCode_ = this.address.countryCode;

// Bonus, there is no need to check if self.$.dialog.opened anymore,
// the dialog is always closed  at this point.
self.$.dialog.open();

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:106:
var components = [];
Nit: You can replace lines 106-112 as follows.

wrapper.push(component.row.map(function(c) {
  return new settings.address.AddressComponentUI(self.address, c);
}));

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

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:25:
* The address being edited.
Nit: Let's add a short comment as follows
"Assigning a non-null value triggers the add/edit address dialog."

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:26:
* @private {chrome.autofillPrivate.AddressEntry | null}
@private {?chrome.autofillPrivate.AddressEntry}

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:37:
* The credit card being edited.
Nit: Same comment here
"Assigning a non-null value triggers the credit card dialog."

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:38:
* @private {chrome.autofillPrivate.CreditCardEntry | null}
Same here.

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

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:63:
attached: function() {
I don't understand why |item_| is necessary. Can you explain?

My understanding is that if this is the "add" case, |creditCard_| is undefined
here. If this is the "edit" case, |creditCard_| is already assigned a value
here. Which means that all the logic in onCreditCardSet_() can be moved here and
the HTML could simply reference |creditCard| instead of |item_|.

[dpapad, 2016-07-19 19:15:22]

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

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:63:
attached: function() {
On 2016/07/19 at 17:42:09, dpapad wrote:
> I don't understand why |item_| is necessary. Can you explain?
> 
> My understanding is that if this is the "add" case, |creditCard_| is undefined
here. If this is the "edit" case, |creditCard_| is already assigned a value
here. Which means that all the logic in onCreditCardSet_() can be moved here and
the HTML could simply reference |creditCard| instead of |item_|.

Actually after re-reading the code, I understand now the necessity of |item_|
existence. The remaining of my comment (about moving the logic here instead of
observing |creditCard_| still holds though.

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:102:
this.creditCard.expirationMonth = '0' + this.creditCard.expirationMonth;
Nit: Let's either use |creditCard| the parameter, or |this.creditCard| the
member variable in this method (they both point to the same object).

[hcarmona, 2016-07-19 23:26:36]

The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 23:27:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hcarmona, 2016-07-19 23:33:06]

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

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:77:
this.countryCode_ = this.address.countryCode;
On 2016/07/19 17:42:09, dpapad wrote:
> The way the dialog is opened currently is fairly indirect. Either
> updateAddressWrapper_() calls open(), or onUpdateCountryCode_() is triggered
> here which calls updateAddressWrapper_().
> 
> Can we make this more explicit by moving the open() call here instead?
>
> if (this.countryCode_ == this.address.countryCode)
>   this.updateAddressWrapper_();
> else
>   this.countryCode_ = this.address.countryCode;
> 
> // Bonus, there is no need to check if self.$.dialog.opened anymore,
> // the dialog is always closed  at this point.
> self.$.dialog.open();

Moving the open call here will cause flicker. I tried making it async,
but that's not enough. This is because most of this dialog is country
specific, so must be created when the country is selected.

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/address_edit_dialog.js:106:
var components = [];
On 2016/07/19 17:42:09, dpapad wrote:
> Nit: You can replace lines 106-112 as follows.
> 
> wrapper.push(component.row.map(function(c) {
>   return new settings.address.AddressComponentUI(self.address, c);
> }));

Nice! Made even smaller.

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

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:25:
* The address being edited.
On 2016/07/19 17:42:09, dpapad wrote:
> Nit: Let's add a short comment as follows
> "Assigning a non-null value triggers the add/edit address dialog."

Done.

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:26:
* @private {chrome.autofillPrivate.AddressEntry | null}
On 2016/07/19 17:42:09, dpapad wrote:
> @private {?chrome.autofillPrivate.AddressEntry}

Done.

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:37:
* The credit card being edited.
On 2016/07/19 17:42:09, dpapad wrote:
> Nit: Same comment here
> "Assigning a non-null value triggers the credit card dialog."

Done.

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:38:
* @private {chrome.autofillPrivate.CreditCardEntry | null}
On 2016/07/19 17:42:09, dpapad wrote:
> Same here.

Done.

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

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:63:
attached: function() {
On 2016/07/19 19:15:22, dpapad wrote:
> On 2016/07/19 at 17:42:09, dpapad wrote:
> > I don't understand why |item_| is necessary. Can you explain?
> > 
> > My understanding is that if this is the "add" case, |creditCard_| is
undefined
> here. If this is the "edit" case, |creditCard_| is already assigned a value
> here. Which means that all the logic in onCreditCardSet_() can be moved here
and
> the HTML could simply reference |creditCard| instead of |item_|.
> 
> Actually after re-reading the code, I understand now the necessity of |item_|
> existence. The remaining of my comment (about moving the logic here instead of
> observing |creditCard_| still holds though.

Cleaned up to remove |item_|, I just added the properties it needed in
order to avoid all the listeners.

https://codereview.chromium.org/2154263004/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/credit_card_edit_dialog.js:102:
this.creditCard.expirationMonth = '0' + this.creditCard.expirationMonth;
On 2016/07/19 19:15:22, dpapad wrote:
> Nit: Let's either use |creditCard| the parameter, or |this.creditCard| the
> member variable in this method (they both point to the same object).

Moved to top and no more local variable.

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

https://codereview.chromium.org/2154263004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:113:
if (Polymer.dom(e).rootTarget == expectedDialog)
Necessary b/c closing a dropdown also triggers |iron-overlay-closed|.

[dpapad, 2016-07-19 23:52:17]

LGTM!

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

https://codereview.chromium.org/2154263004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:113:
if (Polymer.dom(e).rootTarget == expectedDialog)
On 2016/07/19 23:33:06, Hector Carmona wrote:
> Necessary b/c closing a dropdown also triggers |iron-overlay-closed|.

Understood. Would the following (shorter) variation also work (here and
elsewhere)?

var expectedDialog = this.$$('settings-address-edit-dialog');
if (Polymer.dom(e).localTarget == expectedDialog)
  this.activeAddress = null;
 
BTW, this will not be necessary when/if we migrate to native <dialog>, since the
name of the event becomes 'close' instead of 'iron-overlay-closed'.

[hcarmona, 2016-07-20 00:07:51]

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

https://codereview.chromium.org/2154263004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:113:
if (Polymer.dom(e).rootTarget == expectedDialog)
On 2016/07/19 23:52:17, dpapad wrote:
> On 2016/07/19 23:33:06, Hector Carmona wrote:
> > Necessary b/c closing a dropdown also triggers |iron-overlay-closed|.
> 
> Understood. Would the following (shorter) variation also work (here and
> elsewhere)?
> 
> var expectedDialog = this.$$('settings-address-edit-dialog');
> if (Polymer.dom(e).localTarget == expectedDialog)
>   this.activeAddress = null;
>  
> BTW, this will not be necessary when/if we migrate to native <dialog>, since
the
> name of the event becomes 'close' instead of 'iron-overlay-closed'.

Unfortunately the localTarget is the same for both the dialog closing
and the drop down, so it can't be used to distinguish the events.

[dpapad, 2016-07-20 00:10:39]

On 2016/07/20 at 00:07:51, hcarmona wrote:
>
https://codereview.chromium.org/2154263004/diff/40001/chrome/browser/resource...
> File
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js
(right):
> 
>
https://codereview.chromium.org/2154263004/diff/40001/chrome/browser/resource...
>
chrome/browser/resources/settings/passwords_and_forms_page/autofill_section.js:113:
if (Polymer.dom(e).rootTarget == expectedDialog)
> On 2016/07/19 23:52:17, dpapad wrote:
> > On 2016/07/19 23:33:06, Hector Carmona wrote:
> > > Necessary b/c closing a dropdown also triggers |iron-overlay-closed|.
> > 
> > Understood. Would the following (shorter) variation also work (here and
> > elsewhere)?
> > 
> > var expectedDialog = this.$$('settings-address-edit-dialog');
> > if (Polymer.dom(e).localTarget == expectedDialog)
> >   this.activeAddress = null;
> >  
> > BTW, this will not be necessary when/if we migrate to native <dialog>, since
the
> > name of the event becomes 'close' instead of 'iron-overlay-closed'.
> 
> Unfortunately the localTarget is the same for both the dialog closing
> and the drop down, so it can't be used to distinguish the events.

Ah, got it, makes sense now.

[commit-bot: I haz the power, 2016-07-20 01:17:40]

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

[commit-bot: I haz the power, 2016-07-20 01:17:41]

Dry run: This issue passed the CQ dry run.

[hcarmona, 2016-07-20 01:18:36]

The CQ bit was checked by hcarmona@chromium.org

[commit-bot: I haz the power, 2016-07-20 01:19:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 01:37:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-20 01:40:05]

Description was changed from

==========
[MD Settings] Lazy create dialogs in Passwords and Autofill section.

BUG=628833
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD Settings] Lazy create dialogs in Passwords and Autofill section.

BUG=628833
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

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

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