1401411 - Autofill is too aggressive at saving profiles compared to Chrome

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Matthew N. [:MattN]]

e.g. Chrome doesn't seem to save just a first name + last name + organization. They also don't seem to save a (first name, last name, city, state, zip, country code) combo. The main issue is that users are getting too many partial addresses it seems.

Two of my profiles just have an email address (each a different one) + zip code since I needed to enter them as a confirmation on a website. I no longer have access to this website but this case was also below the threshold of 3.

I think we need to do somethings to be less aggressive. e.g. don't count first + last name as two fields for the threshold. Any name fields should count as 1 IMO.

Comment 1

[Juwei Huang[:juwei]]

After discussed with devs, I would also suggestion first name, middle name and last name count as 1.

Comment 2

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

Attachment: Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

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

Comment 3

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

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

https://reviewboard.mozilla.org/r/183108/#review188316

::: browser/extensions/formautofill/FormAutofillHandler.jsm:587
(Diff revision 1)
>      this._normalizeAddress(data.address);
>  
> +    let getAddressFieldCount = (record) => {
> +      let hasName = 0;
> +      let length = 0;
> +      for (let key of Object.keys(record)) {

AFAIK, there is no parent or inhertance of `record`, and I don't expect that we would like to look up the key in the prototype chain. This is why `for ... of Object.keys(...)` here rather than `for ... in`.

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

https://reviewboard.mozilla.org/r/183108/#review189058

::: browser/extensions/formautofill/FormAutofillHandler.jsm:584
(Diff revision 1)
>        });
>      });
>  
>      this._normalizeAddress(data.address);
>  
> +    let getAddressFieldCount = (record) => {

I'm thinking to put this function outside `createRecords` because `createRecords` is big enough and this function may grow over time.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:587
(Diff revision 1)
>      this._normalizeAddress(data.address);
>  
> +    let getAddressFieldCount = (record) => {
> +      let hasName = 0;
> +      let length = 0;
> +      for (let key of Object.keys(record)) {

I'm fine with that.

::: browser/extensions/formautofill/test/unit/test_createRecords.js:34
(Diff revision 1)
> -      "family-name": "Doe",
> +      "organization": "Mozilla",
>        country: "United States",
>      },
>      expectedRecord: {
>        address: {
>          "given-name": "John",
> -        "family-name": "Doe",
> +        organization: "Mozilla",

nit: We should choose one style from `"organization"` or `organization` (w/ or w/o quotes).

Comment 5

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

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

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

Comment 6

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

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

https://reviewboard.mozilla.org/r/183108/#review189058

> I'm thinking to put this function outside `createRecords` because `createRecords` is big enough and this function may grow over time.

The latest patch is with this change and moving the length check in the new function `_isAddressRecordCreatable`. I think the actual purpose of this function should not only count the length but also check it.
`_isCreditCardRecordCreatable` is the same purpose but for credit card records.

Comment 7

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

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

https://reviewboard.mozilla.org/r/183108/#review189058

> The latest patch is with this change and moving the length check in the new function `_isAddressRecordCreatable`. I think the actual purpose of this function should not only count the length but also check it.
> `_isCreditCardRecordCreatable` is the same purpose but for credit card records.

hmm, the debug log looks like should change with `_isAddressRecordCreatable` and `_isCreditCardRecordCreatable` functions.

Comment 8

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

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

Remove the flag since I need to add more tests for credit card cases.

Comment 9

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

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

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

Comment 10

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

Hey Luke, The latest patch is with some credit card cases now. Please review it again. Thanks. :P

Comment 11

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

https://reviewboard.mozilla.org/r/183108/#review189526

Looks good. Thanks.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/130a44290c28
All name related fields should be counted as 1 field only when creating the records. r=lchang

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for eslint failure at browser/extensions/formautofill/test/unit/test_createRecords.js:207:8 | Missing trailing comma:

https://hg.mozilla.org/integration/autoland/rev/27bd931e843d46869717e4160704a49e5c13a74e

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=130a44290c28b51b919c3f0d01ababad2a594c1d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133996953&repo=autoland
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/extensions/formautofill/test/unit/test_createRecords.js:207:8 | Missing trailing comma. (comma-dangle)

Comment 14

[Luke Chang [:lchang] (inactive)]

Thanks for the notice.

Comment 15

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

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

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

Comment 16

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

Hey Sebastian, the latest patch is with eslint fixes, please help to land it again. Thank you.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3faf87ed901
All name related fields should be counted as 1 field only when creating the records. r=lchang

Comment 18

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

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

Comment 19

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

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

Approval Request Comment
[Feature/Bug causing the regression]: New feature.
[User impact if declined]:
Autofill feature is too aggressive at saving the new profile from user's input.
Actually, we should focus on the address related profile.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]:
[Why is the change risky/not risky?]:
This is a minor change since it's about the criteria of saving address profiles.
[String changes made/needed]: No.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8911708 [details]
Bug 1401411 - All name related fields should be counted as 1 field only when creating the records.

Improve a new feature, taking it.
Should be in 57b5

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e28be9682521

Comment 22

[Andrei Vaida [:avaida]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #19)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Sean's assessment on manual testing needs and the fact that this fix has automated coverage.