Closed Bug 921919 Opened 11 years ago Closed 11 years ago

B2G MMS: Notify Gaia SMS AP the MMS read report request and return the read result to the requester

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ctai, Assigned: ctai)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 11 obsolete files)

43.81 KB, patch
Details | Diff | Splinter Review
As a receiver, FFOS is capable of returning MMS read report when the sender is requesting that.
IMO, we've already had markMessageRead() so we might not need new API.
Summary: B2G MMS: Notify Gaia SMS AP the MMS read report request when getting a MMS and open an API for return the result. → B2G MMS: Notify Gaia SMS AP the MMS read report request and return the read result to the requester
Assignee: nobody → ctai
Per talk with Gene, we still need to add an arguments in |markMessageRead| if we don't want to add a new API.
blocking-b2g: --- → 1.3?
Attached patch bug-921919.patch v1.0 (obsolete) — Splinter Review
Attached patch bug-921919.patch v1.1 (obsolete) — Splinter Review
Attachment #812935 - Attachment is obsolete: true
This final decision in agreement is: DOMRequest markMessageRead(in long id, in boolean value [optional] in boolean sendReadReport); When |value| == TRUE && |sendReadReport| == TRUE, it will send the read port back to the requester in addition to marking the message to be read in the DB. If the parameter is not indicated, the send will apply the global setting.
We still need one more attribute in nsIDOMMozMmsMessage.idl. The attribute is: readonly attribute boolean isReadReportRequested; We need this attribute to let gaia developer know is it necessary to ask end user return read report or not.
Thanks for catching this! Nice!
Attached patch bug-921919.patch v1.2 (obsolete) — Splinter Review
Attachment #813424 - Attachment is obsolete: true
Attachment #821496 - Flags: feedback?(vyang)
Comment on attachment 821496 [details] [diff] [review] bug-921919.patch v1.2 Review of attachment 821496 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/interfaces/nsIMmsService.idl @@ +21,5 @@ > void retrieve(in long id, > in nsIMobileMessageCallback request); > + > + void sendReadReport(in DOMString messageID, > + in jsval to); Please use DOMString here. Parse address type before sending read report out. ::: dom/mobilemessage/src/MobileMessageManager.cpp @@ +357,5 @@ > NS_ENSURE_TRUE(mobileMessageDBService, NS_ERROR_FAILURE); > > nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner()); > nsCOMPtr<nsIMobileMessageCallback> msgCallback = new MobileMessageCallback(request); > + nsresult rv = mobileMessageDBService->MarkMessageRead(aId, aValue, aSendReadReport, msgCallback); nit: line wrap ::: dom/mobilemessage/src/gonk/MmsService.js @@ +1206,5 @@ > + // Mandatory fields > + headers["x-mms-message-type"] = MMS.MMS_PDU_TYPE_READ_REC_IND; > + headers["x-mms-mms-version"] = MMS.MMS_VERSION; > + headers["message-id"] = messageID; > + headers["to"] = to; Parse |to| address here. @@ +1335,5 @@ > intermediate.sender = "anonymous"; > } > intermediate.receivers = []; > intermediate.phoneNumber = this.getPhoneNumber(); > + intermediate.sentReadReport = false; Don't assign a default value in |convertIntermediateToSavable|. It's for attributes critical to MobileMessageDatabaseService::saveReceivedMessage. And since it has a default value, it's certainly not critical. Please move this line into MobileMessageDatabaseService::saveReceivedMessage. Besides, please also rename it to |isReadReportSent|. ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +1140,5 @@ > + } > + > + let messageRecord = cursor.value; > + if (messageRecord.type == "sms") { > + messageRecord.sentReadReport = false; Please update all "MMS" messages and set their 'isReadReportSent' attribute to false. @@ +1208,5 @@ > expiryDate = aMessageRecord.timestamp + headers["x-mms-expiry"] * 1000; > } > + let readReport = false; > + if (headers["x-mms-read-report"] != undefined) { > + readReport = headers["x-mms-read-report"]; 'isReadReportRequested', and you can have: let isReadReportRequested = headers["x-mms-read-report"] || false; @@ +1916,5 @@ > (aMessage.type == "mms" && (aMessage.delivery == undefined || > aMessage.transactionId == undefined || > !Array.isArray(aMessage.deliveryStatus) || > + !Array.isArray(aMessage.receivers) || > + aMessage.sentReadReport == undefined)) || You don't need it. Simply assign |aMessage.isReadReportSent| to false whenever it's a MMS. @@ +2295,5 @@ > } > messageRecord.read = value ? FILTER_READ_READ : FILTER_READ_UNREAD; > messageRecord.readIndex = [messageRecord.read, messageRecord.timestamp]; > + if (messageRecord.type == "mms" && !messageRecord.sentReadReport && > + sendReadReport) { if (sendReadReport && messageRecord.type == "mms" && messageRecord.read == FILTER_READ_READ && !messageRecord.isReadReportSend) @@ +2299,5 @@ > + sendReadReport) { > + messageRecord.sentReadReport = true; > + let messageID = messageRecord.headers["message-id"]; > + let to = messageRecord.headers["from"]; > + gMMSService.sendReadReport(messageID, to); Please queue these things up and really send read report in: threadStore.put(threadRecord).onsuccess = function(event) {
Attachment #821496 - Flags: feedback?(vyang)
Attached patch bug-921919.patch v1.3 (obsolete) — Splinter Review
Attachment #821496 - Attachment is obsolete: true
Attached patch bug-921919.patch v1.4 (obsolete) — Splinter Review
Attachment #823178 - Attachment is obsolete: true
Attachment #823179 - Flags: review?(vyang)
Comment on attachment 823179 [details] [diff] [review] bug-921919.patch v1.4 Review of attachment 823179 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MmsService.js @@ +1213,5 @@ > + headers["to"] = to; > + headers["from"] = null; > + headers["x-mms-read-status"] = true; > + > + this.istream = MMS.PduHelper.compose(null, {headers: headers}); throw |Cr.NS_ERROR_FAILURE| if |this.istream| is null and have a catch in MmsService::sendReadReport(). @@ +1222,5 @@ > + if (callback) { > + requestCallback = function (httpStatus, data) { > + callback(httpStatus); > + }; > + } ReadRecTransaction::run is always called without a callback function. So let skip this lines here. @@ +2139,5 @@ > > + sendReadReport: function sendReadReport(messageID, toAddress) { > + if (DEBUG) debug("messageID: " + messageID + " toAddress: " + JSON.stringify(toAddress)); > + let readRecTransaction = new ReadRecTransaction(messageID, toAddress); > + readRecTransaction.run(); try { let transaction = new ReadRecTransaction(messageID, toAddress); transaction.run(); } catch (e) {} ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +2292,5 @@ > } > messageRecord.read = value ? FILTER_READ_READ : FILTER_READ_UNREAD; > messageRecord.readIndex = [messageRecord.read, messageRecord.timestamp]; > + let sendReadReportArguments = null; > + if (sendReadReport && nit: trailing ws
Attachment #823179 - Flags: review?(vyang) → review+
Attached patch bug-921919.patch v1.5 (obsolete) — Splinter Review
Fixed according to vyang's comments.
Attachment #823179 - Attachment is obsolete: true
Attached patch bug-921919.patch v1.6 (obsolete) — Splinter Review
Rebased.
Attachment #823847 - Attachment is obsolete: true
Attached patch bug-921919.patch v1.7 (obsolete) — Splinter Review
Rebased...
Attachment #824432 - Attachment is obsolete: true
Attached patch bug-921919.patch v1.8 (obsolete) — Splinter Review
Remove empty line.
Attachment #825754 - Attachment is obsolete: true
This has a lot of merge conflicts with b2g-inbound. What branch are you rebasing on top of?
Keywords: checkin-needed
Hi, Ryan, I used b2g-inbound, but bug 887159 landed quicker than me. I will rebase again. (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17) > This has a lot of merge conflicts with b2g-inbound. What branch are you > rebasing on top of?
Attached patch bug-921919.patch v1.9 (obsolete) — Splinter Review
Rebased on top of bug 866938 and bug 887159.
Attachment #825784 - Attachment is obsolete: true
Attachment #826582 - Flags: review?(vyang)
Comment on attachment 826582 [details] [diff] [review] bug-921919.patch v1.9 Review of attachment 826582 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, found some more things to be fixed before landing. ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js @@ +237,4 @@ > self.upgradeSchema15(event.target.transaction, next); > break; > case 16: > + if (DEBUG) debug("Upgrade to version 17. Add sentReadReport for incoming MMS."); nit: isReadReportSent @@ +1079,5 @@ > }; > }, > > + /** > + * Add sentReadReport for incoming MMS. ditto @@ +2138,5 @@ > + messageRecord.isReadReportSend = true; > + let messageID = messageRecord.headers["message-id"]; > + let to = messageRecord.headers["from"]; > + sendReadReportArguments = {messageID: messageID, > + toAddress : to.address} Both MESSAGE-ID and FROM could be null because the former is a conditional parameter and the latter is an optional one. Besides, we should have some more strict conditions that we should only send READ REPORT to retrieved MMS messages.
Attachment #826582 - Flags: review?(vyang)
Attached patch bug-921919.patch v1.10 (obsolete) — Splinter Review
Thanks, vyang.
Attachment #826582 - Attachment is obsolete: true
Attachment #826611 - Flags: review?(vyang)
Attachment #826611 - Flags: review?(vyang) → review+
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Attachment #826611 - Attachment is obsolete: true
i had to back this out from b2g-inbound for build failure like https://tbpl.mozilla.org/php/getParsedLog.php?id=30064881&tree=B2g-Inbound
Thanks, sorry for the build failure. (In reply to Carsten Book [:Tomcat] from comment #25) > i had to back this out from b2g-inbound for build failure like > https://tbpl.mozilla.org/php/getParsedLog.php?id=30064881&tree=B2g-Inbound
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Although we've already had full discussions about the API changes to support read report, technically, we still need supuerreview+ before landing when it comes to DOM API changes. No worries. Just a reminder. :)
Thanks for reminder. Will be more careful next time. :) (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #29) > Although we've already had full discussions about the API changes to support > read report, technically, we still need supuerreview+ before landing when it > comes to DOM API changes. No worries. Just a reminder. :)
This landed before gecko 28 branched so will be in 1.3.
blocking-b2g: 1.3? → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: