Store phone numbers in E.164 format

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: MattN, Assigned: lchang)

Tracking

(Blocks 1 bug)

52 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M3], )

Attachments

(6 attachments)

There is discussion about using E.164 format for phone numbers in the web payments specs so we should consider using that for storage if it aligns with competitors.
Priority: -- → P3
Assignee

Updated

2 years ago
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Whiteboard: [form autofill] → [form autofill:M3]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Hi Matt,

I retrieved the phonenumberutils, which was removed in bug 1310863, to our codebase and updated the library a bit to fulfill our need. Unit tests are not yet ready but it would be nice if you can take a look in advance and see if I'm on the right track. Thanks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Depends on: 1368008
Reporter

Comment 7

2 years ago
mozreview-review
Comment on attachment 8871615 [details]
Bug 1352324 - (Part 6) Store tel in E.164 format and calculate its components while reading.

https://reviewboard.mozilla.org/r/143094/#review150738

::: browser/extensions/formautofill/ProfileStorage.jsm:438
(Diff revision 3)
> +      // Set "US" as the default region as we only support "en-US" for now.
> +      let tel = PhoneNumber.Parse(profile.tel, profile.country || "US");

You could fallback to the region of `Services.prefs.getCharPref("browser.search.countryCode")` instead though I really don't understand why it was put under the "search" namespace when it's generally useful…

::: browser/extensions/formautofill/ProfileStorage.jsm:441
(Diff revision 3)
> +        if (tel.countryName) {
> +          profile["tel-country-name"] = tel.countryName;
> +        }

I would rather we just not expose this until we need it so it doesn't cause confusion. I also think the API was poorly named since it doesn't actually return the country name, only the country ISO code from what I can tell.

