1379533 - [Form Autofill] Support two fieldDetails array for Address and Credit Card feature in FormAutofillHandler.

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Sean Lee [:seanlee][:weilonge]]

FormAutofillHandler needs to contain the field details arrays for both Address and Credit Card. This new design helps the consumer to get all Address or Credit Card fields easliy, e.g. preview, submit form, fill form.

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1379533 - Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler.

Review commit: https://reviewboard.mozilla.org/r/155628/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155628/

Comment 2

[Steve Chung [:steveck]]

Comment on attachment 8884744 [details]
Bug 1379533 - Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler.

https://reviewboard.mozilla.org/r/155628/#review161054

I only have a question about how we deal with the original fieldDetails, otherwise it looks fine to me.

::: browser/extensions/formautofill/FormAutofillContent.jsm:482
(Diff revision 1)
> -    if (formHandler.fieldDetails.length < AUTOFILL_FIELDS_THRESHOLD) {
> -      this.log.debug("Ignoring form since it has only", formHandler.fieldDetails.length,
> +    if (formHandler.isValidAddressForm) {
> +      formHandler.addressFieldDetails.forEach(
> +        detail => this._markAsAutofillField(detail.elementWeakRef.get())
> +      );
> +    } else {
> +      this.log.debug("Ignoring address form since it has only",

nit: "Ignoring address related fields since it has only"

::: browser/extensions/formautofill/FormAutofillContent.jsm:492
(Diff revision 1)
> -    formHandler.fieldDetails.forEach(detail =>
> -      this._markAsAutofillField(detail.elementWeakRef.get())
> +    if (formHandler.isValidCreditCardForm) {
> +      formHandler.creditCardFieldDetails.forEach(
> +        detail => this._markAsAutofillField(detail.elementWeakRef.get())
> -    );
> +      );
> +    } else {
> +      this.log.debug("Ignoring credit card form since it's without credit card number field");

nit: "Ignoring credit card related fields since it's without credit card number field"

::: browser/extensions/formautofill/FormAutofillHandler.jsm:57
(Diff revision 1)
>     * the same exact combination of these values.
>     *
>     * A direct reference to the associated element cannot be sent to the user
>     * interface because processing may be done in the parent process.
>     */
>    fieldDetails: null,

Will you plan to remove this later? It seems not necessray to keep all details in another `fieldDetails` separately.

::: browser/extensions/formautofill/test/unit/test_collectFormFields.js:32
(Diff revision 1)
> +      creditCard: false,
> +    },
>      ids: ["given-name", "family-name", "street-addr", "city", "country", "email", "tel"],
>    },
>    {
>      description: "Form with autocomplete properties and 1 token",

Maybe we can enrich the description like "An address and credit card form with ..." since you also add some credit card fields in this test. I think it might not necessary to have a valid address and credit card form test in later part because most of the tests here are valid forms.

::: browser/extensions/formautofill/test/unit/test_collectFormFields.js:175
(Diff revision 1)
>        let doc = MockDocument.createTestDocument("http://localhost:8080/test/",
>                                                  testcase.document);
>        let form = doc.querySelector("form");
>        let formLike = FormLikeFactory.createFromForm(form);
>  
> -      testcase.fieldDetails.forEach((detail, index) => {
> +      Array.of(

nit: maybe [...testcase.addressFieldDetails, ...testcase.creditCardFieldDetails] ? But it's still quite long anyway...

Comment 3

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8884744 [details]
Bug 1379533 - Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155628/diff/1-2/

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8884744 [details]
Bug 1379533 - Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler.

https://reviewboard.mozilla.org/r/155628/#review161054

> Will you plan to remove this later? It seems not necessray to keep all details in another `fieldDetails` separately.

I would prefer to remove this after a deep discussion about the usage of FormAutofillHandler.fieldDetails. A follow-up bug is an option.

> Maybe we can enrich the description like "An address and credit card form with ..." since you also add some credit card fields in this test. I think it might not necessary to have a valid address and credit card form test in later part because most of the tests here are valid forms.

The description is modified but also for "2 token" one.
The valid form test is to verify the getters of `isValidAddressForm` and `isValidCreditCardForm`.
I think it's necessary to verify the case of under 3 fields for address and the case without `cc-number` for credit card.

> nit: maybe [...testcase.addressFieldDetails, ...testcase.creditCardFieldDetails] ? But it's still quite long anyway...

I ever consider this, but `Array.of` seems more clear to me. :P

Comment 5

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8884744 [details]
Bug 1379533 - Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler.

https://reviewboard.mozilla.org/r/155628/#review162808

Overall looks good.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:16
(Diff revision 2)
>  const ADDRESS_REFERENCES = "chrome://formautofill/content/addressReferences.js";
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  this.FormAutofillUtils = {
> +  get AUTOFILL_FIELDS_THRESHOLD() { return 3; },

Just curious why it's not simply a constant variable?

::: browser/extensions/formautofill/test/unit/test_collectFormFields.js:204
(Diff revision 2)
> +      Assert.equal(handler.isValidAddressForm, testcase.isValidForm.address, "Valid Address Form");
> +      Assert.equal(handler.isValidCreditCardForm, testcase.isValidForm.creditCard, "Valid Credit Card Form");

nit: The meesage misled me that the `assert` expects the form is always valid but it's actually for checking whether or not it's valid. Since the code is self-descriptive, I feel we don't need that message.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8884744 [details]
Bug 1379533 - Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler.

https://reviewboard.mozilla.org/r/155628/#review161054

> I would prefer to remove this after a deep discussion about the usage of FormAutofillHandler.fieldDetails. A follow-up bug is an option.

I agree that we should remove this if we're moving to have two separate address and credit card structures. It seems like leaving it around will simply be tech debt unless someone has a valid reason for needing it. i.e. I think we should either have one fieldDetails which we filter for addresses and credit cards or we should have two separate structures but I don't think a mix of both approches is good off the top of my head.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8884744 [details]
Bug 1379533 - Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler.

https://reviewboard.mozilla.org/r/155628/#review163040

::: browser/extensions/formautofill/FormAutofillHandler.jsm:59
(Diff revision 2)
> +  addressFieldDetails: null,
> +
> +  creditCardFieldDetails: null,

These needs JSDoc comments so explain what they are and how they differ from `fieldDetails`

::: browser/extensions/formautofill/FormAutofillHandler.jsm:111
(Diff revision 2)
> +    this.addressFieldDetails = this.fieldDetails.filter(
> +      detail => FormAutofillUtils.isAddressField(detail.fieldName)
> +    );
> +    this.creditCardFieldDetails = this.fieldDetails.filter(
> +      detail => FormAutofillUtils.isCreditCardField(detail.fieldName)
> +    );

To ensure `addressFieldDetails` and `creditCardFieldDetails` stay synchronized with `fieldDetails` changes you could move these two lines to a `fieldDetails` setter.

You could also have `addressFieldDetails` and `creditCardFieldDetails` be read-only by having them be getters which return private variables computed by the `fieldDetails` setter.

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8884744 [details]
Bug 1379533 - Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler.

https://reviewboard.mozilla.org/r/155628/#review162808

> Just curious why it's not simply a constant variable?

If you mean the below snippet, `AUTOFILL_FIELDS_THRESHOLD` is not read-only.
```
this.FormAutofillUtils = {
  AUTOFILL_FIELDS_THRESHOLD: 3,
}
```

> nit: The meesage misled me that the `assert` expects the form is always valid but it's actually for checking whether or not it's valid. Since the code is self-descriptive, I feel we don't need that message.

Since we can not understand the exact meaning in log, I add the message here. For less confusing, I add "Checking" at the end of these two strings.

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8884744 [details]
Bug 1379533 - Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155628/diff/2-3/

Comment 10

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8884744 [details]
Bug 1379533 - Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler.

https://reviewboard.mozilla.org/r/155628/#review163040

> To ensure `addressFieldDetails` and `creditCardFieldDetails` stay synchronized with `fieldDetails` changes you could move these two lines to a `fieldDetails` setter.
> 
> You could also have `addressFieldDetails` and `creditCardFieldDetails` be read-only by having them be getters which return private variables computed by the `fieldDetails` setter.

After the offline discussion with MattN, we can land the current patch without `fieldDetails` setter design. We can discuss this idea in a follow-up bug.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af358ff04adc
Support the array of addressFieldDetails and creditCardFieldDetails in FormAutofillHandler. r=lchang,steveck

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/af358ff04adc