Closed Bug 1387988 Opened 7 years ago Closed 7 years ago

[Form Autofill] Optimize "findLabelElements" function

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

Details

(Whiteboard: [form autofill:MVP])

Attachments

(2 files, 1 obsolete file)

Bug 1387988 - [Form Autofill] Move "findLabelElements" function to FormAutofillHeuristics.jsm.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1387988 - [Form Autofill] Optimize "findLabelElements" function.
59 bytes, text/x-review-board-request
MattN
: review+
gchang
: approval-mozilla-beta+
Details 
We currently use a JS-based "findLabelElements" function to find the corresponding label elements from an input element. We was planning to switch to an identical native API ".labels" after its implementation finished so "findLabelElements" function was treated as a temporary solution and wasn't optimized.

However, we noticed that ".labels" has a performance concern when we tried to switch to it (see bug 1374724) and we might not fix it on schedule. Therefore, we should optimize "findLabelElements" function for production in case we need to ship with it.
My experiments on calling "collectFormFields" 1000 times in each run:

Baseline
2310ms
2336ms
2353ms
2381ms
2282ms

Patches applied
1677ms
1689ms
1633ms
1625ms
1664ms
Comment on attachment 8894842 [details]
Bug 1387988 - [Form Autofill] Move "findLabelElements" function to FormAutofillHeuristics.jsm.

https://reviewboard.mozilla.org/r/166004/#review174214
Attachment #8894842 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8894842 [details]
Bug 1387988 - [Form Autofill] Move "findLabelElements" function to FormAutofillHeuristics.jsm.

https://reviewboard.mozilla.org/r/166004/#review174224

::: browser/extensions/formautofill/test/unit/test_extractLabelStrings.js:1
(Diff revision 1)
> +/* global LabelUtils */

Instead of using `global`, update https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/modules.json
Comment on attachment 8894843 [details]
Bug 1387988 - [Form Autofill] Optimize "findLabelElements" function.

https://reviewboard.mozilla.org/r/166006/#review174220

Clearing review until there are more comments

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:187
(Diff revision 1)
> +  _mappedLabels: null,
> +  _unmappedLabels: null,

This would be easier to review and easier for future readers to understand if it was documented what the key and value are in these maps and what they are for.

::: browser/extensions/formautofill/test/unit/test_getInfo.js:1
(Diff revision 1)
> +/* global LabelUtils */

Remove this after the part 1 fix.
Attachment #8894843 - Flags: review?(MattN+bmo)
Thanks. They are fixed.
Comment on attachment 8894843 [details]
Bug 1387988 - [Form Autofill] Optimize "findLabelElements" function.

https://reviewboard.mozilla.org/r/166006/#review174578

Thanks!

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:187
(Diff revision 2)
> +  // A map object, whose keys are the id's of input elements and each value is
> +  // an array consisting of label elements correponding to the id.
> +  _mappedLabels: null,
> +
> +  // An array consisting of label elements whose correponding input element
> +  // doesn't have an id attribute.
> +  _unmappedLabels: null,

These should use JSDoc syntax with @type

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:187
(Diff revision 2)
>    // The tag name list is from Chromium except for "STYLE":
>    // eslint-disable-next-line max-len
>    // https://cs.chromium.org/chromium/src/components/autofill/content/renderer/form_autofill_util.cc?l=216&rcl=d33a171b7c308a64dc3372fac3da2179c63b419e
>    EXCLUDED_TAGS: ["SCRIPT", "NOSCRIPT", "OPTION", "STYLE"],
> +
> +  // A map object, whose keys are the id's of input elements and each value is

s/input elements/form fields/ ?

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:247
(Diff revision 2)
> +        if (!labels) {
> +          mappedLabels.set(id, [label]);
> +        } else {
> -        labels.push(label);
> +          labels.push(label);
> -      }
> +        }

Nit: Invert the condition to avoid the unnecessary negative to make it easier to comprehend:
```js
if (labels) {
  labels.push(label);
} else {
  mappedLabels.set(id, [label]);
}
```
Attachment #8894843 - Flags: review?(MattN+bmo) → review+
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea2000b5721e
[Form Autofill] Move "findLabelElements" function to FormAutofillHeuristics.jsm. r=MattN
https://hg.mozilla.org/integration/autoland/rev/8290e527cf40
[Form Autofill] Optimize "findLabelElements" function. r=MattN
https://hg.mozilla.org/mozilla-central/rev/ea2000b5721e
https://hg.mozilla.org/mozilla-central/rev/8290e527cf40
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Attachment #8898656 - Attachment is obsolete: true
Attachment #8898656 - Flags: review?(MattN+bmo)
Depends on: 1392888
Comment on attachment 8894843 [details]
Bug 1387988 - [Form Autofill] Optimize "findLabelElements" function.

Approval Request Comment
[Feature/Bug causing the regression]:
Performance improvement.

[User impact if declined]:
The performance will decrease if there are lots of labels in the same page.

[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. It's about performance.

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It should affect Form Autofill system add-on only.

[String changes made/needed]:
N/A
Attachment #8894843 - Flags: approval-mozilla-beta?
Comment on attachment 8894843 [details]
Bug 1387988 - [Form Autofill] Optimize "findLabelElements" function.

Performance improvement. Beta56+.
Attachment #8894843 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: