Closed Bug 811538 Opened 8 years ago Closed 8 years ago

B2G SMS: Applying PhoneNumberJS to SMS DB

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: borjasalguero, Assigned: gwagner)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 811539
Blocks: 806592
Depends on: 809213
Component: DOM → DOM: Device Interfaces
Summary: [SMS API] Applying PhoneNumberJS to SMS DB → B2G SMS: Applying PhoneNumberJS to SMS DB
blocking-basecamp: --- → ?
Why having an IndexedDB initialized at build time for the SMS app is not doable?
Flags: needinfo?(anygregor)
(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 +1-123-456-7890 and sent to 1234567890 show up in the same thread.
Flags: needinfo?(anygregor)
(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 +1-123-456-7890 and sent to
> 1234567890 show up in the same thread.

Oh ok. Comment 0 was a bit confusing...
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?
blocking-basecamp: ? → -
[: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.
blocking-basecamp: - → ?
Sounds good.
blocking-basecamp: ? → +
Assignee: nobody → anygregor
How are we doing here?
Blocks: 796618
Depends on: 815833
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P1
P1, moving into C2 since this is a non-trivial change.
Target Milestone: --- → B2G C2 (20nov-10dec)
Attached patch patch (obsolete) — Splinter Review
Save incoming and outgoing numbers in international format and also convert current database entries.
FYI looks like this will collide with bug 813978.
Attached patch patch (obsolete) — Splinter Review
Attachment #688324 - Attachment is obsolete: true
Attachment #688365 - Flags: review?(bent.mozilla)
(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 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.
Attachment #688365 - Flags: review?(bent.mozilla) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #688365 - Attachment is obsolete: true
Attachment #688467 - Flags: review?(bent.mozilla)
Attached patch patchSplinter Review
Attachment #688467 - Attachment is obsolete: true
Attachment #688467 - Flags: review?(bent.mozilla)
Attachment #688471 - Flags: review?(bent.mozilla)
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...
Attachment #688471 - Flags: review?(bent.mozilla) → review+
(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.
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
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
Depends on: 819213
Depends on: 819526
You need to log in before you can comment on or make changes to this bug.