B2G MMS: don't download message twice on receiving duplicated notification

RESOLVED FIXED in Firefox 23

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: vicamo, Assigned: ctai)

Tracking

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

Firefox Tracking Flags

(blocking-b2g:leo+, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

Attachments

(1 attachment, 16 obsolete attachments)

25.02 KB, patch
Details | Diff | Splinter Review
As suggested in the summary, we should check whether there are already a message retrieved/retrieving with the same transaction id before starting the retrieval. See bug 810067 as well.
Reporter

Updated

7 years ago
Depends on: message-database
Reporter

Updated

7 years ago
No longer blocks: 804679
Assignee: nobody → ctai
Posted patch Patch v1.0 (obsolete) — Splinter Review
Posted patch Patch v1.1 (obsolete) — Splinter Review
Attachment #728174 - Attachment is obsolete: true
Posted patch Patch v1.2 (obsolete) — Splinter Review
Attachment #728855 - Attachment is obsolete: true
Posted patch Patch v1.3 (obsolete) — Splinter Review
Attachment #728860 - Attachment is obsolete: true
Attachment #728892 - Flags: feedback?(vyang)
Attachment #728892 - Flags: feedback?(gene.lian)
Comment on attachment 728892 [details] [diff] [review]
Patch v1.3

Review of attachment 728892 [details] [diff] [review]:
-----------------------------------------------------------------

Need to review again when the following fix is included.

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1267,5 @@
>      if (aMessage.type == "sms") {
>        addresses = [aMessage.receiver];
>      } else if (aMessage.type == "mms") {
>        addresses = aMessage.receivers;
> +      aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"];

This is not quite correct. We didn't prepare ["x-mms-transaction-id"] yet when calling .saveSendingMessage(). We should do something like below in the MmsService.js:

let savableMessage = this.createSavableFromParams(aParams);
let sendTransaction;
try {
  sendTransaction = new SendTransaction(savableMessage);
} catch (e) {
  aRequest.notifySendMmsMessageFailed(...);
  return;
}
gMobileMessageDatabaseService
  .saveSendingMessage(sendTransaction.msg, ...);
Attachment #728892 - Flags: feedback?(gene.lian) → feedback-
Posted patch Patch v1.4 (obsolete) — Splinter Review
Attachment #728892 - Attachment is obsolete: true
Attachment #728892 - Flags: feedback?(vyang)
Posted patch Patch v1.5 (obsolete) — Splinter Review
Attachment #729360 - Attachment is obsolete: true
Posted patch Patch v1.6 (obsolete) — Splinter Review
Attachment #729362 - Attachment is obsolete: true
Attachment #729363 - Flags: feedback?(gene.lian)
Comment on attachment 729363 [details] [diff] [review]
Patch v1.6

Review of attachment 729363 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mms/src/ril/MmsService.js
@@ +1040,5 @@
>     *
>     * @param notification
>     *        The parsed MMS message object.
>     */
>    handleNotificationIndication: function handleNotificationIndication(notification) {

Sorry. Bug 854422 lands first. You have to rebase again. ;P

@@ +1049,5 @@
> +        (function (aRv, aMessageRecord) {
> +      if (Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR === aRv
> +          && aMessageRecord) {
> +        debug("We already get the NotificationIndication = "
> +              + transactionId + " before.");

debug("We already got the NotificationIndication with transactionId = "

@@ +1301,5 @@
>          Services.obs.notifyObservers(aDomMessage, kMmsSentObserverTopic, null);
>        });
>      };
>  
>      let savableMessage = this.createSavableFromParams(aParams);

s/createSavableFromParams/createSendableFromParams
s/savableMessage/sendableMessage

and move the following lines

message["type"] = "mms";
message["deliveryStatusRequested"] = true;
message["timestamp"] = Date.now();
message["receivers"] = receivers;

to another new function .ceateSavableFromSendable(aSendable).

@@ +1304,5 @@
>  
>      let savableMessage = this.createSavableFromParams(aParams);
> +    let sendTransaction;
> +    try {
> +      sendTransaction = new SendTransaction(savableMessage);

s/savableMessage/sendableMessage

@@ +1307,5 @@
> +    try {
> +      sendTransaction = new SendTransaction(savableMessage);
> +    } catch (e) {
> +      debug("Exception: fail to create a SendTransaction instance.");
> +      sendTransactionCb(aDomMessage.id, false);

We cannot call this because .sendTransactionCb() calls .setMessageDelivery() internally, which cannot find a saved record. I think we should call directly call:

aRequest.notifySendMmsMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR);

@@ +1309,5 @@
> +    } catch (e) {
> +      debug("Exception: fail to create a SendTransaction instance.");
> +      sendTransactionCb(aDomMessage.id, false);
> +      return;
> +    }

Call:

let savableMessage = this.ceateSavableFromSendable(sendTransaction.msg);

or

let savableMessage = this.ceateSavableFromSendable(sendableMessage);

@@ +1310,5 @@
> +      debug("Exception: fail to create a SendTransaction instance.");
> +      sendTransactionCb(aDomMessage.id, false);
> +      return;
> +    }
> +    gMobileMessageDatabaseService.saveSendingMessage(sendTransaction.msg,

s/sendTransaction.msg/savableMessage

::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
@@ +83,5 @@
>    void getMessageRecordById(in long aMessageId,
>                              in nsIRilMobileMessageDatabaseRecordCallback aCallback);
> +
> +  /**
> +   * |aTransactionId| DOMString: the transaction id of mms pdu.

s/mms/MMS

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1176,5 @@
>        // TODO Bug 853384 - for some SIMs we cannot retrieve the vaild
>        // phone number, thus setting the SMS' receiver to be null.
>        aMessage.receiver = self;
>      } else if (aMessage.type == "mms") {
> +      aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"];

I'd prefer adding the following assignment in the .convertIntermediateToSavable(...):

intermediate.transactionId = intermediate.headers["x-mms-transaction-id"];

And add a sanity check to ensure the existence of |aMessage.transactionId| at the head of .saveReceivedMessage(). Also, update the comment in the .idl.

Also, I'd prefer moving this assignment to further below:

aMessage.deliveryIndex = [DELIVERY_RECEIVED, timestamp];
aMessage.readIndex = [FILTER_READ_UNREAD, timestamp];
if (aMessage.type == "mms") {
  aMessage.transactionIdIndex = aMessage.transactionId;
}

where deal with all the index assignment stuff.

@@ +1267,5 @@
>      if (aMessage.type == "sms") {
>        addresses = [aMessage.receiver];
>      } else if (aMessage.type == "mms") {
>        addresses = aMessage.receivers;
> +      aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"];

I'd prefer adding the following assignment in the .ceateSavableFromSendable(aSavable):

message["transactionId"] = aSavable.headers["x-mms-transaction-id"];

Add a sanity check for |aMessage.transactionId| at the head of .saveSendingmessage(). Also, update the comment in the .idl.

Also move this assignment to:

// Adding needed indexes and extra attributes for internal use.
// threadIdIndex & participantIdsIndex are filled in saveRecord().
...
if (aMessage.type == "mms") {
  aMessage.transactionIdIndex = aMessage.transactionId;
}

@@ +1374,5 @@
>          messageStore.put(messageRecord);
>        };
>      });
>    },
> +  getMessageRecordByTransactionId: function getMessageRecordByTransactionId(aTransactionId, aCallback) {

New line above this function.
Attachment #729363 - Flags: feedback?(gene.lian)
Posted patch Patch v1.7 (obsolete) — Splinter Review
Attachment #729363 - Attachment is obsolete: true
Posted patch Patch v1.8 (obsolete) — Splinter Review
Revised accroding to Gene's comment.
Attachment #730028 - Attachment is obsolete: true
Attachment #730034 - Flags: review?(gene.lian)
Comment on attachment 730034 [details] [diff] [review]
Patch v1.8

Review of attachment 730034 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mms/src/ril/MmsService.js
@@ +1180,5 @@
>    },
>  
>    /**
>     * A utility function to convert the MmsParameters dictionary object
> +   * to a sendable message.

sendable intermediate object.

@@ +1267,5 @@
> +  },
> +
> +  /**
> +   * A utility function to convert the sendable intermediate object
> +   * to a sendable intermediate object.

s/sendable/savable

@@ +1277,2 @@
>      // The following attributes are needed for saving message into DB.
> +    let saveable = aSendable;

s/saveable/savable and below.

::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
@@ +39,5 @@
>     *   - If |type| == "mms", we also need:
>     *     - |delivery| DOMString: the delivery state of received message
>     *     - |deliveryStatus| DOMString Array: the delivery status of received message
>     *     - |receivers| DOMString Array: the phone numbers of receivers
> +   *     - |transactionId| DOMString: the transaction id from pdu header.

the transaction ID from MMS PDU header.

@@ +58,5 @@
>     *     - |receiver| DOMString: the phone number of receiver
>     *
>     *   - If |type| == "mms", we also need:
>     *     - |receivers| DOMString Array: the phone numbers of receivers
> +   *     - |transactionId| DOMString: the transaction id from pdu header.

Ditto.

@@ +85,5 @@
>    void getMessageRecordById(in long aMessageId,
>                              in nsIRilMobileMessageDatabaseRecordCallback aCallback);
> +
> +  /**
> +   * |aTransactionId| DOMString: the transaction id of MMS pdu.

the transaction ID of MMS PDU.

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +562,5 @@
>        });
>      };
>    },
>  
> +  // Add index of transaction id for MMS.

NIT: please use the following format for a function comment.

/**
 * Add index of transaction id for MMS.
 */

@@ +1162,5 @@
>          (aMessage.type == "sms" && aMessage.messageClass == undefined) ||
>          (aMessage.type == "mms" && (aMessage.delivery == undefined ||
>                                      !Array.isArray(aMessage.deliveryStatus) ||
>                                      !Array.isArray(aMessage.receivers))) ||
> +        (aMessage.type == "mms" && aMessage.transactionId == undefined) ||

Please combine this condition with the above one.

@@ +1220,5 @@
>    saveSendingMessage: function saveSendingMessage(aMessage, aCallback) {
>      if ((aMessage.type != "sms" && aMessage.type != "mms") ||
>          (aMessage.type == "sms" && !aMessage.receiver) ||
>          (aMessage.type == "mms" && !Array.isArray(aMessage.receivers)) ||
> +        (aMessage.type == "mms" && aMessage.transactionId == undefined) ||

Ditto.

@@ +1270,5 @@
>      if (aMessage.type == "sms") {
>        addresses = [aMessage.receiver];
>      } else if (aMessage.type == "mms") {
>        addresses = aMessage.receivers;
> +      aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"];

aMessage.transactionIdIndex = aMessage.transactionId;

@@ +1403,5 @@
> +      request.onerror = function onerror(event) {
> +        if (DEBUG) {
> +          if (event.target)
> +            debug("Caught error on transaction", event.target.errorCode);
> +          aCallback.notify(Ci.nsIMobileMessageCallback.INTERNAL_ERROR, null);

Why is the line in the |if (DEBUG) {}| block?
Attachment #730034 - Flags: review?(gene.lian) → review+
Btw, we still need Vicamo's final review since I'm not a formal RIL reviewer.
Posted patch Patch v1.9 (obsolete) — Splinter Review
Attachment #730034 - Attachment is obsolete: true
Attachment #730574 - Flags: review?(vyang)
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #14)
> Created attachment 730574 [details] [diff] [review]
> Patch v1.9

Modified according to Gene's comment.
(In reply to Gene Lian [:gene] from comment #12)
> Comment on attachment 730034 [details] [diff] [review]
> Patch v1.8
> 
> Review of attachment 730034 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mms/src/ril/MmsService.js
> @@ +1180,5 @@
> >    },
> >  
> >    /**
> >     * A utility function to convert the MmsParameters dictionary object
> > +   * to a sendable message.
> 
> sendable intermediate object.
> 
> @@ +1267,5 @@
> > +  },
> > +
> > +  /**
> > +   * A utility function to convert the sendable intermediate object
> > +   * to a sendable intermediate object.
> 
> s/sendable/savable
> 
> @@ +1277,2 @@
> >      // The following attributes are needed for saving message into DB.
> > +    let saveable = aSendable;
> 
> s/saveable/savable and below.
> 
> ::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
> @@ +39,5 @@
> >     *   - If |type| == "mms", we also need:
> >     *     - |delivery| DOMString: the delivery state of received message
> >     *     - |deliveryStatus| DOMString Array: the delivery status of received message
> >     *     - |receivers| DOMString Array: the phone numbers of receivers
> > +   *     - |transactionId| DOMString: the transaction id from pdu header.
> 
> the transaction ID from MMS PDU header.
> 
> @@ +58,5 @@
> >     *     - |receiver| DOMString: the phone number of receiver
> >     *
> >     *   - If |type| == "mms", we also need:
> >     *     - |receivers| DOMString Array: the phone numbers of receivers
> > +   *     - |transactionId| DOMString: the transaction id from pdu header.
> 
> Ditto.
> 
> @@ +85,5 @@
> >    void getMessageRecordById(in long aMessageId,
> >                              in nsIRilMobileMessageDatabaseRecordCallback aCallback);
> > +
> > +  /**
> > +   * |aTransactionId| DOMString: the transaction id of MMS pdu.
> 
> the transaction ID of MMS PDU.
> 
> ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
> @@ +562,5 @@
> >        });
> >      };
> >    },
> >  
> > +  // Add index of transaction id for MMS.
> 
> NIT: please use the following format for a function comment.
> 
> /**
>  * Add index of transaction id for MMS.
>  */
> 
> @@ +1162,5 @@
> >          (aMessage.type == "sms" && aMessage.messageClass == undefined) ||
> >          (aMessage.type == "mms" && (aMessage.delivery == undefined ||
> >                                      !Array.isArray(aMessage.deliveryStatus) ||
> >                                      !Array.isArray(aMessage.receivers))) ||
> > +        (aMessage.type == "mms" && aMessage.transactionId == undefined) ||
> 
> Please combine this condition with the above one.
> 
> @@ +1220,5 @@
> >    saveSendingMessage: function saveSendingMessage(aMessage, aCallback) {
> >      if ((aMessage.type != "sms" && aMessage.type != "mms") ||
> >          (aMessage.type == "sms" && !aMessage.receiver) ||
> >          (aMessage.type == "mms" && !Array.isArray(aMessage.receivers)) ||
> > +        (aMessage.type == "mms" && aMessage.transactionId == undefined) ||
> 
> Ditto.
> 
> @@ +1270,5 @@
> >      if (aMessage.type == "sms") {
> >        addresses = [aMessage.receiver];
> >      } else if (aMessage.type == "mms") {
> >        addresses = aMessage.receivers;
> > +      aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"];
> 
> aMessage.transactionIdIndex = aMessage.transactionId;
> 
> @@ +1403,5 @@
> > +      request.onerror = function onerror(event) {
> > +        if (DEBUG) {
> > +          if (event.target)
> > +            debug("Caught error on transaction", event.target.errorCode);
> > +          aCallback.notify(Ci.nsIMobileMessageCallback.INTERNAL_ERROR, null);
> 
> Why is the line in the |if (DEBUG) {}| block?

Thanks. :D
This is mms conformance feature. Nominate for leo+.
blocking-b2g: --- → leo?
Reporter

Comment 18

6 years ago
Comment on attachment 730574 [details] [diff] [review]
Patch v1.9

Review of attachment 730574 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mms/src/ril/MmsService.js
@@ +1047,4 @@
>  
>      let transactionId = notification.headers["x-mms-transaction-id"];
> +    gMobileMessageDatabaseService.getMessageRecordByTransactionId(transactionId,
> +        (function (aRv, aMessageRecord) {

Please tear down this long long function into small pieces. It's getting difficult to read, let alone review it :(

@@ +1272,5 @@
> +   *
> +   * @param aSendable
> +   *        The sendable intermediate object.
> +   */
> +  ceateSavableFromSendable : function ceateSavableFromSendable(aSendable) {

nit: c'r'eateSavableFromSendable

@@ +1283,5 @@
> +    let receivers = savable["receivers"] = [];
> +    for (let idx in headersTo) {
> +      receivers.push(headersTo[idx].address);
> +    }
> +    savable["transactionId"] = aSendable.headers["x-mms-transaction-id"];

Transaction id is mandatory for M-Send.req, but not really meaningful in this bug.

::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
@@ +58,5 @@
>     *     - |receiver| DOMString: the phone number of receiver
>     *
>     *   - If |type| == "mms", we also need:
>     *     - |receivers| DOMString Array: the phone numbers of receivers
> +   *     - |transactionId| DOMString: the transaction ID from MMS pdu header.

Ditto

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +200,5 @@
>              if (DEBUG) debug("Upgrade to version 8. Add participant/thread stores.");
>              self.upgradeSchema7(db, event.target.transaction);
>              break;
> +          case 8:
> +            if (DEBUG) debug("Upgrade to version 9. Add index of transaction id for MMS.");

for incoming MMS.

@@ +589,5 @@
> +        cursor.continue();
> +      }
> +      messageRecord.transactionIdIndex = messageRecord.transactionId;
> +      cursor.update(messageRecord);
> +      cursor.continue();

What about:

  if (is incoming mms) {
    update transaction index.
  }
  continue();

Because you missed a return statement after line 589 and transaction id is only meaningful when it's an incoming MMS.

@@ +1222,5 @@
>    saveSendingMessage: function saveSendingMessage(aMessage, aCallback) {
>      if ((aMessage.type != "sms" && aMessage.type != "mms") ||
>          (aMessage.type == "sms" && !aMessage.receiver) ||
> +        (aMessage.type == "mms" && (!Array.isArray(aMessage.receivers) ||
> +                                    aMessage.transactionId == undefined)) ||

Ditto.

@@ +1272,5 @@
>      if (aMessage.type == "sms") {
>        addresses = [aMessage.receiver];
>      } else if (aMessage.type == "mms") {
>        addresses = aMessage.receivers;
> +      aMessage.transactionIdIndex = aMessage.transactionId;

Ditto.

@@ +1390,5 @@
> +        return;
> +      }
> +      let request = messageStore.index("transactionId").get(aTransactionId);
> +
> +      request.onsuccess = function onsuccess(event) {

txn.oncomplete to shorten database transaction period.

@@ +1401,5 @@
> +        }
> +        aCallback.notify(Ci.nsIMobileMessageCallback.SUCCESS_NO_ERROR, messageRecord);
> +      };
> +
> +      request.onerror = function onerror(event) {

txn.onerror
Attachment #730574 - Flags: review?(vyang)
Mandatory MMS conformance bug, blocking.
blocking-b2g: leo? → leo+
Posted patch Patch v1.10 (obsolete) — Splinter Review
Attachment #730574 - Attachment is obsolete: true
Posted patch Patch v1.11 (obsolete) — Splinter Review
Attachment #735638 - Attachment is obsolete: true
Posted patch Patch v1.12 (obsolete) — Splinter Review
Attachment #735654 - Attachment is obsolete: true
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #22)
> Created attachment 735658 [details] [diff] [review]
> Patch v1.12
Modifiied according to comment 18.
Attachment #735658 - Flags: review?(vyang)
Reporter

Comment 24

6 years ago
Comment on attachment 735658 [details] [diff] [review]
Patch v1.12

Review of attachment 735658 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mms/src/ril/MmsService.js
@@ +1093,5 @@
> +                                      reportAllowed);
> +      transaction.run();
> +
> +      if (!success) {
> +        // At this point we could send a message to content to

nit: align to 80 char edge.

@@ +1106,5 @@
> +      // Broadcasting an 'sms-received' system message to open apps.
> +      this.broadcastMmsSystemMessage("sms-received", domMessage);
> +
> +      // Notifying observers an MMS message is received.
> +      Services.obs.notifyObservers(domMessage, kMmsReceivedObserverTopic, null);

Please extract these four lines into another utility function.

@@ +1167,5 @@
> +
> +    // For RETRIEVAL_MODE_AUTOMATIC or RETRIEVAL_MODE_AUTOMATIC_HOME but not
> +    // roaming, proceed to retrieve MMS.
> +    this.retrieveMessage(url, this.retrieveMessageCallback.bind(this, wish, savableMessage));
> +  },

Empty line, please.

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +588,5 @@
> +      let messageRecord = cursor.value;
> +      if ("mms" == messageRecord.type &&
> +          (DELIVERY_NOT_DOWNLOADED == messageRecord.delivery ||
> +           DELIVERY_RECEIVED == messageRecord.delivery)) {
> +        messageRecord.transactionIdIndex = messageRecord.transactionId;

At the time we land this patch, every mms record has no |transactionId| field, doesn't it? You have to fetch it from |messageRecord.headers["x-mms-transaction-id"]|.

@@ +1047,5 @@
>      aMessage.deliveryIndex = [DELIVERY_SENDING, timestamp];
>      aMessage.readIndex = [FILTER_READ_READ, timestamp];
>      aMessage.delivery = DELIVERY_SENDING;
>      aMessage.messageClass = MESSAGE_CLASS_NORMAL;
>      aMessage.read = FILTER_READ_READ;

Please keep this empty line.
Attachment #735658 - Flags: review?(vyang)
Posted patch Patch v1.13 (obsolete) — Splinter Review
Revised the patch for comment 24.
Attachment #735658 - Attachment is obsolete: true
Posted patch Patch v1.14 (obsolete) — Splinter Review
Changed this to self in |send|.
Attachment #736099 - Attachment is obsolete: true
Attachment #736100 - Flags: review?(vyang)
Reporter

Comment 27

6 years ago
Comment on attachment 736100 [details] [diff] [review]
Patch v1.14

Review of attachment 736100 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mms/src/ril/MmsService.js
@@ +1057,5 @@
> +   *         The MMS observer topic.
> +   * @params aDomMessage
> +   *         The nsIDOMMozMmsMessage object.
> +   */
> +  broadcastSysMagAndNotifyObservers: function broadcastSysMagAndNotifyObservers(aName, aObserverTopic, aDomMessage) {

Please have two functions broadcast{Sent,Received}MessageEvent() that takes one parameter |aDomMessage| only.

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +201,5 @@
>              if (DEBUG) debug("Upgrade to version 8. Add participant/thread stores.");
>              self.upgradeSchema7(db, event.target.transaction);
>              break;
> +          case 8:
> +            if (DEBUG) debug("Upgrade to version 9. Add index of transaction id for incoming MMS.");

nit: Add transactionId index for ...

@@ +564,5 @@
>      };
>    },
>  
> +  /**
> +   * Add index of transaction id for MMS.

ditto.

@@ +588,5 @@
> +      let messageRecord = cursor.value;
> +      if ("mms" == messageRecord.type &&
> +          (DELIVERY_NOT_DOWNLOADED == messageRecord.delivery ||
> +           DELIVERY_RECEIVED == messageRecord.delivery)) {
> +        messageRecord.transactionIdIndex = messageRecord.headers["x-mms-transaction-id"];

Line wrap
Attachment #736100 - Flags: review?(vyang) → review+
Posted patch Patch v1.15 (obsolete) — Splinter Review
Rebased and revised.
Attachment #736100 - Attachment is obsolete: true
Posted patch Patch v1.16Splinter Review
Clean space and some mistakes.
Attachment #736643 - Attachment is obsolete: true
Whiteboard: [NO_UPLIFT]
https://hg.mozilla.org/mozilla-central/rev/0614d7cadd76
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Comment on attachment 736707 [details] [diff] [review]
Patch v1.16

Review of attachment 736707 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mms/src/ril/MmsService.js
@@ +1351,5 @@
>            Services.obs.notifyObservers(aDomMessage, kSmsFailedObserverTopic, null);
>            return;
>          }
>  
> +        self.broadcastSentMessageEvent(domMessage);

s/domMessage/aDomMessage

I'm really surprised. This is impossible to work... How could we send an MMS with this typo? No one has been pointing this out since it was checked in on 4/17...

Will fire a bug for this later.
Attachment #736707 - Flags: feedback-
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.