Closed Bug 1416664 Opened 7 years ago Closed 7 years ago

[Form Autofill] Classify the fields into multiple sections for the case of no fields with the section part of autocomplete attr

Categories

(Toolkit :: Form Autofill, enhancement, P3)

Product:
Toolkit
Component:
Form Autofill
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:V2])

Attachments

(3 files)

Bug 1416664 - Identify the sections for the fields without the section part of autocomplete attr.
59 bytes, text/x-review-board-request
ralin
: review+
lchang
: review+
Details
Bug 1416664 - Move the duplication logic to _trimFieldDetails function.
59 bytes, text/x-review-board-request
lchang
: review+
ralin
: review+
Details
Bug 1416664 - Modify the heuristic tests to verify the multiple section feature.
59 bytes, text/x-review-board-request
lchang
: review+
ralin
: review+
Details 
If all fields in a form are without the section part of `autocomplete` attribute, the section heuristics should be able to classify the sections based on the degree of the duplicating degree of the fields.

Unlike bug 1415077, this bug focuses on the case without any section information.
Comment on attachment 8932287 [details]
Bug 1416664 - Identify the sections for the fields without the section part of autocomplete attr.

https://reviewboard.mozilla.org/r/203308/#review208716

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:242
(Diff revision 1)
>    _trimFieldDetails(fieldDetails) {
> -    return fieldDetails.filter(f => f.fieldName && !f._duplicated);
> +    return this._allowDuplicates ? fieldDetails : fieldDetails.filter(f => f.fieldName && !f._duplicated);

The behavior seems uncertain for the caller due to the implicit this._allowDuplicates inside. I would prefer keep it behave consistently with its function name. (or maybe refine the documentation)

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:693
(Diff revision 1)
>      LabelUtils.clearLabelMap();
>  
> -    if (!this._sectionEnabled) {
> -      // When the section feature is disabled, `getFormInfo` should provide a
> +    // When the section feature is disabled, `getFormInfo` should provide a
> -      // single section result.
> +    // single section result.
> -      return [fieldScanner.getFieldDetails(allowDuplicates)];
> +    return fieldScanner.getSectionFieldDetails(!this._sectionEnabled);

I don't see it necessary to pass the private getter here. Any reason it's not determinate in getSectionFieldDetails?
Attachment #8932287 - Flags: review?(ralin)
Comment on attachment 8932288 [details]
Bug 1416664 - Move the duplication logic to _trimFieldDetails function.

https://reviewboard.mozilla.org/r/203310/#review208840

Thanks LGTM!

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:233
(Diff revision 1)
>    _trimFieldDetails(fieldDetails) {
> -    return this._allowDuplicates ? fieldDetails : fieldDetails.filter(f => f.fieldName && !f._duplicated);
> +    if (this._allowDuplicates) {
> +      return fieldDetails;
> +    }
> +
> +    let dedupFieldDetails = [];

nit: deduped(deduplicated)FieldDetails seems more clear to me to know those are filtered and eligible ones.
Attachment #8932288 - Flags: review?(ralin) → review+
Blocks: 1421181
Assignee: nobody → selee
Status: NEW → ASSIGNED
Comment on attachment 8932287 [details]
Bug 1416664 - Identify the sections for the fields without the section part of autocomplete attr.

https://reviewboard.mozilla.org/r/203308/#review208716

> The behavior seems uncertain for the caller due to the implicit this._allowDuplicates inside. I would prefer keep it behave consistently with its function name. (or maybe refine the documentation)

The documentation is refined in the latest patch. The latest comment of `_trimFieldDetails` mentions removing a field with the invalid field name mainly but also the deduplication depends on `this._allowDuplicates`.

> I don't see it necessary to pass the private getter here. Any reason it's not determinate in getSectionFieldDetails?

FieldScanner can not get the pref with `XPCOMUtils.defineLazyGetter` and I guess it's caused by it is `class`, so `this._sectionEnabled` will be passed to FieldScanner constructor. This can prevent the incorrect behavior once the pref is changed  during the heuristics.
Comment on attachment 8932288 [details]
Bug 1416664 - Move the duplication logic to _trimFieldDetails function.

https://reviewboard.mozilla.org/r/203310/#review208840

> nit: deduped(deduplicated)FieldDetails seems more clear to me to know those are filtered and eligible ones.

It's fixed in the latest patch.
Comment on attachment 8932287 [details]
Bug 1416664 - Identify the sections for the fields without the section part of autocomplete attr.

https://reviewboard.mozilla.org/r/203308/#review208716

> The documentation is refined in the latest patch. The latest comment of `_trimFieldDetails` mentions removing a field with the invalid field name mainly but also the deduplication depends on `this._allowDuplicates`.

thanks

> FieldScanner can not get the pref with `XPCOMUtils.defineLazyGetter` and I guess it's caused by it is `class`, so `this._sectionEnabled` will be passed to FieldScanner constructor. This can prevent the incorrect behavior once the pref is changed  during the heuristics.

I initially thought pref can be accessible by XPCOMUtils.defineLazyGetter or perhaps via FormAutofillUtils pref getter. But that's fair enough if passing it to constructor can prevent the pref changed during Scanner's lifecycle,  thanks.
Comment on attachment 8932287 [details]
Bug 1416664 - Identify the sections for the fields without the section part of autocomplete attr.

https://reviewboard.mozilla.org/r/203308/#review209262
Attachment #8932287 - Flags: review?(ralin) → review+
Comment on attachment 8932289 [details]
Bug 1416664 - Modify the heuristic tests to verify the multiple section feature.

https://reviewboard.mozilla.org/r/203312/#review208842

Looks good! Thanks
Attachment #8932289 - Flags: review?(ralin) → review+
Comment on attachment 8932287 [details]
Bug 1416664 - Identify the sections for the fields without the section part of autocomplete attr.

https://reviewboard.mozilla.org/r/203308/#review209706

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:121
(Diff revision 3)
>    }
>  
> -  getSectionFieldDetails(allowDuplicates) {
> -    // TODO: [Bug 1416664] If there is only one section which is not defined by
> -    // `autocomplete` attribute, the sections should be classified by the
> -    // heuristics.
> +  _classifySections() {
> +    let fieldDetails = this._sections[0].fieldDetails;
> +    this._sections = [];
> +    let seenTypes = [];

nit: How about using `Set()` to make values unique.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:252
(Diff revision 3)
> -   * Provide the field details without invalid field name and duplicated fields.
> +   * Provide the field details without invalid field name. The duplicated fields
> +   * will be removed as well if `this._allowDuplicates` is true.
>     *
>     * @param   {Array<Object>} fieldDetails
>     *          The field details for trimming.
>     * @returns {Array<Object>}
>     *          The array with the field details without invalid field name and
>     *          duplicated fields.
>     */
>    _trimFieldDetails(fieldDetails) {
> -    return fieldDetails.filter(f => f.fieldName && !f._duplicated);
> +    return this._allowDuplicates ? fieldDetails : fieldDetails.filter(f => f.fieldName && !f._duplicated);

Looks like we won't remove invalid field names if `_allowDuplicates` is "true". Is it on purpose?

If so, the comment needs an update. Also, if there's a chance that this function will do nothing, maybe naming it to something like `_maybeTrimFieldDetails` or `_getFinalDetails` would be clearer.
Comment on attachment 8932287 [details]
Bug 1416664 - Identify the sections for the fields without the section part of autocomplete attr.

https://reviewboard.mozilla.org/r/203308/#review209716
Attachment #8932287 - Flags: review?(lchang)
Comment on attachment 8932287 [details]
Bug 1416664 - Identify the sections for the fields without the section part of autocomplete attr.

https://reviewboard.mozilla.org/r/203308/#review209706

> nit: How about using `Set()` to make values unique.

Fixed in the latest patch. Thanks.

> Looks like we won't remove invalid field names if `_allowDuplicates` is "true". Is it on purpose?
> 
> If so, the comment needs an update. Also, if there's a chance that this function will do nothing, maybe naming it to something like `_maybeTrimFieldDetails` or `_getFinalDetails` would be clearer.

Since `this._allowDuplicates` is only for debugging purpose, it does not say it should allow the invalid fields. Hence, the invalid fields checking is applied to all condition. The function name is changed to `getFinalDetails` since this function can be used for other post-process requirements.
Comment on attachment 8932287 [details]
Bug 1416664 - Identify the sections for the fields without the section part of autocomplete attr.

https://reviewboard.mozilla.org/r/203308/#review210118

Thanks.
Attachment #8932287 - Flags: review?(lchang) → review+
Comment on attachment 8932288 [details]
Bug 1416664 - Move the duplication logic to _trimFieldDetails function.

https://reviewboard.mozilla.org/r/203310/#review210122
Attachment #8932288 - Flags: review?(lchang) → review+
Comment on attachment 8932289 [details]
Bug 1416664 - Modify the heuristic tests to verify the multiple section feature.

https://reviewboard.mozilla.org/r/203312/#review210128
Attachment #8932289 - Flags: review?(lchang) → review+
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0a82757a7fd
Identify the sections for the fields without the section part of autocomplete attr. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/86f1d09482d8
Move the duplication logic to _trimFieldDetails function. r=lchang,ralin
https://hg.mozilla.org/integration/autoland/rev/93ab8ba7803c
Modify the heuristic tests to verify the multiple section feature. r=lchang,ralin
Keywords: checkin-needed
Blocks: 1409347
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: