843445 - B2G MMS: provide nsIDOMMobileMessageManager.retrieveMMS() to retrieve MMS for the deferred retrieval mode

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

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.

Comment 1

[Gene Lian (I already quit Mozilla)]

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.

Comment 2

[Gene Lian (I already quit Mozilla)]

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

Comment 3

[Dietrich Ayala (:dietrich)]

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

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.0

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.1

Comment 8

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.2

Comment 9

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.2

Comment 10

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.2

Comment 11

[Gene Lian (I already quit Mozilla)]

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 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.3

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.4

Comment 14

[Gene Lian (I already quit Mozilla)]

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 15

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.5

Comment 16

[Gene Lian (I already quit Mozilla)]

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 17

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.6

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

(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.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 21

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS idl v1.0

Comment 22

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS idl v1.1

Comment 23

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.7

Comment 24

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Comment on attachment 727593 [details] [diff] [review]
RetrieveMMS idl v1.1

Need superreview due to the change of nsIDOMMobileMessageManager.idl.

Comment 25

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.8

Comment 26

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.9

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

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);
  ...
});

Comment 28

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.10

Comment 29

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.11

Comment 30

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.12

Comment 31

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.13

Comment 32

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.14

Comment 33

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.15

Comment 34

[Gene Lian (I already quit Mozilla)]

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

Comment 35

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.16

Comment 36

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS idl v1.2

Comment 37

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.17

Comment 38

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

(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().

Comment 39

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.18

Comment 40

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.19

Comment 41

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: 728850: RetrieveMMS API v1.20

Comment 42

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.21

Comment 43

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.22

Rebased upon bug 854422

Comment 44

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: RetrieveMMS API v1.23

Comment 45

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 46

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part2:RetrieveMMS API v1.24

Move the aCallback.notify to right postion.

Comment 47

[Joe Cheng [:jcheng] (please needinfo)]

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

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

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) {
    ...
  });

Comment 49

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

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) {
>     ...
>   });

Comment 50

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

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) {
> >     ...
> >   });

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c390a34a1663

Should this have tests?

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c390a34a1663

Comment 53

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

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

Wrong obsolete.

Comment 54

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

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

Comment 55

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part1: RetrieveMMS idl v1.3

Rebased

Comment 56

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Part 1 patch.

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

Comment 57

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 58

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part1: RetrieveMMS idl v1.4

Fix build fail.

Comment 59

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

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

Comment 60

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/aee727a3f109

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aee727a3f109

Comment 62

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 63

[Vicamo Yang [:vicamo][:vyang]]

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