811538 - B2G SMS: Applying PhoneNumberJS to SMS DB

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Borja Salguero [:borjasalguero]]

We need to apply PhoneNumberJS in Gecko in order to remove it in Gaia SMS App improving the starting time, and getting one unified way of normalizing all phone numbers.

Comment 1

[Mounir Lamouri (:mounir)]

Why having an IndexedDB initialized at build time for the SMS app is not doable?

Comment 2

[Gregor Wagner [:gwagner]]

(In reply to Mounir Lamouri (:mounir) from comment #1)
> Why having an IndexedDB initialized at build time for the SMS app is not
> doable?

Hm I guess I don't understand your question. How is this related to this bug?
This bug is about using the new PhoneNumberJS library to normalize phone numbers that
we store in the DB. So that messages coming from [phone] and sent to 1234567890 show up in the same thread.

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Gregor Wagner [:gwagner] from comment #2)
> (In reply to Mounir Lamouri (:mounir) from comment #1)
> > Why having an IndexedDB initialized at build time for the SMS app is not
> > doable?
> 
> Hm I guess I don't understand your question. How is this related to this bug?
> This bug is about using the new PhoneNumberJS library to normalize phone
> numbers that
> we store in the DB. So that messages coming from [phone] and sent to
> 1234567890 show up in the same thread.

Oh ok. Comment 0 was a bit confusing...

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We weren't really sure during triage what the problems would be of not fixing this bug.

Does this bug prevent us from normalizing phone numbers properly such that users get the wrong behavior?

Or is it just about reducing the amount of JS loaded by the communication app in order to improve comms app starting performance?

Comment 5

[Borja Salguero [:borjasalguero]]

[:sicking] During the work week in SF we stablished as a goal getting our SMS App with the best performance. For acomplishing this purpose we talked about the following 'steps' to follow:
- Change PhoneNumberLib to PhoneNumberJS in Gaia (Done)
- Include PhoneNumberJS in Gecko (Done)
- 'getThreads' in Gecko (Done)
- Apply PhoneNumberJS to SMS DB (?)
- Add 'Error' & 'Sending' states in SMS DB (Assigned)
- Remove 'indexedDB' for 'sending/error' messages in SMS App (Assigned)
- Add PhoneNumberJS to Contacts (?)
- Remove PhoneNumberJS from SMS App (Assigned)

Why this step should be bb+? 
Currently 'threading' in Gecko is 'working' due to SMS App is normalizing properly the phone number of a sent/received SMS. As we agree in SF, this effort should be done in one place, and we decided to do it in Gecko (that's why we added the library there). This change will optimize the starting time in our SMS App and we will have a consistent way of managing 'phone numbers' in Gecko.

What happened if this bug is not fixed asap?
Optimization is completely blocked, due to we are making almost the same effort in SMS App than before ('getThreads()' is working because SMS App, without SMS App will not work anymore). SMS would use two DB (Gecko and 'indexedDB' for SMS App) to maintain and synchronize, and Gecko will have a 'Object Store' completely dependent of SMS App (Gecko doesnt have the capability of normalizing numbers in SMS DB until this bug will be fixed)... 
It's important that other bugs related (#806594 #774621 #808819 & #811539) are bb+, and all these bugs depends on this one! So IMHO we should be consistent with our criteria and adding bb+ to this one as well.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sounds good.

Comment 7

[Andreas Gal :gal]

How are we doing here?

Comment 8

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 9

[Alex Keybl [:akeybl]]

P1, moving into C2 since this is a non-trivial change.

Comment 11

[Gregor Wagner [:gwagner]]

Attachment: patch

Save incoming and outgoing numbers in international format and also convert current database entries.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

FYI looks like this will collide with bug 813978.

Comment 13

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 14

[Gregor Wagner [:gwagner]]

(In reply to ben turner [:bent] from comment #12)
> FYI looks like this will collide with bug 813978.

Thanks! The merge is annoying but it shouldn't be hard.

Comment 15

[Gregor Wagner [:gwagner]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=c70fcc17a506

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 688365 [details] [diff] [review]
patch

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

We chatted about this on irc, but we need a few cursor.update() calls.

Comment 17

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 18

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 688471 [details] [diff] [review]
patch

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

r=me with these addressed.

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +373,5 @@
> +      }
> +
> +      let needsUpdate = false;
> +      let message = cursor.value;
> +      if (message.receiver && message.receiver !== "undefined") {

Since this is identical to the sender case below you could combine this into a |for each(["receiver, sender"))| loop... But this is ok too so don't do it unless you want to.

@@ +375,5 @@
> +      let needsUpdate = false;
> +      let message = cursor.value;
> +      if (message.receiver && message.receiver !== "undefined") {
> +        debug("upgrade message.receiver from: " + message.receiver + "\n");
> +        let parsedNumber = PhoneNumberUtils.parse(message.receiver.toString());

Is the |toString()| here needed? I would be really surprised if we let a non-null thing in that wasn't a string.

@@ +383,5 @@
> +        }
> +
> +        debug("upgrade message.receiver to: " + message.receiver + "\n");
> +      } else {
> +        if (message.receiver === "undefined") {

Nit: make this 'else if'. Below too.

@@ +389,5 @@
> +          needsUpdate = true;
> +        }
> +      }
> +
> +      if (message.sender && message.sender !== "undefined") {

Hm, what if we change these if conditions to:

  if (message.sender) {
    if (message.sender === "undefined") {
      // ...
    } else {
      // ...
    }
  }

@@ +395,5 @@
> +        let parsedNumber = PhoneNumberUtils.parse(message.sender.toString());
> +        if (parsedNumber && parsedNumber.internationalNumber) {
> +          message.sender = parsedNumber.internationalNumber;
> +          needsUpdate = true;
> +          debug("upgrade message.sender to: " + message.sender + "\n");

Shouldn't all these debug() calls be prefixed with |if (DEBUG)|? Otherwise we do all the string concats without using them.

@@ +419,5 @@
> +
> +      let entry = cursor.value;
> +      if (entry.senderOrReceiver) {
> +        debug("upgrade mostRecentStore from: " + entry.senderOrReceiver + "\n");
> +        let parsedNumber = PhoneNumberUtils.parse(entry.senderOrReceiver.toString());

toString() again.

@@ +429,5 @@
> +      }
> +
> +      cursor.continue();
> +    }
> +

Nit: remove extra newline.

@@ +579,5 @@
>      }
>  
> +    if (receiver) {
> +      debug("change receiver from: " + receiver);
> +      let parsedNumber = PhoneNumberUtils.parse(receiver.toString());

I don't know if we need to worry about parsing this... Will |this.mRIL.rilContext.icc.msisdn| return something that will ever parse?

@@ +607,5 @@
>                     read:           FILTER_READ_UNREAD};
>      return this.saveMessage(message);
>    },
>  
> +  saveSentMessage: function saveSentMessage(aReceiver, aBody, aDate) {

Nit: No debug messages here.

@@ +735,5 @@
>        };
>  
>        txn.onerror = function onerror(event) {
> +        if (DEBUG) {
> +          if (event.target)

This shouldn't be necessary...

Comment 20

[Gregor Wagner [:gwagner]]

(In reply to ben turner [:bent] from comment #19)
> Comment on attachment 688471 [details] [diff] [review]
> patch
> 
> Review of attachment 688471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with these addressed.
> 
> 
> @@ +375,5 @@
> > +      let needsUpdate = false;
> > +      let message = cursor.value;
> > +      if (message.receiver && message.receiver !== "undefined") {
> > +        debug("upgrade message.receiver from: " + message.receiver + "\n");
> > +        let parsedNumber = PhoneNumberUtils.parse(message.receiver.toString());
> 
> Is the |toString()| here needed? I would be really surprised if we let a
> non-null thing in that wasn't a string.
> 

fixed

> @@ +383,5 @@
> > +        }
> > +
> > +        debug("upgrade message.receiver to: " + message.receiver + "\n");
> > +      } else {
> > +        if (message.receiver === "undefined") {
> 
> Nit: make this 'else if'. Below too.
> 

fixed

> @@ +389,5 @@
> > +          needsUpdate = true;
> > +        }
> > +      }
> > +
> > +      if (message.sender && message.sender !== "undefined") {
> 
> Hm, what if we change these if conditions to:
> 
>   if (message.sender) {
>     if (message.sender === "undefined") {
>       // ...
>     } else {
>       // ...
>     }
>   }
> 

fixed


> @@ +395,5 @@
> > +        let parsedNumber = PhoneNumberUtils.parse(message.sender.toString());
> > +        if (parsedNumber && parsedNumber.internationalNumber) {
> > +          message.sender = parsedNumber.internationalNumber;
> > +          needsUpdate = true;
> > +          debug("upgrade message.sender to: " + message.sender + "\n");
> 
> Shouldn't all these debug() calls be prefixed with |if (DEBUG)|? Otherwise
> we do all the string concats without using them.
> 

fixed

> 
> @@ +579,5 @@
> >      }
> >  
> > +    if (receiver) {
> > +      debug("change receiver from: " + receiver);
> > +      let parsedNumber = PhoneNumberUtils.parse(receiver.toString());
> 
> I don't know if we need to worry about parsing this... Will
> |this.mRIL.rilContext.icc.msisdn| return something that will ever parse?

It's your own number afaik. I think we should convert it.


> 
> @@ +735,5 @@
> >        };
> >  
> >        txn.onerror = function onerror(event) {
> > +        if (DEBUG) {
> > +          if (event.target)
> 
> This shouldn't be necessary...

It is. Today I got event.target === null in debug mode.

Comment 21

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d29847ff05c4

Comment 22

[Ed Morley [:emorley]]

Backed out for B2G marionette-webapi failures in test_getmessage.js, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17626313&tree=Mozilla-Inbound

Note: The test was busted on the push before too for something else, but this push broke test_getmessage.js on top.

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56f34c1cc509

Comment 23

[Gregor Wagner [:gwagner]]

I couldn't run the modified tests on try server. After 24h marionette tests are still pending.

https://hg.mozilla.org/integration/mozilla-inbound/rev/61c7ade03f3a
https://hg.mozilla.org/integration/mozilla-inbound/rev/06da32434ea1

Comment 24

[Gregor Wagner [:gwagner]]

Fix tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/309e7959dff6

Comment 25

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63e3a9b10f15

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/61c7ade03f3a
https://hg.mozilla.org/mozilla-central/rev/06da32434ea1
https://hg.mozilla.org/mozilla-central/rev/309e7959dff6
https://hg.mozilla.org/mozilla-central/rev/63e3a9b10f15

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b497b931947b
https://hg.mozilla.org/releases/mozilla-aurora/rev/51acfa2df426
https://hg.mozilla.org/releases/mozilla-aurora/rev/2ea5ed291435
https://hg.mozilla.org/releases/mozilla-aurora/rev/23fb9fbf0a8e

https://hg.mozilla.org/releases/mozilla-beta/rev/1f3faeb42ebf
https://hg.mozilla.org/releases/mozilla-beta/rev/a29583811be5
https://hg.mozilla.org/releases/mozilla-beta/rev/23fd06a974d0
https://hg.mozilla.org/releases/mozilla-beta/rev/815a69e1b1cf