::: browser/extensions/formautofill/ProfileStorage.jsm:453
(Diff revision 3)
> +        // to figure out how to parse numbers from other regions when we support
> +        // new countries in the future.
> +        if (tel.nationalNumber && tel.countryName == "US") {

You should be able to do this as long as the country code is 1:

"The country calling code for all countries participating in the NANP is 1. In international format, an NANP number should be listed as +13015550100" https://en.wikipedia.org/wiki/North_American_Numbering_Plan#Modern_plan
Attachment #8871615 - Flags: review?(MattN+bmo) → review+
Reporter

Comment 8

2 years ago
mozreview-review
Comment on attachment 8871613 [details]
Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5.

https://reviewboard.mozilla.org/r/143090/#review150748

::: commit-message-f81bc:1
(Diff revision 1)
> +Bug 1352324 - (Part 1) Retrieve PhoneNumberUtils from ad87ed1bbed5, r=MattN

Nit: "Retrieve PhoneNumber utility JSMs from ad87ed1bbed5"
Attachment #8871613 - Flags: review?(MattN+bmo) → review+
Reporter

Comment 9

2 years ago
mozreview-review
Comment on attachment 8871613 [details]
Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5.

https://reviewboard.mozilla.org/r/143090/#review150752

::: browser/extensions/formautofill/PhoneNumber.jsm:1
(Diff revision 1)
> +/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

We should bring the tests back too…
Comment on attachment 8871613 [details]
Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5.

https://reviewboard.mozilla.org/r/143090/#review150756

::: browser/extensions/formautofill/PhoneNumber.jsm:2
(Diff revision 1)
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> +
> +// Don't modify this code. Please use:
> +// https://github.com/andreasgal/PhoneNumber.js
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["PhoneNumber"];

These JSMs are also missing license headers. Github says it's Apache 2.0 so it's not just a matter of adding the MPL 2.0 header I think?
Comment on attachment 8871614 [details]
Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill.

https://reviewboard.mozilla.org/r/143092/#review150758

::: browser/extensions/formautofill/PhoneNumber.jsm:4
(Diff revision 1)
>  // Don't modify this code. Please use:
>  // https://github.com/andreasgal/PhoneNumber.js

What is our plan with respect to maintaing this library? Are we planning to submit pull requests upstream for your non-autofill-specific changes?
Assignee

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8871614 [details]
Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill.

https://reviewboard.mozilla.org/r/143092/#review150758

> What is our plan with respect to maintaing this library? Are we planning to submit pull requests upstream for your non-autofill-specific changes?

As we discussed, we'll maintain this library by our own in Form Autofill codebase. I'll update this comment.
Assignee

Comment 13

2 years ago
mozreview-review-reply
Comment on attachment 8871615 [details]
Bug 1352324 - (Part 6) Store tel in E.164 format and calculate its components while reading.

https://reviewboard.mozilla.org/r/143094/#review150738

> I would rather we just not expose this until we need it so it doesn't cause confusion. I also think the API was poorly named since it doesn't actually return the country name, only the country ISO code from what I can tell.

Yeah, it's a bit confusing that the PhoneNumber library use `countryName` to stand for the country code. Agree with you that we don't need it currently so I'll remove it. Thanks.

> You should be able to do this as long as the country code is 1:
> 
> "The country calling code for all countries participating in the NANP is 1. In international format, an NANP number should be listed as +13015550100" https://en.wikipedia.org/wiki/North_American_Numbering_Plan#Modern_plan

Thanks for pointing it out.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 23

2 years ago
mozreview-review-reply
Comment on attachment 8871613 [details]
Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5.

https://reviewboard.mozilla.org/r/143090/#review150752

> We should bring the tests back too…

The old tests are not suit our test framework so I based the tests from the original repo and created our own tests in a separated commit (part 4).
Comment on attachment 8871615 [details]
Bug 1352324 - (Part 6) Store tel in E.164 format and calculate its components while reading.

Hi Matt,

I added some unit tests to this commit. Although you've already reviewed it, would you mind taking a look again?
Comment on attachment 8871613 [details]
Bug 1352324 - (Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5.

Hi Gervase,

In this patch, we are going to bring back some PhoneNumber libraries, which existed in the central but was later removed in bug 1310863, and maintain them in the near future. We noticed that these files lack the licensing header. It was in our codebase so I suppose they're under MPL v2.0. However, these files were originally imported from GitHub [1], where they are declared Apache License v2.0. Could you please help us to figure out which license we can use? Thanks a lot.


[1] https://github.com/andreasgal/PhoneNumber.js
Flags: needinfo?(gerv)
We should use the Apache License 2.0, and (given that upstream is unlikely to be maintaining the code) add license headers ourselves.

Gerv
Flags: needinfo?(gerv)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Gervase, I see. Thanks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8877028 [details]
Bug 1352324 - (Part 2) Update the metadata for PhoneNumber utility to v8.4.1.

https://reviewboard.mozilla.org/r/148360/#review157594
Attachment #8877028 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8871614 [details]
Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill.

https://reviewboard.mozilla.org/r/143092/#review157596

::: .eslintignore:82
(Diff revision 6)
> +# imported from 3rd-party library
> +browser/extensions/formautofill/PhoneNumber.jsm
> +browser/extensions/formautofill/PhoneNumberNormalizer.jsm
> +browser/extensions/formautofill/PhoneNumberMetaData.jsm

Perhaps we should put them in their own directory to make it more clear they were a separate project

::: .eslintignore:82
(Diff revision 6)
>  # generated or library files in activity-stream
>  browser/extensions/activity-stream/data/content/activity-stream.bundle.js
>  browser/extensions/activity-stream/vendor/**
>  # imported from chromium
>  browser/extensions/mortar/**
> +# imported from 3rd-party library

If we're going to maintain them now then why not update them to current eslint style in a separate commit?
Assignee

Comment 38

2 years ago
mozreview-review-reply
Comment on attachment 8871614 [details]
Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill.

https://reviewboard.mozilla.org/r/143092/#review157596

> Perhaps we should put them in their own directory to make it more clear they were a separate project

Do you mean put them in `browser/extensions/formautofill/PhoneNumberUtils/`?

> If we're going to maintain them now then why not update them to current eslint style in a separate commit?

Makes sense. I'll fix them in a separate commit.
Reporter

Comment 39

2 years ago
mozreview-review-reply
Comment on attachment 8871614 [details]
Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill.

https://reviewboard.mozilla.org/r/143092/#review157596

> Do you mean put them in `browser/extensions/formautofill/PhoneNumberUtils/`?

Yes, though maybe lowercase since I don't think we often use uppercase in paths

> Makes sense. I'll fix them in a separate commit.

Thanks
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 46

2 years ago
mozreview-review-reply
Comment on attachment 8871614 [details]
Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill.

https://reviewboard.mozilla.org/r/143092/#review157596

> Yes, though maybe lowercase since I don't think we often use uppercase in paths

Updated. Thanks.

> Thanks

Fixed in the next commit. I left PhoneNumberMetaData.jsm in .eslintignore as it is generated by script.
Comment on attachment 8871614 [details]
Bug 1352324 - (Part 3) Update PhoneNumber utility to fulfill Form Autofill.

https://reviewboard.mozilla.org/r/143092/#review162316
Attachment #8871614 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8882026 [details]
Bug 1352324 - (Part 4) Fix eslint errors of PhoneNumber utility.

https://reviewboard.mozilla.org/r/153098/#review162318

::: .eslintignore:82
(Diff revision 1)
> +# imported from 3rd-party library
> +browser/extensions/formautofill/phonenumberutils/PhoneNumberMetaData.jsm

Nit: Maybe "Generated data files" would be clearer
Attachment #8882026 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8877091 [details]
Bug 1352324 - (Part 5) Add unit tests for PhoneNumber.jsm and PhoneNumberNormalizer.jsm.

https://reviewboard.mozilla.org/r/148422/#review162322

Thanks!
Attachment #8877091 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 59

2 years ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b27e73bea124
(Part 1) Retrieve PhoneNumber utility JSMs from ad87ed1bbed5. r=MattN
https://hg.mozilla.org/integration/autoland/rev/5ebb632fa7bb
(Part 2) Update the metadata for PhoneNumber utility to v8.4.1. r=MattN
https://hg.mozilla.org/integration/autoland/rev/e27d693ddcc4
(Part 3) Update PhoneNumber utility to fulfill Form Autofill. r=MattN
https://hg.mozilla.org/integration/autoland/rev/27971d8110df
(Part 4) Fix eslint errors of PhoneNumber utility. r=MattN
https://hg.mozilla.org/integration/autoland/rev/b69772478582
(Part 5) Add unit tests for PhoneNumber.jsm and PhoneNumberNormalizer.jsm. r=MattN
https://hg.mozilla.org/integration/autoland/rev/7aec8351b1db
(Part 6) Store tel in E.164 format and calculate its components while reading. r=MattN
You need to log in before you can comment on or make changes to this bug.