Julien Wajsberg [:julienw] Reporter
	Description • 11 years ago

We're trying hard to now include any normalization library in gaia, therefore it would be very useful (and probably mandatory for some use cases) to get the normalized values along the value that the user entered.

Asking leo for this because we'll need this for other leo+ bugs.

	Rick Waldron [:rwaldron]
	Comment 1 • 11 years ago

Once this lands, the `Utils.removeNonDialables` can safely be removed

	Reuben Morais [:reuben]
	Comment 2 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #0)
> We're trying hard to not include any normalization library in gaia

Why?

	Mounir Lamouri (:mounir)
	Comment 3 • 11 years ago

I do not think having a contact API automatically normalizing numbers is a good idea. That means developers might set something and then read another value. That also means that users will see a number in the UI that isn't exactly what they entered, right?

Why exactly not having a library for normalisation in Gaia is important?

	Julien Wajsberg [:julienw] Reporter
	Comment 4 • 11 years ago

What I ask here is having an _another_ property keeping that normalized value. Of course we want to keep the value the user entered.

AFAIK the mozContacts API already has this information, so it's only a matter of exposing it.


(In reply to Reuben Morais [:reuben] from comment #2)
> (In reply to Julien Wajsberg [:julienw] from comment #0)
> > We're trying hard to not include any normalization library in gaia
> 
> Why?

I wasn't part of this decision (that was about 6 months ago) but I think the rationale is that all clients would need to do this, and therefore it makes sense to have it in the API. I can hear the arguments that the normalization rules will change over time and therefore this can be problematic for a standardized API. I really don't mind whatever decision we have here, rather I want to stick to one choice and not change every 6 months, and have consistent API working well.

I think we need it in Gecko anyway, and that might be part of the reason we choose to do all this in Gecko.

If we want to revert that decision, there should be a discussion on the mailing list, and not on a bug.

	Mounir Lamouri (:mounir)
	Comment 5 • 11 years ago

The decision to move the normalization out of the client didn't involve the WebAPI team as far as I am aware. I've heard that the library was huge and was a creating memory pressure problems but I also believe that this is wrong that all clients will need to normalize numbers. Except the Contacts, Messaging and Telephony apps (which are built-in) it sounds pretty unlikely that an application will need to play with phone number normalization.

Regarding standardization, Jonas and I want to move the Contacts API to a simple storage so moving any kind of logic to it wouldn't work well.

	bhavana bajaj [:bajaj]
	Comment 6 • 11 years ago

Blocking only because this is needed for other blockers

	Julien Wajsberg [:julienw] Reporter
	Comment 7 • 11 years ago

(note that I'm ok with WONTFIX this if Bug 883929 is leo+; we need only one of them, and I tend to think that using bug 883929 would be cleaner).

	Alex Keybl [:akeybl]
	Comment 8 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #7)
> (note that I'm ok with WONTFIX this if Bug 883929 is leo+; we need only one
> of them, and I tend to think that using bug 883929 would be cleaner).

Which would be lower risk?

	Julien Wajsberg [:julienw] Reporter
	Comment 9 • 11 years ago

I'd say both are equally low risk, since they involve adding a property in the returned contact (in what I suppose should be done).

	Ben Kelly [:bkelly, not reviewing]
	Comment 10 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #7)
> (note that I'm ok with WONTFIX this if Bug 883929 is leo+; we need only one
> of them, and I tend to think that using bug 883929 would be cleaner).

I think from a performance point of view it would be better to pursue bug 883929.  IPC overhead seems non-trivial at the moment, so adding a field to every contact would likely make it harder to scale mozContacts.getAll().

	Jan Jongboom [:janjongboom] (inactive)
	Comment 11 • 11 years ago

I think most problems would be solved if when doing a match query against mozContacts.find the phone number that we matched against would also be included in the query result.

F.e. match query on 0624710190, PhoneNumber.js (in Gecko) knows we're in NL so normalizes it to +31624710190, matches against Mobile1 that had the international representation. Now we can show that in UI.

	Julien Wajsberg [:julienw] Reporter
	Comment 12 • 11 years ago

Jan, that's basically bug 883929, I guess :)

Ben, bug 883929 would do that too, right ?

	Jan Jongboom [:janjongboom] (inactive)
	Comment 13 • 11 years ago

Yep, I think there's another duplicate floating around of that one because I created one as well at some point. Should get better at searching bugzilla :p

	Reuben Morais [:reuben]
	Comment 14 • 11 years ago

(In reply to Jan Jongboom [:janjongboom] from comment #11)
> F.e. match query on 0624710190, PhoneNumber.js (in Gecko) knows we're in NL
> so normalizes it to +31624710190

It doesn't do that. Normalization just removes non-dialable characters, we don't parse the input value. Gaia needs access to PhoneNumberJS so it can normalize the value entered by the user and the value returned by the API, and find the overlap.

	Jan Jongboom [:janjongboom] (inactive)
	Comment 15 • 11 years ago

Uh, not as far as I know. What we do in Gecko is:

- Someone saves a contact
- We store the local and international version of the phone number next to the original input
- When we do a `match` search we generate a local and international number from the input value
- Check this against the already normalized data
- Match? Return that contact

And here is the code in mozilla-central https://hg.mozilla.org/mozilla-central/file/76820c6dff7b/dom/contacts/fallback/ContactDB.jsm#l331

	Reuben Morais [:reuben]
	Comment 16 • 11 years ago

You're looking at the wrong part of it. We do index the (inter)national versions of the number, but we don't parse *the input value*, and we don't know which one of the entries in the multiEntry index was matched either.

https://hg.mozilla.org/mozilla-central/file/50332b66c7a1/dom/contacts/fallback/ContactDB.jsm#l874

	Jan Jongboom [:janjongboom] (inactive)
	Comment 17 • 11 years ago

I don't know what you mean with *the input value* but I see quite some normalization calls in that piece of code as well.

882         let normalized = PhoneNumberUtils.normalize(options.filterValue,
883                                                     /*numbersOnly*/ true);

But yeah, at the moment we don't know which one is matched, but that should be patched then.

	Ben Kelly [:bkelly, not reviewing]
	Comment 18 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #12)
> Ben, bug 883929 would do that too, right ?

True, bug 883929 would also add fields to the returned values.  It just seemed like it would be less of a problem for filtered search results vs. every contact.  It sounds like bug 883929 may be difficult, though.

Would another possibility be to provide the normalized field for every contact, but have ContactsAPI provide a getter that calculates the value just-in-time on the client side?  That way it would still be encapsulated in the API and hidden from the app, but we wouldn't pay the IPC costs.

	Julien Wajsberg [:julienw] Reporter
	Comment 19 • 11 years ago

(In reply to Ben Kelly [:bkelly] from comment #18)
> (In reply to Julien Wajsberg [:julienw] from comment #12)
> > Ben, bug 883929 would do that too, right ?
> 
> True, bug 883929 would also add fields to the returned values.  It just
> seemed like it would be less of a problem for filtered search results vs.
> every contact.  It sounds like bug 883929 may be difficult, though.

We just need to know the property that matched and the actual value that matched. I think we don't need to know the transformed input that matched. Therefore I think we can have this quite easily (or I may miss something).
 
> Would another possibility be to provide the normalized field for every
> contact, but have ContactsAPI provide a getter that calculates the value
> just-in-time on the client side?  That way it would still be encapsulated in
> the API and hidden from the app, but we wouldn't pay the IPC costs.

sounds good to me if that's possible.

	Reuben Morais [:reuben]
	Comment 20 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #19)
> > Would another possibility be to provide the normalized field for every
> > contact, but have ContactsAPI provide a getter that calculates the value
> > just-in-time on the client side?  That way it would still be encapsulated in
> > the API and hidden from the app, but we wouldn't pay the IPC costs.
> 
> sounds good to me if that's possible.

Note that that solution will give you the normalized value for every telephone number in the contact, but won't tell you which one matched for that specific contact. You'd then have to normalize the filterValue passed to the API and find the match in the contact's numbers. That is exactly what ContactDB would do if we decide to send the matched value back from a search.

Adding this functionality in ContactDB means we pay an extra cost in IPC, adding it in the client side means every client has to load PhoneNumberJS with its large data tables. I think we'll end up paying the IPC cost here, since the devices have very limited memory.

	Julien Wajsberg [:julienw] Reporter
	Comment 21 • 11 years ago

(In reply to Reuben Morais [:reuben] from comment #20)
> (In reply to Julien Wajsberg [:julienw] from comment #19)
> > > Would another possibility be to provide the normalized field for every
> > > contact, but have ContactsAPI provide a getter that calculates the value
> > > just-in-time on the client side?  That way it would still be encapsulated in
> > > the API and hidden from the app, but we wouldn't pay the IPC costs.
> > 
> > sounds good to me if that's possible.
> 
> Note that that solution will give you the normalized value for every
> telephone number in the contact, but won't tell you which one matched for
> that specific contact. You'd then have to normalize the filterValue passed
> to the API and find the match in the contact's numbers. That is exactly what
> ContactDB would do if we decide to send the matched value back from a search.


> 
> Adding this functionality in ContactDB means we pay an extra cost in IPC,
> adding it in the client side means every client has to load PhoneNumberJS
> with its large data tables. I think we'll end up paying the IPC cost here,
> since the devices have very limited memory.

I was thinking about that IPC cost this morning. I was thinking that we could have "standard fields" and "extended fields"; standard fields would be always returned, but the caller would need to ask for specific "extended fields" to get these.

eg |mozContacts.find({ filterValue: xxx, extendedFields: ['normalizedValue'] });|

	Ben Kelly [:bkelly, not reviewing]
	Comment 22 • 11 years ago

(In reply to Reuben Morais [:reuben] from comment #20)
> (In reply to Julien Wajsberg [:julienw] from comment #19)
> > > Would another possibility be to provide the normalized field for every
> > > contact, but have ContactsAPI provide a getter that calculates the value
> > > just-in-time on the client side?  That way it would still be encapsulated in
> > > the API and hidden from the app, but we wouldn't pay the IPC costs.
> > 
> > sounds good to me if that's possible.
> 
> Note that that solution will give you the normalized value for every
> telephone number in the contact, but won't tell you which one matched for
> that specific contact. You'd then have to normalize the filterValue passed
> to the API and find the match in the contact's numbers. That is exactly what
> ContactDB would do if we decide to send the matched value back from a search.

Ah, ok.  I only mentioned it because comment 16 sounded like it was hard or undesirable to do in ContactDB.

> Adding this functionality in ContactDB means we pay an extra cost in IPC,
> adding it in the client side means every client has to load PhoneNumberJS
> with its large data tables. I think we'll end up paying the IPC cost here,
> since the devices have very limited memory.

Thanks.  I was not aware of the large penalty for PhoneNumberJS.

(In reply to Julien Wajsberg [:julienw] from comment #21)
> I was thinking about that IPC cost this morning. I was thinking that we
> could have "standard fields" and "extended fields"; standard fields would be
> always returned, but the caller would need to ask for specific "extended
> fields" to get these.
> 
> eg |mozContacts.find({ filterValue: xxx, extendedFields: ['normalizedValue']
> });|

FYI, there is a proof-of-concept that does something like this in bug 873873.

	bhavana bajaj [:bajaj]
	Comment 23 • 11 years ago

Passing to David for assigment

	Julien Wajsberg [:julienw] Reporter
	Comment 24 • 11 years ago

Gregor is currently working on it afaik

	Gregor Wagner [:gwagner]
	Comment 26 • 11 years ago

And Michael is finishing it :)
Thanks!

	Gregor Wagner [:gwagner]
	Comment 28 • 11 years ago

Comment on attachment 768766 [details] [diff] [review]
Contacts Patch

Review of attachment 768766 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! But I want to see more tests :)

::: dom/contacts/fallback/ContactDB.jsm
@@ +353,5 @@
>          };
>        } else if (currVersion == 10) {
>          if (DEBUG) debug("Adding object store for database revision");
>          db.createObjectStore(REVISION_STORE).put(0, REVISION_KEY);
> +      } else if (currVersion === 11) {

add an if (DEBUG) ... explanation for the upgrade.

@@ +477,5 @@
>            if (aContact.properties[field][i]) {
>              if (field == "tel" && aContact.properties[field][i].value) {
>                let number = aContact.properties.tel[i].value.toString();
>                let normalized = PhoneNumberUtils.normalize(number);
> + 

nit: whitespace

::: dom/contacts/tests/test_contacts_basics.html
@@ +750,5 @@
> +    } catch () {
> +      ok(true, "Successfully failed to set readonly attribute normalizedValue");
> +      next();
> +    }
> +  },

We need a few more tests here. Also add a test where you pass in an object like { name:' test', 'tel': {normalizedValue: '1234'}} and when you retrieve the object normalizedValue shouldn't be set.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 29 • 11 years ago

Fixed the test case.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 30 • 11 years ago

Removed ability to manually set normalizedValue. Added more test cases.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 31 • 11 years ago

Comment on attachment 768843 [details] [diff] [review]
Updated Patch

The patch in 883770 makes this code out of date. I will update the patch here.

	Mounir Lamouri (:mounir)
	Comment 33 • 11 years ago

When I asked on IRC why we need to return the normalised value, I was told that numbers like 800-ABCDEF can't be called so we need to return this number with digits only. Is there any reason why the Telephony API can't call numbers with letters?

	Julien Wajsberg [:julienw] Reporter
	Comment 34 • 11 years ago

Sorry Mounir, that's not the reason. AFAIK the telephony API can do it if they use the same normalize function (I think they do).

Here, we need it to do _matching_.

	Gregor Wagner [:gwagner]
	Comment 35 • 11 years ago

Comment on attachment 770048 [details] [diff] [review]
normalizedValue patch

Review of attachment 770048 [details] [diff] [review]:
-----------------------------------------------------------------

+sicking for SR

::: dom/contacts/fallback/ContactDB.jsm
@@ +369,5 @@
> +          if (cursor) {
> +            if (cursor.value.properties.tel) {
> +              cursor.value.properties.tel.forEach(
> +                function(tel) {
> +                  tel.normalizedValue = PhoneNumberUtils.normalize(tel.value.toString());

I think we need another check if tel.value exists.

@@ +489,5 @@
> +              if (aContact.properties[field][i].value) {
> +                let number = aContact.properties.tel[i].value.toString();
> +                let normalized = PhoneNumberUtils.normalize(number);
> +
> +

nit: remove one newline

	Gregor Wagner [:gwagner]
	Comment 36 • 11 years ago

And you didn't fix my first comments so lets do another round.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 37 • 11 years ago

Updating the patch per Gregor's comments.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 38 • 11 years ago

In general I'm not a fan of "normalizing" numbers, since it seems like it so often isn't possible to do reliably. I.e. many times we have to guess at which country code to normalize to.

I prefer instead to express it as having a fuzzy-match function which can test if two phone numbers are the same. I.e. that we expose such a fuzzy-match function from gecko to apps.

Would that at all work here?

	Julien Wajsberg [:julienw] Reporter
	Comment 39 • 11 years ago

This probably would work :)

but do we want to do this for leo ?

	Gregor Wagner [:gwagner]
	Comment 40 • 11 years ago

(In reply to Jonas Sicking (:sicking) from comment #38)
> In general I'm not a fan of "normalizing" numbers, since it seems like it so
> often isn't possible to do reliably. I.e. many times we have to guess at
> which country code to normalize to.
> 
> I prefer instead to express it as having a fuzzy-match function which can
> test if two phone numbers are the same. I.e. that we expose such a
> fuzzy-match function from gecko to apps.
> 
> Would that at all work here?

I think we are talking about 2 different versions of normalizing here. In this case, normalizing only means we are getting rid of non-dialable chars and do  ITU E.161 conversion: 800ABC -> 800222
We are not talking about parsing/converting it to different formats here.
Another solution would be to extract the 10 lines from PhoneNumberJS that deals with normalization and expose it to gaia apps. We wouldn't need to include the whole library all the time and it would be a slim library.

	Julien Wajsberg [:julienw] Reporter
	Comment 41 • 11 years ago

I really like the "fuzzy match API" idea. Because most of the time, if we need normalized values, it's actually to match.

I'm concerned about IPC overhead if we need to call it a lot though.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 42 • 11 years ago

I don't think we should worry about IPC overhead prematurely. Worst case the fuzzymatch function can probably be implemented entirely within the child process since it doesn't need to interact with hardware at all.

	Gregor Wagner [:gwagner]
	Comment 45 • 11 years ago

Now with all the files.

	Gregor Wagner [:gwagner]
	Comment 46 • 11 years ago

Now with webidl

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 47 • 11 years ago

(In reply to Gregor Wagner [:gwagner] from comment #46)
> Created attachment 775896 [details] [diff] [review]
> Prototype fuzzyMatching
> 
> Now with webidl

Looks good! I'm going to add some tests.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 48 • 11 years ago

diff --git a/dom/phonenumberutils/PhoneNumberUtils.jsm b/dom/phonenumberutils/PhoneNumberUtils.jsm
@@ -88,13 +95,70 @@ this.PhoneNumberUtils = {
+    let parsed1 = this.parse(aNumber1);
+    let parsed2 = this.parse(aNumber2);
+    if (parsed1 && parsed2) {
+      if (parsed1.internationalNumber === parsed2.internationalNumber || parsed1.nationalNumber === parsed2.nationalNumber) {
+        return true;
+      }
+    }

Can you give me an example of a case where the normalized numbers wouldn't match, but the parsed national or international numbers would? My thought is that since both these numbers are going to be considered in the same country, the would have already matched for normalized numbers.

	Gregor Wagner [:gwagner]
	Comment 49 • 11 years ago

(In reply to Michael Henretty [:mikehenrty] from comment #48)
> diff --git a/dom/phonenumberutils/PhoneNumberUtils.jsm
> b/dom/phonenumberutils/PhoneNumberUtils.jsm
> @@ -88,13 +95,70 @@ this.PhoneNumberUtils = {
> +    let parsed1 = this.parse(aNumber1);
> +    let parsed2 = this.parse(aNumber2);
> +    if (parsed1 && parsed2) {
> +      if (parsed1.internationalNumber === parsed2.internationalNumber ||
> parsed1.nationalNumber === parsed2.nationalNumber) {
> +        return true;
> +      }
> +    }
> 
> Can you give me an example of a case where the normalized numbers wouldn't
> match, but the parsed national or international numbers would? My thought is
> that since both these numbers are going to be considered in the same
> country, the would have already matched for normalized numbers.

Normalizing doesn't use any country information. Normalizing just runs some basic regular expressions on the number so that we have a uniform representation we can use for applying the country specific regular expressions.
Common normalization steps are replacing non-dialable chars, country specific pre-fixes like multiple '+' or '00' or letters to numbers according to the ITU E.161 standard.

For example in the US the normalized version of 415-123-4567 is 4151234567 and the normalized version of +14151234567 is +14151234567.
So even after the normalization step we have 2 different numbers.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 50 • 11 years ago

Fixed a couple of minor issues, and added some more tests.

Gregor, what are you thoughts on doing this for Leo?

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 51 • 11 years ago

After talking to Mounir, we want to restrict the new PhoneNumberService (which exposes the FuzzyMatch functionality) to certified apps only. This will take me a day or two to implement properly in webIDL and then get a review.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 52 • 11 years ago

WIP patch for PhoneNumberService with Fuzzy Matching exposed on the navigator object to certified apps only.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 53 • 11 years ago

(WIP) Working FuzzyMatching for certified apps only. Needs more tests now.

	Gregor Wagner [:gwagner]
	Comment 54 • 11 years ago

Comment on attachment 784209 [details] [diff] [review]
fuzzy.patch

Review of attachment 784209 [details] [diff] [review]:
-----------------------------------------------------------------

Needs some cleanup but looks good so far!

::: dom/base/Navigator.cpp
@@ +1635,5 @@
>  bool
> +Navigator::HasPhoneNumberSupport(JSContext* /* unused */, JSObject* aGlobal)
> +{
> +  return true;
> +  //uint16_t appStatus;

Initialize the variable with the lowest app status.

::: dom/phonenumberutils/PhoneNumberService.js
@@ +69,5 @@
> +    if (aNumber1 === aNumber2) {
> +      simpleMatch = true;
> +    } else if (PhoneNumberNormalizer.Normalize(aNumber1) === PhoneNumberNormalizer.Normalize(aNumber2)) {
> +      simpleMatch = true;
> +    }

This can be simplified :).
I don't think we need the simpleMatch variable.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 55 • 11 years ago

Added chrome tests to bypass app status security.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 56 • 11 years ago

I think this patch is just about ready.

We now have tests which validate that the PhoneNumberService API is restricted to certified apps, and that the API works as expected and only uses IPC when it has to for complicated fuzzy logic.

Try is not done yet, but looking pretty good so far: https://tbpl.mozilla.org/?tree=Try&rev=413cce930214

I've also created a simple app that uses fuzzy match, and installed it as both a certified and non-certified app. Everything is behaving as expected there.

Note: we can simplify slightly after Bug 900994, but since that won't land in b2g-18 and this is a leo+, let's stick with this solution for consistency.

Gregor, thoughts? Should we pass to Mounir for super?

	Julien Wajsberg [:julienw] Reporter
	Comment 57 • 11 years ago

(changing the title for the new patch)

	Gregor Wagner [:gwagner]
	Comment 58 • 11 years ago

(In reply to Michael Henretty [:mikehenrty] from comment #56)
> Created attachment 786075 [details] [diff] [review]
> fuzzy.patch
> 
> I think this patch is just about ready.
> 
> We now have tests which validate that the PhoneNumberService API is
> restricted to certified apps, and that the API works as expected and only
> uses IPC when it has to for complicated fuzzy logic.
> 
> Try is not done yet, but looking pretty good so far:
> https://tbpl.mozilla.org/?tree=Try&rev=413cce930214
> 
> I've also created a simple app that uses fuzzy match, and installed it as
> both a certified and non-certified app. Everything is behaving as expected
> there.
> 
> Note: we can simplify slightly after Bug 900994, but since that won't land
> in b2g-18 and this is a leo+, let's stick with this solution for consistency.
> 
> Gregor, thoughts? Should we pass to Mounir for super?

This is only exposed to certified apps so I don't think we need a super review here.
The patch looks very familiar so I will ask fabrice for an additional review.

For the leo+ question: I don't think the use-case we are fixing here should be a blocker. We should take this patch but not for 1.1 in my opinion. But I will let others decide.

	Gregor Wagner [:gwagner]
	Comment 59 • 11 years ago

Comment on attachment 786075 [details] [diff] [review]
fuzzy.patch

Review of attachment 786075 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me but since I wrote part of this patch I shouldn't review it.

::: dom/phonenumberutils/PhoneNumberService.js
@@ +45,5 @@
> +        break;
> +      case "PhoneNumberService:FuzzyMatch:Return:OK":
> +        req = this.getRequest(msg.requestID);
> +        if (req) {
> +          let bool = msg.result;

nit: no need to create bool here.

::: dom/phonenumberutils/tests/test_phonenumberservice.xul
@@ +21,5 @@
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +var PhoneNumberService = Components.classes["@mozilla.org/phoneNumberService;1"]
> +                                   .getService().wrappedJSObject;
> +Services.prefs.setIntPref("dom.phonenumber.substringmatching.BR", 8);

Since you are setting it, can you test it?

@@ +37,5 @@
> +
> +var req;
> +var index = 0;
> +var mozPhoneNumberService = PhoneNumberService;
> +//var mozPhoneNumberService = window.navigator.mozPhoneNumberService;

nit: remove

::: dom/phonenumberutils/tests/test_phonenumberutils_basics.html
@@ +22,5 @@
> +"use strict";
> +
> +var substringLength = 8;
> +SpecialPowers.setIntPref("dom.phonenumber.substringmatching.BR", substringLength);
> +SpecialPowers.setCharPref("ril.lastKnownSimMcc", "310");

Why do we need this here?

	Reuben Morais [:reuben]
	Comment 60 • 11 years ago

(In reply to Michael Henretty [:mikehenrty] from comment #56)
> Note: we can simplify slightly after Bug 900994, but since that won't land
> in b2g-18 and this is a leo+, let's stick with this solution for consistency.

Things that also won't land on b2g18: JS-implemented WebIDL, WebIDL navigator, Func extended attribute… If you want to land this on b2g18 you'll need a very different patch.

	Gregor Wagner [:gwagner]
	Comment 61 • 11 years ago

(In reply to Reuben Morais [:reuben] from comment #60)
> (In reply to Michael Henretty [:mikehenrty] from comment #56)
> > Note: we can simplify slightly after Bug 900994, but since that won't land
> > in b2g-18 and this is a leo+, let's stick with this solution for consistency.
> 
> Things that also won't land on b2g18: JS-implemented WebIDL, WebIDL
> navigator, Func extended attribute… If you want to land this on b2g18 you'll
> need a very different patch.

A much simpler one :)

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 62 • 11 years ago

Addresses Gregor's comments.

	Julien Wajsberg [:julienw] Reporter
	Comment 63 • 11 years ago

(In reply to Gregor Wagner [:gwagner] from comment #58)
> For the leo+ question: I don't think the use-case we are fixing here should
> be a blocker. We should take this patch but not for 1.1 in my opinion. But I
> will let others decide.

I asked leo? again on the blocked bug 877718; if it's being leo- we'll leo- this bug too.

	Julien Wajsberg [:julienw] Reporter
	Comment 64 • 11 years ago

bug 877718 was put to koi? so moving this bug as well.

	[:fabrice] Fabrice Desré
	Comment 65 • 11 years ago

Comment on attachment 786511 [details] [diff] [review]
fuzzy.patch

Review of attachment 786511 [details] [diff] [review]:
-----------------------------------------------------------------

That looks mostly good, but I'd like to do a second pass with the nits fixed.

::: dom/phonenumberutils/PhoneNumberService.js
@@ +25,5 @@
> +
> +function PhoneNumberService()
> +{
> +  if (DEBUG) debug("Constructor");
> +  this.wrappedJSObject = this;

It looks like you don't need that.

@@ +40,5 @@
> +    switch (aMessage.name) {
> +      case "PhoneNumberService:FuzzyMatch:Return:KO":
> +        req = this.getRequest(msg.requestID);
> +        if (req)
> +          Services.DOMRequest.fireError(req.request, msg.errorMsg);

nit: if (req) {...}

@@ +51,5 @@
> +        break;
> +      default:
> +        if (DEBUG) debug("Wrong message: " + aMessage.name);
> +    }
> +    this.removeRequest(msg.requestID);

you end up here even in the default case (wrong message) but you have probably no request at this point.

You could change all that to do:
let req = this.takeRequest(msg.requestID);
if (!req) {
  return;
}
and remove the getRequests() calls.

@@ +89,5 @@
> +
> +  // Called from DOMRequestIpcHelper
> +  uninit: function uninit() {
> +    if (DEBUG) debug("uninit call");
> +  },

No need to define this function if you're not doing any cleanup.

::: dom/phonenumberutils/PhoneNumberUtils.jsm
@@ +121,5 @@
> +        return true;
> +      }
> +    }
> +    let countryName = this.getCountryName();
> +    if (Services.prefs.getPrefType("dom.phonenumber.substringmatching." + countryName) == Ci.nsIPrefBranch.PREF_INT) {

nit: is that less than 80 chars?

@@ +154,5 @@
> +
> +let inParent = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
> +                 .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT;
> +if (inParent) {
> +  Cu.import("resource://gre/modules/PhoneNumber.jsm");

You already import this one at the beginning of the file.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 66 • 11 years ago

Updated the patch based on :fabrice comments.

(In reply to Fabrice Desré [:fabrice] from comment #65)
> ::: dom/phonenumberutils/PhoneNumberService.js
> @@ +25,5 @@
> > +
> > +function PhoneNumberService()
> > +{
> > +  if (DEBUG) debug("Constructor");
> > +  this.wrappedJSObject = this;
> 
> It looks like you don't need that.

The wrappedJSObject is only for testing, i.e. so I could import PhoneNumberService in the context of a mochitest-chrome test (test_phonenumberservice.xul).

I did this because the PhoneNumberService API is restricted to certified apps only, and importing PhoneNumberService using xpcom was the only way I could figure out how to bypass the permission check (Func="Navigator::HasPhoneNumberSupport" in the webidl file). Is there is a better way to either import a JS-implemented xpcom object (ie without using wrappedJSObject), or a better way to test certified-only API's?

	[:fabrice] Fabrice Desré
	Comment 67 • 11 years ago

Can't you just QI to nsIDOMGlobalPropertyInitializer when you need to call init(window)?

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 68 • 11 years ago

After speaking with sicking about how to properly test certified APIs, he suggested adding a permission specifically for the phone number service, and then having the chrome test use SpecialPowers to add the permission for the test. This patch does that. Fabrice, any other concerns?

	Candice Serran (:cserran)
	Comment 69 • 11 years ago

Candice Serran changed story state to started in Pivotal Tracker

	[:fabrice] Fabrice Desré
	Comment 70 • 11 years ago

Comment on attachment 788475 [details] [diff] [review]
fuzzy.patch

Review of attachment 788475 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed

::: dom/base/Navigator.cpp
@@ +7,5 @@
>  // Needs to be first.
>  #include "base/basictypes.h"
>  
>  #include "Navigator.h"
> +#include "nsIPrincipal.h"

You don't use that.

::: dom/phonenumberutils/PhoneNumberUtils.jsm
@@ +100,5 @@
>      return isPlain;
>    },
>  
>    normalize: function Normalize(aNumber, aNumbersOnly) {
> +    var normalized = PhoneNumberNormalizer.Normalize(aNumber, aNumbersOnly);

s/var/let

@@ +138,5 @@
> +    let msg = aMessage.data;
> +
> +    switch (aMessage.name) {
> +      case "PhoneNumberService:FuzzyMatch":
> +        let result = this.fuzzyMatch(msg.options.number1, msg.options.number2);

no reason for result here, you can just move this.fuzzyMatch() to sendAsyncMessage

::: dom/phonenumberutils/tests/test_phonenumberservice.xul
@@ +27,5 @@
> +
> +var pm = SpecialPowers.Cc["@mozilla.org/permissionmanager;1"]
> +                      .getService(SpecialPowers.Ci.nsIPermissionManager);
> +
> +pm.addFromPrincipal(window.document.nodePrincipal, "phonenumberservice", 

nit: trailing whitespace

@@ +82,5 @@
> +      is(req.result, false, "missing first argument should not match");
> +      next();
> +    };
> +    req.onerror = onFailure;
> +  },

I would love to see more failure case tests that are testing real numbers and not edge cases.

	[:fabrice] Fabrice Desré
	Comment 71 • 11 years ago

Comment on attachment 788475 [details] [diff] [review]
fuzzy.patch

Review of attachment 788475 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed

::: dom/base/Navigator.cpp
@@ +7,5 @@
>  // Needs to be first.
>  #include "base/basictypes.h"
>  
>  #include "Navigator.h"
> +#include "nsIPrincipal.h"

You don't use that.

::: dom/phonenumberutils/PhoneNumberUtils.jsm
@@ +100,5 @@
>      return isPlain;
>    },
>  
>    normalize: function Normalize(aNumber, aNumbersOnly) {
> +    var normalized = PhoneNumberNormalizer.Normalize(aNumber, aNumbersOnly);

s/var/let

@@ +138,5 @@
> +    let msg = aMessage.data;
> +
> +    switch (aMessage.name) {
> +      case "PhoneNumberService:FuzzyMatch":
> +        let result = this.fuzzyMatch(msg.options.number1, msg.options.number2);

no reason for result here, you can just move this.fuzzyMatch() to sendAsyncMessage

::: dom/phonenumberutils/tests/test_phonenumberservice.xul
@@ +27,5 @@
> +
> +var pm = SpecialPowers.Cc["@mozilla.org/permissionmanager;1"]
> +                      .getService(SpecialPowers.Ci.nsIPermissionManager);
> +
> +pm.addFromPrincipal(window.document.nodePrincipal, "phonenumberservice", 

nit: trailing whitespace

@@ +82,5 @@
> +      is(req.result, false, "missing first argument should not match");
> +      next();
> +    };
> +    req.onerror = onFailure;
> +  },

I would love to see more failure case tests that are testing real numbers and not edge cases.

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 72 • 11 years ago

Ready to go!

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 73 • 11 years ago

Comment on attachment 770618 [details] [diff] [review]
Patch with normalizedValue

This patch is no longer relevant.

	Gregor Wagner [:gwagner]
	Comment 74 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/7d1dfd3f5527

	Ryan VanderMeulen [:RyanVM]
	Comment 75 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/7d1dfd3f5527

	Julien Wajsberg [:julienw] Reporter
	Comment 76 • 11 years ago

Just to be sure: the Normalized lives in the app process, right ?

	Gregor Wagner [:gwagner]
	Comment 77 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #76)
> Just to be sure: the Normalized lives in the app process, right ?

The normalize logic lives in both. The parsing logic including the huge regexp table is only in the parent.

	Julien Wajsberg [:julienw] Reporter
	Comment 78 • 11 years ago

What I'd like to know is whether calling `mozPhoneNumberService.normalize` will trigger an IPC call.

	Gregor Wagner [:gwagner]
	Comment 79 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #78)
> What I'd like to know is whether calling `mozPhoneNumberService.normalize`
> will trigger an IPC call.

Nope

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 80 • 11 years ago

(In reply to Julien Wajsberg [:julienw] from comment #78)
> What I'd like to know is whether calling `mozPhoneNumberService.normalize`
> will trigger an IPC call.

Also note, that mozPhoneNumberService.fuzzyMatch() will not trigger an IPC call either if the two numbers can be matched after they are normalized. IPC is only triggered if we need to do the complex country code logic.

	Julien Wajsberg [:julienw] Reporter
	Comment 81 • 11 years ago

Oh, great, thanks !

	Candice Serran (:cserran)
	Comment 82 • 11 years ago

Gregor Wagner changed story state to finished in Pivotal Tracker

	Candice Serran (:cserran)
	Comment 83 • 11 years ago

Gregor Wagner changed story state to delivered in Pivotal Tracker

	Candice Serran (:cserran)
	Comment 84 • 11 years ago

Gregor Wagner changed story state to accepted in Pivotal Tracker

	bhavana bajaj [:bajaj]
	Comment 85 • 11 years ago

Given this fixed on 26, I am removing the koi nom here.

	Rick Waldron [:rwaldron]
	Comment 86 • 11 years ago

I'm trying to finish up #877718, but it seems like none of the methods of navigator.mozPhoneNumberService (shown here https://hg.mozilla.org/mozilla-central/rev/7d1dfd3f5527) are actually defined in Nightly?

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 87 • 11 years ago

I just verified it doesn't work in the latest m-c. I'll do a bisect tomorrow to see where this broke.

	Rick Waldron [:rwaldron]
	Comment 88 • 11 years ago

(In reply to Michael Henretty [:mikehenrty] from comment #87)
> I just verified it doesn't work in the latest m-c. I'll do a bisect tomorrow
> to see where this broke.

Thanks!

	Rick Waldron [:rwaldron]
	Comment 89 • 11 years ago

Mike, 

I was reviewing https://hg.mozilla.org/mozilla-central/rev/7d1dfd3f5527 to get an understanding for how this API will work (as I plan for #877718) and was curious about the async design of the API. It will be awkward to use a DOMRequest object where we need the fuzzy comparison result in loops: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.js#L1845-L1847, https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/utils.js#L248-L250, https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/utils.js#L463-L467. Is there any design insight you can share? Thanks!

	Michael Henretty [:mikehenrty][:mhenretty] Assignee
	Comment 90 • 11 years ago

(In reply to Rick Waldron from comment #89)
> It will be awkward to use a
> DOMRequest object where we need the fuzzy comparison result in loops:

Unfortunately, Fuzzy Match needs to be asynchronous. Some of the matching logic requires using the phone number metadata for all countries, which is quite a large file. For performance reasons, we only load that file in the parent process, and the children pass messages to leverage it's logic. Does that answer your question?

	Reuben Morais [:reuben]
	Comment 91 • 11 years ago

(In reply to Michael Henretty [:mikehenrty] from comment #90)
> (In reply to Rick Waldron from comment #89)
> > It will be awkward to use a
> > DOMRequest object where we need the fuzzy comparison result in loops:
> 
> Unfortunately, Fuzzy Match needs to be asynchronous. Some of the matching
> logic requires using the phone number metadata for all countries, which is
> quite a large file. For performance reasons, we only load that file in the
> parent process, and the children pass messages to leverage it's logic. Does
> that answer your question?

Could we perhaps add a batch matching functionality? That way a user can collect all the input data (hopefully not an expensive operation in itself) and then do a single call. This also minimizes IPC costs, logic to handle asyncness in the user, etc. Kinda like mozSettings allows you to get and set multiple settings with a single call.

	Gregor Wagner [:gwagner]
	Comment 92 • 11 years ago

(In reply to Rick Waldron from comment #89)
> Mike, 
> 
> I was reviewing https://hg.mozilla.org/mozilla-central/rev/7d1dfd3f5527 to
> get an understanding for how this API will work (as I plan for #877718) and
> was curious about the async design of the API. It will be awkward to use a
> DOMRequest object where we need the fuzzy comparison result in loops:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_ui.
> js#L1845-L1847,
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/utils.js#L248-
> L250,
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/utils.js#L463-
> L467. Is there any design insight you can share? Thanks!

We can try to be smarter here and we don't want to have an IPC roundtrip for every loop iteration.
File a followup bug if you need that. Like Reuben says, we should be able to handle multiple numbers with a single call if needed and it becomes a problem.

	Rick Waldron [:rwaldron]
	Comment 93 • 11 years ago

Thanks for all of the follow up info! 

(In reply to Gregor Wagner [:gwagner] from comment #92)
> We can try to be smarter here and we don't want to have an IPC roundtrip for
> every loop iteration.
> File a followup bug if you need that. Like Reuben says, we should be able to
> handle multiple numbers with a single call if needed and it becomes a
> problem.


(In reply to Reuben Morais [:reuben] from comment #91)
> Could we perhaps add a batch matching functionality? That way a user can
> collect all the input data (hopefully not an expensive operation in itself)
> and then do a single call. This also minimizes IPC costs, logic to handle
> asyncness in the user, etc. Kinda like mozSettings allows you to get and set
> multiple settings with a single call.

While I do appreciate the proactive attention to our issue I'm afraid that batch processing doesn't address the use case. Let me try to better illustrate... for brevity, we'll have two users, A and B. A is the user with a FirefoxOS phone, B is a friend of A.

A creates a new contact entry for B. B has two numbers, the are entered as:
  1. number: "999-123-4567", type: "Mobile", carrier: "Fonez-4-Me"
  2. number: "999-BUY-A-CAR", type: "Work", carrier: "Fonez-4-Work"

B sends A a text message from their work phone. 

The message is received by the Messages app with the sender number normalized to "+19992892227" (this is always how they are received).

The Messages app then does a lookup request with mozContacts.find(...), which will result in a contact record for B. 

The Messages app needs to display the "{type} | {carrier}" information in the header of the conversation UI, to determine which type and carrier the number "+19992892227" is associated with, we loop through the contact record's "tel" property, whose value is an array and looks like this: 

  tel: [
    {
      value: '999-123-4567',
      type: ['Mobile'],
      carrier: "Fonez-4-Me"
    },
    {
      value: '999-BUY-A-CAR',
      type: ['Work'], 
      carrier: "Fonez-4-Work"
    }
  ]

In the loop, we need to compare the value of our thread's sender property, which is "+19992892227" (remember: always normalized) to the value stored in the contact record (which is the value that A actually typed into the contact when it was created). 
 
So it's not that we need to check all of the numbers stored in a contact to determine the correct contact—we need to determine which number is the correct number to use for a given contact. 

If there is no way around this, then we will proceed with updating the application to derive the correct tel record asynchronously.