Closed
Bug 849741
Opened 12 years ago
Closed 12 years ago
B2G MMS: provide nsIDOMMobileMessageManager.onreceived event
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: airpingu, Assigned: ctai)
References
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 13 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #844431 +++ After bug 844431 is done, we can have a |nsIDOMMobileMessageManager| interface to support firing an onreceived event whenever receiving an MMS notification. We also need to wait on the bug 845643 done.
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #724828 -
Flags: review?(vyang)
Reporter | ||
Comment 5•12 years ago
|
||
Comment on attachment 724828 [details] [diff] [review] Deal with onreceived event v1.2 Review of attachment 724828 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1028,5 @@ > // for the resended notification indication. > return; > } > + // Notifing new comming notification indication through notifyObservers. > + let notifyMessage = this.createMmsMessageFromRecord(messageRecord); Per off-line discussion, we need to be careful to use .createMmsMessageFromRecord(), which expects the content as a blob instead of a string. Two solutions: 1. Don't save a string for content into DB when receiving an MMS. Instead, always a blob. 2. During the sending process, we need to convert the blob to string to be saved into DB, when the content type is "text/...". If we do this, we need to restore the string back to a blob in the implementation of .createMmsMessageFromRecord(). @@ +1087,5 @@ > transaction.run(); > return; > } > + // Notifing new retrieved MMS message through notifyObservers. > + let retrievedMessage = this.createMmsMessageFromRecord(messageRecord); ditto.
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #724828 -
Attachment is obsolete: true
Attachment #724828 -
Flags: review?(vyang)
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #6) > Created attachment 725316 [details] [diff] [review] > Deal with onreceived event v1.3 Modify according to Gene's comment.
Assignee | ||
Updated•12 years ago
|
Attachment #725316 -
Flags: review?(vyang)
Comment 8•12 years ago
|
||
Comment on attachment 725316 [details] [diff] [review] Deal with onreceived event v1.3 Review of attachment 725316 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling the review because the change in database service. Probably incorrect. :( ::: dom/mms/src/ril/MmsService.js @@ +1027,5 @@ > // response the notification. Hope the end user will clean some space > // for the resended notification indication. > return; > } > + // Notifing new comming notification indication through notifyObservers. nit: new line @@ +1086,5 @@ > transactionId, MMS.MMS_PDU_STATUS_DEFERRED, reportAllowed); > transaction.run(); > return; > } > + // Notifing new retrieved MMS message through notifyObservers. nit: new line @@ +1089,5 @@ > } > + // Notifing new retrieved MMS message through notifyObservers. > + let retrievedMessage = this.createMmsMessageFromRecord(messageRecord); > + Services.obs.notifyObservers(retrievedMessage, kMmsReceivedObserverTopic, null); > + remove this empty line, please ::: dom/mms/src/ril/WspPduHelper.jsm @@ +2292,5 @@ > let octetArray = Octet.decodeMultiple(data, contentEnd); > let content = null; > if (octetArray) { > + content = new Blob([octetArray], > + {"type" : headers["content-type"].media}); feel like having: content = new Blob([octetArray], { type : headers["content-type"].media}); ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1112,5 @@ > aMessage.readIndex = [FILTER_READ_UNREAD, timestamp]; > if (aMessage.type == "sms") { > aMessage.delivery = DELIVERY_RECEIVED; > } > + aMessage.deliveryStatus = [DELIVERY_STATUS_NOT_APPLICABLE]; If this is the only change made to db service, then this patch would probably break the whole database. You should also modify setMessageDelivery(), createMessageFromRecord(), etc.
Attachment #725316 -
Flags: review?(vyang)
Reporter | ||
Comment 9•12 years ago
|
||
Don't need to call this.createMmsMessageFromRecord() anymore after Bug 847738 lands. Let's check in Bug 847738 first to make tasks simple here.
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 725316 [details] [diff] [review] Deal with onreceived event v1.3 Review of attachment 725316 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1014,5 @@ > > notification = this.convertFromIntermediateToSavable(notification); > > gMobileMessageDatabaseService.saveReceivedMessage(notification, > (function (rv, messageRecord) { s/messageRecord/domMessage @@ +1029,5 @@ > return; > } > + // Notifing new comming notification indication through notifyObservers. > + let notifyMessage = this.createMmsMessageFromRecord(messageRecord); > + Services.obs.notifyObservers(notifyMessage, kMmsReceivedObserverTopic, null); Don't need to call this.createMmsMessageFromRecord() anymore. Just calling: Services.obs.notifyObservers(domMessage, kMmsReceivedObserverTopic, null); @@ +1072,5 @@ > } > > messageRecord = this.mergeRetrievalConfirmationIntoRecord(retrievedMsg, messageRecord); > gMobileMessageDatabaseService.saveReceivedMessage(messageRecord, > (function (rv, messageRecord) { ditto. @@ +1088,5 @@ > return; > } > + // Notifing new retrieved MMS message through notifyObservers. > + let retrievedMessage = this.createMmsMessageFromRecord(messageRecord); > + Services.obs.notifyObservers(retrievedMessage, kMmsReceivedObserverTopic, null); ditto. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1112,5 @@ > aMessage.readIndex = [FILTER_READ_UNREAD, timestamp]; > if (aMessage.type == "sms") { > aMessage.delivery = DELIVERY_RECEIVED; > } > + aMessage.deliveryStatus = [DELIVERY_STATUS_NOT_APPLICABLE]; I don't think this is correct. We should save a single string for SMS's .deliveryStatus and save an array of strings for MMS's .deliveryStatus. Please refer to what I've done in the .saveSendingMessage(). I've already supported .setMessageDelivery() and .createDomMessageFromRecord() to properly interpret .deliveryStatus based on SMS/MMS branches. Don't need to worry about them I'd think. :)
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #725316 -
Attachment is obsolete: true
Attachment #725987 -
Flags: feedback?(gene.lian)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #725987 -
Attachment is obsolete: true
Attachment #725987 -
Flags: feedback?(gene.lian)
Assignee | ||
Updated•12 years ago
|
Attachment #725989 -
Flags: feedback?(gene.lian)
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #725989 -
Attachment is obsolete: true
Attachment #725989 -
Flags: feedback?(gene.lian)
Assignee | ||
Updated•12 years ago
|
Attachment #725990 -
Flags: feedback?(gene.lian)
Reporter | ||
Comment 15•12 years ago
|
||
Comment on attachment 725987 [details] [diff] [review] Deal with onreceived event v2.0 Review of attachment 725987 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1101,5 @@ > return; > } > + > + // Notifing new retrieved MMS message through notifyObservers. > + Services.obs.notifyObservers(retrievedMessage, domMessage, null); Oops, minor nit: Services.obs.notifyObservers(domMessage, kMmsReceivedObserverTopic, null); Btw, I think you forget to remove another TODO comment above. ::: dom/mms/src/ril/WspPduHelper.jsm @@ +2292,5 @@ > let octetArray = Octet.decodeMultiple(data, contentEnd); > let content = null; > if (octetArray) { > + content = new Blob([octetArray], > + {type : headers["content-type"].media}); Please align this as Vicamo suggested. ::: dom/mobilemessage/src/Constants.cpp @@ +16,5 @@ > > const char* kMmsSendingObserverTopic = "mms-sending"; > const char* kMmsSentObserverTopic = "mms-sent"; > const char* kMmsFailedObserverTopic = "mms-failed"; > +const char* kMmsReceivedObserverTopic = "mms-received"; Vicamo used to suggest we should share this with sms-received but not sure we should do that in this bug or address it in another one. I don't have strong opinion on that. Let's confirm with Vicamo. ::: dom/mobilemessage/src/MobileMessageManager.cpp @@ +461,5 @@ > + NS_ERROR("Got a 'mms-received' topic without a valid message!"); > + return NS_OK; > + } > + > + DispatchTrustedMmsEventToSelf(FAILED_EVENT_NAME, message); s/FAILED_EVENT_NAME/RECEIVED_EVENT_NAME
Attachment #725987 -
Flags: feedback+
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #725990 -
Attachment is obsolete: true
Attachment #725990 -
Flags: feedback?(gene.lian)
Reporter | ||
Comment 17•12 years ago
|
||
Comment on attachment 725993 [details] [diff] [review] Deal with onreceived event v2.2 Review of attachment 725993 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1171,5 @@ > + // Set |aMessage.deliveryStatus|. Note that for MMS record > + // it must be an array of strings; For SMS, it's a string. > + let deliveryStatus = aMessage.deliveryStatusRequested > + ? DELIVERY_STATUS_PENDING > + : DELIVERY_STATUS_NOT_APPLICABLE; Sorry, I missed this. You don't need deliveryStatusRequested. That is for saveSendingMessage() only. Just directly using DELIVERY_STATUS_NOT_APPLICABLE. @@ +1176,2 @@ > if (aMessage.type == "sms") { > + aMessage.deliveryStatus = deliveryStatus; aMessage.deliveryStatus = DELIVERY_STATUS_NOT_APPLICABLE; @@ +1188,5 @@ > + return; > + } > + aMessage.deliveryStatus = []; > + for (let i = 0; i < receivers.length; i++) { > + aMessage.deliveryStatus.push(deliveryStatus); aMessage.deliveryStatus.push(DELIVERY_STATUS_NOT_APPLICABLE);
Attachment #725993 -
Flags: feedback+
Comment 18•12 years ago
|
||
schung also need to do some work after this bug lands
Whiteboard: [target 3/22]
Assignee | ||
Updated•12 years ago
|
Attachment #726028 -
Flags: feedback?(schung)
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #726028 -
Attachment is obsolete: true
Attachment #726028 -
Flags: feedback?(schung)
Assignee | ||
Updated•12 years ago
|
Attachment #726058 -
Flags: feedback?(schung)
Comment 23•12 years ago
|
||
Comment on attachment 726058 [details] [diff] [review] Deal with onreceived event v2.4 Review of attachment 726058 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1135,5 @@ > if (aMessage.type === undefined || > aMessage.sender === undefined || > (aMessage.type == "sms" && aMessage.messageClass === undefined) || > (aMessage.type == "mms" && aMessage.delivery === undefined) || > + (aMessage.type == "mms" && aMessage.deliveryStatus === undefined) || Please also help correct "===": if (aMessage.type == undefined || aMessage.sender == undefined || (aMessage.type == "sms" && aMessage.messageClass === undefined) || (aMessage.type == "mms" && (aMessage.delivery == undefined || aMessage.deliveryStatus == undefined)) || aMessage.timestamp == undefined) {
Attachment #726058 -
Flags: feedback?(schung) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 25•12 years ago
|
||
Conflicted with: const DEBUG = true; Please provide a new patch and a=leo+. Thanks!
Keywords: checkin-needed
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 27•12 years ago
|
||
(In reply to Gene Lian [:gene] from comment #25) > Please provide a new patch and a=leo+. Thanks! Sorry, no this. Not yet leo+.
Reporter | ||
Comment 28•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/48e028f2a5d7
Keywords: checkin-needed
Reporter | ||
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 29•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/48e028f2a5d7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Updated•12 years ago
|
blocking-b2g: leo? → leo+
Reporter | ||
Comment 30•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0bc3eee6dfac
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → fixed
Reporter | ||
Comment 31•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Whiteboard: [target 3/22] → [NO_UPLIFT]
Reporter | ||
Comment 32•12 years ago
|
||
Added [NO_UPLIFT] per recent commercial RIL compatibility issue. Waiting on further decision to keep the patch in b2g18 or to back it out. ------------------------------ If we really want to back them out, backing the following MMS bugs should be enough to make the commercial RIL compatible: Bug 854422 - B2G MMS: should call .NotifyResponseTransaction() with MMS_PDU_STATUS_RETRIEVED after an MMS is retrieved under the RETRIEVAL_MODE_AUTOMATIC mode (a follow-up for bug 845643) Bug 850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages Bug 850530 - B2G MMS: Use the same attribute name for delivery (s/state/delivery) like SMS Bug 852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments. Bug 853725 - B2G MMS: fail to read nsIDOMMozMmsMessage.receivers for a received MMS (a follow-up of bug 849741). Bug 853329 - B2G MMS: other Android phones cannot read attachments sent from FFOS Bug 852471 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) (follow-up fix) Bug 852460 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event (follow-up fix) Bug 849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event Bug 847756 - B2G MMS: provide nsIDOMMobileMessageManager.markMessageRead(). Bug 847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete(). Bug 847738 - B2G MMS: provide nsIDOMMobileMessageManager.getMessage(). Bug 844431 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) Bug 845643 - B2G MMS: Save retrieved MMS into database.
Reporter | ||
Comment 33•12 years ago
|
||
Following the previous comment, some more needs to back out: Bug 792321 - Check max values of MMS parameters in sendRequest. Bug 833291 - B2G SMS & MMS: getMessages it's not working with PhoneNumberJS Bug 844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS Bug 839436 - B2G MMS: make DB be able to save MMS messages.
Reporter | ||
Comment 34•12 years ago
|
||
Confirming with Michael to see we should back out to Bug 839436 or just Bug 844431 (if we eventually decide to back out). Please see Bug 857632, comment #17.
Reporter | ||
Comment 36•12 years ago
|
||
Per off-line discussion with Michael, we decided not to back out the MMS bugs that have already been in mozilla-b2g18. Removing [NO_UPLIFT] to make the check-in status sync'ed.
Whiteboard: [NO_UPLIFT]
Updated•11 years ago
|
Flags: in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•