913313 - [wasabi][CDMA] There is no tick icon next to the message sent after enabling "Delivery reports".

Status: VERIFIED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

+++ This bug was initially created as a clone of Bug #911026 +++

There is no tick icon next to sent message even delivery report is on. Due to no UI indicator, I cannot tell whether no delivery report is received or it's SMS UI issue.

* Build Number                
Gaia mozillaorg/master - 09cfa8afaf88b49fc58ee33ec29071038b7cea2c
Gecko mozillaorg/master - 6e7aba3837664574da9fafb9f4ff017276e500ba
BuildID   20130829061633
Version   26.0a1

* Reproduce Steps
1. Enable delivery report 
2. Send a sms to other device.

* Expected Result
Tick icon will be seen on DUT in the message just sent after it's received by MT side.

* Actual Result
No tick icon.

* Occurrence rate
100%

Comment 1

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

This bug is cloned from bug 911026 to track CDMA SMS-DELIVER-REPORT support status.

Comment 2

[Francis Lee [:frlee]]

according to 9/25 triage result, change to koi+

Comment 3

[Preeti Raghunath(:Preeti)]

Vicamo,

Please provide updates if this bug has been fixed or not.

Comment 4

[Ken Chang[:kenkai]]

Bevis is happy to help this bug..:)

Comment 5

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

Delivery Report in CDMA SMS is not ready yet.

Summarize the action items to follow up:
CDMA SMS Delivery Report (3GPP2 C.S0015-B_v2.0):
- For MO, 
  - Howto Enable Delivery Report:
    - Seting the flag of BEARER_DATA.REPLY_OPTION.REPORT_REQ in TELESERVICE Layer.
      - See 4.5.11 Reply Option for detailed information.
  - Make sure the messageRef in the response of RIL_REQUEST_CDMA_SEND_SMS is stored to track the the delivery report when arrived.

- For MT, 
  - Different from the SMS in GSM implementation, the delivery report reused the same UNSOL_RESPONSE_CDMA_NEW_SMS from RIL instead of UNSOL_RESPONSE_NEW_SMS_STATUS_REPORT used in GSM network.
  - How to identify a MT CDMA SMS as a delivery report:
    - Make sure that the TELESERVICE_IDENTIFIER is equal to WMT or WEMT and the BEARER_DATA.MESSAGE_IDENTIFIER.MESSAGE_TYPE is set to MESSAGE_TYPE_DELIVERY_ACK
    - See 4.5.1 Message Identifier for detailed information.
  - Check if the BEARER_DATA.MESSAGE_IDENTIFIER.MESSAGE_ID is equal to the messageRef of the MO SMS sent previously.

Comment 6

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

Attachment: Part 1: Add Support of CDMA Delivery Report. V1. r=gene

Add support for CDMA SMS Status Report:
1. MO side, Add new method |encodeUserDataReplyOption(options)| in CdmaPDUHelper to enable the DAK_REQ flag if |requestStatusReport| is set to true.
2. MT side, identify the Delivery ACK SMS from normal CDMA SMS by checking if the BEARER_DATA.MESSAGE_IDENTIFIER.MESSAGE_TYPE is set to MESSAGE_TYPE_DELIVERY_ACK.
3. Add new method |_processCdmaSmsStatusReport(message)| to check the Error Class inside Message Status Subparameter to identify the delivery status of the corresponding SUBMIT SMS.
5. Need better suggestion of the naming of subMsgType to be more clear indicates the msgType defined in BEARER_DATA.MESSAGE_IDENTIFIER.MESSAGE_TYPE compared to the message.messageType which presents the type of Teleservice Identifier.
6. Minor change to rename the constant/method names correctly.

Comment 7

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

Update test result of the patch in APTG network:
1. It seems that the MESSAGE_STATUS is always absent in the Delivery ACK SMS sent from APTG SMS Center.
2. The report only contains human-readable text in the message body such as "status:Not Sent ,dest:0980781686" or "status:Sent ,dest:0980781686".
3. This blocks us to identify delivery status with the |tick| icon in the UI by checking the MESSAGE_STATUS in the report.

Comment 8

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

After compared with Android behavior in APTG network,
The device will always treated the delivery report as positive result. (No Good!)

We should improve it in this way:
1. If MESSAGE_STATUS is available, we check the delivery status according to MESSAGE_STATUS.
2. If MESSAGE_STATUS is absent but USER_DATA is available, we should display the report as normal incoming SMS.
3. If both MESSAGE_STATUS/USER_DATA are absent, we treat the delivery status as positive result.

Bevis

Comment 9

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

Attachment: Part 1: Add Support of CDMA Delivery Report. V2. r=gene

Add one more condition to Treat Status Report as normal incoming SMS if the Status report didn't include MESSAGE_STATUS but USER_DATA. (encountered in APTG network)

Comment 10

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

Attachment: Example Screenshot to compare the Delivery Report with/without Message Status

Comment 11

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

