Closed
Bug 913313
Opened 11 years ago
Closed 11 years ago
[wasabi][CDMA] There is no tick icon next to the message sent after enabling "Delivery reports".
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(blocking-b2g:koi+, firefox26 wontfix, firefox27 wontfix, firefox28 verified, b2g-v1.2 verified)
People
(Reporter: vicamo, Assigned: bevis)
References
Details
(Whiteboard: [FT:RIL] burirun3)
Attachments
(5 files, 9 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
+++ 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%
Reporter | ||
Comment 1•11 years ago
|
||
This bug is cloned from bug 911026 to track CDMA SMS-DELIVER-REPORT support status.
Updated•11 years ago
|
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Comment 2•11 years ago
|
||
according to 9/25 triage result, change to koi+
Assignee: nobody → vyang
blocking-b2g: koi? → koi+
Updated•11 years ago
|
Summary: [wasabi][SMS] There is no tick icon next to the message sent after enabling "Delivery reports". → [wasabi][SMS][CDMA] There is no tick icon next to the message sent after enabling "Delivery reports".
Comment 3•11 years ago
|
||
Vicamo, Please provide updates if this bug has been fixed or not.
Flags: needinfo?(vyang)
Updated•11 years ago
|
Summary: [wasabi][SMS][CDMA] There is no tick icon next to the message sent after enabling "Delivery reports". → [wasabi][CDMA] There is no tick icon next to the message sent after enabling "Delivery reports".
Updated•11 years ago
|
Target Milestone: --- → 1.2 C5(Nov22)
Updated•11 years ago
|
Flags: needinfo?(vyang)
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
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.
Attachment #824508 -
Flags: review?(gene.lian)
Assignee | ||
Comment 7•11 years ago
|
||
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.
Assignee | ||
Comment 8•11 years ago
|
||
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
Assignee | ||
Comment 9•11 years ago
|
||
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)
Attachment #824508 -
Attachment is obsolete: true
Attachment #824508 -
Flags: review?(gene.lian)
Attachment #825150 -
Flags: review?(gene.lian)
Assignee | ||
Comment 10•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: [FT:RIL] → [FT:RIL] burirun3
Assignee | ||
Comment 12•11 years ago
|
||
Add corresponding unit test for further regression test.
Attachment #826557 -
Attachment is obsolete: true
Attachment #826558 -
Flags: review?(gene.lian)
Assignee | ||
Comment 13•11 years ago
|
||
Compact the assignment by applying the |Destructuring assignment|.
Attachment #825150 -
Attachment is obsolete: true
Attachment #825150 -
Flags: review?(gene.lian)
Attachment #826639 -
Flags: review?(gene.lian)
Comment 15•11 years ago
|
||
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) };
Attachment #826639 -
Flags: review?(gene.lian) → review+
Comment 16•11 years ago
|
||
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?
Attachment #826558 -
Flags: review?(gene.lian) → review+
Assignee | ||
Comment 17•11 years ago
|
||
revise patch with gene's suggestion.
Attachment #826639 -
Attachment is obsolete: true
Assignee | ||
Comment 18•11 years ago
|
||
revise patch with gene's suggestion.
Attachment #826558 -
Attachment is obsolete: true
Updated•11 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 21•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 22•11 years ago
|
||
Add Bug-id into patch summary.
Attachment #827798 -
Attachment is obsolete: true
Assignee | ||
Comment 24•11 years ago
|
||
obsolete the old patch.
Attachment #827878 -
Attachment is obsolete: true
Attachment #827960 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #827961 -
Attachment description: 827960: 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+ → 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+
Assignee | ||
Comment 25•11 years ago
|
||
add bug id into patch summary.
Attachment #827961 -
Attachment is obsolete: true
Comment 26•11 years ago
|
||
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•11 years ago
|
||
checkin-needed isn't needed as a reminder for koi+ uplifts.
Keywords: checkin-needed
Comment 28•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0929f425293c https://hg.mozilla.org/mozilla-central/rev/04071dba39cc
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 29•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/1cd3f7077ada https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/dc717297b578
status-b2g-v1.2:
--- → fixed
status-firefox26:
--- → wontfix
status-firefox27:
--- → wontfix
status-firefox28:
--- → fixed
Comment 30•11 years ago
|
||
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•11 years ago
|
||
verified on master build Gaia: 3553a6f0cb38864578752d913f6a762a964928e1 Gecko: b7effd61f438a18ac3facd5da4856233d075b74f BuildID 20131108110851 Version 28.0a1
Comment 32•11 years ago
|
||
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.
Resolution: WORKSFORME → FIXED
Assignee | ||
Comment 33•11 years ago
|
||
Hi Enpei & Peipei, Thanks for the verification in different network. Bevis
You need to log in
before you can comment on or make changes to this bug.
Description
•