921918 - B2G MMS: When receiving a read report, notify Gaia SMS AP which receiver has read the MMS.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

As a sender, FFOS is capable of displaying the read status to specify if the receiver has read the MMS.

Comment 1

[Gene Lian (I already quit Mozilla)]

My plate is too full. Assign this to Ctai who is available working on this. Thank you very much!

Comment 2

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

Attachment: bug-921918-1.patch part1 idl v1.0

IDL part

Comment 3

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

Attachment: bug-921918-2.patch part2, implementation v1.0

1. Add new observer topic for read status change.
2. Change DOM message related code for read status and read timestamp.
3. Add |setMessageReadReportByEnvelopeId| into MobileMessageDatabaseService.js.
4. Handle incoming M-Read-Orig.ind in MmsService.js.

Comment 4

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

Attachment: bug-921918-2.patch part2, implementation v1.1

1. Add new observer topic for read status change.
2. Change DOM message related code for read status and read timestamp.
3. Add |setMessageReadReportByEnvelopeId| into MobileMessageDatabaseService.js.
4. Handle incoming M-Read-Orig.ind in MmsService.js.
5. Fix trailing white space.

Comment 5

[Gene Lian (I already quit Mozilla)]

Comment on attachment 828424 [details] [diff] [review]
bug-921918-1.patch part1 idl v1.0

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

::: dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl
@@ +20,5 @@
>    DOMString? receiver;
>    DOMString? deliveryStatus;
> +  DOMString? readStatus;
> +  jsval readTimestamp; // Date object. null if not available (e.g.,
> +                       // |delivery| = "received" or not yet delivered).

s/delivered/read/

