Closed
Bug 845643
Opened 11 years ago
Closed 11 years ago
B2G MMS: Save retrieved MMS into database.
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: ctai, Assigned: ctai)
References
Details
Attachments
(1 file, 12 obsolete files)
13.95 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ctai
blocking-b2g: --- → leo?
Updated•11 years ago
|
Summary: B2G MMS: Save retrieved MM into database. → B2G MMS: Save retrieved MMS into database.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #719795 -
Attachment is obsolete: true
Attachment #719796 -
Flags: feedback?(vyang)
Updated•11 years ago
|
Whiteboard: [by 3/8]
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #719796 -
Attachment is obsolete: true
Attachment #719796 -
Flags: feedback?(vyang)
Assignee | ||
Updated•11 years ago
|
Attachment #720549 -
Flags: feedback?(vyang)
Assignee | ||
Updated•11 years ago
|
Attachment #720549 -
Flags: feedback?(gene.lian)
Comment 4•11 years ago
|
||
Comment on attachment 720549 [details] [diff] [review] WIP-Save received MMS Review of attachment 720549 [details] [diff] [review]: ----------------------------------------------------------------- Thank you :) ::: dom/mms/src/ril/MmsService.js @@ +824,5 @@ > > + notification.type = "mms"; > + notifyMsg.pduType = "NOTIFICATION_IND"; > + gMobileMessageDatabaseService.saveReceivedMessage(notifyMsg, > + function (rv, record) { Use |messageRecord| instead. @@ +850,5 @@ > + // should not store into database. > + if (MMS.MMS_PDU_STATUS_RETRIEVED == mmsStatus) { > + retrievedMsg.type = "mms"; > + retrievedMsg.pduType = "RETRIEVE_CONF"; > + retrievedMsg.id = record.id; for (let field in retrievedMsg.headers) { messageRecord.headers[field] = retrievedMsg.headers[field]; } if (retrievedMsg.parts) { messageRecord.parts = retrievedMsg.parts; } if (retrievedMsg.content) { messageRecord.content = retrievedMsg.content; } ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +647,5 @@ > return false; > }, > > saveRecord: function saveRecord(aRecord, aCallback) { > + if (aMessage.pduType !== "RETRIEVE_CONF") { if (aRecord.id === null) { // Assign a new id. } @@ +740,5 @@ > * nsIRilMobileMessageDatabaseService API > */ > > saveReceivedMessage: function saveReceivedMessage(aMessage, aCallback) { > + if (aMessage.type == "mms") { Since we handle all MMS specific attributes in one if-block, let's open another interface method for saving mms stuff. @@ +741,5 @@ > */ > > saveReceivedMessage: function saveReceivedMessage(aMessage, aCallback) { > + if (aMessage.type == "mms") { > + if (aMessage.timestamp === undefined) { If we're going to create another interface method, then checkings here might also include "type", "sender", ... @@ +748,5 @@ > + } > + return; > + } > + aMessage.deliveryStatus = (aMessage.pduType == "NOTIFICATION_IND") ? > + DELIVERY_STATUS_PENDING:DELIVERY_STATUS_SUCCESS; In MMS, deliveryStatus should be an array, which has the size of the number of receivers, which is one in this (received) case. @@ +751,5 @@ > + aMessage.deliveryStatus = (aMessage.pduType == "NOTIFICATION_IND") ? > + DELIVERY_STATUS_PENDING:DELIVERY_STATUS_SUCCESS; > + aMessage.sender = ""; > + if (aMessage.headers && aMessage.headers.from && aMessage.headers.from.address) { > + aMessage.sender = aMessage.headers.from.address; Fill necessary attributes outside first. If header field 'From' is omitted, set to null. @@ +755,5 @@ > + aMessage.sender = aMessage.headers.from.address; > + } > + aMessage.receiver = ""; > + if (aMessage.headers && aMessage.headers.to && aMessage.headers.to.address) { > + aMessage.receiver = aMessage.headers.from.address; Fill necessary attributes outside first. And, this attribute should be an array of combined values of To & Cc header fields. @@ +757,5 @@ > + aMessage.receiver = ""; > + if (aMessage.headers && aMessage.headers.to && aMessage.headers.to.address) { > + aMessage.receiver = aMessage.headers.from.address; > + } > + aMessage.timestamp = (aMessage.pduType == "NOTIFICATION_IND") ? Fill necessary attributes outside first. Set to now at saving indication pdu. @@ +760,5 @@ > + } > + aMessage.timestamp = (aMessage.pduType == "NOTIFICATION_IND") ? > + new Date():aMessage.headers.date; > + // Adding needed indexes and extra attributes for internal use. > + aMessage.deliveryIndex = [DELIVERY_RECEIVED, aMessage.timestamp]; We need another DELIVERY_NOT_DOWNLOADED for MMS notification. @@ +762,5 @@ > + new Date():aMessage.headers.date; > + // Adding needed indexes and extra attributes for internal use. > + aMessage.deliveryIndex = [DELIVERY_RECEIVED, aMessage.timestamp]; > + aMessage.numberIndex = [[aMessage.sender, aMessage.timestamp], > + [aMessage.receiver, aMessage.timestamp]]; There could be multiple receivers. @@ +763,5 @@ > + // Adding needed indexes and extra attributes for internal use. > + aMessage.deliveryIndex = [DELIVERY_RECEIVED, aMessage.timestamp]; > + aMessage.numberIndex = [[aMessage.sender, aMessage.timestamp], > + [aMessage.receiver, aMessage.timestamp]]; > + aMessage.readIndex = [FILTER_READ_UNREAD, aMessage.timestamp]; There could be multiple read status reports, one for each recipient. But maybe open as another bug :( @@ +764,5 @@ > + aMessage.deliveryIndex = [DELIVERY_RECEIVED, aMessage.timestamp]; > + aMessage.numberIndex = [[aMessage.sender, aMessage.timestamp], > + [aMessage.receiver, aMessage.timestamp]]; > + aMessage.readIndex = [FILTER_READ_UNREAD, aMessage.timestamp]; > + aMessage.delivery = DELIVERY_RECEIVED; We need another DELIVERY_NOT_DOWNLOADED for MMS notification.
Attachment #720549 -
Flags: feedback?(vyang)
Updated•11 years ago
|
Attachment #720549 -
Flags: feedback?(gene.lian)
Comment 5•11 years ago
|
||
this is required to support MMS user stories. Leo+
blocking-b2g: leo? → leo+
Assignee | ||
Comment 6•11 years ago
|
||
Attachment #720549 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #722091 -
Attachment is obsolete: true
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #722097 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #722098 -
Flags: feedback?(vyang)
Assignee | ||
Updated•11 years ago
|
Attachment #722098 -
Flags: feedback?(gene.lian)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #722098 -
Attachment is obsolete: true
Attachment #722098 -
Flags: feedback?(vyang)
Attachment #722098 -
Flags: feedback?(gene.lian)
Assignee | ||
Updated•11 years ago
|
Attachment #722618 -
Flags: feedback?(vyang)
Comment 10•11 years ago
|
||
Comment on attachment 722618 [details] [diff] [review] Save retrieved MMS to DB Review of attachment 722618 [details] [diff] [review]: ----------------------------------------------------------------- Please rebase after bug 833291.
Attachment #722618 -
Flags: feedback?(vyang)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #722618 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #722699 -
Flags: review?(vyang)
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #11) > Created attachment 722699 [details] [diff] [review] > Save retrieved MMS to DB rebased patch.
Comment 13•11 years ago
|
||
expect to land on 3/11
Comment 14•11 years ago
|
||
Chia-Hung is implementing receiving part and Gene is implementing sending part.
Comment 15•11 years ago
|
||
Comment on attachment 722699 [details] [diff] [review] Save retrieved MMS to DB Review of attachment 722699 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +827,5 @@ > + notification.sender = null; > + if (notification.headers && notification.headers.from) { > + notification.sender = notification.headers.from; > + } > + notification.receiver = []; Have a |convertFromIntermediateToSavable()|. @@ +844,5 @@ > + // Because MMSC will resend the notification indication once we don't > + // response the notification. Hope the end user will clean some space > + // for the resended notification indication. > + return; > + } new line. @@ +859,5 @@ > + > + // If the mmsStatus is still MMS_PDU_STATUS_DEFERRED after retry, we > + // should not store into database. > + if (MMS.MMS_PDU_STATUS_RETRIEVED == mmsStatus) { > + messageRecord.delivery = MMS.DELIVERY_RECEIVED; Move all the converting process into |convertFromIntermediateToSavable()| ::: dom/mms/src/ril/mms_consts.js @@ +98,5 @@ > +this.DELIVERY_RECEIVED = "received"; > +this.DELIVERY_NOT_DOWNLOADED = "not-downloaded"; > +this.DELIVERY_SENDING = "sending"; > +this.DELIVERY_SENT = "sent"; > +this.DELIVERY_ERROR = "error"; Please move to MmsService.
Attachment #722699 -
Flags: review?(vyang)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #722699 -
Attachment is obsolete: true
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #723754 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #723755 -
Flags: review?(vyang)
Comment 18•11 years ago
|
||
Comment on attachment 723755 [details] [diff] [review] Save retrieved MMS to DB Review of attachment 723755 [details] [diff] [review]: ----------------------------------------------------------------- Thank you :) ::: dom/mms/src/ril/MmsService.js @@ +808,5 @@ > + * @param pervousRecord > + * Pervous record store in the database. > + */ > + convertFromIntermediateToSavable: function convertFromIntermediateToSavable(intermediateMsg, > + pervousRecord) { Spelling: previous @@ +818,5 @@ > + if (intermediateMsg.headers && intermediateMsg.headers.from) { > + intermediateMsg.sender = intermediateMsg.headers.from.address; > + } > + intermediateMsg.receivers = []; > + return intermediateMsg; Indentation. @@ +825,5 @@ > + if (pervousRecord == null) { > + return null; > + } > + if (MMS.MMS_PDU_TYPE_RETRIEVE_CONF == intermediateMsg.headers["x-mms-message-type"]) { > + pervousRecord.delivery = DELIVERY_RECEIVED; Let's have split retrieval confirmation handling into another function, named "mergeRetrievalConfirmationIntoRecord"? @@ +859,5 @@ > + } > + } else { > + pervousRecord.receivers.push(intermediateMsg.headers.cc.address); > + } > + } for each (let type in ["cc", "to"]) { // ... } And please also have a comment why we ignore BCC here. @@ +964,5 @@ > + } > + > + let transaction = > + new NotifyResponseTransaction(transactionId, mmsStatus, reportAllowed); > + transaction.run(); Please move these lines into: if (MMS.MMS_PDU_STATUS_RETRIEVED != mmsStatus) { ... return; } messageRecord = this.convertFromIntermediateToSavable(retrievedMsg, messageRecord); ... @@ +967,5 @@ > + new NotifyResponseTransaction(transactionId, mmsStatus, reportAllowed); > + transaction.run(); > + }).bind(this)); > + return; > + } new line ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1087,5 @@ > } > + if (aMessage.type == "sms") { > + aMessage.receiver = this.getRilIccInfoMsisdn(); > + } > + if (aMessage.type == "mms" && aMessage.receivers.length == 0) { |else if| @@ +1104,4 @@ > aMessage.deliveryStatus = DELIVERY_STATUS_SUCCESS; > aMessage.read = FILTER_READ_UNREAD; > > return this.saveRecord(aMessage, [aMessage.sender], aCallback); We have to take care |[aMessage.sender]| here. Actually that array means "all participants other than myself". So, for SMS, it's |aMessage.sender| only; for MMS, it's |aMessage.sender| plus all receivers besides myself. You might have: let threadParticipants = [aMessage.sender]; if (aMessage.type == "mms") { aMessage.receiver = this.getRilIccInfoMsisdn(); } else if (aMessage.type == "mms") { let self = this.getRilIccInfoMsisdn(); if (!aMessage.receivers.length) { if (self) { aMessage.receivers.push(self); } } else { let xxx = aMessage.receivers.slice(); if (self) { xxx.splice(xxx.indexOf(self)); } threadParticipants.concat(xxx); } }
Attachment #723755 -
Flags: review?(vyang)
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #723755 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #723799 -
Flags: review?(vyang)
Assignee | ||
Updated•11 years ago
|
Attachment #723799 -
Flags: review?(vyang)
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #723799 -
Attachment is obsolete: true
Attachment #723804 -
Flags: review?(vyang)
Comment 21•11 years ago
|
||
Comment on attachment 723804 [details] [diff] [review] Save retrieved MMS to DB v3.0 Review of attachment 723804 [details] [diff] [review]: ----------------------------------------------------------------- Great! Lunch time! ::: dom/mms/src/ril/MmsService.js @@ +812,5 @@ > + intermediate.timestamp = Date.now(); > + intermediate.sender = null; > + if (intermediate.headers.from) { > + intermediate.sender = intermediate.headers.from.address; > + } We may have undefined sender here. @@ +828,5 @@ > + */ > + mergeRetrievalConfirmationIntoRecord: function mergeRetrievalConfirmationIntoRecord(intermediate, record) { > + if (record == null) { > + return null; > + } Do we need it? @@ +835,5 @@ > + record.timestamp = Date.parse(intermediate.headers["Date"]); > + } > + if (intermediate.headers.from) { > + record.sender = intermediate.headers.from.address; > + } Possible undefined sender, too. @@ +855,5 @@ > + for (let field in intermediate.headers) { > + record.headers[field] = intermediate.headers[field]; > + } > + if (intermediate.parts) { > + record.parts = intermediate.parts; Indentation. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1096,5 @@ > + } > + } else { > + let slicedReceivers = aMessage.receivers.slice(); > + if (self && (slicedReceivers.indexOf(self) !== -1)) { > + slicedReceivers.splice(slicedReceivers.indexOf(self), 1); cache |slicedReceivers.indexOf(self)|.
Attachment #723804 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #723804 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 24•11 years ago
|
||
Please remove checkin-needed when pushing to inbound.
Keywords: checkin-needed
Comment 25•11 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM] from comment #24) > Please remove checkin-needed when pushing to inbound. Sorry for that. Just forgot :(
Comment 26•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/90fbe56a05b1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Comment 27•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/baaa721f947f
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → fixed
Updated•11 years ago
|
Whiteboard: [by 3/8] → [NO_UPLIFT]
Comment 28•11 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.
Comment 29•11 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.
Comment 30•11 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.
Comment 31•11 years ago
|
||
Gene, no need to back out Bug 839436 or just Bug 844431 as commercial RIL is not compatible with the SMS interfaces changes.
Comment 32•11 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
•