This is the Gecko counter part of bug 835750

Noming since this is part of bug 835750 which is TEF+ already

Marking status-b2g18 and status-b2g18-v1.0.0 as affected, please update the status to fixed once this is verified landed on v1-train/mozilla-b2g18 and v1.0.0/mozilla-b2g18_v_1_0_0

Part 3 is marionette test cases. Waiting for feedback+.

Comment on attachment 709006 [details] [diff] [review] Part 1/3: add PhoneNumberUtils.isViablePhoneNumber Gregor is probably a better reviewer here.

Comment on attachment 709006 [details] [diff] [review] Part 1/3: add PhoneNumberUtils.isViablePhoneNumber Review of attachment 709006 [details] [diff] [review]: ----------------------------------------------------------------- We usually patch https://github.com/andreasgal/PhoneNumber.js and merge it here. Please also add some plain PhoneNumberJS tests for the final version. We already have an IsViable function with the current country code? Why doesn't this work? ::: dom/phonenumberutils/PhoneNumber.jsm @@ +18,3 @@ > const NON_ALPHA_CHARS = /[^a-zA-Z]/g; > const NON_DIALABLE_CHARS = /[^,#+\*\d]/g; > + const PLUS_CHARS = "+\uFF0B"; You can also use PLUS_CHARS.source if you don't want to change this. Maybe add _SOURCE instead of _PATTERN if you want to define the regular string in the reminder of the patch? @@ +23,5 @@ > > + const VALID_PUNCTUATION = "-x\u2010-\u2015\u2212\u30FC\uFF0D-\uFF0F \u00A0" > + + "\u200B\u2060\u3000()\uFF08\uFF09\uFF3B\uFF3D." > + + "\\[\\]/~\u2053\u223C\uFF5E"; > + const VALID_DIGITS = "0-9\uFF10-\uFF19\u0660-\u0669\u06F0-\u06F9"; Maybe add UNICODE_DIGITS.source? @@ +29,5 @@ > + const VALID_PHONE_NUMBER = "[" + PLUS_CHARS + "]*(?:[" + VALID_PUNCTUATION > + + "]*[" + VALID_DIGITS + "]){3,}[" > + + VALID_PUNCTUATION + VALID_ALPHA + VALID_DIGITS > + + "]*"; > + const RFC3966_EXTN_PREFIX = ";ext="; That's only used once. @@ +37,5 @@ > + + "(?:ext(?:ensi(?:o\u0301?|\u00F3))?n?|\uFF45\uFF58\uFF54\uFF4E?|" > + + "[,x\uFF58#\uFF03~\uFF5E]|int|anexo|\uFF49\uFF4E\uFF54)" > + + "[:\\.\uFF0E]?[ \u00A0\\t,-]*" > + + CAPTURING_EXTN_DIGITS + "#?|[- ]+([" > + + VALID_DIGITS + "]{1,5})#"; Any explanation what's going on here? @@ +385,5 @@ > > + function isViablePhoneNumber(number) { > + if (number == null || number.length < 3) { > + return false; > + } What about 2 digit numbers without punctuation? I think they are also valid.

(In reply to Gregor Wagner [:gwagner] from comment #7) > Please also add some plain PhoneNumberJS tests for the final > version. Sure, if this is a feedback+. > We already have an IsViable function with the current country > code? Why doesn't this work? If you mean |IsValidNumber(number, md)|, then it takes an additional parameter md, and we have to look up meta data array by some country code. Since we only loosely validate the number here, we don't need country code or per country possible patterns. > ::: dom/phonenumberutils/PhoneNumber.jsm > Any explanation what's going on here? I'll add some comments. > What about 2 digit numbers without punctuation? I think they > are also valid. "Most PBXs support variable-length dial plans that use 3 to 11 digits."[1] Do you have any known, working 2 digit numbers? [1]: http://en.wikipedia.org/wiki/Numbering_plan

Vicamo, why have a check in RadioInterfaceLayer.js and not in Gaia?

Comment on attachment 709006 [details] [diff] [review] Part 1/3: add PhoneNumberUtils.isViablePhoneNumber Yeah looks good to me.

Can someone move this forward while Taipei is in holiday ?

Andrew, please, could you assign someone on this ?

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

(In reply to Anshul from comment #9) > Vicamo, why have a check in RadioInterfaceLayer.js and not in > Gaia? Isn't this the purpose of this bug?

(In reply to Gregor Wagner [:gwagner] from comment #7) > Comment on attachment 709006 [details] [diff] [review] > ::: dom/phonenumberutils/PhoneNumber.jsm > @@ +18,3 @@ > > const NON_ALPHA_CHARS = /[^a-zA-Z]/g; > > const NON_DIALABLE_CHARS = /[^,#+\*\d]/g; > > + const PLUS_CHARS = "+\uFF0B"; > > You can also use PLUS_CHARS.source if you don't want to change > this. Maybe add _SOURCE instead of _PATTERN if you want to > define the regular string in the reminder of the patch? PLUS_CHARS.source is "^[+\uFF0B]+", but in other places I want only "+\uFF0B" or at most "[+\uFF0B]". VALID_ALPHA and VALID_DIGITS both have the same situation.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #14) > (In reply to Anshul from comment #9) > > Vicamo, why have a check in RadioInterfaceLayer.js and not in > > Gaia? > > Isn't this the purpose of this bug? Yep, it is. Even if the check is done on Gaia also this is something that belongs to the platform.

Github pull request: https://github.com/andreasgal/PhoneNumber.js/pull/10 try https://tbpl.mozilla.org/?tree=Try&rev=ac33c14c22e8

Comment on attachment 714315 [details] [diff] [review] Part 2/2 Review of attachment 714315 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/RadioInterfaceLayer.js @@ +1985,5 @@ > + this.handleCallError({ > + callIndex: -1, > + error: RIL.RIL_CALL_FAILCAUSE_TO_GECKO_CALL_ERROR[RIL.CALL_FAIL_UNOBTAINABLE_NUMBER] > + }); > + return; Maybe add a dump here that prints if isViablePhoneNumber returns false? We might event want to see this in a non debug build. Or do we have any gaia feedback here?

Is this now ready to land?

fix marionette test failures.

Add debug message for non-viable phone number passed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdd0daca35d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/d5afa0f17822

https://hg.mozilla.org/mozilla-central/rev/cdd0daca35d1 https://hg.mozilla.org/mozilla-central/rev/d5afa0f17822

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ab6027d9159d https://hg.mozilla.org/releases/mozilla-b2g18/rev/8e0db1c10cbf https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/35194af77dc6 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5d344067e0e7