1415073 - [Form Autofill] Support the multiple section records in the `FormAutofillHandler.createRecords` process

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P3)

Description

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

`FormAutofillHandler.createRecords` process should provide the result with multiple Address and CreditCard records within sections in the form.

The result of `FormAutofillHandler.createRecords` will look like:

let result = {
  address: [{
    guid: filledRecordGUID
    record: {...},
    untouchedFields: [...],
  },{
    ...
  }],
  creditCard: [{
    guid: filledRecordGUID
    record: {...},
    untouchedFields: [...],
  }],
}

The related process could be changed for the multiple section:
FormAutofillHandler.createRecords()
FormAutofillContent.notify()
FormAutofillContent._onFormSubmit()
FormAutofillParent._onFormSubmit()

Comment 1

[Ray Lin[:ralin]]

Attachment: Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

TODO: fix test cases.

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

Comment 2

[Ray Lin[:ralin]]

Some notes:
1. doorhanger show should aggregate in a promise chain
2. telemetry should trigger conditionally, e.g. only record when a form with one section, until we can tell the data is representative for all cases.
3. how the section order would affect the merging strategy if multiple sections are auto-filled with the same profile.

Comment 3

[Ray Lin[:ralin]]

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

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

Comment 4

[Ray Lin[:ralin]]

(In reply to Ray Lin[:ralin] from comment #2)
> Some notes:
> 1. doorhanger show should aggregate in a promise chain
In the patch, now the doorhanger will present respectively in order.

> 2. telemetry should trigger conditionally, e.g. only record when a form with
> one section, until we can tell the data is representative for all cases.
Any irrational case will invalidate filling time to ensure the data was recorded on the same basis.  

> 3. how the section order would affect the merging strategy if multiple
> sections are auto-filled with the same profile.
Bug 1415022 should address this problem. Nonetheless, with this patch all the merges will be processed by first-come, first-served strategy, user is still able to decide update or not except no extra information on doorhanger.

Comment 5

[Ray Lin[:ralin]]

BTW, I'll add test cases soon later in a separate commit. Thanks.

Comment 6

[Ray Lin[:ralin]]

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

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

Comment 7

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

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

https://reviewboard.mozilla.org/r/201480/#review209256

::: browser/extensions/formautofill/FormAutofillParent.jsm:551
(Diff revision 3)
> -      this._onCreditCardSubmit(creditCard, target, timeStartedFillingMS);
> +    // Transmit the telemetry immediately in the meantime form submitted, and handle these pending
> +    // doorhanger at a later.
> +    const pendingDoorhangers = [
> +      ...address.map(addrRecord => this._onAddressSubmit(addrRecord, target, timeStartedFillingMS)),
> +      ...creditCard.map(ccRecord => this._onCreditCardSubmit(ccRecord, target, timeStartedFillingMS)),
> +    ].filter(pendingDorrhanger => !!pendingDorrhanger && typeof pendingDorrhanger == "function");

In the original design, Credit Card and Address door hangers will be in the awesome bar at the same and won't block to each other.

Can you split Credit Card and Address door hangers in their own promise chain?

This can benfit users to choose their desicions for Credit Card or Address separately.

BTW, Credit Card door hanger icons is the same as Address one. We can discuss it in a separate bug.

Comment 8

[Ray Lin[:ralin]]

Attachment: Bug 1415073 - Update the unit tests accordingly with new submission records.

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

Comment 9

[Ray Lin[:ralin]]

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

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

Comment 10

[Ray Lin[:ralin]]

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

https://reviewboard.mozilla.org/r/201480/#review209256

> In the original design, Credit Card and Address door hangers will be in the awesome bar at the same and won't block to each other.
> 
> Can you split Credit Card and Address door hangers in their own promise chain?
> 
> This can benfit users to choose their desicions for Credit Card or Address separately.
> 
> BTW, Credit Card door hanger icons is the same as Address one. We can discuss it in a separate bug.

Thanks for pointing this out, the doorhangers are pending in two separate promise chains now. 

> BTW, Credit Card door hanger icons is the same as Address one. We can discuss it in a separate bug.

Agree, or maybe Steve know more context if there's any reason we make them identical.

Comment 11

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

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

https://reviewboard.mozilla.org/r/201480/#review209618

::: browser/extensions/formautofill/FormAutofillParent.jsm:552
(Diff revision 4)
> +    // doorhangers at a later.
> +    await Promise.all([
> +      address.map(addrRecord => this._onAddressSubmit(addrRecord, target, timeStartedFillingMS)),
> +      creditCard.map(ccRecord => this._onCreditCardSubmit(ccRecord, target, timeStartedFillingMS)),
> +    ].map(pendingDoorhangers => {
> +      return pendingDoorhangers.filter(pendingDorrhanger => !!pendingDorrhanger &&

nit: pendingDorrhanger -> pendingDoorhanger

Comment 12

[Ray Lin[:ralin]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #11)
> Comment on attachment 8930342 [details]
> Bug 1415073 - Refactor records structure of form autofill submission to
> adapt multiple sections.
> 
> https://reviewboard.mozilla.org/r/201480/#review209618
> 
> ::: browser/extensions/formautofill/FormAutofillParent.jsm:552
> (Diff revision 4)
> > +    // doorhangers at a later.
> > +    await Promise.all([
> > +      address.map(addrRecord => this._onAddressSubmit(addrRecord, target, timeStartedFillingMS)),
> > +      creditCard.map(ccRecord => this._onCreditCardSubmit(ccRecord, target, timeStartedFillingMS)),
> > +    ].map(pendingDoorhangers => {
> > +      return pendingDoorhangers.filter(pendingDorrhanger => !!pendingDorrhanger &&
> 
> nit: pendingDorrhanger -> pendingDoorhanger

oops, what a folly typo...I'll fix it right away. Thanks.

Comment 13

[Ray Lin[:ralin]]

Sorry to ask that, any other issues having the review request cancelled except for the typo? I'd like to fix them at once to avoid spam. Thanks :D

Comment 14

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

Sorry, I remove the review flag accidentally. Please set the flag again. Thank you.

Comment 15

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

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

https://reviewboard.mozilla.org/r/201480/#review209674

BTW, the patch looks good to me. I give it r+ once I got the request. Thank you.

::: browser/extensions/formautofill/FormAutofillParent.jsm:543
(Diff revision 4)
>      let {profile: {address, creditCard}, timeStartedFillingMS} = data;
>  
> -    if (address) {
> -      this._onAddressSubmit(address, target, timeStartedFillingMS);
> -    }
> -    if (creditCard) {
> +    // Don't record filling time if any type of records has more than one
> +    // section being populated.
> +    if (address.length > 1 || creditCard.length > 1) {
> +      timeStartedFillingMS = null;

It looks like we will miss the telemetry data for any two section address/credit card form, and I know it's about the category for multple section form can not be fit to the current design. My suggestion is to explain the detail so we can improve this if needed. At least, we should know it's a limitation.

Comment 16

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

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

https://reviewboard.mozilla.org/r/201480/#review209674

> It looks like we will miss the telemetry data for any two section address/credit card form, and I know it's about the category for multple section form can not be fit to the current design. My suggestion is to explain the detail so we can improve this if needed. At least, we should know it's a limitation.

BTW, I know the case one credit card section + one address section will have `timeStartedFillingMS` here.

Comment 17

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

Comment on attachment 8932768 [details]
Bug 1415073 - Update the unit tests accordingly with new submission records.

https://reviewboard.mozilla.org/r/203820/#review209678

LGTM! Thank you.

Comment 18

[Ray Lin[:ralin]]

> Sorry, I remove the review flag accidentally. Please set the flag again. Thank you.
I was just worrying if some issues should but weren't published :P

(In reply to Sean Lee [:seanlee][:weilonge] from comment #15)
> Comment on attachment 8930342 [details]
> Bug 1415073 - Refactor records structure of form autofill submission to
> adapt multiple sections.
> 
> https://reviewboard.mozilla.org/r/201480/#review209674
> 
> BTW, the patch looks good to me. I give it r+ once I got the request. Thank
> you.
> 
> ::: browser/extensions/formautofill/FormAutofillParent.jsm:543
> (Diff revision 4)
> >      let {profile: {address, creditCard}, timeStartedFillingMS} = data;
> >  
> > -    if (address) {
> > -      this._onAddressSubmit(address, target, timeStartedFillingMS);
> > -    }
> > -    if (creditCard) {
> > +    // Don't record filling time if any type of records has more than one
> > +    // section being populated.
> > +    if (address.length > 1 || creditCard.length > 1) {
> > +      timeStartedFillingMS = null;
> 
> It looks like we will miss the telemetry data for any two section
> address/credit card form, and I know it's about the category for multple
> section form can not be fit to the current design. My suggestion is to
> explain the detail so we can improve this if needed. At least, we should
> know it's a limitation.
yeah, as yet keep the sent record on the same basis as it was is the safest way I can think of now. We can have further discussion about which category should those additional cases belong to, however, for now I'm afraid I can't provide helpful details unless we ensure those records are really representative on dashboard.

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

https://reviewboard.mozilla.org/r/201480/#review209710

Looks good and only some nits, thanks.

::: browser/extensions/formautofill/FormAutofillContent.jsm:419
(Diff revision 4)
>        this.log.debug("Form element could not map to an existing handler");
>        return true;
>      }
>  
>      let records = handler.createRecords();
> -    if (!Object.keys(records).length) {
> +    if (!records || !Object.values(records).some(typeRecords => typeRecords.length)) {

nit: It looks like record returned from createRecord will have default structure, so there's no need to check `!records`

::: browser/extensions/formautofill/FormAutofillHandler.jsm:988
(Diff revision 4)
>          }
>          this.timeStartedFillingMS = Date.now();
>          break;
>      }
>    }
>  

nit: Maybe having a jsdoc of `createRecords` would be better.

::: browser/extensions/formautofill/FormAutofillParent.jsm:419
(Diff revision 4)
>        if (FormAutofillUtils.isAutofillAddressesFirstTimeUse) {
>          Services.prefs.setBoolPref(FormAutofillUtils.ADDRESSES_FIRST_TIME_USE_PREF, false);
> -        FormAutofillDoorhanger.show(target, "firstTimeUse").then((state) => {
> +        showDoorhanger = async () => {
> +          const state = await FormAutofillDoorhanger.show(target, "firstTimeUse");
>            if (state !== "open-pref") {
>              return;

nit: Since you return false in `_onCreditCardSubmit` if there's no need to show the doorhanger, maybe we should return false for address as well.

Comment 20

[Ray Lin[:ralin]]

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201480/diff/4-5/

Comment 21

[Ray Lin[:ralin]]

Comment on attachment 8932768 [details]
Bug 1415073 - Update the unit tests accordingly with new submission records.

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

Comment 22

[Ray Lin[:ralin]]

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

https://reviewboard.mozilla.org/r/201480/#review209710

Thank you Sean and Steve for the review :D

> nit: It looks like record returned from createRecord will have default structure, so there's no need to check `!records`

yeh, indeed. I've removed the check.

> nit: Maybe having a jsdoc of `createRecords` would be better.

A brief jsdoc is added, and I didn't add too much details since the doc Section.createRecords is self-explanatory to how the record be comprised of.

> nit: Since you return false in `_onCreditCardSubmit` if there's no need to show the doorhanger, maybe we should return false for address as well.

The return is within showDoorhanger callback function instead of the scope of _onCreditCardSubmit, so I didn't modify this line.

Comment 23

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

Comment on attachment 8930342 [details]
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections.

https://reviewboard.mozilla.org/r/201480/#review209994

LGTM. Thanks.

Comment 24

[Steve Chung [:steveck]]

Comment on attachment 8932768 [details]
Bug 1415073 - Update the unit tests accordingly with new submission records.

https://reviewboard.mozilla.org/r/203820/#review210054

::: browser/extensions/formautofill/test/unit/test_createRecords.js:299
(Diff revision 2)
> -      creditCard: undefined,
> +      address: [],
> +      creditCard: [],
> +    },
> +  },
> +  {
> +    description: "A form with multiple sections",

Not sure if we could support the multiple section without section token(for example, the credit card sections below could work even there's no section-one/two in autocomplete property). If so, we could probably leverage these credit card sections test for the non-token section case(or simply skip this comment if we'll still need token for detecting section).

Comment 25

[Ray Lin[:ralin]]

Comment on attachment 8932768 [details]
Bug 1415073 - Update the unit tests accordingly with new submission records.

https://reviewboard.mozilla.org/r/203820/#review210054

> Not sure if we could support the multiple section without section token(for example, the credit card sections below could work even there's no section-one/two in autocomplete property). If so, we could probably leverage these credit card sections test for the non-token section case(or simply skip this comment if we'll still need token for detecting section).

Right, it makes sense, but I think we'll still need it(at least one set of fields) until bug 1416664 landed.

Comment 26

[Ray Lin[:ralin]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27dcd8cc469363d2cd8a933bc6918ba123e53011

(please ignore the irrelevant commits which don't affect the result)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4acb1f17f252
Refactor records structure of form autofill submission to adapt multiple sections. r=seanlee,steveck
https://hg.mozilla.org/integration/autoland/rev/6cab14274c26
Update the unit tests accordingly with new submission records. r=seanlee,steveck

Comment 28

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4acb1f17f252
https://hg.mozilla.org/mozilla-central/rev/6cab14274c26