Closed Bug 1375382 Opened 7 years ago Closed 7 years ago

[Form Autofill] Handle filling in country field select element

Categories

(Toolkit :: Form Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: scottwu, Assigned: lchang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M3])

Attachments

(1 file)

Form autofill should try to fill country field select element. FormAutofillHandler will first look for the country key (ex. US) on both the option values and texts of a country select element. If there's no exact match, we should try to identify them.
Whiteboard: [form autofill:MVP] → [form autofill:M4]
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Comment on attachment 8891179 [details]
Bug 1375382 - [Form Autofill] Handle filling in country field select element.

https://reviewboard.mozilla.org/r/162384/#review169160

::: browser/extensions/formautofill/FormAutofillUtils.jsm:289
(Diff revision 3)
> +          break;
> -    }
> +        }
>  
> -    if (fieldName === "country") {
> -      // TODO: Support matching countries (Bug 1375382)
> +        let matchingOption;
> +        alternativeCountryNames.some(name => {
> +          let reName = new RegExp("\\b" + this.escapeRegExp(name) + "\\b", "i");

`reName` should be kept in a cache since `name` is a const value in a const array.
This makes `RegExp("\\b" + this.escapeRegExp(name) + "\\b", "i")` can not be precompiled.

::: browser/extensions/formautofill/ProfileStorage.jsm:1325
(Diff revision 3)
> +      if (!address.country) {
> +        for (let country in FormAutofillUtils.ALTERNATIVE_COUNTRY_NAMES) {
> +          let found = FormAutofillUtils.ALTERNATIVE_COUNTRY_NAMES[country].find(name => {
> +            let reName = new RegExp("\\b" + FormAutofillUtils.escapeRegExp(name) + "\\b", "i");
> +            return reName.test(countryName);
> +          });

In this snippet, there are three cross-JSM calls in the `for` loop.
I suggest to write a helper in `FormAutofillUtils` and use it here.
```JS
// This should be in FormAutofillUtils.
findCountryCode(countryName) {
  for (let country in this.ALTERNATIVE_COUNTRY_NAMES) {
    let found = this.ALTERNATIVE_COUNTRY_NAMES[country].find(name => {
      let reName = this.getCountryRegexp(name);
      return reName.test(countryName);
    });
    if (found) {
      return country;
    }
  }
  return null;
},

```

You seem want to handle more matching cases, so we should have a test for the above cases in `test_transformFields.js` as well.
Attachment #8891179 - Flags: review?(selee)
Updated accordingly. Thanks.
Comment on attachment 8891179 [details]
Bug 1375382 - [Form Autofill] Handle filling in country field select element.

https://reviewboard.mozilla.org/r/162384/#review171040

::: browser/extensions/formautofill/FormAutofillUtils.jsm:235
(Diff revision 4)
> +      let dataset = this.getCountryAddressData(country);
> +      this._collator[country] = new Intl.Collator(dataset.lang, {sensitivity: "base", ignorePunctuation: true});

This assumes that there is one language per country which isn't correct so that should be noted here. You can get other languages by splitting the `languages` property on "~" but that still isn't ideal as I think only one gets actually used. It seems like the locale of the page should be taken into account to do this properly.

I'm fine with this for the MVP if there's a comment and a bug. Maybe we should have a whiteboard for V2 stuff involving rollout in more locales: "[form autofill:V2]"?

::: browser/extensions/formautofill/FormAutofillUtils.jsm:305
(Diff revision 4)
> +      case "address-level1":
> -      if (!Array.isArray(dataset.sub_keys)) {
> +        if (!Array.isArray(dataset.sub_keys)) {

If you're switching to use `case` then you should add {} braces so that variables have block scope
Whiteboard: [form autofill:M4] → [form autofill:M3]
Comment on attachment 8891179 [details]
Bug 1375382 - [Form Autofill] Handle filling in country field select element.

https://reviewboard.mozilla.org/r/162384/#review171990

The patch looks good to me. I will review it again once the issues that MattN addressed are fixed. Thanks.
Attachment #8891179 - Flags: review?(selee)
Comment on attachment 8891179 [details]
Bug 1375382 - [Form Autofill] Handle filling in country field select element.

https://reviewboard.mozilla.org/r/162384/#review171040

> This assumes that there is one language per country which isn't correct so that should be noted here. You can get other languages by splitting the `languages` property on "~" but that still isn't ideal as I think only one gets actually used. It seems like the locale of the page should be taken into account to do this properly.
> 
> I'm fine with this for the MVP if there's a comment and a bug. Maybe we should have a whiteboard for V2 stuff involving rollout in more locales: "[form autofill:V2]"?

Thanks for pointing it out. I should have noticed that. I'll add a comment for now and file a bug to follow up how we can better deal with the languages.
Comment on attachment 8891179 [details]
Bug 1375382 - [Form Autofill] Handle filling in country field select element.

https://reviewboard.mozilla.org/r/162384/#review172112

LGTM! Thank you.
Attachment #8891179 - Flags: review?(selee) → review+
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40da3955d4a1
[Form Autofill] Handle filling in country field select element. r=seanlee
https://hg.mozilla.org/mozilla-central/rev/40da3955d4a1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.