943749 - B2G SMS: Support to Read Class 2 SMS received and stored in SIM

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

1. According to |4. SMS Data Coding Scheme| in 3GPP 23.040 (
http://www.etsi.org/deliver/etsi_ts/123000_123099/123038/10.00.00_60/ts_123038v100000p.pdf)
we know that when a Class 2 SMS comes, the UE has to be stored in SIM when SIM storage of SMS is available. Otherwise, the UE has to report an error accordingly.

2. According to current implementation of the RIL interface in AOSP, UNSOLICITED_RESPONSE_NEW_SMS_ON_SIM is use to indicate that a new Class 2 SMS is received and stored in the specified index of the EF_SMS in SIM.
3. After testing in the real network with Unagi, the class 2 SMS is notified from UNSOLICITED_RESPONSE_NEW_SMS_ON_SIM instead of UNSOLICITED_RESPONSE_NEW_SMS for normal incoming SMS.

Fire this bug to support to 
1. Copy the incoming class 2 SMS in SIM into the MobileMessagingDatabase when it comes by adopting UNSOLICITED_RESPONSE_NEW_SMS_ON_SIM.
2. Behave as normal incoming SMS to allow user to read the copied one from the Message Inbox.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch_Part_1: Support Class 2 SMS received and stored in SIM. r=gene

This patch provides the implementation of copying Class 2 SMS received in SIM to the Message box to allow user to read it as normal incoming SMS.
The patch has been verified with NowSMS in the live network and the incoming SMS can be stored/displayed correctly in the message box.

Hi Gene,

I'd like to apply this patch to support Class 2 SMS for B2G.
BTW, this is not related to any 1.3 committed features.
Hence, you can take your time to review it and give me your suggestion when available. :)

Thanks & regards,
Bevis Tseng

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8339807 [details] [diff] [review]
Patch_Part_1: Support Class 2 SMS received and stored in SIM. r=gene

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

We don't have binary SMS in database.

Comment 3

[Gene Lian (I already quit Mozilla)]

Sorry, my review queue is really full. I'd like to leave this one to Vicamo. Thanks!

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Vicamo,

As we discussed, I'd like to update my desciption in comment#1 to prevent confusing of storing binary SMS into Database:

Fire this bug to support to 
1. Copy and Decode the incoming class 2 SMS in SIM to text message with same flow of normal incoming SMS into the MobileMessagingDatabase when it comes by adopting UNSOLICITED_RESPONSE_NEW_SMS_ON_SIM.
2. Behave as normal incoming SMS to allow user to read the copied one from the Message Inbox.

Thanks & regards,
Bevis Tseng

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8339807 [details] [diff] [review]
Patch_Part_1: Support Class 2 SMS received and stored in SIM. r=gene

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

r=me, thank you.

::: dom/system/gonk/ril_worker.js
@@ +6210,5 @@
> +    function onerror(errorMsg) {
> +      if (DEBUG) {
> +        debug("Failed to Read NEW SMS on SIM #" + recordNumber + ", errorMsg: " + errorMsg);
> +      }
> +    }.bind(this));

nit: |this| pointer is not referenced throughout the callback function.

@@ +12366,5 @@
> +    }
> +
> +    ICCIOHelper.loadLinearFixedEF({fileId: ICC_EF_SMS,
> +                                   recordNumber: recordNumber,
> +                                   callback: callback.bind(this),

ditto

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch_Part_1_v2: Support Class 2 SMS received and stored in SIM. r=vyang

Update patch to
1. update correct reviewer to vyang.
2. remove unnecessary .bind(this).

Comment 7

[Gene Lian (I already quit Mozilla)]

There are some conflicts. Could you please fix it?

-      this.workerMessenger.send("ackSMS", {
-        result: (success ? RIL.PDU_FCS_OK
-                         : RIL.PDU_FCS_MEMORY_CAPACITY_EXCEEDED)
-      });
+      // Note: Ack has been done by modem for NEW_SMS_ON_SIM
+      if (message.simStatus === undefined) {
+        this.workerMessenger.send("ackSMS", {
+          result: (success ? RIL.PDU_FCS_OK
+                           : RIL.PDU_FCS_MEMORY_CAPACITY_EXCEEDED)
+        });
+      }

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Oops. 
Sorry, I should re-base it and upload the patch again after finishing the try server.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch_Part_1_v3: Support Class 2 SMS received and stored in SIM. r=vyang

Update re-based patch and the corresponding try server result as followed:
https://tbpl.mozilla.org/?tree=Try&rev=a20850344e62

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch_Part_1_v3: Support Class 2 SMS received and stored in SIM. r=vyang

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

the corresponding try server result as followed:
https://tbpl.mozilla.org/?tree=Try&rev=a20850344e62

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ae261f3c5429

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae261f3c5429

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Somehow there is a rebase error. Bug 935401 got landed before this and it moved some functions, including |ICCRecordHelper.readSMS|, to |SimRecordHelper|.  This patch is never working because it's still looking for |ICCRecordHelper.readSMS| even now.  Will file a follow-up instead.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Link related bugs up.