Closed Bug 1407759 Opened 5 years ago Closed 5 years ago

[Form Autofill] <SELECT> element should only support country, address-level*, and cc-exp*

Categories

(Toolkit :: Form Manager, enhancement, P1)

x86
macOS
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(1 file, 1 obsolete file)

There is unnecessary that a <SELECT> element tries to match all regexps. e.g. tel field never belongs to a <SELECT> element. So <SELECT> element should only support country, address-level*, and cc-exp*.
Comment on attachment 8917533 [details]
Bug 1407759 - SELECT element supports some fieldNames only. (e.g. cc-exp*, country, address-level*)

https://reviewboard.mozilla.org/r/188492/#review197032

TGTM, thanks.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:580
(Diff revision 2)
> +    ];
> +    let regexps = isAutoCompleteOff ? FIELDNAMES_IGNORING_AUTOCOMPLETE_OFF : Object.keys(this.RULES);
> +
> +    if (!FormAutofillUtils.isAutofillCreditCardsAvailable) {
> +      if (isAutoCompleteOff) {
> +        if (!this._regexpListOf_CcUnavailable_AcOff) {

nit: A comment for how _regexpListOf_CcUnavailable_AcOff is used might help to understand the purpose of cache.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:585
(Diff revision 2)
> +        if (!this._regexpListOf_CcUnavailable_AcOff) {
> +          this._regexpListOf_CcUnavailable_AcOff = regexps.filter(name => !FormAutofillUtils.isCreditCardField(name));
> +        }
> +        regexps = this._regexpListOf_CcUnavailable_AcOff;
> +      } else {
> +        if (!this._regexpListOf_CcUnavailable_AcOn) {

nit: Same here

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:592
(Diff revision 2)
> +    if (elementTagName == "SELECT") {
> +      const FIELDNAMES_FOR_SELECT_ELEMENT = [

I think here can have something equivalent to this._regexpListOf_CcUnavailable_AcOff as we discussed in the other day.

::: browser/extensions/formautofill/test/unit/test_getInfo.js:259
(Diff revision 2)
>      Assert.deepEqual(value, testcase.expectedReturnValue);
>      LabelUtils.clearLabelMap();
>    });
>  });
> +
> +add_task(async function () {

nit: named function might be easier to indicate the error in stack log.
Attachment #8917533 - Flags: review?(ralin) → review+
(In reply to Ray Lin[:ralin] from comment #3)
> Comment on attachment 8917533 [details]
> Bug 1407759 - SELECT element supports some fieldNames only. (e.g. cc-exp*,
> country, address-level*)
> 
> https://reviewboard.mozilla.org/r/188492/#review197032
> 
> TGTM, thanks.
s/TGTM/LGTM/ :p
Comment on attachment 8917533 [details]
Bug 1407759 - SELECT element supports some fieldNames only. (e.g. cc-exp*, country, address-level*)

https://reviewboard.mozilla.org/r/188492/#review197460

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:592
(Diff revision 2)
> +    if (elementTagName == "SELECT") {
> +      const FIELDNAMES_FOR_SELECT_ELEMENT = [

Just a quick thought. Maybe we can make the cache look like `this._regexpListOf[isAutoCompleteOff][isAutofillCreditCardsAvailable][isSelectElement]`.
Attachment #8917533 - Flags: review?(lchang) → review+
Comment on attachment 8917533 [details]
Bug 1407759 - SELECT element supports some fieldNames only. (e.g. cc-exp*, country, address-level*)

https://reviewboard.mozilla.org/r/188492/#review197460

> Just a quick thought. Maybe we can make the cache look like `this._regexpListOf[isAutoCompleteOff][isAutofillCreditCardsAvailable][isSelectElement]`.

Thanks for the review! After going through the patch again, I use a bitmap-like way to implement the cache, and the patch is more clear than the previous version.
Comment on attachment 8917533 [details]
Bug 1407759 - SELECT element supports some fieldNames only. (e.g. cc-exp*, country, address-level*)

https://reviewboard.mozilla.org/r/188492/#review197566

Nice refactor thanks :D, just thought of some concerns.

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:568
(Diff revision 4)
>      }
>  
>      return fieldScanner.trimmedFieldDetail;
>    },
>  
> +  _regExpTableHashValue(b0, b1, b2) {

nit: If we've already know all the arguments are boolean-like, do you think it's fine to simplify to more generic:

_regExpTableHashValue() {
  return [...arguments].reduce((p, c, i) => p | !!c << i, 0);
}

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:572
(Diff revision 4)
>  
> +  _regExpTableHashValue(b0, b1, b2) {
> +    return (b0 ? 1 : 0) | (b1 ? 1 : 0) << 1 | (b2 ? 1 : 0) << 2;
> +  },
> +
> +  _setRegExpListCache(regexps, b0, b1, b2) {

just came to my mind, IIUC, this utils object seems a singleton for all forms in the same process, should we use a weekmap to store the cache array over the <form> as key?
Comment on attachment 8917533 [details]
Bug 1407759 - SELECT element supports some fieldNames only. (e.g. cc-exp*, country, address-level*)

https://reviewboard.mozilla.org/r/188492/#review197566

> nit: If we've already know all the arguments are boolean-like, do you think it's fine to simplify to more generic:
> 
> _regExpTableHashValue() {
>   return [...arguments].reduce((p, c, i) => p | !!c << i, 0);
> }

correct my exmaple as it's better practice to avoid using arguments directly.
_regExpTableHashValue(...signBits) {
  return signBits.reduce((p, c, i) => p | !!c << i, 0);
}

> just came to my mind, IIUC, this utils object seems a singleton for all forms in the same process, should we use a weekmap to store the cache array over the <form> as key?

Please ignore this issue, I realized a while later I commented that the caches are applicable to all cases.
Comment on attachment 8917533 [details]
Bug 1407759 - SELECT element supports some fieldNames only. (e.g. cc-exp*, country, address-level*)

https://reviewboard.mozilla.org/r/188492/#review197566

> correct my exmaple as it's better practice to avoid using arguments directly.
> _regExpTableHashValue(...signBits) {
>   return signBits.reduce((p, c, i) => p | !!c << i, 0);
> }

It's applied. Thanks for the suggestion.
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Flags: needinfo?(selee)
Keywords: checkin-needed
Attachment #8917533 - Attachment is obsolete: true
Comment on attachment 8922156 [details]
Bug 1407759 - SELECT element supports some fieldNames only. (e.g. cc-exp*, country, address-level*)

https://reviewboard.mozilla.org/r/193162/#review198396

carry over r+
Attachment #8922156 - Flags: review?(ralin) → review+
Comment on attachment 8922156 [details]
Bug 1407759 - SELECT element supports some fieldNames only. (e.g. cc-exp*, country, address-level*)

https://reviewboard.mozilla.org/r/193162/#review198398
Attachment #8922156 - Flags: review?(lchang) → review+
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f93a6baffb7
SELECT element supports some fieldNames only. (e.g. cc-exp*, country, address-level*) r=lchang,ralin
Thanks for landing.
There is an invisible issue which can not be resolved (a mozreview bug probably), and I push an identical patch for landing.
Flags: needinfo?(selee)
https://hg.mozilla.org/mozilla-central/rev/7f93a6baffb7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.