Closed Bug 1415073 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Form Autofill, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: selee, Assigned: ralin)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:V2])

Attachments

(2 files)

`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()
Depends on: 1415077
Assignee: nobody → ralin
Component: Form Manager → Form Autofill
Status: NEW → ASSIGNED
Blocks: 1415022
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.
Depends on: 1420169
(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.
BTW, I'll add test cases soon later in a separate commit. Thanks.
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.
Attachment #8930342 - Flags: review?(selee)
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 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
Attachment #8930342 - Flags: review?(selee)
(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.
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
Sorry, I remove the review flag accidentally. Please set the flag again. Thank you.
Flags: needinfo?(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/#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 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 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.
Attachment #8932768 - Flags: review?(selee) → review+
> 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.
Flags: needinfo?(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

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.
Attachment #8930342 - Flags: review?(schung) → review+
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 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.
Attachment #8930342 - Flags: review?(selee) → review+
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).
Attachment #8932768 - Flags: review?(schung) → review+
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.
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27dcd8cc469363d2cd8a933bc6918ba123e53011

(please ignore the irrelevant commits which don't affect the result)
Pushed by ralin@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/4acb1f17f252
https://hg.mozilla.org/mozilla-central/rev/6cab14274c26
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.