Closed Bug 796618 Opened 12 years ago Closed 7 years ago

[META] Phone number normalization utils for comms apps

Categories

(Firefox OS Graveyard :: Gaia, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ghtobz, Unassigned)

References

Details

(Whiteboard: [label:dialer][label:contacts][label:sms][LOE:M][label:feature][gh-assignee:borjasalguero]dupme)

[GitHub issue by autonome on 2012-09-10T20:56:31Z, https://github.com/mozilla-b2g/gaia/issues/4528] Not getting a platform fix for V1. Need to implement it in Gaia. https://bugzilla.mozilla.org/show_bug.cgi?id=743363 @albertopq @etiennesegonzac @arcturus Which comms apps need this, and what should the API look like? And who can implement it?
[GitHub comment by arcturus on 2012-09-10T21:47:54Z] @autonome as far as I know will be involved almost all of them (all have contacts integration): Dialer: has the call log + showing the contact detail when an incoming/outgoing call Contacts: self explanatory Sms: we do the threading by contacts as well I don't have the who yet (@borjasalguero and @steveck-chung have been doing a lot of work around this for SMS), we will try to propose and discuss solutions for this ASAP. Thanks!
[GitHub comment by fabricedesre on 2012-09-11T03:41:56Z] I found this other library : https://github.com/floere/phony . We should look if porting this to JS is giving good results with a hopefully smaller codebase.
[GitHub comment by borjasalguero on 2012-09-12T11:49:30Z] There is a proposal based on @steveck-chung work. https://github.com/mozilla-b2g/gaia/pull/4628
[GitHub comment by autonome on 2012-09-12T21:54:52Z] Thanks y'all. Will the work in that pr be usable by other apps? Or is SMS the only app that needs this number disambiguation feature for V1?
[GitHub comment by borjasalguero on 2012-09-13T06:30:11Z] This library it's only needed in SMS App and Dialer. Once this PR will be approved we are going to add the same functionality to Dialer and the loop will be closed by now. We cant neglect that this is a workaround for 28th September deadline, because everybody agree that this functionality should be part of Gecko.
[GitHub comment by autonome on 2012-09-14T18:55:52Z] @steveck-chung can you update on where things are at in pr 4628?
[GitHub comment by vingtetun on 2012-09-14T20:28:22Z] @autonome PR 4628 has been merged. Code needs to be ported to the communications app now.
[GitHub comment by albertopq on 2012-09-17T13:07:38Z] @gtorodelvalle, are you going to port it to the Dialer?
[GitHub comment by gtorodelvalle on 2012-09-17T13:18:40Z] Yes ;-) I'll get in touch with Borja and we will organize the best way to do it :-)
[GitHub comment by gtorodelvalle on 2012-09-17T13:19:13Z] I mean @borjasalguero ;-)
[GitHub comment by borjasalguero on 2012-09-17T13:33:49Z] Im on it. Im trying to analyze how to implement it without impact the 'on_call.html', trying to minimize the effect of adding the library. I hope to have it in Gecko soon! :'(
[GitHub comment by steveck-chung on 2012-09-17T16:49:36Z] Hi @borjasalguero , If you have any problem in using these libs, please contact me and I can check if there is anything I anything I can help with.
[GitHub comment by borjasalguero on 2012-09-17T17:06:38Z] @steveck-chung Could you take a look to this branch https://github.com/borjasalguero/gaia/tree/feature_phonelibnumber_dialer ? You can change whatever you consider if is needed. Thanks again Steve!
[GitHub comment by autonome on 2012-09-26T23:44:16Z] Been a while. Any update here?
[GitHub comment by borjasalguero on 2012-09-26T23:58:58Z] We are waiting to take a decision on that. The PR is #5231
Please reassign to somebody on your team (borjasalguero doesn't have a BZ account).
Assignee: nobody → jmcanterafonseca
Priority: -- → P1
Assignee: jmcanterafonseca → fbsc
Importing a 5MB lib is unacceptable. My understanding is that what's blocking is a concrete list of markets that need to be supported in v1. needsinfo? to gal to find that out from our friends.
Flags: needinfo?(gal)
1 0 17 49 2848K 224K 7476K 26M 2389M 47181 35245 sleeping 502 2073 173 38 18 1 0 17 53 6500K 224K 11M 27M 2390M 47186 35245 sleeping 502 3123 172 38 1 The google library is complete insanity. We have to remove it from the code base. It must not spread to any other apps. Above is a snapshot of loading the metadata.js file only. It contains an in-memory object graph representing number formating information. The metadata alone is 4MB in memory. The library also consumes more than 1MB for source code, and an amount I can't easily measure for compiled code. In other words, this library uses several percent of our total device memory (and I mean total, including kernel, video memory, gecko, what have you not). At the core of the library is a meta data table, which should go into indexeddb, and about 100 lines of parsing code. We can re-use most of the tests from the library. Lets do that. I am happy to help with the design if needed.
Flags: needinfo?(gal)
You can measure the memory usage more accurately with get-about-memory.py.
Only for clarifying this issue, because I think that there are a lot of noise and things are simpler than it looks ;). As you know in our communications app we need a way of distinguish, univocally, a phone number. This issue is not straightforward, ‘numbering’ plans are different and non-standard all over the world, so creating a library for solving this problem it’s not an issue of days. Everybody agree that this functionality should be part of Gecko, so we should push to have this functionality ready in our backend asap (This bug was reported 7 months ago... -https://github.com/mozilla-b2g/gaia/issues/857-). However, with only few weeks to have all apps working properly, and with no support in the backend we created this workaround for having it in Gaia. That’s why, after searching the most complete & tested one, we decided to use ‘phonenumberlib’ from Google (used in Android, Gmail and Facebook). This is only a temporary solution, so will be removed once this feature will be in Gecko. And of course if somebody find a cool library for solving this issue better we could use it for sure! Nobody wants to loose RAM or performance :). But as I said, getting it working properly is tough. Probably we should start now working in this functionality in Backend, because I think we all agree that it is the right solution. So let's work on it! ;) However, we should also try to find a way to check if we could do a workaround at the Gaia level as it's a key point for our product. Original PR was here https://github.com/mozilla-b2g/gaia/pull/5231, and we could work based on that.
:borjasalguero, just talked to :gal he has a first version of a super-slimmed down library. He will share with you so you can maintain it.
Priority: P1 → --
Priority: -- → P3
Whiteboard: [label:dialer][label:contacts][label:sms][LOE:M][label:feature][gh-assignee:borjasalguero] → [label:dialer][label:contacts][label:sms][LOE:M][label:feature][gh-assignee:borjasalguero]dupme
Milestoning for C2 (deadline of 12/10), as this meets the criteria of "known bugs with LOE:M".
Target Milestone: --- → B2G C2 (20nov-10dec)
To carry my voice from bug 810444 into this one (assuming that duping made sense). I have a heck of a time being able to figure out if my texts go through or not since I have to choose wisely which of the two numbers to reply to. Right now I would go to say that my phone is not functional and I can't do texting properly. If this is what is tried to be fixed in here that is awesome! What I do know is that texting used to work reasonably well 4 weeks ago but it is now a mess. Thanks for all your hardwork!
There are some changes coming, so in a near future more than 'adding' normalization to our comms app, we should delete the PhoneNumberJS because will be part of Gecko (for SMS App the work is on progress). Probably we could split this one into bugs as 'Handling normalization properly in SMS APP', 'Handling normalization properly in Dialer'. What do you think?
(In reply to Borja Salguero [:borjasalguero] from comment #25) > There are some changes coming, so in a near future more than 'adding' > normalization to our comms app, we should delete the PhoneNumberJS because > will be part of Gecko (for SMS App the work is on progress). Probably we > could split this one into bugs as 'Handling normalization properly in SMS > APP', 'Handling normalization properly in Dialer'. What do you think? Yep makes sense, 2 patches, 2 bugs.
[:vingtetun] I think that we could consider it a META. I will create the pending bugs.
(In reply to Borja Salguero [:borjasalguero] from comment #29) > [:vingtetun] I think that we could consider it a META. I will create the > pending bugs. Nice. Can you then add the dependency on this bugs in the 'depends on' field. And then I will mark this bug as non-blocking (we don't block on meta bug) and the dependency should blocks.
Hi, I'm now on the stable build of 11-14. Things are better than when I reported bug 810444. Thanks! There is one last thing to notice. If I start a convo with a number without starting with "+1" I will receive back text messages from the "+1" version of that number and have two entries. From there on, I keep on replying to the +1 entry. In fact, I have deleted all of the entries that started with +1. Let me know if you would like me to file a different bug without having a great severity.
Component: Gaia → Gaia::Dialer
blocking-basecamp: + → ---
Summary: phone number normalization utils for comms apps → [META] Phone number normalization utils for comms apps
Assignee: fbsc → nobody
Component: Gaia::Dialer → Gaia
[:vingtetun] Updated as a meta and all dependencies added. Thanks!
Clearing C2 milestone -- not a blocker.
Target Milestone: B2G C2 (20nov-10dec) → ---
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.