811539 - [SMS] Delete PhoneNumberJS once will be available in Gecko

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect, P2)

Description

[Borja Salguero [:borjasalguero]]

Delete the workaround and delegate all normalization of phone number in Gecko.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Argh. I misread the bug. Let's reopen it and sorry for the spam.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Borja are you running a custom build with bug 811538 in order to go ahead or are you waiting for the code to land on beta?

Comment 4

[Borja Salguero [:borjasalguero]]

Before deleting PhoneNumberJS there are some code in Gecko that should be ready:
- We need to have 'error/sending' states landed in SMS DB. Because for keeping our 'pendingDB' in SMS App we need PhoneNumberJS.
- We need to have it in 'Contacts DB', due to for requesting a contact currently I need PhoneNumberJS for generating all possibilities given a phone number.
- We need to have it in 'SMS DB' as well.

The patch in Gaia is almost done, but I have to check a lot of scenarios separately as you can see.

Comment 5

[Andrew Overholt [:overholt]]

If we can land this by Monday for C2, that's awesome, but in case we can't I'm preemptively moving it to C3 and have ensured the blockers are all scheduled for C2.

Comment 6

[Gregor Wagner [:gwagner]]

I tested with a gaia branch where I removed PhoneNumberJS from gaia. Sending a sms to a local number and receiving with international number works fine. Both messages are displayed in the same thread. 
Saving a contact with local number and receiving a sms with international number finds the appropriate contact from the contacts DB as well.

My code:
https://github.com/gregorwagner/gaia/tree/phone

Comment 7

[Borja Salguero [:borjasalguero]]

[:gwagner] I will take a look at your code today. We are working on landing Building Blocks first in order to have the UI ready and as soon as the PR will be ready Im gonna start with it. Thanks!

Comment 8

[Dietrich Ayala (:dietrich)]

Driver retriage: It sounds like there's no user impact to leaving this code in the tree. Please submit a patch after all higher priority bugs are fixed, and we'll take on approval.

If there *is* user impact to leaving this code in, then renominate and explain.

Comment 9

[Gregor Wagner [:gwagner]]

(In reply to Dietrich Ayala (:dietrich) from comment #8)
> Driver retriage: It sounds like there's no user impact to leaving this code
> in the tree. Please submit a patch after all higher priority bugs are fixed,
> and we'll take on approval.
> 
> If there *is* user impact to leaving this code in, then renominate and
> explain.

This bug is more about relying on the gecko version and removing the usage of the already redundant PhoneNumberJS version in gaia. I spent a lot of time fixing bugs in the gecko version and most of the fixes are not merged to the gaia version.
Removing PhoneNumberJS from gaia once it becomes dead code is optional but it shouldn't take longer than 10 mins.

It's hard to argue about user impact but I believe that keeping both versions will result in random bugs that require 10x more time than fixing this bug.

Comment 10

[Borja Salguero [:borjasalguero]]

(In reply to Gregor Wagner [:gwagner] from comment #9)
> (In reply to Dietrich Ayala (:dietrich) from comment #8)
> > Driver retriage: It sounds like there's no user impact to leaving this code
> > in the tree. Please submit a patch after all higher priority bugs are fixed,
> > and we'll take on approval.
> > 
> > If there *is* user impact to leaving this code in, then renominate and
> > explain.
> 
> This bug is more about relying on the gecko version and removing the usage
> of the already redundant PhoneNumberJS version in gaia. I spent a lot of
> time fixing bugs in the gecko version and most of the fixes are not merged
> to the gaia version.
> Removing PhoneNumberJS from gaia once it becomes dead code is optional but
> it shouldn't take longer than 10 mins.
> 

Only for adding some comments here. Removing the Library is not 10 mins. due to our 'PendingMsg' DB is tied to this library, so removing the library means modifying the code of 'sending/error' message handling. But as yesterday Building Blocks were landed, today I will work on the solution and I will try to get the patch ready asap.

> It's hard to argue about user impact but I believe that keeping both
> versions will result in random bugs that require 10x more time than fixing
> this bug.

Agree. Furthermore loading time it's affected loading the library.

Comment 11

[Gregor Wagner [:gwagner]]

(In reply to Borja Salguero [:borjasalguero] from comment #10)
> (In reply to Gregor Wagner [:gwagner] from comment #9)
> > (In reply to Dietrich Ayala (:dietrich) from comment #8)
> > > Driver retriage: It sounds like there's no user impact to leaving this code
> > > in the tree. Please submit a patch after all higher priority bugs are fixed,
> > > and we'll take on approval.
> > > 
> > > If there *is* user impact to leaving this code in, then renominate and
> > > explain.
> > 
> > This bug is more about relying on the gecko version and removing the usage
> > of the already redundant PhoneNumberJS version in gaia. I spent a lot of
> > time fixing bugs in the gecko version and most of the fixes are not merged
> > to the gaia version.
> > Removing PhoneNumberJS from gaia once it becomes dead code is optional but
> > it shouldn't take longer than 10 mins.
> > 
> 
> Only for adding some comments here. Removing the Library is not 10 mins. due
> to our 'PendingMsg' DB is tied to this library, so removing the library
> means modifying the code of 'sending/error' message handling. 

That's what I meant to say with "once it becomes dead code" :)
Do you need anything else from the platform side? Most of us are not available next week so it would be great if we could resolve it this week.
Let me know if you need help here.

Comment 12

[Borja Salguero [:borjasalguero]]

Hi Gregor! Only thing is the bug 822055, but it's about SMS and I would need feedback from [:vicamo], so it seems that it's the final 'fixing' needed for removing the library from Gaia! Thanks for your support and your work! ;)

Comment 13

[Borja Salguero [:borjasalguero]]

Attachment: PR

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 14

[Borja Salguero [:borjasalguero]]

This patch removes the library, so the starting time has been optimized a lot. This patch fix also some issues that were not working properly, and due to removing the library had this changes implicitly, everything works as expected now (for example we were using partially 'getAllThreads' method from Gecko, or messages were sorted in the wrong way when rendered...). IMHO we should update the status of this bug to BB+ because without it SMS App it's not working in the right way, and the patch it's done. Wdyt?

Comment 15

[Julien Wajsberg [:julienw]]

We have all the code in Gecko already, removing this redundant code in Sms will make the code smaller, and this will certainly fixes some bugs because we fixed bugs in the Gecko code that we didn't fixed in the gaia code.

We want to merge this because now we have 2 copies of nearly the same file and this will lead to subtle bug in some phone number handling. Also, if we have future bug, we'll be able to fix them only in one place.

The risk is low because we mostly remove stuff that we already do in gecko. I tend to like patches that remove code: less code usually means less bugs ;)

Then please reconsiderate bb? :)

Comment 16

[David Scravaglieri [:scravag]]

The approval will let it land Julien, please don't renominate

Comment 17

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 698632 [details]
PR

As Julien and Borja has commented the patch is ready, and the performance in the SMS app will increase.

a=me

Please merge once you get the positive review.

Thanks!

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 698632 [details]
PR

r=me

Comment 19

[Julien Wajsberg [:julienw]]

master: 3204365bfe5e927773b557ca46d1c5c485de56dd