Closed
Bug 945089
Opened 11 years ago
Closed 11 years ago
There is some case that concatenated SMS data is lost
Categories
(Firefox OS Graveyard :: RIL, defect)
Firefox OS Graveyard
RIL
Tracking
(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 fixed, b2g-v1.4 fixed, b2g-v2.0 unaffected)
Tracking | Status | |
---|---|---|
firefox28 | --- | wontfix |
firefox29 | --- | wontfix |
firefox30 | --- | fixed |
b2g-v1.4 | --- | fixed |
b2g-v2.0 | --- | unaffected |
People
(Reporter: mu-ota, Assigned: bevis)
References
Details
(Whiteboard: [FT:RIL])
Attachments
(4 files, 12 obsolete files)
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bevis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.57 Safari/537.36 Steps to reproduce: 1.First Concatenated SMS is received. (waiting next concatenated SMSs:in case receivedSegments is smaller than segmentMaxSeq) 2.B2g process is restart(ex B2g process is killed) Actual results: The received concatenated SMS data(in hashmap) is lost. (Concatenated SMS is not stored in DB until all concatenated SMSs have received) Expected results: Concatenated SMS is stored in DB.(even if All SMSs has not received)
Reporter | ||
Comment 1•11 years ago
|
||
I think this case maybe is little. But Data Packet lost is serious issue, because User payed money packet data.
Updated•11 years ago
|
Component: Gaia::SMS → RIL
Comment 2•11 years ago
|
||
This may need to pull nearly everything related to SMS out from ril_worker to some higher layer, store TPDUs in yet another database.
Comment 3•11 years ago
|
||
Hi Vicamo-san, When the device save concatenated SMS data for non-volatilization, I think that it is necessary to consider it about the cleaning timing of concatenated SMS data. I am glad when you consider it in conjunction with the contents which Ohta-san listed.
Comment 4•11 years ago
|
||
Hi at-kitamura-san, I would prefer to address this issue after bug 873351, which is going to move all SMS processing code out of RadioInterfaceLayer. Then we can continue to extract more SMS code, especially concatenated SMS segments caching, into SmsService. Not a must, but will surely have a cleaner source code layout.
Comment 6•11 years ago
|
||
Vicamo, after discussing with KDDI, it is a 1.4+ bug. Let's see if it's necessary to only fix it after fixing bug 873351? And can you take this bug?
blocking-b2g: --- → 1.4+
Flags: needinfo?(vyang)
Updated•11 years ago
|
Flags: needinfo?(vyang)
QA Contact: vyang
Comment 8•11 years ago
|
||
I think Vicamo want to take this bug.
Assignee: nobody → vyang
QA Contact: vyang
Reporter | ||
Updated•11 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 9•11 years ago
|
||
This doesn't fall under the QC blocking feature list & DSDS feature list for 1.4. Renoming.
blocking-b2g: 1.4+ → 1.4?
Comment 10•11 years ago
|
||
Ken Please review if this is needed for any of the features in 1.4? The new ask list from QC.
Flags: needinfo?(kchang)
Comment 12•11 years ago
|
||
Vicamo is busy on preparing IPv6 patch for tokyo's workshop. Bevis, please take this bug. Thanks very much.
Assignee: vyang → btseng
Updated•11 years ago
|
Whiteboard: [FT:RIL]
Target Milestone: --- → 1.4 S3 (14mar)
Comment 19•11 years ago
|
||
Comment on attachment 8387393 [details] [diff] [review] Patch Part 1 v1: Save SMS PDU into DB to prevent data lost of the concatenated SMS when device reboot. r=vyang Review of attachment 8387393 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm @@ +1356,5 @@ > + /** > + * This smsSegmentStore is used to store uncomplete SMS segments. > + * Each entry looks like this: > + * > + * { hash: <String> (primary key), nit: please leave key composition job to database itself. Composing keys outside db itself means the way to compose it becomes a convention to all db clients. This loses some flexibility when we're to refactor/improve the performance of that db because it's quite impossible to ask all implementations to follow at the same time. So, define as few attributes as possible and leave db task, schema maintenance to db itself. This also follows we have to store |iccId| as well. @@ +1357,5 @@ > + * This smsSegmentStore is used to store uncomplete SMS segments. > + * Each entry looks like this: > + * > + * { hash: <String> (primary key), > + * messageType: <Number>, Please group the attributes. Some of them are only referenced once. * [ Only referenced at the first time. ] * * messageType: <Number>, * teleservice: <Number>, * ... @@ +1361,5 @@ > + * messageType: <Number>, > + * teleservice: <Number>, > + * SMSC: <String>, > + * sentTimestamp: <Number>, > + * sender: <String>, * [ Referenced in every call to |saveSmsSegment()| ] * * sender: <String>, @@ +1365,5 @@ > + * sender: <String>, > + * encoding: <Number>, > + * messageClass: <String>, > + * header: { > + * segmentRef: <Number>, Since we're to reassign corresponding fields to a new savable object, we may omit this header structure. Just use |obj.segmentRef| ... @@ +1372,5 @@ > + * originatorPort: <Number>, > + * destinationPort: <Number>, > + * }, > + * mwi: { > + * discard: <Boolean>, ditto @@ +1382,5 @@ > + * serviceCategory: <Number>, > + * language: <String>, > + * > + * // Handy fields for concatenation. > + * segmentMaxSeq: <Number>, nit: we can have it from |header.segmentMaxSeq|. @@ +1385,5 @@ > + * // Handy fields for concatenation. > + * segmentMaxSeq: <Number>, > + * receivedSegments: <Number>, > + * segments: [], > + * timestamp: <Number> } Please also group fields created by mmdb itself. @@ +1388,5 @@ > + * segments: [], > + * timestamp: <Number> } > + * > + */ > + let smsSegmentStore = db.createObjectStore(SMS_SEGMENT_STORE_NAME, nit: unreferenced `smsSegmentStore`. @@ +1389,5 @@ > + * timestamp: <Number> } > + * > + */ > + let smsSegmentStore = db.createObjectStore(SMS_SEGMENT_STORE_NAME, > + { keyPath: "hash" }); IndexedDB records are sorted by primary key in ascending order. Using string primary key here means there will be many record re-orders whenever a new record is inserted. Please use a numeric 'id' and set 'autoIncrement' to true. You'll also need an index for that hash string. @@ +2498,5 @@ > + aSmsSegment.segments = []; > + aSmsSegment.timestamp = Date.now(); > + if (aSmsSegment.encoding == RIL.PDU_DCS_MSG_CODING_8BITS_ALPHABET) { > + aSmsSegment.segments[seq] = aSmsSegment.data; > + delete aSmsSegment.data; Why should we delete |aSmsSegment.data| here? @@ +2501,5 @@ > + aSmsSegment.segments[seq] = aSmsSegment.data; > + delete aSmsSegment.data; > + } else { > + aSmsSegment.segments[seq] = aSmsSegment.body; > + delete aSmsSegment.body; ditto @@ +2506,5 @@ > + } > + > + let addRequest = segmentStore.add(aSmsSegment); > + addRequest.onsuccess = function(event) { > + aCallback.notify(Cr.NS_OK, null); Please use |txn.oncomplete| to execute callbacks as possible, or it will block further transactions and reduce IndexedDB throughput. @@ +2508,5 @@ > + let addRequest = segmentStore.add(aSmsSegment); > + addRequest.onsuccess = function(event) { > + aCallback.notify(Cr.NS_OK, null); > + }; > + addRequest.onerror = function(event) { ditto. @@ +2516,5 @@ > + } > + } > + aCallback.notify(Cr.NS_ERROR_FAILURE, null); > + }; > + } else { nit: bail-out early. @@ +2539,5 @@ > + // If the segments of a WAP Push are not received in sequence > + // (e.g., SMS with seq == 1 is not the 1st segment received by the device), > + // we have to retrieve the port information from 1st segment and > + // save it into the cached msgRecord.header. > + if (aSmsSegment.teleservice === RIL.PDU_CDMA_MSG_TELESERIVCIE_ID_WAP && seq === 1) { nit: line wrap at 80 chars boundary. ::: dom/system/gonk/RadioInterfaceLayer.js @@ +2966,5 @@ > + * Hash map for received multipart sms fragments. Messages are hashed with > + * its sender address and concatenation reference number. Three additional > + * attributes `segmentMaxSeq`, `receivedSegments`, `segments` are inserted. > + */ > + _receivedSmsSegmentsMap: null, We have already pushed the segments into database, why do we need another runtime cache map?
Attachment #8387393 -
Flags: review?(vyang)
Updated•11 years ago
|
Attachment #8387394 -
Flags: review?(vyang) → review+
Comment 20•11 years ago
|
||
Comment on attachment 8387395 [details] [diff] [review] Patch Part 3: Add new test cases to test the concatenation of text/binary sms. r=vyang Review of attachment 8387395 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/tests/marionette/test_mt_sms_concatenation.js @@ +108,5 @@ > +} > + > +function sendRawSmsAndWait(aPdus) { > + for (let pdu of aPdus) { > + sendRawSmsToEmulator(pdu); Please help document the `aPdu` parameter of |sendRawSmsToEmulator|. "A hex string representing the whole SMS T-PDU." would be nice. Thank you :) @@ +109,5 @@ > + > +function sendRawSmsAndWait(aPdus) { > + for (let pdu of aPdus) { > + sendRawSmsToEmulator(pdu); > + } let promises = []; promises.push(waitForManagerEvent("received")); for (let pdu of aPdus) { promises.push(sendRawSmsToEmulator(pdu)); } return Promise.all(promises); @@ +123,5 @@ > + > +function verifyBinaryMessage(aMessage) { > + is(aMessage.type, "mms", "MmsMessage type"); > + is(aMessage.delivery, "not-downloaded", "MmsMessage delivery"); > + // remove duplicated M-Notification.ind for next test. nit: an empty line before this comment. @@ +125,5 @@ > + is(aMessage.type, "mms", "MmsMessage type"); > + is(aMessage.delivery, "not-downloaded", "MmsMessage delivery"); > + // remove duplicated M-Notification.ind for next test. > + return Promise.resolve() > + .then(deleteMessagesById([aMessage.id])); You can simply do: return deleteMessagesById([aMessage.id]); @@ +134,5 @@ > + return sendRawSmsAndWait(buildTextPdus(PDU_DCS_NORMAL_UCS2)) > + .then(function(aReceivedMessage) { > + return Promise.resolve() > + .then(verifyTextMessage.bind(null, aReceivedMessage, "normal")); > + }); You may have a simpler form instead: return sendRawSmsAndWait(buildTextPdus(PDU_DCS_NORMAL_UCS2)) .then((aReceivedMessage) => verifyTextMessage(aReceivedMessage, "normal")); @@ +138,5 @@ > + }); > +} > + > +function testClass0Text() { > + log("testClass0Text()"); Looks like you can unify testNormalText and testClass0Text as testText(aDcs, aMessageClass) @@ +156,5 @@ > + }); > +} > + > +function testClass0Binary() { > + log("testClass0Binary()"); ditto. testBinary(aDcs) @@ +165,5 @@ > + }); > +} > + > +startTestCommon(function testCaseMain() { > + manager.onreceived = function(event) { Please create an utility function 'waitForManagerEvent' instead. See http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/tests/marionette/head.js#154 @@ +169,5 @@ > + manager.onreceived = function(event) { > + receivedMessage = event.message; > + }; > + > + SpecialPowers.setCharPref("dom.mms.retrieval_mode", "manual"); Use |SpecialPowers.pushPrefEnv(<inPrefs>, <callback>)| instead. @@ +175,5 @@ > + return Promise.resolve() > + .then(testNormalText) > + .then(testClass0Text) > + .then(testNormalBinary) > + .then(testClass0Binary) .then(() => testText(PDU_DCS_NORMAL_UCS2, "normal")) .then(() => testText(PDU_DCS_CLASS0_UCS2, "class-0")) .then(() => testBinary(PDU_DCS_NORMAL_8BIT)) .then(() => testBinary(PDU_DCS_CLASS0_8BIT)) @@ +180,5 @@ > + .then(function() { > + // Clear |manager.onreceived| handler. > + manager.onreceived = null; > + // Restore retrieval_mode. > + SpecialPowers.clearUserPref("dom.mms.retrieval_mode"); Don't need this if you use pushPrefEnv.
Attachment #8387395 -
Flags: review?(vyang)
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 8387393 [details] [diff] [review] Patch Part 1 v1: Save SMS PDU into DB to prevent data lost of the concatenated SMS when device reboot. r=vyang Review of attachment 8387393 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm @@ +1356,5 @@ > + /** > + * This smsSegmentStore is used to store uncomplete SMS segments. > + * Each entry looks like this: > + * > + * { hash: <String> (primary key), Will do @@ +1357,5 @@ > + * This smsSegmentStore is used to store uncomplete SMS segments. > + * Each entry looks like this: > + * > + * { hash: <String> (primary key), > + * messageType: <Number>, Will do @@ +1361,5 @@ > + * messageType: <Number>, > + * teleservice: <Number>, > + * SMSC: <String>, > + * sentTimestamp: <Number>, > + * sender: <String>, will do. @@ +1365,5 @@ > + * sender: <String>, > + * encoding: <Number>, > + * messageClass: <String>, > + * header: { > + * segmentRef: <Number>, Because the message.header and message.mwi will still be used by RadioInterface.handleSmsReceived(), this is to ensure that both Class 0 SMS concatenation handled in RadioInterface._processReceivedSmsSegment() and MobileMessageDB.jsm will provide same structure for further use in RadioInterface.handleSmsReceived(). @@ +1372,5 @@ > + * originatorPort: <Number>, > + * destinationPort: <Number>, > + * }, > + * mwi: { > + * discard: <Boolean>, ditto @@ +1382,5 @@ > + * serviceCategory: <Number>, > + * language: <String>, > + * > + * // Handy fields for concatenation. > + * segmentMaxSeq: <Number>, Will do. @@ +1385,5 @@ > + * // Handy fields for concatenation. > + * segmentMaxSeq: <Number>, > + * receivedSegments: <Number>, > + * segments: [], > + * timestamp: <Number> } will do. @@ +1388,5 @@ > + * segments: [], > + * timestamp: <Number> } > + * > + */ > + let smsSegmentStore = db.createObjectStore(SMS_SEGMENT_STORE_NAME, will do @@ +1389,5 @@ > + * timestamp: <Number> } > + * > + */ > + let smsSegmentStore = db.createObjectStore(SMS_SEGMENT_STORE_NAME, > + { keyPath: "hash" }); will do. @@ +2498,5 @@ > + aSmsSegment.segments = []; > + aSmsSegment.timestamp = Date.now(); > + if (aSmsSegment.encoding == RIL.PDU_DCS_MSG_CODING_8BITS_ALPHABET) { > + aSmsSegment.segments[seq] = aSmsSegment.data; > + delete aSmsSegment.data; Just wonder if this will duplicate the storage in DB because we have already appended the data/body into the segments[] for further process. @@ +2501,5 @@ > + aSmsSegment.segments[seq] = aSmsSegment.data; > + delete aSmsSegment.data; > + } else { > + aSmsSegment.segments[seq] = aSmsSegment.body; > + delete aSmsSegment.body; ditto @@ +2506,5 @@ > + } > + > + let addRequest = segmentStore.add(aSmsSegment); > + addRequest.onsuccess = function(event) { > + aCallback.notify(Cr.NS_OK, null); will do @@ +2508,5 @@ > + let addRequest = segmentStore.add(aSmsSegment); > + addRequest.onsuccess = function(event) { > + aCallback.notify(Cr.NS_OK, null); > + }; > + addRequest.onerror = function(event) { will do @@ +2516,5 @@ > + } > + } > + aCallback.notify(Cr.NS_ERROR_FAILURE, null); > + }; > + } else { Will do. @@ +2539,5 @@ > + // If the segments of a WAP Push are not received in sequence > + // (e.g., SMS with seq == 1 is not the 1st segment received by the device), > + // we have to retrieve the port information from 1st segment and > + // save it into the cached msgRecord.header. > + if (aSmsSegment.teleservice === RIL.PDU_CDMA_MSG_TELESERIVCIE_ID_WAP && seq === 1) { will do. ::: dom/system/gonk/RadioInterfaceLayer.js @@ +2966,5 @@ > + * Hash map for received multipart sms fragments. Messages are hashed with > + * its sender address and concatenation reference number. Three additional > + * attributes `segmentMaxSeq`, `receivedSegments`, `segments` are inserted. > + */ > + _receivedSmsSegmentsMap: null, The reason is that we still need what we did in ril_worker for the concatenation of Class 0 SMS which has to be handled irrespectively to the storage according 3GPP TS 23.040: `When a mobile terminated message is class 0 and the MS has the capability of displaying short messages, the MS shall display the message immediately and send an acknowledgement to the SC when the message has successfully reached the MS irrespective of whether there is memory available in the (U)SIM or ME. The message shall not be automatically stored in the (U)SIM or ME.`
Assignee | ||
Comment 22•11 years ago
|
||
Thanks for suggestion! I'll revise the test case again. :) (In reply to Vicamo Yang [:vicamo][:vyang] from comment #20)
Assignee | ||
Comment 24•11 years ago
|
||
Separate new ObjectStore implementation to new patch.
Attachment #8387393 -
Attachment is obsolete: true
Attachment #8389880 -
Flags: review?(vyang)
Assignee | ||
Comment 25•11 years ago
|
||
Separate the logic of using sms-segment store for sms concatenation to new patch.
Attachment #8389882 -
Flags: review?(vyang)
Assignee | ||
Comment 26•11 years ago
|
||
refine test case.
Attachment #8387395 -
Attachment is obsolete: true
Attachment #8389885 -
Flags: review?(vyang)
Assignee | ||
Comment 27•11 years ago
|
||
Comment on attachment 8389885 [details] [diff] [review] Patch Part 3 v2: Add new test cases to test the concatenation of text/binary sms. r=vyang cancel review for minor change.
Attachment #8389885 -
Flags: review?(vyang)
Assignee | ||
Comment 28•11 years ago
|
||
Comment on attachment 8389882 [details] [diff] [review] Patch Part 1.2 v2: Move SMS concatenation logic from ril_worker.js to RadioInterfaceLayer.js. r=vyang cancel review for minor change.
Attachment #8389882 -
Flags: review?(vyang)
Assignee | ||
Comment 29•11 years ago
|
||
Fix TestRILCodeQuality issue found in try server.
Attachment #8389882 -
Attachment is obsolete: true
Attachment #8390218 -
Flags: review?(vyang)
Assignee | ||
Comment 30•11 years ago
|
||
Attachment #8389885 -
Attachment is obsolete: true
Attachment #8390220 -
Flags: review?(vyang)
Assignee | ||
Comment 31•11 years ago
|
||
apply Promise.all() & waitForManagerEvent() for testing.
Attachment #8390220 -
Attachment is obsolete: true
Attachment #8390220 -
Flags: review?(vyang)
Attachment #8390224 -
Flags: review?(vyang)
Assignee | ||
Comment 32•11 years ago
|
||
Comment on attachment 8389880 [details] [diff] [review] Patch Part 1.1 v2: Create new ObjectStore for saving uncomplete SMS segments. r=vyang cancel review because the the segment in db was not deleted after concatenation is complete.
Attachment #8389880 -
Flags: review?(vyang)
Updated•11 years ago
|
Attachment #8390224 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 33•11 years ago
|
||
update patch to ensure that the segments of a complete sms is deleted correctly.
Attachment #8389880 -
Attachment is obsolete: true
Attachment #8390261 -
Flags: review?(vyang)
Comment 34•11 years ago
|
||
Comment on attachment 8390261 [details] [diff] [review] Patch Part 1.1 v3: Create new ObjectStore for saving uncomplete SMS segments. r=vyang Review of attachment 8390261 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm @@ +2504,5 @@ > + if (DEBUG) debug("Transaction " + txn + " completed."); > + aCallback.notify(Cr.NS_OK, completeMessage); > + }; > + > + txn.onerror = function onerror(event) { use onabort instead. @@ +2519,5 @@ > + debug("Saving SMS Segment: " + aSmsSegment.hash + ", seq: " + seq); > + } > + let getRequest = segmentStore.index("hash").get(aSmsSegment.hash); > + getRequest.onsuccess = function(event) { > + let msgRecord = event.target.result; nit: please rename to segmentRecord. We use messageRecord, threadRecord, participantRecord to clarify what are we manipulating. @@ +2532,5 @@ > + } else { > + aSmsSegment.segments[seq] = aSmsSegment.body; > + } > + > + let addRequest = segmentStore.add(aSmsSegment); nit: |addRequest| is assigned but never referenced. @@ +2533,5 @@ > + aSmsSegment.segments[seq] = aSmsSegment.body; > + } > + > + let addRequest = segmentStore.add(aSmsSegment); > + } else { nit: bail-out early. @@ +2559,5 @@ > + // we have to retrieve the port information from 1st segment and > + // save it into the msgRecord. > + if (aSmsSegment.teleservice === RIL.PDU_CDMA_MSG_TELESERIVCIE_ID_WAP > + && seq === 1) { > + if (!msgRecord.originatorPort && aSmsSegment.originatorPort) { nit: Since port information is only available in the 1st segment and we don't have duplicated 1st segment here, the |!msgRecord.originatorPort| check is certainly redundant. @@ +2563,5 @@ > + if (!msgRecord.originatorPort && aSmsSegment.originatorPort) { > + msgRecord.originatorPort = aSmsSegment.originatorPort; > + } > + > + if (!msgRecord.destinationPort && aSmsSegment.destinationPort) { ditto. @@ +2570,5 @@ > + } > + > + if (msgRecord.receivedSegments < msgRecord.segmentMaxSeq) { > + if (DEBUG) debug("Message is incomplete."); > + let updateRequest = segmentStore.put(msgRecord); nit: |updateRequest| is assigned but never referenced. @@ +2574,5 @@ > + let updateRequest = segmentStore.put(msgRecord); > + return; > + } > + > + // Rebuild full body Please move these lines into |txn.oncomplete|. Don't block DB transaction as possible. @@ +2597,5 @@ > + > + completeMessage = msgRecord; > + > + // Delete Record in DB > + let deleteRequest = segmentStore.delete(msgRecord.id); nit: |deleteRequest| is assigned but never referenced.
Attachment #8390261 -
Flags: review?(vyang)
Assignee | ||
Comment 35•11 years ago
|
||
address nits in comment 34.
Attachment #8390261 -
Attachment is obsolete: true
Attachment #8390306 -
Flags: review?(vyang)
Updated•11 years ago
|
Attachment #8390306 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 36•11 years ago
|
||
update patch to refine sendAckSMS()
Attachment #8390218 -
Attachment is obsolete: true
Attachment #8390218 -
Flags: review?(vyang)
Attachment #8390455 -
Flags: review?(vyang)
Assignee | ||
Comment 37•11 years ago
|
||
remove handy fields useful only for concatenation from completed message.
Attachment #8390306 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8390629 -
Flags: review?(vyang)
Assignee | ||
Comment 38•11 years ago
|
||
Update patch to: 1. refine handleSmsMultipart(). 2. precisely purge unnecessary fields from completed message.
Attachment #8390455 -
Attachment is obsolete: true
Attachment #8390455 -
Flags: review?(vyang)
Attachment #8390631 -
Flags: review?(vyang)
Comment 39•11 years ago
|
||
Comment on attachment 8390629 [details] [diff] [review] Patch Part 1.1 v5: Create new ObjectStore for saving uncomplete SMS segments. r=vyang Review of attachment 8390629 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm @@ +2534,5 @@ > + }; > + > + txn.onabort = function onerror(event) { > + if (DEBUG) debug("Caught error on transaction", event.target.errorCode); > + aCallback.notify(Cr.NS_ERROR_FAILURE, null, null); The callback takes only two parameters.
Attachment #8390629 -
Flags: review?(vyang) → review+
Comment 40•11 years ago
|
||
Comment on attachment 8390631 [details] [diff] [review] Patch Part 1.2 v5: Move SMS concatenation logic from ril_worker.js to RadioInterfaceLayer.js. r=vyang Review of attachment 8390631 [details] [diff] [review]: ----------------------------------------------------------------- Thank you! Looks we're almost there. A few nits to be addressed. ::: dom/system/gonk/RadioInterfaceLayer.js @@ +3031,5 @@ > + > + let isMultipart = (segment.segmentMaxSeq && (segment.segmentMaxSeq > 1)); > + let messageClass = segment.messageClass; > + > + let handleReceivedAndAck = function (aRvOfIncompleteMsg, aCompleteMessage) { nit: remove that SP between keyword 'function' and left parenthesis. @@ +3034,5 @@ > + > + let handleReceivedAndAck = function (aRvOfIncompleteMsg, aCompleteMessage) { > + if (aCompleteMessage) { > + if (this.handleSmsReceived( > + this._purgeCompleteSmsMessage(aCompleteMessage))) { I'd rather have: this._purgeCompleteSmsMessage(aCompleteMessage); if (this.handleSmsReceived(aCompleteMessage)) { ... } @@ +3045,5 @@ > + }.bind(this); > + > + // No need to access SmsSegmentStore for Class 0 SMS and Single SMS. > + if (!isMultipart || > + (messageClass == RIL.GECKO_SMS_MESSAGE_CLASSES[RIL.PDU_DCS_MSG_CLASS_0])) { nit: Alignment. if (!isMultipart || (messageClass == ... ^ | HERE @@ +3197,5 @@ > > /** > + * Handle ACK response of received SMS. > + */ > + sendAckSms: function(aRv, aMessageClass) { Please just pass |aMessage| to this function. @@ +3198,5 @@ > /** > + * Handle ACK response of received SMS. > + */ > + sendAckSms: function(aRv, aMessageClass) { > + if (aMessageClass !== RIL.GECKO_SMS_MESSAGE_CLASSES[RIL.PDU_DCS_MSG_CLASS_2]) { nit: bail-out early. if (aMessageClass === ...) { return; } ::: dom/system/gonk/ril_worker.js @@ +8784,5 @@ > replace: false, > header: message.header, > body: message.body, > data: message.data, > + sentTimestamp: message[PDU_CDMA_MSG_USERDATA_TIMESTAMP], nit: rename the same 'timestamp' attribute in 'GsmPDUHelper.readMessage' as well.
Attachment #8390631 -
Flags: review?(vyang)
Assignee | ||
Comment 41•11 years ago
|
||
address the problem in comment 39.
Attachment #8390629 -
Attachment is obsolete: true
Attachment #8392036 -
Flags: review+
Assignee | ||
Comment 42•11 years ago
|
||
Address the nits in comment 40.
Attachment #8390631 -
Attachment is obsolete: true
Attachment #8392037 -
Flags: review?(vyang)
Comment 43•11 years ago
|
||
Comment on attachment 8392037 [details] [diff] [review] Patch Part 1.2 v6: Move SMS concatenation logic from ril_worker.js to RadioInterfaceLayer.js. r=vyang Review of attachment 8392037 [details] [diff] [review]: ----------------------------------------------------------------- Thank you :)
Attachment #8392037 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 44•11 years ago
|
||
try server result is green. \o/ https://tbpl.mozilla.org/?tree=Try&rev=2f7063e5f249
Keywords: checkin-needed
Comment 45•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/46d7fdaa71d3 https://hg.mozilla.org/integration/b2g-inbound/rev/fcdeb2d43ce2 https://hg.mozilla.org/integration/b2g-inbound/rev/7989daec63cf https://hg.mozilla.org/integration/b2g-inbound/rev/edb2d87a71fb
Flags: in-testsuite+
Keywords: checkin-needed
Comment 46•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/46d7fdaa71d3 https://hg.mozilla.org/mozilla-central/rev/fcdeb2d43ce2 https://hg.mozilla.org/mozilla-central/rev/7989daec63cf https://hg.mozilla.org/mozilla-central/rev/edb2d87a71fb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
status-b2g-v1.4:
--- → fixed
status-firefox28:
--- → wontfix
status-firefox29:
--- → wontfix
status-firefox30:
--- → fixed
Updated•11 years ago
|
status-b2g-v2.0:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•