Cache the matching result of select elements so we don't need to calculate it every time when previewing and filling

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lchang, Assigned: lchang)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M4][ETA:7/28])

Attachments

(1 attachment)

After bug 1365544 landed, we are able to preview and fill an inexact matching field to a select element. We currently search the matching option element each time when we are doing preview and filling. To increase the performance, we should cache the result.

Comment 2

2 years ago
mozreview-review
Comment on attachment 8887840 [details]
Bug 1382148 - Cache the matching result of select elements so we don't need to calculate it every time when previewing and filling.

https://reviewboard.mozilla.org/r/158758/#review166032

Are you going to write the test for `_matchSelectOptions` in this commit or another one?
I suppose the test could be in [1] as well.

[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js

::: browser/extensions/formautofill/FormAutofillHandler.jsm:174
(Diff revision 1)
> +      if (cache[originalValue]) {
> +        profile[fieldName] = cache[originalValue];
> +      } else {
> +        let option = FormAutofillUtils.findSelectOption(element, profile, fieldName);
> +        if (option && option.value) {
> +          profile[fieldName] = cache[originalValue] = option.value;

Could we keep `option` element and store it in `profile["-moz-state-option"]`?

`element.querySelector([value=${value}])` could address to a wrong option element if there are two state/country elements in a form.
Attachment #8887840 - Flags: review?(selee)
Assignee

Comment 3

2 years ago
mozreview-review-reply
Comment on attachment 8887840 [details]
Bug 1382148 - Cache the matching result of select elements so we don't need to calculate it every time when previewing and filling.

https://reviewboard.mozilla.org/r/158758/#review166032

> Could we keep `option` element and store it in `profile["-moz-state-option"]`?
> 
> `element.querySelector([value=${value}])` could address to a wrong option element if there are two state/country elements in a form.

Since we can only get the serialized profile from the result object, it seems unlikely to carry an element reference.

The `querySelector` should not fail in the case you mentioned as we query it from a specified element and there won't be two fields with the same `fieldName` in one `fieldDetails` under our current assumption.

Though there's a concern about `querySelector([value=${value}])` indeed if two options have the same `value`. Not sure how to deal with this case yet.
Comment hidden (mozreview-request)
I changed the caching approach and added some tests. Note that "country" will be addressed in bug 1375382 so the tests haven't covered "country" yet.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8887840 [details]
Bug 1382148 - Cache the matching result of select elements so we don't need to calculate it every time when previewing and filling.

https://reviewboard.mozilla.org/r/158758/#review167854

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:139
(Diff revision 2)
> +                 <option value="">California</option>
> +               </select>
> +               </form>`,
> +    profileData: [Object.assign({}, DEFAULT_PROFILE)],
> +    expectedResult: [{
> +      "address-level1": "CA",

This test does not really cover `__matchSelectOptions` function. This is still pass even without `this._matchSelectOptions(profile);` in `FormAutofillHandler.getAdaptedProfiles()`.

These `"CA"` values actually comes from the profile rather than the behavior of `this._matchSelectOptions()`.

I also think that checking the option element instead of the value only is necessary.

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:177
(Diff revision 2)
>      let formLike = FormLikeFactory.createFromForm(form);
>      let handler = new FormAutofillHandler(formLike);
>  
>      handler.collectFormFields();
>      let adaptedAddresses = handler.getAdaptedProfiles(testcase.profileData);
> -    Assert.deepEqual(adaptedAddresses, testcase.expectedResult);
> +    do_check_records_matches(testcase.expectedResult, adaptedAddresses);

This change makes the test cleaner but it will lose the benefit to check other fields should not be changed.
It still prefers the original version.
Attachment #8887840 - Flags: review?(selee)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 8

2 years ago
mozreview-review
Comment on attachment 8887840 [details]
Bug 1382148 - Cache the matching result of select elements so we don't need to calculate it every time when previewing and filling.

https://reviewboard.mozilla.org/r/158758/#review168228

This patch is able to distinguish the different options even they are with the same value.
Please add a test to verify this case and fix the issues I addressed.
Thank you.

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:132
(Diff revision 3)
> +      "address-line2": "line2",
> +      "address-line3": "line3",
> +      "address-level1": "CA",
> +      "country": "US",
> +    }],
> +    expectedElements: [{

Is this vaiable only for `<select>` element? If so, `expectedSelectedElements` is better.

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:198
(Diff revision 3)
> +      "street-address": "2 Harrison St\nline2\nline3",
> +      "-moz-street-address-one-line": "2 Harrison St line2 line3",
> +      "address-line1": "2 Harrison St",
> +      "address-line2": "line2",
> +      "address-line3": "line3",
> +      "country": "US",

In the previous version, there is a check for `undefined` case but this version. Could you add it back?

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:218
(Diff revision 3)
>      handler.collectFormFields();
>      let adaptedAddresses = handler.getAdaptedProfiles(testcase.profileData);
>      Assert.deepEqual(adaptedAddresses, testcase.expectedResult);
> +
> +    if (testcase.expectedElements) {
> +      testcase.expectedElements.forEach((expectedElements, i) => {

For `(expectedElements, i)`, do you mean `(expectedElement, i)`?

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js:229
(Diff revision 3)
> +          let value = testcase.profileData[i][field];
> +          let cache = handler._cacheValue.matchingSelectOption.get(select);
> +          let targetOption = cache[value] && cache[value].get();
> +          do_check_neq(targetOption, null);
> +
> +          do_check_eq(targetOption == expectedOption, true);

Please use the same assert library in a file, and I think `do_check_eq(targetOption, expectedOption);` is clearer?

::: browser/extensions/formautofill/test/unit/test_getAdaptedProfiles.js
(Diff revision 3)
> +        }
> +      });
> +    }
>    });
>  }
> -

nit: unnecessary change?
Attachment #8887840 - Flags: review?(selee) → review+
Assignee

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8887840 [details]
Bug 1382148 - Cache the matching result of select elements so we don't need to calculate it every time when previewing and filling.

https://reviewboard.mozilla.org/r/158758/#review168228

> In the previous version, there is a check for `undefined` case but this version. Could you add it back?

`undefined` will cause `deepEqual` fail because the property is deleted instead of `undefined`.

> nit: unnecessary change?

There were two blank lines.
Comment hidden (mozreview-request)

Comment 11

2 years ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7ce1154b734f -d 8227a596f3fd: rebasing 410664:7ce1154b734f "Bug 1382148 - Cache the matching result of select elements so we don't need to calculate it every time when previewing and filling. r=seanlee" (tip)
merging browser/extensions/formautofill/FormAutofillHandler.jsm
merging browser/extensions/formautofill/test/unit/test_autofillFormFields.js
warning: conflicts while merging browser/extensions/formautofill/test/unit/test_autofillFormFields.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)

Comment 13

2 years ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1eea95104a3a
Cache the matching result of select elements so we don't need to calculate it every time when previewing and filling. r=seanlee
https://hg.mozilla.org/mozilla-central/rev/1eea95104a3a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.