Closed
Bug 847738
Opened 12 years ago
Closed 12 years ago
B2G MMS: provide nsIDOMMobileMessageManager.getMessage()
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: airpingu, Assigned: airpingu)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [RIL_INTERFACE])
Attachments
(2 files, 8 obsolete files)
(deleted),
patch
|
airpingu
:
review+
airpingu
:
superreview+
ctai
:
feedback+
|
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 add: DOMRequest getMessage(in long id); and then deprecate the following one in the |nsIDOMMobileMessageManager|: nsIDOMMozSmsRequest getMessage(in long id); This function needs to be compatible with both SMS and MMS.
Comment 1•12 years ago
|
||
this is required to support MMS user stories. Leo+
blocking-b2g: leo? → leo+
Assignee | ||
Comment 2•12 years ago
|
||
We'll make up the IPC structure at bug 847744. For now, we just hope to provide a non-oop version to let Gaia have some runnable APIs to use due to the (crazy) MMS timeline.
Attachment #722621 -
Flags: feedback?(vyang)
Attachment #722621 -
Flags: feedback?(mounir)
Comment 3•12 years ago
|
||
Comment on attachment 722621 [details] [diff] [review] Patch Review of attachment 722621 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/MobileMessageManager.cpp @@ +207,3 @@ > { > + nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner()); > + NS_ADDREF(*aRequest = request); NS_ADDREF just before you're going to return it. Or there will be memory leakage. @@ +214,3 @@ > nsCOMPtr<nsIMobileMessageDatabaseService> mobileMessageDBService = > do_GetService(MOBILE_MESSAGE_DATABASE_SERVICE_CONTRACTID); > NS_ENSURE_TRUE(mobileMessageDBService, NS_ERROR_FAILURE); Please move |new DOMRequest()| and |new MobileMessageCallback| after |do_GetService()|. This way, when |do_GetService| fails, we don't have to destruct instanciated but unused objects. @@ +214,5 @@ > nsCOMPtr<nsIMobileMessageDatabaseService> mobileMessageDBService = > do_GetService(MOBILE_MESSAGE_DATABASE_SERVICE_CONTRACTID); > NS_ENSURE_TRUE(mobileMessageDBService, NS_ERROR_FAILURE); > + > + mobileMessageDBService->GetMessageMoz(aId, msgCallback); Please check the return value as well. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +425,5 @@ > aRecord.timestamp, > aRecord.read); > }, > > + createMessageFromRecord: function createMessageFromRecord(aRecord) { Please also remove |createSmsMessageFromRecord|.
Attachment #722621 -
Flags: feedback?(vyang)
Comment 4•12 years ago
|
||
Comment on attachment 722621 [details] [diff] [review] Patch Review of attachment 722621 [details] [diff] [review]: ----------------------------------------------------------------- Not reviewing MobileMessageDatabaseService.js. I can't review the changes in MobileMessageCallback because that file do not exist in the tree and isn't added in this bug. Could you point me to the patch where this is added? ::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl @@ +39,5 @@ > > nsIDOMDOMRequest sendMMS(in jsval parameters /* MMSParameters */); > > [binaryname(GetMessageMoz)] > + nsIDOMDOMRequest getMessage(in long id); I'm not sure I understand why you guys are using DOMRequest for MMS while SmsRequset are still used for SMS. I would have kept SmsRequest or switch everything to DOMRequset... ::: dom/mobilemessage/src/MobileMessageCallback.cpp @@ +35,5 @@ > { > } > > +nsresult > +MobileMessageCallback::NotifySuccess(nsISupports *aMessage) Where is that coming from? This is changing a file that isn't in the tree so it's pretty hard to understand the reason why this is being done. @@ +61,5 @@ > return NS_OK; > } > > +nsresult > +MobileMessageCallback::NotifyError(int32_t aError) ditto ::: dom/mobilemessage/src/MobileMessageManager.cpp @@ +207,3 @@ > { > + nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner()); > + NS_ADDREF(*aRequest = request); Don't use NS_ADDREF but .forget() and do that just before the return. @@ +208,5 @@ > + nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner()); > + NS_ADDREF(*aRequest = request); > + > + nsCOMPtr<nsIMobileMessageCallback> msgCallback = new MobileMessageCallback(request); > + NS_ENSURE_TRUE(msgCallback, NS_ERROR_FAILURE); No need for NS_ENSURE_TRUE. |new| can't fail. @@ +220,1 @@ > return NS_OK; return mobileMessageDBService->GetMessageMoz(aId, msgCallback); ::: dom/mobilemessage/src/SmsRequest.cpp @@ +376,5 @@ > + if (!message) { > + return NS_ERROR_NOT_IMPLEMENTED; > + } > + > + SmsMessage* smsMessage = static_cast<SmsMessage*>(message.get()); I think I understand what you are trying to do but I haven't seen the code that make this needed (likely related to MobileMessageCallback?).
Attachment #722621 -
Flags: feedback?(mounir) → feedback-
Comment 5•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #4) > I'm not sure I understand why you guys are using DOMRequest for MMS while > SmsRequset are still used for SMS. I would have kept SmsRequest or switch > everything to DOMRequset... Because we have to deliver a working MMS DOM by March 8 as possible. For SMS DOMRequest, we have bug 749086 and bug 838467 for DOMCursor.
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #4) > Comment on attachment 722621 [details] [diff] [review] > Patch > > Review of attachment 722621 [details] [diff] [review]: > ----------------------------------------------------------------- > > I can't review the changes in MobileMessageCallback because that file do not > exist in the tree and isn't added in this bug. Could you point me to the > patch where this is added? Sorry for confusting. The bug actually depends on bug 844431, where the part 3-1 and part 3-2 patches implement the new MobileMessageCallback.
Assignee | ||
Comment 7•12 years ago
|
||
Hi Jonas, Need your superreview for the IDL parts. Why I add .type for both nsIDOMSmsMessage and nsIDOMMmsMessage is because .getMessage() can now return either a SMS or MMS message. The content needs to know how to interpret the returned structure by its type. Thanks! Hi Mounir, Please see comment #5 and comment #6 for why we hope to use nsIMobileMessageCallback/nsIDOMMobileMessageManager and reserve the nsISmsRequest/nsIDOMSmsManager at the same time. We have to be careful to not to break the SMS functionalities for this moment until we can really deprecate the nsISmsRequest/nsIDOMSmsManager APIs in the future. I guess letting Vicamo take this review might be appropriate? Thanks! Hi Vicamo, This patch addresses your comment #3 and Mounir's comment #4. Thanks!
Attachment #722621 -
Attachment is obsolete: true
Attachment #723060 -
Flags: superreview?(jonas)
Attachment #723060 -
Flags: review?(vyang)
Comment on attachment 723060 [details] [diff] [review] Patch, V2 Review of attachment 723060 [details] [diff] [review]: ----------------------------------------------------------------- The .idl changes look fine. Yes, having a .type makes it easier for JS to detect what type of message it's dealing with. It's technically possible to do "x instanceof MozMmsMessage", but that is unreliable when you are dealing with multiple globals. It's supposed to get better with some changes happening soon, but I'd prefer to still stick a .type there to keep things simple.
Attachment #723060 -
Flags: superreview?(jonas) → superreview+
Updated•12 years ago
|
Attachment #723060 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #723060 -
Attachment is obsolete: true
Attachment #725327 -
Flags: superreview+
Attachment #725327 -
Flags: review?(vyang)
Assignee | ||
Updated•12 years ago
|
Attachment #725327 -
Flags: feedback?(ctai)
Assignee | ||
Comment 11•12 years ago
|
||
Fix a nit.
Attachment #725327 -
Attachment is obsolete: true
Attachment #725327 -
Flags: review?(vyang)
Attachment #725327 -
Flags: feedback?(ctai)
Attachment #725345 -
Flags: superreview+
Attachment #725345 -
Flags: review?(vyang)
Assignee | ||
Updated•12 years ago
|
Attachment #725345 -
Flags: feedback?(ctai)
Assignee | ||
Comment 12•12 years ago
|
||
Hi Vicamo, Fix another nit. Hi Ctai, Could you please take a look on the following change? Thanks! savableMessage = this.mergeRetrievalConfirmation(retrievedMessage, savableMessage);
Attachment #725345 -
Attachment is obsolete: true
Attachment #725345 -
Flags: review?(vyang)
Attachment #725345 -
Flags: feedback?(ctai)
Attachment #725351 -
Flags: superreview+
Attachment #725351 -
Flags: review?(vyang)
Attachment #725351 -
Flags: feedback?(ctai)
Assignee | ||
Comment 14•12 years ago
|
||
Hi Vicamo, Fix another nit. Please take this review when you're available. Thanks! Hi Ctai, Could you please take a look on the following change? Now we have to use the original savableMessage to include the new info of retrievedMessage, instead of using the returned DB record (now the callback returns a wrapped DOM MMS message). Does that sound reasonable to you? savableMessage = this.mergeRetrievalConfirmation(retrievedMessage, savableMessage);
Attachment #725351 -
Attachment is obsolete: true
Attachment #725351 -
Flags: review?(vyang)
Attachment #725351 -
Flags: feedback?(ctai)
Attachment #725391 -
Flags: superreview+
Attachment #725391 -
Flags: review?(vyang)
Attachment #725391 -
Flags: feedback?(ctai)
Assignee | ||
Comment 15•12 years ago
|
||
Hi Vicamo, Please take this review when you're available. Thanks! Hi Ctai, Could you please take a look on the following change? Now we have to use the original savableMessage to include the new info of retrievedMessage, instead of using the returned DB record (now the callback returns a wrapped DOM MMS message). Does that sound reasonable to you? savableMessage = this.mergeRetrievalConfirmation(retrievedMessage, savableMessage);
Attachment #725391 -
Attachment is obsolete: true
Attachment #725391 -
Flags: review?(vyang)
Attachment #725391 -
Flags: feedback?(ctai)
Attachment #725393 -
Flags: superreview+
Attachment #725393 -
Flags: review?(vyang)
Attachment #725393 -
Flags: feedback?(ctai)
Comment 16•12 years ago
|
||
Comment on attachment 725393 [details] [diff] [review] Patch, V3.2 Review of attachment 725393 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +938,5 @@ > * > * @param intermediate > + * Intermediate MMS message parsed from PDU, carrying retrieval confirmation. > + * @param savable > + * The indexedDB savable message to be merged with the retrieval confirmation. trailing ws ::: dom/mobilemessage/interfaces/nsIMobileMessageCallback.idl @@ +51,1 @@ > void notifyNoMessageInList(); These notify functions are removed in bug 849739 -- MMS getThreads(). However, let's not get blocked by that and having some progress first. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +573,5 @@ > + aMessageRecord.messageClass, > + aMessageRecord.timestamp, > + aMessageRecord.read); > + } else if (aMessageRecord.type == "mms") { > + let headers = aMessageRecord["headers"]; new line @@ +577,5 @@ > + let headers = aMessageRecord["headers"]; > + let subject = headers["subject"]; > + if (subject == undefined) { > + subject = ""; > + } new line @@ +670,5 @@ > let getRequest = aMessageStore.get(firstMessageId); > let self = this; > getRequest.onsuccess = function onsuccess(event) { > + let messageRecord = event.target.result; > + let domMessage = self.createDomMessageFromRecord(messageRecord); Message list are refactored in bug 838467, too. But let's just forget it here and rebase on it if necessary.
Attachment #725393 -
Flags: review?(vyang) → review+
Assignee | ||
Comment 18•12 years ago
|
||
Ready to land. Hi Ctai, Could you please take a look on the following change? Now we have to use the original savableMessage to include the new info of retrievedMessage, instead of using the returned DB record (now the callback returns a wrapped DOM MMS message). Does that sound reasonable to you? savableMessage = this.mergeRetrievalConfirmation(retrievedMessage, savableMessage); Anyway, let's directly land this. If this causes problem, let's further solve it at Bug 849741, which also needs to rebase based on this.
Attachment #725393 -
Attachment is obsolete: true
Attachment #725393 -
Flags: feedback?(ctai)
Attachment #725733 -
Flags: superreview+
Attachment #725733 -
Flags: review+
Attachment #725733 -
Flags: feedback?(ctai)
Assignee | ||
Comment 20•12 years ago
|
||
Ready to land but the inbound is closed right now. :( Hi Ctai, Could you please take a look on the following change? Now we have to use the original savableMessage to include the new info of retrievedMessage, instead of using the returned DB record (now the callback returns a wrapped DOM MMS message). Does that sound reasonable to you? savableMessage = this.mergeRetrievalConfirmation(retrievedMessage, savableMessage); Anyway, let's directly land this. If this causes problem, let's further solve it at Bug 849741, which also needs to rebase based on this.
Attachment #725733 -
Attachment is obsolete: true
Attachment #725733 -
Flags: feedback?(ctai)
Attachment #725738 -
Flags: superreview+
Attachment #725738 -
Flags: review+
Attachment #725738 -
Flags: feedback?(ctai)
Assignee | ||
Comment 21•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/44b0df91e49d
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/44b0df91e49d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Updated•12 years ago
|
Attachment #725738 -
Flags: feedback?(ctai) → feedback+
Assignee | ||
Updated•12 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 24•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/82027f7517b1
status-b2g18:
--- → fixed
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → wontfix
status-firefox22:
--- → fixed
Assignee | ||
Updated•12 years ago
|
Whiteboard: [by 3/8] → [NO_UPLIFT][RIL_INTERFACE]
Assignee | ||
Comment 25•12 years ago
|
||
Added [NO_UPLIFT][RIL_INTERFACE] 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.
Assignee | ||
Comment 26•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.
Assignee | ||
Comment 27•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.
Assignee | ||
Comment 28•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][RIL_INTERFACE] → [RIL_INTERFACE]
Updated•11 years ago
|
Flags: in-moztrap-
You need to log in
before you can comment on or make changes to this bug.
Description
•