::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
@@ +102,5 @@
> +   * |aReceiver| DOMString: the phone number of receiver (for MMS; can be null).
> +   * |aReadStatus| DOMString: the new read status to update.
> +   * |aCallback| nsIRilMobileMessageDatabaseCallback: an optional callback.
> +   */
> +  void setMessageReadReportByEnvelopeId(in DOMString aEnvelopeId,

I'd prefer using the original setMessageDeliveryByEnvelopeId(...) and add an extra parameter |aReadStatus| so that we can share most of the existing logic. What do you think?

Comment 6

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #5)
> Review of attachment 828424 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
> @@ +102,5 @@
> > +   * |aReceiver| DOMString: the phone number of receiver (for MMS; can be null).
> > +   * |aReadStatus| DOMString: the new read status to update.
> > +   * |aCallback| nsIRilMobileMessageDatabaseCallback: an optional callback.
> > +   */
> > +  void setMessageReadReportByEnvelopeId(in DOMString aEnvelopeId,
> 
> I'd prefer using the original setMessageDeliveryByEnvelopeId(...) and add an
> extra parameter |aReadStatus| so that we can share most of the existing
> logic. What do you think?

No, just don't want to bloat a function with numerous parameters and complex logic.  Extract common part of |setMessageDeliveryByEnvelopeId| into another utility function and let the core function be crystal clear and self-evident.

Comment 8

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

Attachment: bug-921918-1.patch part1 idl v1.1

1. Change the comment for readTimestamp
2. Change |setMessageReadReportByEnvelopeId| to |setMessageReadStatusByEnvelopeId|

Comment 9

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

Attachment: bug-921918-2.patch part2, implementation v1.2

1. Add new common function |findReceiverFromDeliveryInfo| for |updateMessageDeliveryById| and |setMessageReadStatusByEnvelopeId|.

Comment 10

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

Comment on attachment 829023 [details] [diff] [review]
bug-921918-2.patch part2, implementation v1.2

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

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +585,5 @@
> +
> +    // Get |info.readTimestamp|.
> +    if (!JS_DefineProperty(aCx, infoJsObj,
> +        "readTimestamp", info.readTimestamp,
> +        NULL, NULL, JSPROP_ENUMERATE)) {

if (!JS_DefineProperty(aCx, infoJsObj, "readTimestamp", info.readTimestamp,
                       NULL, NULL, JSPROP_ENUMERATE)) {

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1903,5 @@
> +                                        address,
> +                                        readStatus,
> +                                        (function notifySetReadResult(aRv, aDomMessage) {
> +      if (DEBUG) debug("Marking the read status is done.");
> +      // TODO bug 832140 handle !Components.isSuccessCode(aRv)

Please still have check for aRv and return from the callback if aRv != Cr.NS_OK.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +21,5 @@
>  
>  const DEBUG = false;
>  const DISABLE_MMS_GROUPING_FOR_RECEIVING = true;
>  
> +XPCOMUtils.defineLazyGetter(this, "MMSPdu", function () {

let's just have "MMS" because we have that in MmsService.

@@ +254,5 @@
>              self.upgradeSchema17(event.target.transaction, next);
>              break;
>            case 18:
> +            if (DEBUG) debug("Upgrade to version 19. Add readStatus and readTimestamp.");
> +            self.upgradeSchema17(event.target.transaction, next);

nit: upgradeSchema18

@@ +2078,5 @@
> +      getRequest.onsuccess = function onsuccess(event) {
> +        messageRecord = event.target.result;
> +        if (!messageRecord) {
> +          if (DEBUG) debug("envelopeId = " + envelopeId + " is not found");
> +          return;

This triggers a transaction oncomplete event with a undefined |messageRecord|, and then we will have an exception thrown in |self.createDomMessageFromRecord|.

@@ +2084,5 @@
> +
> +        let isRecordUpdated = false;
> +
> +        // Update |messageRecord.deliveryInfo[].readStatus| if needed.
> +        if (!receiver) {

|receiver| must be defined here because M-Read-Orig.ind PDU forbids insert-address-token in the mandatory "from" header field.

::: dom/mobilemessage/src/gonk/mms_consts.js
@@ +97,5 @@
>  
> +// X-Mms-Read-Status values
> +//@see OMA-TS-MMS_ENC-V1_3-20110913-A clause 7.3.38
> +this.MMS_READ_STATUS_READ                       = 128;
> +this.MMS_READ_STATUS_DELETED_WITHOUT_BEING_READ = 129;

Never referenced.

Comment 11

[Gene Lian (I already quit Mozilla)]

Comment on attachment 829022 [details] [diff] [review]
bug-921918-1.patch part1 idl v1.1

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

::: dom/mobilemessage/interfaces/nsIDOMMozMmsMessage.idl
@@ +21,5 @@
>    DOMString? deliveryStatus;
>    jsval      deliveryTimestamp; // Date object; null if not available (e.g.,
>                                  // |delivery| = "received" or not yet delivered).
> +  DOMString? readStatus;
> +  jsval readTimestamp; // Date object. null if not available (e.g.,

Align |readTimestamp| to other attributes.

@@ +22,5 @@
>    jsval      deliveryTimestamp; // Date object; null if not available (e.g.,
>                                  // |delivery| = "received" or not yet delivered).
> +  DOMString? readStatus;
> +  jsval readTimestamp; // Date object. null if not available (e.g.,
> +                       // |readStatus| = "not-applicable" or not yet read).

s/"not-applicable"/"received"/

since we're going to have more readStatus other than "success" (your original "read") and "not-applicable".

::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
@@ +93,2 @@
>     */
>    void setMessageDeliveryByEnvelopeId(in DOMString aEnvelopeId,

Since you're changing IDL here, could you please rename this as well:

s/setMessageDeliveryByEnvelopeId/setMessageDeliveryStatusByEnvelopeId

and drop the |aDelivery| to make it more symmetric to |setMessageReadStatusByEnvelopeId|.

and make

1. The caller:

    gMobileMessageDatabaseService
      .setMessageDeliveryByEnvelopeId(envelopeId,
                                      address,
                                      deliveryStatus,
                                      ...)
2. The DB implementation:

  setMessageDeliveryByEnvelopeId: function setMessageDeliveryByEnvelopeId(
      envelopeId, receiver, deliveryStatus, callback) {
    this.updateMessageDeliveryById(envelopeId, "envelopeId",
                                   receiver, null, deliveryStatus,
                                   null, callback);

  },

What do you think?

@@ +99,5 @@
>  
>    /**
> +   * |aEnvelopeId| DOMString: the "message-id" specified in the MMS PDU headers.
> +   * |aReceiver| DOMString: the phone number of receiver (for MMS; can be null).
> +   * |aReadStatus| DOMString: the new read status to update.

s/to update/to be updated/

Comment 12

[Gene Lian (I already quit Mozilla)]

Comment on attachment 829023 [details] [diff] [review]
bug-921918-2.patch part2, implementation v1.2

Btw, I don't see the logic for firing onreadsuccess/onreaderror events through IPC.

Comment 13

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #11)
> 1. The caller:
> 
>     gMobileMessageDatabaseService
>       .setMessageDeliveryByEnvelopeId(envelopeId,
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
setMessageDeliveryStatusByEnvelopeId

>                                       address,
>                                       deliveryStatus,
>                                       ...)
> 2. The DB implementation:
> 
>   setMessageDeliveryByEnvelopeId: function setMessageDeliveryByEnvelopeId(
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
setMessageDeliveryStatusByEnvelopeId

>       envelopeId, receiver, deliveryStatus, callback) {
>     this.updateMessageDeliveryById(envelopeId, "envelopeId",
>                                    receiver, null, deliveryStatus,
>                                    null, callback);
> 
>   },

Comment 14

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

Attachment: bug-921918-1.patch part1 idl v1.2

s/setMessageDeliveryByEnvelopeId/setMessageDeliveryStatusByEnvelopeId

Comment 15

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

Attachment: bug-921918-2.patch part2, fix-deliveryStatus v1.0

Fix the wrong updateMessageDeliveryById.

Comment 16

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

Attachment: bug-921918-3.patch part3 implementation v1.0

Comment 17

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

Comment on attachment 829181 [details] [diff] [review]
bug-921918-3.patch part3 implementation v1.0

1. s/setMessageDeliveryByEnvelopeId/setMessageDeliveryStatusByEnvelopeId in MobileMessageDatabaseService.js
2. use |findReceiverFromDeliveryInfo| in |setMessageReadStatusByEnvelopeId| 
3. Add ipc part.
4. In sending MMS, if we request read report, set readStatus to READ_STATUS_PENDING. Else set to READ_STATUS_NOT_APPLICABLE.
5. Following W3C spec, change to READ_STATUS_SUCCESS, READ_STATUS_PENDING, READ_STATUS_ERROR.

Comment 18

[Gene Lian (I already quit Mozilla)]

Comment on attachment 829178 [details] [diff] [review]
bug-921918-1.patch part1 idl v1.2

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

Nice! r=gene (note that please don't use sr=gene).

Comment 19

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

Taking this because Chia-Hung is on vocation. :)

Comment 20

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

Vicamo, Thanks.
But it is bussiness trip....not vacation.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #19)
> Taking this because Chia-Hung is on vocation. :)

Comment 21

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

Depends on bug 934931 because attachment 829180 [details] [diff] [review] (part 2) was moved into bug 934931.

Comment 22

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

Attachment: part 1/4: interface changes

update commit summary only

Comment 23

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

Attachment: part 2/4: DOM

This patch was made from attachment 829181 [details] [diff] [review] by extracting lines other than backend changes.

Comment 24

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

Attachment: part 3.a/4: iccId is optional

I divide part 3 into four smaller pieces and will merge them before landing.  In this part I'm going to refactor the dictionary structure used in |saveReceivedMessage|.  The main purpose is to reduce the required fields, and to clarify what do we actually need.

In this piece, I only update the comment for the |iccId| field.

Comment 25

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

Attachment: part 3.b/4: assign received |aMmsMessage.sender| in mmdb

Move |sender| field assignment of a received MMS message into db to reduce duplicated code and to reduce a mandatory field in the passed dictionary object.

Comment 26

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

Attachment: part 3.c/4: don't pass transactionId

Assign it in db because we can always have it from |message.headers["x-mms-transaction-id"]|.

Comment 27

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

Attachment: part 3.d/4: construct mms deliveryInfo in db

Passing |deliveryInfo| array causes some duplicated code.  Besides we know that a received MMS message can have only one MmsDeliveryInfo entry and the |deliveryTimestamp| is always null (numeric zero), but the code doesn't reflect this well.

Comment 28

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

Attachment: part 4/4: WIP

Comment 29

[Gene Lian (I already quit Mozilla)]

Comment on attachment 831572 [details] [diff] [review]
part 3.a/4: iccId is optional

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

r=gene

Comment 30

[Gene Lian (I already quit Mozilla)]

Comment on attachment 831578 [details] [diff] [review]
part 3.b/4: assign received |aMmsMessage.sender| in mmdb

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1849,5 @@
>        }
>        return;
>      }
> +
> +    let threadParticipants;

let threadParticipants = [aMessage.sender];

@@ +1858,5 @@
> +        aMessage.sender = "anonymous";
> +      }
> +
> +      threadParticipants =
> +        this.fillReceivedMmsThreadParticipants(aMessage, [aMessage.sender]);

This is not correct use of fillReceivedMmsThreadParticipants.

Should be:

this.fillReceivedMmsThreadParticipants(aMessage, threadParticipants);

@@ +1859,5 @@
> +      }
> +
> +      threadParticipants =
> +        this.fillReceivedMmsThreadParticipants(aMessage, [aMessage.sender]);
> +    } else { // SMS

Drop this else for SMS.

Comment 31

[Gene Lian (I already quit Mozilla)]

Comment on attachment 831580 [details] [diff] [review]
part 3.c/4: don't pass transactionId

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1596,5 @@
>                                                              mmsStatus,
>                                                              retrievedMessage) {
>      if (DEBUG) debug("retrievedMessage = " + JSON.stringify(retrievedMessage));
>  
> +    let transactionId = savableMessage.headers["x-mms-transaction-id"];

...

Our NotifyResponseTransaction doesn't work because transactionId is null...

Thanks for catching this.

Comment 32

[Gene Lian (I already quit Mozilla)]

Comment on attachment 831581 [details] [diff] [review]
part 3.d/4: construct mms deliveryInfo in db

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

::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
@@ -41,5 @@
>     *     - |sender| DOMString: the phone number of sender
>     *
>     *   - If |type| == "mms", we also need:
>     *     - |delivery| DOMString: the delivery state of received message
> -   *     - |deliveryStatus| DOMString Array: the delivery status of received message

Hmm... we forgot update this when introducing |deliveryInfo|.

::: dom/mobilemessage/src/gonk/MmsService.js
@@ -2162,5 @@
>          if (DEBUG) debug("Delivery of message record is not 'not-downloaded'.");
>          aRequest.notifyGetMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR);
>          return;
>        }
> -      if (DELIVERY_STATUS_PENDING == aMessageRecord.deliveryStatus) {

Hmm... This was wrong. Sorry for not catching this review. Thanks!

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1905,5 @@
>        aMessage.transactionIdIndex = aMessage.headers["x-mms-transaction-id"];
>        aMessage.isReadReportSent = false;
>  
> +      // As a receiver, we don't need to care about the delivery status of
> +      // others.

Please add one more sentence:

, so we put a single element with self's phone number in the |deliveryInfo| array.

Comment 33

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

Attachment: part 1/4: interface changes : v2

rebase only.

Comment 34

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

Attachment: part 2/4: DOM : v2

correct commit summary: r=khuey

Comment 35

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

Attachment: part 3.a/4: iccId is optional : v2

1) rebase
2) update r=gene

Comment 36

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

Attachment: part 3.b/4: assign received |aMmsMessage.sender| in mmdb : v2

rebase & address comment 30.

Comment 37

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

Attachment: part 3.c/4: don't pass transactionId : v2

rebase & update r=gene

Comment 38

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

Attachment: part 3.d/4: construct mms deliveryInfo in db : v2

rebase, add comment 32 and update r=gene

Comment 39

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

Attachment: part 3.e/4: fix debug doesn't work when only turned on in pref.

Comment 40

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

Attachment: part 3.f/4: share similar works to callback with a DOM message in a transaction

This patch tries to share common behaviour that will also be used later in part 4.b/4.  Also fixes an error that we try to create a DomMessage from an undefined messageRecord.

Comment 41

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

Attachment: part 3.g/4: fix possible "undefined" value for messageRecord.envelopeIdIndex

Previous part 3.f/4 raises this issue up to table.

The second time we try to call |setMessageDeliveryByMessageId| with an undefined |aEnvelopeId|, |messageRecord.envelopeIdIndex| is actually assigned to string "undefined" because of xpconnnect.  But since we set a UNIQUE flag on this index, trying to put this record into database raises a ConstrainedError.

Comment 42

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

Attachment: part 4.a/4: accommodate to setMessageDeliveryByEnvelopeId interface change

Comment 43

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

Attachment: part 4.b/4: Gonk RIL implementation : WIP

Existing test cases passed, still to be verified on physical device.  We can't have new test cases for this bug right now because of the lack of MMS test framework.

Comment 44

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

Attachment: part 4.b/4: Gonk RIL implementation

Verified with Unagi.  Somehow our Gaia sends markMessageRead no more, I can only test M-Read-Orig.ind along with a Galaxy SII.

Full try: https://tbpl.mozilla.org/?tree=Try&rev=376165e7a73d

Comment 45

[Gene Lian (I already quit Mozilla)]

I'll try to take this review by the end of tomorrow.

Comment 46

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8334748 [details] [diff] [review]
part 3.f/4: share similar works to callback with a DOM message in a transaction

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1531,5 @@
> +        notifyResult(Cr.NS_ERROR_FAILURE, null);
> +        return;
> +      }
> +
> +      let capture = {};

Hmm... You are passing around an object to capture the messageRecod. Sounds a bit weird to me but I don't have better solutions. ;)

@@ +1797,5 @@
>              + " envelopeId: " + envelopeId);
>      }
>  
>      let self = this;
> +    this.newTxnWithCallback(callback, function(aCapture, aTransaction,

Why do you pass in the |aTransaction|? I don't see any codes are using that? Did I misunderstand anything?

Comment 47

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8335154 [details] [diff] [review]
part 4.b/4: Gonk RIL implementation

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1878,5 @@
> +
> +    // From OMA-TS-MMS_ENC-V1_3-20110913-A subclause 9.4 "X-Mms-Read-Status",
> +    // in M-Read-Rec-Orig.ind the X-Mms-Read-Status could be
> +    // MMS.MMS_READ_STATUS_{ READ, DELETED_WITHOUT_BEING_READ }.
> +    let readStatus = mmsReadStatus ? MMS.DOM_READ_STATUS_SUCCESS

I assume |readStatus| will be assigned with MMS.DOM_READ_STATUS_SUCCESS only when |mmsReadStatus| == MMS.MMS_READ_STATUS_READ.

Please double check the "?" operator is correctly working right here. Thanks!

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +2156,5 @@
> +            ", receiver: " + aReceiver + ", readStatus: " + aReadStatus);
> +    }
> +
> +    let self = this;
> +    this.newTxnWithCallback(aCallback, function(aCapture, aTransaction,

Again, I don't know why we need to input |aTransaction| which is not used in this callback. Please correct me if I misunderstood. Tentatively remove the review tag.

@@ +2176,5 @@
> +            return;
> +          }
> +
> +          aEntry.readStatus = aReadStatus;
> +          aEntry.readTimestamp = Date.now();

if (aReadStatus == MMS.DOM_READ_STATUS_SUCCESS) {
  aEntry.readTimestamp = Date.now();
}

Comment 48

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #47)
> Comment on attachment 8335154 [details] [diff] [review]
> part 4.b/4: Gonk RIL implementation
> 
> Review of attachment 8335154 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/gonk/MmsService.js
> @@ +1878,5 @@
> > +
> > +    // From OMA-TS-MMS_ENC-V1_3-20110913-A subclause 9.4 "X-Mms-Read-Status",
> > +    // in M-Read-Rec-Orig.ind the X-Mms-Read-Status could be
> > +    // MMS.MMS_READ_STATUS_{ READ, DELETED_WITHOUT_BEING_READ }.
> > +    let readStatus = mmsReadStatus ? MMS.DOM_READ_STATUS_SUCCESS
> 
> I assume |readStatus| will be assigned with MMS.DOM_READ_STATUS_SUCCESS only
> when |mmsReadStatus| == MMS.MMS_READ_STATUS_READ.
> 
> Please double check the "?" operator is correctly working right here. Thanks!

"x-mms-read-status" is wired to BooleanValue, so there is no such thing "MMS.MMS_READ_STATUS_READ".

http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/gonk/MmsPduHelper.jsm#1681

> ::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
> @@ +2156,5 @@
> > +            ", receiver: " + aReceiver + ", readStatus: " + aReadStatus);
> > +    }
> > +
> > +    let self = this;
> > +    this.newTxnWithCallback(aCallback, function(aCapture, aTransaction,
> 
> Again, I don't know why we need to input |aTransaction| which is not used in
> this callback. Please correct me if I misunderstood. Tentatively remove the
> review tag.

No special reason.  Just think might need sometime.  Will remove.

> @@ +2176,5 @@
> > +            return;
> > +          }
> > +
> > +          aEntry.readStatus = aReadStatus;
> > +          aEntry.readTimestamp = Date.now();
> 
> if (aReadStatus == MMS.DOM_READ_STATUS_SUCCESS) {
>   aEntry.readTimestamp = Date.now();
> }

Hmmm, that justifies the attribute name 'readTimestamp'.  Will correct it.

Comment 49

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #48)
> "x-mms-read-status" is wired to BooleanValue, so there is no such thing
> "MMS.MMS_READ_STATUS_READ".

Similar BooleanValue wired header fields are:
1) "x-mms-cancel-status": Cancel Request Successfully received | Cancel Request corrupted
2) "x-mms-sender-visibility": Hide | Show
3) "x-mms-read-status": Read | Deleted without being read

"x-mms-sender-visibility" will be the most confusing one because "true" means "hide" and "false" means "show".

Comment 50

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

Attachment: part 2/4: DOM : v3

rebase only.

Comment 51

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

Attachment: part 3.c/4: don't pass transactionId : v3

check property exists by |hasOwnProperty| before delete it.

Comment 52

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

Attachment: part 3.f/4: share similar works to callback with a DOM message in a transaction : v2

address comment 46 -- remove aTransaction argument.

Comment 53

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

Attachment: part 3.g/4: fix possible "undefined" value for messageRecord.envelopeIdIndex : v2

rebase only.

Comment 54

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

Attachment: part 3.h/4: decode some MMS header fields as enum

Comment 55

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

Attachment: part 4.b/4: Gonk RIL implementation : v2

1) use MMS.MMS_PDU_READ_STATUS_READ
2) remove 'aTransaction' argument from 'newTxnWithCallback'

Comment 56

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

https://tbpl.mozilla.org/?tree=Try&rev=ef48d08bdaa7

Comment 57

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8336756 [details] [diff] [review]
part 4.b/4: Gonk RIL implementation : v2

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

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1227,5 @@
>        }
>  
> +      // For all sent and received MMS messages, we have to add their
> +      // |readStatus| and |readTimestamp| attributes in |deliveryInfo| array.
> +      let isReadReportRequested =

s/isReadReportRequested/readReportRequested

to have a more consistent coding style since we're just having a bug doing so.

@@ +1230,5 @@
> +      // |readStatus| and |readTimestamp| attributes in |deliveryInfo| array.
> +      let isReadReportRequested =
> +        messageRecord.headers["x-mms-read-report"] || false;
> +      for (let element of messageRecord.deliveryInfo) {
> +        element.readStatus = isReadReportRequested

s/isReadReportRequested/readReportRequested

@@ +2189,5 @@
> +            return;
> +          }
> +
> +          aEntry.readStatus = aReadStatus;
> +          aEntry.readTimestamp = Date.now();

Comment #48.

if (aReadStatus == MMS.DOM_READ_STATUS_SUCCESS) {
  aEntry.readTimestamp = Date.now();
}

Comment 58

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8334712 [details] [diff] [review]
part 1/4: interface changes : v2

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

Sorry I have to pull back the review+. Please see Bug 939302. We'd better use long-long to specify timestamps. Sorry we have to fix some other patches as well.

Comment 59

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

Comment on attachment 8334712 [details] [diff] [review]
part 1/4: interface changes : v2

Bug 939302 is not even fully reviewed.

Comment 60

[Gene Lian (I already quit Mozilla)]

No, the DOM IDL part has been reviewed by Jonas and Boris.

Comment 61

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

Attachment: part 4.b/4: Gonk RIL implementation : v3

Address comment 57.

Comment 62

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

Attachment: part 1/4: interface changes : v3

rebase only.

Comment 63

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

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #60)
> No, the DOM IDL part has been reviewed by Jonas and Boris.

I said "fully" reviewed.  Besides, don't forget there are plenty of test cases to fix.

Comment 64

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

https://hg.mozilla.org/integration/b2g-inbound/rev/89433bb31453
https://hg.mozilla.org/integration/b2g-inbound/rev/94f875f9a8de
https://hg.mozilla.org/integration/b2g-inbound/rev/4780d72117e3
https://hg.mozilla.org/integration/b2g-inbound/rev/307e7260ada2
https://hg.mozilla.org/integration/b2g-inbound/rev/6bdd99eabb57
https://hg.mozilla.org/integration/b2g-inbound/rev/8446a3ac5060
https://hg.mozilla.org/integration/b2g-inbound/rev/529304b6bd08
https://hg.mozilla.org/integration/b2g-inbound/rev/a9664889638e
https://hg.mozilla.org/integration/b2g-inbound/rev/dc75d4de9df4
https://hg.mozilla.org/integration/b2g-inbound/rev/dafc51a29fb3
https://hg.mozilla.org/integration/b2g-inbound/rev/f38875c274c0
https://hg.mozilla.org/integration/b2g-inbound/rev/f668eb3f9829

Comment 65

[Carsten Book [:Tomcat]]

Hi Vicamo, 

sorry had to backout this changes in https://hg.mozilla.org/integration/b2g-inbound/pushloghtml?changeset=70779672ea78 because this checkin caused a bustage on Windows XP Debug builds with the error mentioned here https://tbpl.mozilla.org/php/getParsedLog.php?id=31032938&tree=B2g-Inbound

Comment 66

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

(In reply to Carsten Book [:Tomcat] from comment #65)
> Hi Vicamo, 
> 
> sorry had to backout this changes in
> https://hg.mozilla.org/integration/b2g-inbound/
> pushloghtml?changeset=70779672ea78 because this checkin caused a bustage on
> Windows XP Debug builds with the error mentioned here
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31032938&tree=B2g-Inbound

I think that's a CLOBBER issue actually.

Comment 67

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

full try with original patches: https://tbpl.mozilla.org/?tree=Try&rev=b79196ac95fe

Comment 68

[Carsten Book [:Tomcat]]

Hey Vicamo, when the try run pass could you touch the clobber file.

You should also then file a "clobber was needed for foo" bug for that touch of the clobber file.

Comment 69

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

(In reply to Carsten Book [:Tomcat] from comment #68)
> Hey Vicamo, when the try run pass could you touch the clobber file.
> You should also then file a "clobber was needed for foo" bug for that touch
> of the clobber file.

No, I'll have an additional clobber patch at the end.  Technically we haven't land anything.  See bug 938950, bug 934646.  Don't need to file a new bug for the clobber.

Comment 70

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

Attachment: part 4.c/4: clobber

Comment 71

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

Re-land with one additional patch for clobber as part 4.c/4.
https://hg.mozilla.org/integration/b2g-inbound/rev/a180721f1f8b
https://hg.mozilla.org/integration/b2g-inbound/rev/08a2163d4000
https://hg.mozilla.org/integration/b2g-inbound/rev/1cce6e770603
https://hg.mozilla.org/integration/b2g-inbound/rev/a60a1b5e511b
https://hg.mozilla.org/integration/b2g-inbound/rev/535b84dce91b
https://hg.mozilla.org/integration/b2g-inbound/rev/601e0b7b0ff5
https://hg.mozilla.org/integration/b2g-inbound/rev/c7112ca224bb
https://hg.mozilla.org/integration/b2g-inbound/rev/6f154a172ad5
https://hg.mozilla.org/integration/b2g-inbound/rev/a66f5c500b6f
https://hg.mozilla.org/integration/b2g-inbound/rev/694c768df2ed
https://hg.mozilla.org/integration/b2g-inbound/rev/f17fc9d77dec
https://hg.mozilla.org/integration/b2g-inbound/rev/8d558039569a
https://hg.mozilla.org/integration/b2g-inbound/rev/5f27eb843a1e

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a180721f1f8b
https://hg.mozilla.org/mozilla-central/rev/08a2163d4000
https://hg.mozilla.org/mozilla-central/rev/1cce6e770603
https://hg.mozilla.org/mozilla-central/rev/a60a1b5e511b
https://hg.mozilla.org/mozilla-central/rev/535b84dce91b
https://hg.mozilla.org/mozilla-central/rev/601e0b7b0ff5
https://hg.mozilla.org/mozilla-central/rev/c7112ca224bb
https://hg.mozilla.org/mozilla-central/rev/6f154a172ad5
https://hg.mozilla.org/mozilla-central/rev/a66f5c500b6f
https://hg.mozilla.org/mozilla-central/rev/694c768df2ed
https://hg.mozilla.org/mozilla-central/rev/f17fc9d77dec
https://hg.mozilla.org/mozilla-central/rev/8d558039569a
https://hg.mozilla.org/mozilla-central/rev/5f27eb843a1e

Comment 74

[Andrew Overholt [:overholt]]

This landed before gecko 28 branched so will be in 1.3.