Closed Bug 1022925 Opened 6 years ago Closed 2 years ago

Integrate a country/region validation data source for use with form autofill

Categories

(Toolkit :: Form Autofill, defect)

defect
Not set
Points:
8

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: MattN, Assigned: steveck)

References

(Blocks 4 open bugs)

Details

(Whiteboard: [form autofill:V2])

Attachments

(2 files)

Some country/region information which would be good to have for a better UX with form autofill:
* List of countries (bug 1020774)
* List of address-level values below the country (bug 1020855 for level 1). Ideally this would support multiple languages for each region.
* Postal code formats (as specific as possible i.e. not just at the country level)
* Which address fields to show (e.g. are required) for a given country (bug 1022898)
* The order to show fields for a given country (bug 1022898)

A possible candidate is libaddressinput which is what Chrome is using:
* https://code.google.com/p/libaddressinput/
* https://code.google.com/p/libaddressinput/wiki/AddressValidationMetadata

Some data may be in ICU (e.g. the country list).

Licensing should be addressed before choosing a solution.
Flags: firefox-backlog+
Points: --- → 8
Although Bug 1365544 already introduced the Google's open-source address metadata. We have addressReferences that only contains US data and we defined other address related information in FormAutofillUtils. We will move the file into country specific folder and the structure would be:

country-info/ => addressReferences.js
                 addressReferencesExt.js // Define other information like country name alias

                 // Some countries have address level-2 list in metadata(like Taiwan), so we'll create sub folder for them.
                 TW/                       =>  addressReferences.js
                                               addressReferencesExt.js
                 ...

We'll load the root addressReferences/addressReferencesExt when needed, and add/load sub folder if the website in specific country usually support level-2 address as select with multiple languages or using special code.
See Also: → 1365544
Component: Form Manager → Form Autofill
Assignee: nobody → schung
Comment on attachment 8927266 [details]
Bug 1022925 - Part 1: move addressReferences to addressmetadata folder.

https://reviewboard.mozilla.org/r/198578/#review204344
Attachment #8927266 - Flags: review?(scwwu) → review+
Comment on attachment 8927267 [details]
Bug 1022925 - Part 2: Move alternative name to extension file.

https://reviewboard.mozilla.org/r/198580/#review204354

::: browser/extensions/formautofill/FormAutofillUtils.jsm:15
(Diff revision 2)
>  
> -const ADDRESS_REFERENCES = "resource://formautofill/addressmetadata/addressReferences.js";
> +const ADDRESS_METADATA_PATH = "resource://formautofill/addressmetadata/";
> +const ADDRESS_REFERENCES = "addressReferences.js";
> +const ADDRESS_REFERENCES_EXT = "addressReferencesExt.js";
>  
> -// TODO: We only support US in MVP. We are going to support more countries in
> +const SUPPORTED_COUNTRY_LIST = ["US"];

I'm not sure if it's a good idea to keep this array in utils. Maybe we can move this to the addressReferencesExt, or even move all the address related script into another addressUtils?
Comment on attachment 8927266 [details]
Bug 1022925 - Part 1: move addressReferences to addressmetadata folder.

https://reviewboard.mozilla.org/r/198578/#review204418
Attachment #8927266 - Flags: review?(lchang) → review+
Whiteboard: [form autofill:V2]
Comment on attachment 8927267 [details]
Bug 1022925 - Part 2: Move alternative name to extension file.

https://reviewboard.mozilla.org/r/198580/#review204754

Is it a lot of work to have some tests covering this? If it is, I'm okay with testing them in future bugs, when the data are used.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:15
(Diff revision 2)
>  
> -const ADDRESS_REFERENCES = "resource://formautofill/addressmetadata/addressReferences.js";
> +const ADDRESS_METADATA_PATH = "resource://formautofill/addressmetadata/";
> +const ADDRESS_REFERENCES = "addressReferences.js";
> +const ADDRESS_REFERENCES_EXT = "addressReferencesExt.js";
>  
> -// TODO: We only support US in MVP. We are going to support more countries in
> +const SUPPORTED_COUNTRY_LIST = ["US"];

One thing we talked about is having supported countries in prefs so that they could be turned on and off (bug 1413494). If addressReferencesExt is meant to be static, maybe it's best to keep it here.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:86
(Diff revision 2)
>      "cc-exp-month": "creditCard",
>      "cc-exp-year": "creditCard",
>      "cc-exp": "creditCard",
>    },
> -  _addressDataLoaded: false,
> +
> +// Status of address data loading. We'll load all the countries with basic level 1

nit: indent comments
Attachment #8927267 - Flags: review?(scwwu) → review+
Blocks: 1417818
Comment on attachment 8927267 [details]
Bug 1022925 - Part 2: Move alternative name to extension file.

https://reviewboard.mozilla.org/r/198580/#review205314

Since the lazy-loading rules of the address metadata is slightly complicated, I'm thinking it would be clearer to create an object to specifically handle this data and maintain the loading status. Besides, we should have unit tests for these lazy-loading process.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:251
(Diff revision 2)
>     * @returns {object}
>     */
> -  getCountryAddressData(country) {
> +  getCountryAddressData(country, level1 = null) {
>      // Load the addressData if needed
> -    if (!this._addressDataLoaded) {
> -      Object.assign(this, this.loadDataFromScript(ADDRESS_REFERENCES));
> +    if (!this._addressDataLoaded.country) {
> +      Object.assign(this, this.loadAddressReferencesScript(ADDRESS_METADATA_PATH));

I feel `Object.assign` is a bit risky here. Since there should be only one variable, `addressData`, in the metadata, how about simply doing `this.addressData = sanbox.addressData` in `loadAddressReferenceScript`.
Comment on attachment 8927267 [details]
Bug 1022925 - Part 2: Move alternative name to extension file.

https://reviewboard.mozilla.org/r/198580/#review205314

> I feel `Object.assign` is a bit risky here. Since there should be only one variable, `addressData`, in the metadata, how about simply doing `this.addressData = sanbox.addressData` in `loadAddressReferenceScript`.

I still return sandbox in the latest patch because the loadAddressReferenceScript if not only for the root addressData/extension. If the given path is the subfolder, the sandbox will only contain level 1/2 addressData. I'll replace `Object.assign` only for root address in getCountryAddressData.
Comment on attachment 8927267 [details]
Bug 1022925 - Part 2: Move alternative name to extension file.

https://reviewboard.mozilla.org/r/198580/#review206632

::: browser/extensions/formautofill/FormAutofillUtils.jsm:65
(Diff revision 4)
> +      // Return empty object if script loading failed.
> +      return {};

Doesn't this cause `_loadScripts` to return nothing when failed to load `ADDRESS_REFERENCES_EXT` (even `ADDRESS_REFERENCES` was loaded successfully)? Should we assume that `addressReferencesExt.js` always exists?

::: browser/extensions/formautofill/FormAutofillUtils.jsm:70
(Diff revision 4)
> +      // Return empty object if script loading failed.
> +      return {};
> +    }
> +
> +    if (extSandbox.addressData) {
> +      for (let country in extSandbox.addressData) {

nit: Since this function will be also used for level2 data, the keys of `extSandbox.addressData` could be "data/US/CA". Therefore, `country` might not be precise. How about `for (let region in ...` or simply `for (let key in ...`?

::: browser/extensions/formautofill/FormAutofillUtils.jsm:101
(Diff revision 4)
> +    if (!level1) {
> +      return this._addressData[`data/${country}`];
> +    }
> +    // If level1 is set, load addressReferences under country folder with specific
> +    // country/level 1 for level 2 information.
> +    if (!this._dataLoaded.level1.includes(country)) {

nit: I suggest using `new Set()` for `_dataLoaded.level1`. You can use `_dataLoaded.level1.has(country)` here.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:373
(Diff revision 4)
> -      let alternativeCountryNames = ALTERNATIVE_COUNTRY_NAMES[country];
> +      let metaData = this.getCountryAddressData(country);
> +      let alternativeCountryNames = metaData.alternative_names || [metaData.name];

nit: `metadata` is one word.

::: browser/extensions/formautofill/addressmetadata/addressReferences.js:13
(Diff revision 4)
> +// WARNING: DO NOT change the any value or add additional property in addressData
> +// We only allow the supported countries metadata copied from libaddressinput directly.
> +// Please edit addressReferencesExt.js instead if you want to add new property as complement
> +// or overwrite the existing property.

nit: "WARNING: DO NOT change any value or add additional properties in addressData. We only allow the metadata of the supported countries that is copied from libaddressinput directly. Please edit addressReferencesExt.js instead if you want to add new properties as the complement or overwrite the existing properties.

::: browser/extensions/formautofill/addressmetadata/addressReferencesExt.js:10
(Diff revision 4)
> +// This addressDataExt has same country name key like addressData, but with additional
> +// information like alternative names that does not belong to the metadata from libaddressinput
> +// (https://chromium-i18n.appspot.com/ssl-aggregate-address).

nit: "addressDataExt" uses the same key as "addressData" in "addressReferences.js" and contains the information we need but absent in "libaddressinput" such as alternative names.

::: browser/extensions/formautofill/addressmetadata/addressReferencesExt.js:16
(Diff revision 4)
> +// information like alternative names that does not belong to the metadata from libaddressinput
> +// (https://chromium-i18n.appspot.com/ssl-aggregate-address).
> +
> +// TODO: We only support the alternative name of US in MVP. We are going to support more countries in
> +//       bug 1370193.
> +var addressData = {

nit: I prefer `addressDataExt` so we can differentiate it from libaddressinput.

::: browser/extensions/formautofill/test/unit/test_addressDataLoader.js:1
(Diff revision 4)
> +/* global AddressDataLoader */

The exported symbol should be accepted by default as what we did in "test_extractLabelString.js" or "test_findLabelStrings.js". Could you try again if it works without `/* global ... */`?

::: browser/extensions/formautofill/test/unit/test_addressDataLoader.js:36
(Diff revision 4)
> +  let undefinedMetadata = FormAutofillUtils.getCountryAddressData("US", "CA");
> +  Assert.equal(undefinedMetadata, undefined, "metadata should be undefined");
> +  Assert.equal(AddressDataLoader._dataLoaded.level1[0], "US",
> +               "level 1 state array should be set even there's no valid metadata");
> +  AddressDataLoader._loadScripts.reset();

Maybe also test if `_loadScripts` is called here.
Attachment #8927267 - Flags: review?(lchang)
Comment on attachment 8927267 [details]
Bug 1022925 - Part 2: Move alternative name to extension file.

https://reviewboard.mozilla.org/r/198580/#review206632

> Doesn't this cause `_loadScripts` to return nothing when failed to load `ADDRESS_REFERENCES_EXT` (even `ADDRESS_REFERENCES` was loaded successfully)? Should we assume that `addressReferencesExt.js` always exists?

You're right that it's not necessary to assume the addressReferencesExt.js always exists. I'll change it to `return sandbox || {}` since it's not possible that only ext exists.

> The exported symbol should be accepted by default as what we did in "test_extractLabelString.js" or "test_findLabelStrings.js". Could you try again if it works without `/* global ... */`?

Looks like we added LabelUtils in the eslint modules json in tools/lint/eslint/modules.json. I thought we might need to decouple the code from central(maybe not necessary any more), but I'm fine to add this in modules.json
Comment on attachment 8927267 [details]
Bug 1022925 - Part 2: Move alternative name to extension file.

https://reviewboard.mozilla.org/r/198580/#review206744

::: browser/extensions/formautofill/FormAutofillUtils.jsm:65
(Diff revision 5)
> +      // Will return only address references if extension loading failed or empty sandbox if
> +      // address references loading failed.
> +      return sandbox;

I think you can simply leave it blank here.
Attachment #8927267 - Flags: review?(lchang) → review+
Thanks for the review!
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/44f9fc2f03bb
Part 1: move addressReferences to addressmetadata folder. r=lchang,scottwu
https://hg.mozilla.org/integration/autoland/rev/edbf6d586c9a
Part 2: Move alternative name to extension file. r=lchang,scottwu
Keywords: checkin-needed
Backed out for failing browser/base/content/test/static/browser_all_files_referenced.js 

Backout: https://hg.mozilla.org/integration/autoland/rev/857428a0497a1f98472319469f61cca7bdc1919d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=edbf6d586c9aa40325cf1958cc00f0fbdd470bb4

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=146414013&repo=autoland&lineNumber=3256

  INFO -  226 INFO Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 174}]
3255
12:55:10     INFO -  Buffered messages finished
3256
12:55:10    ERROR -  227 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 2, expected 0
3257
12:55:10     INFO -  Stack trace:
3258
12:55:10     INFO -  chrome://mochikit/content/browser-test.js:test_is:1269
3259
12:55:10     INFO -  chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:588
3260
12:55:10     INFO -  Not taking screenshot here: see the one that was previously logged
3261
12:55:10    ERROR -  228 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: resource://formautofill/addressmetadata/addressReferences.js -
3262
12:55:10     INFO -  Stack trace:
3263
12:55:10     INFO -  chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:590
3264
12:55:10     INFO -  Not taking screenshot here: see the one that was previously logged
3265
12:55:10    ERROR -  229 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | unreferenced file: resource://formautofill/addressmetadata/addressReferencesExt.js -
Flags: needinfo?(schung)
Add the addressMetadata folder path in browser_all_files_referenced.js exceptionPath list
Flags: needinfo?(schung)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/53397147e99f
Part 1: move addressReferences to addressmetadata folder. r=lchang,scottwu
https://hg.mozilla.org/integration/autoland/rev/58b9e2a4a9f6
Part 2: Move alternative name to extension file. r=lchang,scottwu
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/53397147e99f
https://hg.mozilla.org/mozilla-central/rev/58b9e2a4a9f6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.