In deferred retrieval mode, we need to use MMS DOM API to notify the user when the message delivered. If user wants to download the MM, system will trigger the download through MMS DOM API.

After bug 844431 is done, we can have a |nsIDOMMobileMessageManager| interface to add: DOMRequest retrieveMMS(in long id); which is a MMS-specific function to retrieve MMS message for the deferred retrieval mode. Btw, the original API name defined in bug 760065 is |retrieveMMSMessage| but I'd prefer simply using |retrieveMMS| because we used |sendMMS| for sending MMS.

Directly assign this to you, whcih you might be interested in.

Can't block without more information. It's not clear what MMS functionality requires this, or which user story it's in support of.

Blocks user story bug 840076 and Re-nominate for leo?. In bug 840076, we're providing options for retrieval modes. However, I can't find corresponding definitions to these modes. For example, the MANUAL retrieval mode requires user reaction to a not-downloaded MMS message and therefore needs an DOM API for this.

Cancel re-nomination because bug 840076 is marked as leo-.

Comment on attachment 726540 [details] [diff] [review] RetrieveMMS API v1.2 Review of attachment 726540 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1249,5 @@ > }); > }, > > + retrieve: function retrieve(id, aRequest) { > + let failCallback = function failCallback() { s/failCallback/retrieveFailedCb and below. @@ +1250,5 @@ > }, > > + retrieve: function retrieve(id, aRequest) { > + let failCallback = function failCallback() { > + aRequest.notifySendMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR); I think we can directly call .notifyGetMessageFailed() since we're not sending an MMS; retrieving an MMS instead. Or even a new one like .notifyRetrieveMmsFailed(). Need to discuss with Vicamo. @@ +1251,5 @@ > > + retrieve: function retrieve(id, aRequest) { > + let failCallback = function failCallback() { > + aRequest.notifySendMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR); > + Services.obs.notifyObservers(mmsMessage, kMmsFailedObserverTopic, null); You will pass in mmsMessage for this function. Right? But it's indeed hard to get one within MmsService. Btw, we should use a new topic like: kMmsRetrieveFailedObserverTopic = "mms-retrieved-failed" s/mmsMessage/domMessage @@ +1253,5 @@ > + let failCallback = function failCallback() { > + aRequest.notifySendMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR); > + Services.obs.notifyObservers(mmsMessage, kMmsFailedObserverTopic, null); > + } > + gMobileMessageDatabaseService.getMessageRecordById(id, (function (getRecordRv, messageRecord) { s/function(...)/function notifyResult(...) s/getRecordRv/aRv s/messageRecord/aMessageRecord Hmm... may be we need to have the third parameter like |domMessage| for the callback so that you can pass |domMessage| into the retrieveFailedCb(). Not sure. @@ +1259,5 @@ > + debug("Function getMessageRecordById() return error."); > + failCallback(); > + return; > + } > + if ((!messageRecord.headers) || (!messageRecord.headers["x-mms-content-location"])) { Don't need the parentheses for these two conditions. @@ +1272,5 @@ > + let wish = messageRecord.headers["x-mms-delivery-report"]; > + this.retrieveMessage(url, (function responseNotify(mmsStatus, retrievedMsg) { > + // `The absence of the field does not indicate any default > + // value.` So we go checking the same field in retrieved > + // message instead. s/'...'/... @@ +1273,5 @@ > + this.retrieveMessage(url, (function responseNotify(mmsStatus, retrievedMsg) { > + // `The absence of the field does not indicate any default > + // value.` So we go checking the same field in retrieved > + // message instead. > + if ((wish == null) && retrievedMsg) { Don't need the parentheses for the first condition. @@ +1277,5 @@ > + if ((wish == null) && retrievedMsg) { > + wish = retrievedMsg.headers["x-mms-delivery-report"]; > + } > + let reportAllowed = this.getReportAllowed( > + this.confSendDeliveryReport, wish); let reportAllowed = this.getReportAllowed(this.confSendDeliveryReport, wish); If too long, use let reportAllowed = this.getReportAllowed(this.confSendDeliveryReport, wish); or let reportAllowed = this.getReportAllowed(this.confSendDeliveryReport, wish); or let reportAllowed = this.getReportAllowed(this.confSendDeliveryReport, wish); The line of function name needs to be followed with a parameter at least. @@ +1292,5 @@ > + debug("Could not store MMS " + domMessage.id + > + ", error code " + rv); > + failCallback(); > + return; > + } Maybe we need to call aRequest.notifyMessageGot(domMessage) here or a new one like aRequest.notifyMmsRetrieved(domMessage). To discuss with Vicamo. @@ +1293,5 @@ > + ", error code " + rv); > + failCallback(); > + return; > + } > + // Notifing new retrieved MMS message through notifyObservers. // Notifying observers a new MMS message is retrieved. @@ +1294,5 @@ > + failCallback(); > + return; > + } > + // Notifing new retrieved MMS message through notifyObservers. > + Services.obs.notifyObservers(domMessage, kMmsReceivedObserverTopic, null); May be we should use a new topic like: kMmsRetrievedObserverTopic = "mms-retrieved" if we have a pair above: kMmsRetrievedFailedObserverTopic. Not sure. To discuss with Vicamo. @@ +1297,5 @@ > + // Notifing new retrieved MMS message through notifyObservers. > + Services.obs.notifyObservers(domMessage, kMmsReceivedObserverTopic, null); > + let transaction = new AcknowledgeTransaction(transactionId, wish); > + transaction.run(); > + }).bind(this) I don't think you need this bind here. ::: dom/mobilemessage/interfaces/nsIDOMMobileMessageManager.idl @@ +43,5 @@ > nsIDOMDOMRequest markMessageRead(in long id, in boolean aValue); > > nsIDOMMozSmsRequest getThreadList(); > > + nsIDOMDOMRequest retrieveMMS(in long id); What is the returned result for the DOMRequest here? I'll decide how we implement the IPC callbacks in the MmsService.js. Need to discuss with Vicamo. ::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl @@ +20,5 @@ > +{ > + /** > + * |aRecord| Object: the mobile-message database record > + */ > + void notify(in nsresult aRv, in jsval aRecord); s/aRecord/aMessageRecord @@ +62,5 @@ > [optional] in nsIRilMobileMessageDatabaseCallback aCallback); > + > + /** > + * |aMessageId| Number: the message's DB record ID. > + * |aCallback| nsIRilMobileMessageDatabaseCallback: a callback which take s/take/takes ::: dom/mobilemessage/src/MobileMessageManager.cpp @@ +332,5 @@ > + nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner()); > + nsCOMPtr<nsIMobileMessageCallback> msgCallback = new MobileMessageCallback(request); > + > + request.forget(aRequest); > + return mmsService->Retrieve(id, msgCallback); To avoid memory leak when failing to do .Retrieve(). We need to early return before .forget(). Better to write: nsresult rv = mobileMessageDBService->Retrieve(aId, msgCallback); NS_ENSURE_SUCCESS(rv, rv); request.forget(aRequest); return NS_OK; ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1337,5 @@ > }; > }); > }, > > + getMessageRecordById: function getMessageRecordById(aMessageId, callback) { s/callback/aCallback @@ +1344,5 @@ > + if (!callback) { > + return; > + } > + callback.notify(rv, messageRecord); > + } The following codes can be shared with .getMessage(). Let's find a way to refactor the common part.

Comment on attachment 727030 [details] [diff] [review] RetrieveMMS API v1.4 Review of attachment 727030 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mms/src/ril/MmsService.js @@ +1257,5 @@ > }); > }, > > + retrieve: function retrieve(id, aRequest) { > + gMobileMessageDatabaseService.getMessageRecordById(id, (function notifyResult(aRv, aMessageRecord) { Per off-line discussion: gMobileMessageDatabaseService .getMessageRecordById(id, function notifyResult(aRv, aMessageRecord) { // codes... }.bind(this)); Don't need to add a parentheses for a function when using .bind(this). @@ +1274,5 @@ > + let url = aMessageRecord.headers["x-mms-content-location"].uri; > + let transactionId = aMessageRecord.headers["x-mms-transaction-id"]; > + // For X-Mms-Report-Allowed > + let wish = aMessageRecord.headers["x-mms-delivery-report"]; > + this.retrieveMessage(url, (function responseNotify(mmsStatus, retrievedMsg) { No need to wrap the function with a parentheses for .bind(this). ::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl @@ +63,5 @@ > + > + /** > + * |aMessageId| Number: the message's DB record ID. > + * |aCallback| nsIRilMobileMessageDatabaseCallback: a callback which takes > + * result flag and message record as parameters. nit: this line should two spaces right from the start of |aCallback|. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1337,5 @@ > }; > }); > }, > > + getMessageRecordById: function getMessageRecordById(aMessageId, aCallback) { Just like your suggestion, let .getMessage() call your new function and wrap the DB record to a DOM message in it's callback. Be careful to check this refactor, because it touches SMS functions. :)

Comment on attachment 727072 [details] [diff] [review] RetrieveMMS API v1.5 Review of attachment 727072 [details] [diff] [review]: ----------------------------------------------------------------- r=gene but we still need Vicamo's final review. ::: dom/mms/src/ril/MmsService.js @@ +1258,5 @@ > }, > > + retrieve: function retrieve(id, aRequest) { > + gMobileMessageDatabaseService > + .getMessageRecordById(id, function notifyResult(aRv, aMessageRecord) { Check aMessageRecord.type here. If not MMS, then aRequest.notifyGetMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR); @@ +1259,5 @@ > > + retrieve: function retrieve(id, aRequest) { > + gMobileMessageDatabaseService > + .getMessageRecordById(id, function notifyResult(aRv, aMessageRecord) { > + if (Cr.nsIMobileMessageCallback.SUCCESS_NO_ERROR != aRv) { Align the if and below to .getMessageRecordById(...) ::: dom/mobilemessage/src/MobileMessageManager.cpp @@ +332,5 @@ > + nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner()); > + nsCOMPtr<nsIMobileMessageCallback> msgCallback = new MobileMessageCallback(request); > + > + nsresult rv = mmsService->Retrieve(id, msgCallback); > + NS_ENSURE_SUCCESS(rv, rv); Add a new line here. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +558,5 @@ > }); > }; > }, > > + retrieveAndCheckMessageRecord: function retrieveAndCheckMessageRecord(aMessageId, aSucessCb, aFailCb) { I'd prefer: _getMessageRecordById: function _getMessageRecordById(aMessageId, aSuccessCb, aErrorCb) _function usually means a local private function. @@ +1383,5 @@ > }, > > + getMessageRecordById: function getMessageRecordById(aMessageId, aCallback) { > + if (DEBUG) debug("Retrieving message with ID " + aMessageId); > + this.retrieveAndCheckMessageRecord(aMessageId, s/retrieveAndCheckMessageRecord/_getMessageRecordById @@ +1384,5 @@ > > + getMessageRecordById: function getMessageRecordById(aMessageId, aCallback) { > + if (DEBUG) debug("Retrieving message with ID " + aMessageId); > + this.retrieveAndCheckMessageRecord(aMessageId, > + function sucessCb(aMessageRecord) { s/sucessCb/successCb @@ +1389,5 @@ > + if (!aCallback) { > + return; > + } > + aCallback.notify(Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR, aMessageRecord); > + }, function failCb(aRv) { Make this a new line aligned with function successCb(...). s/failCb/errorCb @@ +1445,5 @@ > > getMessage: function getMessage(messageId, aRequest) { > if (DEBUG) debug("Retrieving message with ID " + messageId); > let self = this; > + this.retrieveAndCheckMessageRecord(aMessageId, s/retrieveAndCheckMessageRecord/_getMessageRecordById @@ +1446,5 @@ > getMessage: function getMessage(messageId, aRequest) { > if (DEBUG) debug("Retrieving message with ID " + messageId); > let self = this; > + this.retrieveAndCheckMessageRecord(aMessageId, > + function sucessCb(aMessageRecord) { s/sucessCb/successCb @@ +1449,5 @@ > + this.retrieveAndCheckMessageRecord(aMessageId, > + function sucessCb(aMessageRecord) { > + let domMessage = self.createDomMessageFromRecord(aMessageRecord); > + aRequest.notifyMessageGot(domMessage); > + }, function failCb(aRv) { Make this a new line aligned with function successCb(...). s/failCb/errorCb

Comment on attachment 727087 [details] [diff] [review] RetrieveMMS API v1.6 Review of attachment 727087 [details] [diff] [review]: ----------------------------------------------------------------- r- for transaction id and _getMessageRecordById. Besides, this patch fails builds in platforms other than B2G because you don't have corresponding fixes to fallback/, android/ and ipc/. ::: dom/mms/src/ril/MmsService.js @@ +1022,5 @@ > > let savableMessage = this.convertIntermediateToSavable(notification); > > gMobileMessageDatabaseService.saveReceivedMessage(savableMessage, > + function (rv, domMessage) { Actually I prefer keeping the parentheses. That reminds the reader this function might be either binded with additional parameters, or invoked inlinely in the end. @@ +1264,5 @@ > + debug("Function getMessageRecordById() return error."); > + aRequest.notifyGetMessageFailed(aRv); > + return; > + } > + if("mms" != aMessageRecord.type) { Please always remember to add an ws between keywords and left parenthesis. @@ +1277,5 @@ > + return; > + } > + > + let url = aMessageRecord.headers["x-mms-content-location"].uri; > + let transactionId = aMessageRecord.headers["x-mms-transaction-id"]; Please see MMS-ENC, Table 5 in page 25 about "X-MmsTransaction-ID". The transaction id to filled in M-Acknowledge.ind PDU is from the X-MmsTransaction-ID field of M-ReTrieve.conf, not M-Notification.ind. The one you get here is from the latter. ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +558,5 @@ > }); > }; > }, > > + _getMessageRecordById: function _getMessageRecordById(aMessageId, aSuccessCb, aErrorCb) { Please have one callback only. Then you don't need an additional _getMessageRecordById, just implement the body in getMessageRecordById directly.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18) > Besides, this patch fails builds in platforms other than B2G because you don't have > corresponding fixes to fallback/, android/ and ipc/. Sorry, that's not true because you add new methods in nsIRilMobileMessageDatabaseService, not nsIMobileMessageDatabaseService.

Sorry yet again, didn't see my previous comment #5.

Comment on attachment 727593 [details] [diff] [review] RetrieveMMS idl v1.1 Need superreview due to the change of nsIDOMMobileMessageManager.idl.

Comment on attachment 728057 [details] [diff] [review] RetrieveMMS API v1.9 Review of attachment 728057 [details] [diff] [review]: ----------------------------------------------------------------- Please keep parentheses as mentioned in comment 18. ::: dom/mms/src/ril/MmsService.js @@ +1022,5 @@ > > let savableMessage = this.convertIntermediateToSavable(notification); > > gMobileMessageDatabaseService.saveReceivedMessage(savableMessage, > + function (rv, domMessage) { Please see my review comment 18. @@ +1258,5 @@ > }, > > + retrieve: function retrieve(id, aRequest) { > + gMobileMessageDatabaseService > + .getMessageRecordById(id, function notifyResult(aRv, aMessageRecord) { gMobileMessageDatabaseService.getMessageRecordById(id, function notifyResult(aRv, aMessageRecord) { if (...) @@ +1261,5 @@ > + gMobileMessageDatabaseService > + .getMessageRecordById(id, function notifyResult(aRv, aMessageRecord) { > + if (Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR != aRv) { > + debug("Function getMessageRecordById() return error."); > + aRequest.notifyGetMessageFailed(aRv); You're actually abusing the interface from current API's point of view. It's not something seriously bad, I think. But, please file another bug and let's merge notify{Get,Send,MarkRead}MessageFailed, notifyMessage{Sent,Got}, etc. @@ +1287,5 @@ > + // PDU during deferred retrieval. This transaction ID is used by the MMS > + // Client and MMS Proxy-Relay to provide linkage between the originated > + // M-Retrieve.conf and the response M-Acknowledge.ind PDUs. > + let transactionId = retrievedMsg.headers["x-mms-transaction-id"]; > + // The absence of the field does not indicate any default New line please. @@ +1300,5 @@ > + debug("retrievedMsg = " + JSON.stringify(retrievedMsg)); > + aMessageRecord = this.mergeRetrievalConfirmation(retrievedMsg, aMessageRecord); > + gMobileMessageDatabaseService.saveReceivedMessage(aMessageRecord, > + function (rv, domMessage) { > + let success = Components.isSuccessCode(rv); gMobileMessageDatabaseService.saveReceivedMessage(aMessageRecord, function (rv, domMessage) { let success = Components.isSuccessCode(rv); ... });

Please see bug 850680 to call .broadcastMmsSystemMessage() just before notifying observers an MMS is received.

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #37) > Created attachment 728836 [details] [diff] [review] > RetrieveMMS API v1.17 Rebased version after Bug 850680 and also add calling .broadcastMmsSystemMessage().

Rebased upon bug 854422

Comment on attachment 729447 [details] [diff] [review] RetrieveMMS API v1.23 Review of attachment 729447 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1378,5 @@ > + txn.onerror = function onerror(event) { > + if (DEBUG) { > + if (event.target) { > + debug("Caught error on transaction", event.target.errorCode); > + aCallback.notify(Ci.nsIMobileMessageCallback.INTERNAL_ERROR, null); This just can't be correct. The if-block is bounded by DEBUG.

Move the aCallback.notify to right postion.

leo+ as this is a part of MMS. No_UPLIFT for now before the whole MMS is completed

Comment on attachment 729461 [details] [diff] [review] Part2:RetrieveMMS API v1.24 Review of attachment 729461 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js @@ +1402,5 @@ > } > + aRequest.notifyGetMessageFailed(aRv, null); > + } > + }; > + this.getMessageRecordById(aMessageId, notifyCallback); You can simple have: this.getMessageRecordById(aMessageId, function (aRv, aMessageRecord) { ... });

I think what you want is below: this.getMessageRecordById(aMessageId, { notify: function notify(aRv, aMessageRecord) { ... } }); (In reply to Vicamo Yang [:vicamo][:vyang] from comment #48) > Comment on attachment 729461 [details] [diff] [review] > 729447: RetrieveMMS API v1.24 > > Review of attachment 729461 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js > @@ +1402,5 @@ > > } > > + aRequest.notifyGetMessageFailed(aRv, null); > > + } > > + }; > > + this.getMessageRecordById(aMessageId, notifyCallback); > > You can simple have: > > this.getMessageRecordById(aMessageId, function (aRv, aMessageRecord) { > ... > });

The function "getMessageRecordById" used in getMessage() is not wrapped by idl. So we can't simplified as below usage: this.getMessageRecordById(aMessageId, function (aRv, aMessageRecord) { ... }); (In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #49) > I think what you want is below: > > this.getMessageRecordById(aMessageId, { > notify: function notify(aRv, aMessageRecord) { > ... > } > }); > > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #48) > > Comment on attachment 729461 [details] [diff] [review] > > 729447: RetrieveMMS API v1.24 > > > > Review of attachment 729461 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js > > @@ +1402,5 @@ > > > } > > > + aRequest.notifyGetMessageFailed(aRv, null); > > > + } > > > + }; > > > + this.getMessageRecordById(aMessageId, notifyCallback); > > > > You can simple have: > > > > this.getMessageRecordById(aMessageId, function (aRv, aMessageRecord) { > > ... > > });

https://hg.mozilla.org/integration/mozilla-inbound/rev/c390a34a1663 Should this have tests?

Comment on attachment 728835 [details] [diff] [review] RetrieveMMS idl v1.2 Wrong obsolete.

Reopening because the part 1 of this bug haven't been checked in.

Rebased

Part 1 patch. https://hg.mozilla.org/integration/mozilla-inbound/rev/a54f620cf84c

Build failures Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/488db939e3e1

Fix build fail.

Try server result: https://tbpl.mozilla.org/?tree=Try&rev=b2dba3673d5e

Uplift WIP: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/838467/b2g18 , verifying.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/51c4987b8206 https://hg.mozilla.org/releases/mozilla-b2g18/rev/3847fb528e55