Attachment: Part 2: Add Unit Test to verify the parsing of CDMA SMS Delivery Report and the flag of enabling Delivery Report. r=gene

Add corresponding unit test for further regression testing.

Comment 12

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

Attachment: Part 2: Add Unit Test to verify the parsing of CDMA SMS Delivery Report and the flag of enabling Delivery Report V2. r=gene

Add corresponding unit test for further regression test.

Comment 13

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

Attachment: Part 1: Add Support of CDMA Delivery Report. V3. r=gene

Compact the assignment by applying the |Destructuring assignment|.

Comment 14

[Gene Lian (I already quit Mozilla)]

I'll try to catch up the review by today.

Comment 15

[Gene Lian (I already quit Mozilla)]

Comment on attachment 826639 [details] [diff] [review]
Part 1: Add Support of CDMA Delivery Report. V3. r=gene

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

Nice patch!

::: dom/system/gonk/ril_worker.js
@@ +4317,5 @@
> +
> +    delete this._pendingSentSmsMap[message.msgId];
> +
> +    if (message.errorClass === -1
> +        && message.body) {

Don't wrap this line.

@@ +4319,5 @@
> +
> +    if (message.errorClass === -1
> +        && message.body) {
> +      // process as normal incoming SMS
> +      // if errorClass is invalid but message body is available.

Usually, the comment format should be

// {upper_case_letter}...{period}.

So,

// Process as normal incoming SMS, if errorClass is invalid
// but message body is available.

@@ +8880,5 @@
> +    return this.constructDecodedMsg(message);
> +  },
> +
> +  constructDecodedMsg: function cdma_constructDecodedMsg(message) {
> +    let msg = null;

Please drop this line.

@@ +8882,5 @@
> +
> +  constructDecodedMsg: function cdma_constructDecodedMsg(message) {
> +    let msg = null;
> +
> +    // User Data

// CDMA User Data.

@@ +8888,5 @@
> +                      (message[PDU_CDMA_MSG_USERDATA_BODY])
> +                      ? [message[PDU_CDMA_MSG_USERDATA_BODY].header,
> +                         message[PDU_CDMA_MSG_USERDATA_BODY].body,
> +                         message[PDU_CDMA_MSG_USERDATA_BODY].encoding]
> +                      : [null, null, null];

The semantic looks weird to me. Did you see any other examples writing it in this way within this file? Maybe it's fine. :)

Please also do:

let userData = message[PDU_CDMA_MSG_USERDATA_BODY];
[message.header, message.body, message.encoding] =
  userData ? [userData.header, userData.body, userData.encoding] : [null, null, null];

@@ +8890,5 @@
> +                         message[PDU_CDMA_MSG_USERDATA_BODY].body,
> +                         message[PDU_CDMA_MSG_USERDATA_BODY].encoding]
> +                      : [null, null, null];
> +
> +    // Message Status:

// CDMA Message Status:

@@ +8897,5 @@
> +    [message.errorClass, message.msgStatus] =
> +                         (message[PDU_CDMA_MSG_USER_DATA_MSG_STATUS])
> +                         ? [message[PDU_CDMA_MSG_USER_DATA_MSG_STATUS].errorClass,
> +                            message[PDU_CDMA_MSG_USER_DATA_MSG_STATUS].msgStatus]
> +                         : ((message.body)? [-1, -1]: [0, 0]);

let msgStatus = message[PDU_CDMA_MSG_USER_DATA_MSG_STATUS];
[message.errorClass, message.msgStatus] =
  msgStatus ? [msgStatus.errorClass, msgStatus.msgStatus]
            : (message.body ? [-1, -1] : [0, 0]);

@@ +8903,2 @@
>      // Transform message to GSM msg
> +    msg = {

Keep the original |let msg = ...|

@@ +8909,5 @@
> +      recipient:        null,
> +      pid:              PDU_PID_DEFAULT,
> +      epid:             PDU_PID_DEFAULT,
> +      dcs:              0,
> +      mwi:              null, //message[PDU_CDMA_MSG_USERDATA_BODY].header ? message[PDU_CDMA_MSG_USERDATA_BODY].header.mwi : null,

Since you're here, could you please remove the comment? I don't understand why we need to keep this. Do you?

@@ +9452,5 @@
>    },
>  
>    /**
> +   *
> +   * User data subparameter decoder : Message Status

Add one blank line with "*" here.

@@ +9458,5 @@
> +   */
> +  decodeUserDataMsgStatus: function cdma_decodeUserDataMsgStatus() {
> +    let result = {};
> +    result.errorClass = BitBufferHelper.readBits(2);
> +    result.msgStatus = BitBufferHelper.readBits(6);

let result = {
  errorClass: BitBufferHelper.readBits(2),
  msgStatus: BitBufferHelper.readBits(6)
};

Comment 16

[Gene Lian (I already quit Mozilla)]

Comment on attachment 826558 [details] [diff] [review]
Part 2: Add Unit Test to verify the parsing of CDMA SMS Delivery Report and the flag of enabling Delivery Report V2. r=gene

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

Thank you!

::: dom/system/gonk/tests/test_ril_worker_sms_cdma.js
@@ +47,5 @@
> +      recipient:        null,
> +      pid:              PDU_PID_DEFAULT,
> +      epid:             PDU_PID_DEFAULT,
> +      dcs:              0,
> +      mwi:              null, //message[PDU_CDMA_MSG_USERDATA_BODY].header ? message[PDU_CDMA_MSG_USERDATA_BODY].header.mwi : null,

I think we should drop this comment. What do you think?

@@ +74,5 @@
> +
> +    // Check if pending token is removed.
> +    do_check_true((errorClass === 2)
> +                   ? !!sentSmsMap[msgId]
> +                   : !sentSmsMap[msgId]);

Please put them in single line if it's OK.

::: dom/system/gonk/tests/test_ril_worker_sms_cdmapduhelper.js
@@ +30,5 @@
> +
> +  do_check_eq(testDataBuffer.length, expectedDataBuffer.length);
> +
> +  for (let i = 0; i < expectedDataBuffer.length; i++) {
> +  	do_check_eq(testDataBuffer[i], expectedDataBuffer[i]);

Please examine your codes again to ensure you didn't use TABs.

@@ +52,5 @@
> +
> +  let helper = worker.CdmaPDUHelper;
> +  function test_MsgStatus(octet) {
> +  	let testDataBuffer = [octet];
> +  	worker.BitBufferHelper.startRead(testDataBuffer);

TABs...

::: dom/system/gonk/tests/xpcshell.ini
@@ +11,2 @@
>  [test_ril_worker_sms_nl_tables.js]
> +[test_ril_worker_sms_cdmapduhelper.js]

Why not just putting this one after the other one above?

Comment 17

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

Attachment: Part 1: Add Support of CDMA Delivery Report. V4. r=gene

revise patch with gene's suggestion.

Comment 18

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

Attachment: Part 2: Add Unit Test to verify the parsing of CDMA SMS Delivery Report and the flag of enabling Delivery Report V3. r=gene

revise patch with gene's suggestion.

Comment 19

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

Attachment: Part 1: [koi+] Add Support of CDMA Delivery Report. V1 r=gene a=koi+

Add patch for koi+ branch to prevent conflict.

Comment 20

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

Attachment: Part 2: [koi+] Add Unit Test to verify the parsing of CDMA SMS Delivery Report and the flag of enabling Delivery Report V1. r=gene a=koi+

Add patch for koi+ branch to prevent conflict.

Comment 21

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

update try server result for both 1.2 & 1.3:
https://tbpl.mozilla.org/?tree=Try&rev=ae0fcb6c9c1f
https://tbpl.mozilla.org/?tree=Try&rev=d109b1a2f78f

Comment 22

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

Attachment: Part 2: Add Unit Test to verify the parsing of CDMA SMS Delivery Report and the flag of enabling Delivery Report V4. r=gene

Add Bug-id into patch summary.

Comment 23

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

Attachment: Part 2: [koi+] Add Unit Test to verify the parsing of CDMA SMS Delivery Report and the flag of enabling Delivery Report V2. r=gene a=koi+

Add Bug-id into patch summary.

Comment 24

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

Attachment: Part 2: [koi+] Add Unit Test to verify the parsing of CDMA SMS Delivery Report and the flag of enabling Delivery Report V2. r=gene a=koi+

obsolete the old patch.

Comment 25

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

Attachment: Part 2: [koi+] Add Unit Test to verify the parsing of CDMA SMS Delivery Report and the flag of enabling Delivery Report V2. r=gene a=koi+

add bug id into patch summary.

Comment 26

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/b2g-inbound/rev/0929f425293c
https://hg.mozilla.org/integration/b2g-inbound/rev/04071dba39cc

Keep the checkin-needed flag since we still need to land patches for the koi+ branch.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

checkin-needed isn't needed as a reminder for koi+ uplifts.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0929f425293c
https://hg.mozilla.org/mozilla-central/rev/04071dba39cc

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/1cd3f7077ada
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/dc717297b578

Comment 30

[Enpei]

Verified on Wasabi v1.2 build
Gaia:     b401bfed469e1d6db24e910f78732bad32843e8a
Gecko:    134c78dbcfc2f7c1bb665f6da46581f4785ce140
BuildID   20131108031537
Version   26.0

But since APTG network is the case 2 of comment 8, I get a sms saying the message status is sent.

Comment 31

[Enpei]

verified on master build
Gaia:     3553a6f0cb38864578752d913f6a762a964928e1
Gecko:    b7effd61f438a18ac3facd5da4856233d075b74f
BuildID   20131108110851
Version   28.0a1

Comment 32

[Enpei]

v1.2 verified on China Telecom network, it will show a checked icon next to the message when another phone receives the message.

Thanks for Peipei's help to verify the bug.

Comment 33

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

Hi Enpei & Peipei,

Thanks for the verification in different network.

Bevis