1417818 - [Form Autofill] Add CA/DE address metadata in addressReferences

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P3)

Comment 1

[Steve Chung [:steveck]]

We'll need to copy the address metadata from libaddressinput[1] into our addressReferences.js to support CA/DE

[1] https://chromium-i18n.appspot.com/ssl-address

Comment 2

[Steve Chung [:steveck]]

Attachment: Bug 1417818 - Add CA/DE metadata from libaddressinput.

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

Comment 3

[Steve Chung [:steveck]]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review207692

::: browser/app/profile/firefox.js:1714
(Diff revision 1)
>  pref("extensions.formautofill.creditCards.used", 0);
>  pref("extensions.formautofill.firstTimeUse", true);
>  pref("extensions.formautofill.heuristics.enabled", true);
>  pref("extensions.formautofill.loglevel", "Warn");
>  // Comma separated list of countries Form Autofill supports
> -pref("extensions.formautofill.supportedCountries", "US");
> +pref("extensions.formautofill.supportedCountries", "US,CA,DE");

Although we could not handle the fr province name right now, we could still support the province name in en for CA after this patch(I tried it on amazon.ca and it works). So I think we could still claim that we support CA address now, wdty?

::: browser/extensions/formautofill/addressmetadata/addressReferencesExt.js:16
(Diff revision 1)
>  //  contains the information we need but absent in "libaddressinput" such as alternative names.
>  
>  // TODO: We only support the alternative name of US in MVP. We are going to support more countries in
>  //       bug 1370193.
>  var addressDataExt = {
> +  "data/DE": {languages: "de"},

I add this mainly for Collator, but it seems work if we don't provide the locale for Intl.Collator API(locale is optional). We might need more testing for de locale to see if it works fine for all the de locales(For example, de-at => “Austrian German" and so on).

Comment 4

[Steve Chung [:steveck]]

BTW I think maybe we could discuss about the DEFAULT_COUNTRY_CODE in the FormAutofillUtils. It was applied in getAbbreviatedStateName and findAddressSelectOption if country is undefined, but integrate this logic in getCountryAddressData might be an option instead of returning US metadata if not found. Furthermore, maybe we could even return US as DEFAULT_COUNTRY_CODE if "browser.search.countryCode" is not in the supported list.

Comment 5

[Scott Wu [:scottwu]]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review207692

> Although we could not handle the fr province name right now, we could still support the province name in en for CA after this patch(I tried it on amazon.ca and it works). So I think we could still claim that we support CA address now, wdty?

I think we can have `CA,US` now and add `DE` to the list in Bug 1383687, when fields could be rearranged and hidden.

> I add this mainly for Collator, but it seems work if we don't provide the locale for Intl.Collator API(locale is optional). We might need more testing for de locale to see if it works fine for all the de locales(For example, de-at => “Austrian German" and so on).

Or can we use country code directly? Maybe I over-complicated things when implementing `getCollators`. I think we can leave things as it is until we actually need it.

Comment 6

[Steve Chung [:steveck]]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

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

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review207692

> I think we can have `CA,US` now and add `DE` to the list in Bug 1383687, when fields could be rearranged and hidden.

Ok and I added TODO comment about this.

> Or can we use country code directly? Maybe I over-complicated things when implementing `getCollators`. I think we can leave things as it is until we actually need it.

Agree. I'll remove the lang de for now and we can add it back once we really need it.

Comment 8

[Scott Wu [:scottwu]]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review207998

::: browser/extensions/formautofill/test/unit/test_transformFields.js:389
(Diff revision 2)
>      },
>    },
>    {
>      description: "Has unsupported \"country\"",
>      address: {
> -      "country": "CA",
> +      "country": "TV",

In the future `TV` will become supported. Could we use a fictional country? or just `AA` like above?

Comment 10

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review207998

> In the future `TV` will become supported. Could we use a fictional country? or just `AA` like above?

This test intentionally focuses on countries that are not supported yet because we need to make sure that the unsupported country code (but is actually stored in the storage) won't be exposed.

Comment 11

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review208380

::: browser/extensions/formautofill/FormAutofillUtils.jsm:285
(Diff revision 2)
>      return sandbox;
>    },
>  
>    // Get country address data and fallback to US if not found.
>    // See AddressDataLoader.getData for more details of addressData structure.
> -  getCountryAddressData(country, level1 = null) {
> +  getCountryAddressData(country = FormAutofillUtils.DEFAULT_COUNTRY_CODE, level1 = null) {

As discussed, let's implement 2-level fallback mechanism.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:411
(Diff revision 2)
>    },
>  
>    /**
>     * Try to find the abbreviation of the given state name
>     * @param   {string[]} stateValues A list of inferable state values.
>     * @param   {string} country A country name to be identified.

Use `[country]` to indicate that it's optional.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:414
(Diff revision 2)
>     * Try to find the abbreviation of the given state name
>     * @param   {string[]} stateValues A list of inferable state values.
>     * @param   {string} country A country name to be identified.
>     * @returns {string} The matching state abbreviation.
>     */
> -  getAbbreviatedStateName(stateValues, country = this.DEFAULT_COUNTRY_CODE) {
> +  getAbbreviatedStateName(stateValues, country) {

Will it be used for countries other than US? If so, the function name and the parameter name may need a change. If not, the parameter, `country`, seems unnecessary.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:464
(Diff revision 2)
> -    let country = address.country || this.DEFAULT_COUNTRY_CODE;
> +    let country = address.country;
>      let dataset = this.getCountryAddressData(country);
>      let collators = this.getCollators(country);

nit: Since `country` is identical to `address.country` now, I prefer to use the latter directly.

```js
let dataset = this.getCountryAddressData(address.country);
let collators = this.getCollators(address.country);
```

::: browser/extensions/formautofill/FormAutofillUtils.jsm:663
(Diff revision 2)
>        element.removeAttribute("data-localization");
>      }
>    },
>  };
>  
>  XPCOMUtils.defineLazyGetter(this.FormAutofillUtils, "DEFAULT_COUNTRY_CODE", () => {

nit: `DEFAULT_REGION`

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

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

Comment 13

[Steve Chung [:steveck]]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

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

Comment 14

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

https://reviewboard.mozilla.org/r/202324/#review209698

Thanks.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:283
(Diff revision 4)
>    // Get country address data and fallback to US if not found.
>    // See AddressDataLoader.getData for more details of addressData structure.

nit: Use jsdoc format.

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8931219 [details]
Bug 1417818 - Add CA/DE metadata from libaddressinput.

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

Comment 16

[Steve Chung [:steveck]]

Thanks for the review!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b54a120c2b7
Add CA/DE metadata from libaddressinput. r=lchang,scottwu

Comment 18

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/6b54a120c2b7