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

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Luke Chang [:lchang] (inactive)]

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 1

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1382148 - Cache the matching result of select elements so we don't need to calculate it every time when previewing and filling.

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

Comment 2

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

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.

Comment 3

[Luke Chang [:lchang] (inactive)]

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 4

[Luke Chang [:lchang] (inactive)]

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.

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

Comment 5

[Luke Chang [:lchang] (inactive)]

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

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

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.

Comment 7

[Luke Chang [:lchang] (inactive)]

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.

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

Comment 8

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

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?

Comment 9

[Luke Chang [:lchang] (inactive)]

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 10

[Luke Chang [:lchang] (inactive)]

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.

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

Comment 11

[Mozilla Autoland]

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 12

[Luke Chang [:lchang] (inactive)]

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.

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/1eea95104a3a