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

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ctai, Assigned: ctai)

Tracking

({dev-doc-needed})

unspecified
mozilla23
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 fixed)

Details

Attachments

(2 attachments, 30 obsolete attachments)

18.89 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
6.10 KB, patch
Details | Diff | Splinter Review
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.
Summary: B2G MMS: Use DOM APIs to send and retrieve MM in deferred retrieval → B2G MMS: Use DOM APIs to send and retrieve MMS in deferred retrieval
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.
blocking-b2g: --- → leo?
Depends on: 844431
No longer depends on: b2g-mms-dom-api
Summary: B2G MMS: Use DOM APIs to send and retrieve MMS in deferred retrieval → B2G MMS: provide nsIDOMMobileMessageManager.retrieveMMS() to retrieve MMS for the deferred retrieval mode
Directly assign this to you, whcih you might be interested in.
Assignee: nobody → ctai
Can't block without more information. It's not clear what MMS functionality requires this, or which user story it's in support of.
blocking-b2g: leo? → -
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.
Blocks: 840076
blocking-b2g: - → leo?
Cancel re-nomination because bug 840076 is marked as leo-.
blocking-b2g: leo? → ---
Whiteboard: [target 3/22]
Posted patch RetrieveMMS API v1.1 (obsolete) — Splinter Review
Attachment #726513 - Attachment is obsolete: true
Attachment #726516 - Flags: review?(vyang)
Attachment #726516 - Flags: review?(vyang) → feedback?(vyang)
Attachment #726516 - Flags: feedback?(gene.lian)
Posted patch RetrieveMMS API v1.2 (obsolete) — Splinter Review
Attachment #726516 - Attachment is obsolete: true
Attachment #726516 - Flags: feedback?(vyang)
Attachment #726516 - Flags: feedback?(gene.lian)
Posted patch RetrieveMMS API v1.2 (obsolete) — Splinter Review
Attachment #726537 - Attachment is obsolete: true
Posted patch RetrieveMMS API v1.2 (obsolete) — Splinter Review
Attachment #726538 - Attachment is obsolete: true
Attachment #726540 - Flags: feedback?(gene.lian)
Attachment #726540 - Flags: feedback?(vyang)
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.
Attachment #726540 - Flags: feedback?(gene.lian) → feedback-
Posted patch RetrieveMMS API v1.3 (obsolete) — Splinter Review
Attachment #726540 - Attachment is obsolete: true
Attachment #726540 - Flags: feedback?(vyang)
Posted patch RetrieveMMS API v1.4 (obsolete) — Splinter Review
Attachment #727027 - Attachment is obsolete: true
Attachment #727030 - Flags: feedback?(gene.lian)
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. :)
Attachment #727030 - Flags: feedback?(gene.lian) → feedback+
Posted patch RetrieveMMS API v1.5 (obsolete) — Splinter Review
Attachment #727030 - Attachment is obsolete: true
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
Attachment #727072 - Flags: review+
Posted patch RetrieveMMS API v1.6 (obsolete) — Splinter Review
Attachment #727072 - Attachment is obsolete: true
Attachment #727087 - Flags: review?(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.
Attachment #727087 - Flags: review?(vyang) → review-
(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.
blocking-b2g: --- → leo?
Sorry yet again, didn't see my previous comment #5.
blocking-b2g: leo? → ---
Posted patch RetrieveMMS idl v1.1 (obsolete) — Splinter Review
Attachment #727574 - Attachment is obsolete: true
Posted patch RetrieveMMS API v1.7 (obsolete) — Splinter Review
Attachment #727087 - Attachment is obsolete: true
Comment on attachment 727593 [details] [diff] [review]
RetrieveMMS idl v1.1

Need superreview due to the change of nsIDOMMobileMessageManager.idl.
Attachment #727593 - Flags: superreview?(jonas)
Posted patch RetrieveMMS API v1.8 (obsolete) — Splinter Review
Attachment #727600 - Attachment is obsolete: true
Posted patch RetrieveMMS API v1.9 (obsolete) — Splinter Review
Attachment #728055 - Attachment is obsolete: true
Attachment #728057 - Flags: review?(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);
  ...
});
Attachment #728057 - Flags: review?(vyang)
Posted patch RetrieveMMS API v1.10 (obsolete) — Splinter Review
Attachment #728057 - Attachment is obsolete: true
Posted patch RetrieveMMS API v1.11 (obsolete) — Splinter Review
Attachment #728142 - Attachment is obsolete: true
Posted patch RetrieveMMS API v1.12 (obsolete) — Splinter Review
Attachment #728148 - Attachment is obsolete: true
Posted patch RetrieveMMS API v1.13 (obsolete) — Splinter Review
Attachment #728149 - Attachment is obsolete: true
Posted patch RetrieveMMS API v1.14 (obsolete) — Splinter Review
Attachment #728153 - Attachment is obsolete: true
Posted patch RetrieveMMS API v1.15 (obsolete) — Splinter Review
Attachment #728155 - Attachment is obsolete: true
Attachment #728171 - Flags: review?(vyang)
Please see bug 850680 to call .broadcastMmsSystemMessage() just before notifying observers an MMS is received.
Depends on: 850680
Posted patch RetrieveMMS API v1.16 (obsolete) — Splinter Review
Attachment #728171 - Attachment is obsolete: true
Attachment #728171 - Flags: review?(vyang)
Posted patch RetrieveMMS idl v1.2 (obsolete) — Splinter Review
Attachment #727593 - Attachment is obsolete: true
Attachment #727593 - Flags: superreview?(jonas)
Posted patch RetrieveMMS API v1.17 (obsolete) — Splinter Review
Attachment #728834 - Attachment is obsolete: true
Attachment #728835 - Flags: superreview?(jonas)
Attachment #728836 - Flags: review?(vyang)
(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().
Posted patch RetrieveMMS API v1.18 (obsolete) — Splinter Review
Attachment #728836 - Attachment is obsolete: true
Attachment #728836 - Flags: review?(vyang)
Attachment #728850 - Flags: review?(vyang)
Posted patch RetrieveMMS API v1.19 (obsolete) — Splinter Review
Attachment #728850 - Attachment is obsolete: true
Attachment #728850 - Flags: review?(vyang)
Posted patch 728850: RetrieveMMS API v1.20 (obsolete) — Splinter Review
Attachment #728857 - Attachment is obsolete: true
Attachment #728858 - Flags: review?(vyang)
Posted patch RetrieveMMS API v1.21 (obsolete) — Splinter Review
Attachment #728858 - Attachment is obsolete: true
Attachment #728858 - Flags: review?(vyang)
Attachment #728899 - Flags: review?(vyang)
Posted patch RetrieveMMS API v1.22 (obsolete) — Splinter Review
Rebased upon bug 854422
Attachment #728899 - Attachment is obsolete: true
Attachment #728899 - Flags: review?(vyang)
Attachment #729446 - Flags: review?(vyang)
Posted patch RetrieveMMS API v1.23 (obsolete) — Splinter Review
Attachment #729446 - Attachment is obsolete: true
Attachment #729446 - Flags: review?(vyang)
Attachment #729447 - Flags: review?(vyang)
Depends on: 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.
Attachment #729447 - Flags: review?(vyang)
Move the aCallback.notify to right postion.
Attachment #729447 - Attachment is obsolete: true
Attachment #729461 - Flags: review?(vyang)
leo+ as this is a part of MMS. No_UPLIFT for now before the whole MMS is completed
blocking-b2g: --- → leo+
Whiteboard: [target 3/22] → [target 3/22] NO_UPLIFT
Attachment #728835 - Flags: superreview?(jonas) → superreview+
Whiteboard: [target 3/22] NO_UPLIFT → [NO_UPLIFT]
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) {
    ...
  });
Attachment #729461 - Flags: review?(vyang) → review+
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) {
> >     ...
> >   });
No longer blocks: 855605
Attachment #728835 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c390a34a1663
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 728835 [details] [diff] [review]
RetrieveMMS idl v1.2

Wrong obsolete.
Attachment #728835 - Attachment is obsolete: false
Reopening because the part 1 of this bug haven't been checked in.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #729461 - Attachment description: 729447: RetrieveMMS API v1.24 → Part2:RetrieveMMS API v1.24
Posted patch Part1: RetrieveMMS idl v1.3 (obsolete) — Splinter Review
Rebased
Attachment #728835 - Attachment is obsolete: true
Fix build fail.
Attachment #735030 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/aee727a3f109
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.