850140 - B2G MMS: implement MmsService.handleDeliveryIndication() to handle delivery report

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

This is not a critical issue because it doesn't block any MMS user stories so far. In the future, two things we need to do here:

1. Use gMobileMessageDatabaseService.setMessageDelivery() to reset the delivery status to "success" or "error" for a specific receiver.

2. Fire "mms-delivery-success" or "mms-delivery-error" observer topics to MobileMessageManager.

Comment 1

[Gene Lian (I already quit Mozilla)]

For this moment, I'm wondering do we really need to do the delivery report after sending MMS. Let PM decide this.

Comment 2

[Mounir Lamouri (:mounir)]

(In reply to Gene Lian [:gene] from comment #1)
> For this moment, I'm wondering do we really need to do the delivery report
> after sending MMS. Let PM decide this.

It is important for feature parity between SMS and MMS. Why would SMS have that but not MMS?

Comment 3

[Gene Lian (I already quit Mozilla)]

(In reply to Mounir Lamouri (:mounir) from comment #2)
> It is important for feature parity between SMS and MMS. Why would SMS have
> that but not MMS?

Yes, I agree with you! I just don't see any MMS story addressing this. If we used to have a similar SMS story, then we should implement this as well.

Comment 4

[Gene Lian (I already quit Mozilla)]

This one also blocks the DOM APIs because we need to notify the Gaia about the delivery reports through the following events in nsIDOMMozMobileMessageManager:

  [implicit_jscontext] attribute jsval ondeliverysuccess;
  [implicit_jscontext] attribute jsval ondeliveryerror;

Comment 5

[Gene Lian (I already quit Mozilla)]

Also need to be IPC'ed.

Comment 6

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

(In reply to Gene Lian [:gene] from comment #5)
> Also need to be IPC'ed.

We don't really have to depend on IPC to land this bug. Any reasons?

Comment 7

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #6)
> (In reply to Gene Lian [:gene] from comment #5)
> > Also need to be IPC'ed.
> 
> We don't really have to depend on IPC to land this bug. Any reasons?

Don't we need to fire delivery-success or delivery-error observer topics from parent?

Comment 8

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

(In reply to Gene Lian [:gene] from comment #7)
> Don't we need to fire delivery-success or delivery-error observer topics
> from parent?

Sorry, it seems I misunderstood you. This bug blocks bug 847744.

Comment 9

[Chris Lee [:clee]]

Kev, do we require delivery report for MMS?  Thanks.

Comment 10

[Gene Lian (I already quit Mozilla)]

(In reply to Chris Lee [:clee] from comment #9)
> Kev, do we require delivery report for MMS?  Thanks.

If this one is indeed required, I'll come back to work on this ASAP. Btw, another similar MMS feature should be discussed as well. That is, *MMS Read Report* addressed at Bug 852863, which is a bit different from Delivery Report.

Comment 11

[Kev Needham [:kev]]

There's a user story for when messages fail to be sent, is there a reason why this wouldn't be used? I don't see it as a blocking issue, but as a matter of course I'd expect there to be an acknowledgement of when it has been sent in order to satisfy the criteria of letting the user know if the delivery is unsuccessful. This seems like something that should be there.

Comment 12

[Lucas Adamski [:ladamski]]

Plus-ing, but for the future we can provide a reference to a user story as rationale?  We do have a list of them somewhere, correct?

Comment 13

[Chris Lee [:clee]]

(In reply to Lucas Adamski from comment #12)
> Plus-ing, but for the future we can provide a reference to a user story as
> rationale?  We do have a list of them somewhere, correct?

Yes, we do have a full list of all v1.1 users stories.

Comment 14

[Ken Chang[:kenkai]]

After discussing with Lucas, this bug isn't in current user story. Remove it from Leo+ list.

Comment 15

[Dietrich Ayala (:dietrich)]

(In reply to Kev Needham [:kev] from comment #11)
> There's a user story for when messages fail to be sent, is there a reason
> why this wouldn't be used? I don't see it as a blocking issue, but as a
> matter of course I'd expect there to be an acknowledgement of when it has
> been sent in order to satisfy the criteria of letting the user know if the
> delivery is unsuccessful. This seems like something that should be there.

This is not sufficient to *block* on this feature at this point. Unless a carrier or OEM is demanding it's required, we should not include.

NI? on Moz Product and TEF Product to sign off on *not* including this feature.

Comment 16

[Daniel Coloma:dcoloma]

Is this about including a setting in the UI to allow the users to require a delivery report?

Comment 17

[Kev Needham [:kev]]

Per comment #10 and #16, stating an assumption that this bug is not a read/delivery report, which is a status indicator of when a sent message is successfully delivered to a handset after being submitted to the network (similar to an email read receipt).

While there are no blocking requirements (e.g the device ship for 1.1 would not be held) the mandatory requirements that must be fulfilled in a follow-on version for handling sent messages and displaying delivery status are as follows, and should be added as a blocker for 1.2 if they cannot be included in 1.1:

- Display of a list of messages and the status for them when the Outbox folder is open. Messages will be listed in a chronological order, showing the newest on the top. Messages show the intended recipients.
- Support of a graphical indication (e.g. icon) on the screen to the user when the sending is carried out in the background.
- In this folder the options for the user are:
-- “send message, when selected”
-- “delete message, when selected”
-- “Send all messages”
- The Outbox folder is used to temporary store the message prior to being sent. Each message, after is being composed and the “send” option is chosen, will be moved into the Outbox folder.
- After the terminal has sent the message successfully to the network the message will no longer appear in the Outbox but instead be shown in the „sent“ folder and a pop up displayed.
- In the same fashion, when a user chooses to re-send or forward a message, the message is moved to the Outbox folder after the “send” button is pushed.
- After the user has selected “send” and the phone has placed the message in the Outbox, the UI of the phone will be freed and the full functionality of the phone will be available to the user.
- The Outbox will retain its state even though the phone boots itself or is turned off by the user.
- Retry schema must attempt 4 or 5 retries. Recommended 4 retries with retry at following Iintervals-1 minute,5 minutes,10 minutes, 30 minutes. 
- STOP resending after the fourth or fifth retry(as per retry schema). The user gets informed about the failure with a warning message.
- If the error is considered permanent the retry schema will stop and a splash screen is shown to the user.
- Status of the messages in the Outbox are:
-- Status outbox message: Sending: a connection is being made for the message to be sent
-- Status outbox message: Resending: a previous sending has failed; the phone will try to send the message again after a time-out period; User is able to choose “send” action which will restart the sending immediately.
-- Status outbox message: Failed: the maximum number of sending attempts has been reached and the sending has been failed; when a message turns into a “Failed” status, the UI will pop-up a notification inform the user about the failure.
-- Background sending: The background process to send the MMS to the Relay/Server starts immediately after initiating ‘send’.
- If the sending of the message fails the customer has to be informed of the failing, then the message will be marked as failed in the Outbox.

Comment 18

[Dietrich Ayala (:dietrich)]

Re comment #16, we already have that, for SMS. This would also apply that setting for MMS.

Per comment #17, not blocking on this for now. Not ideal, but we need to reduce feature scope, and can ship this improvement in the next release, so koi+.

Comment 19

[Gene Lian (I already quit Mozilla)]

I'll upload my patch tomorrow.

Comment 20

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, MobileMessageDatabaseService

Comment 21

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, MmsService

Comment 22

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, provide and correct DB functions

Comment 23

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, handle delivery report

Comment 24

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

Comment on attachment 774509 [details] [diff] [review]
Part 1, provide and correct DB functions

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

::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
@@ +69,5 @@
>     * |aReceiver| DOMString: the phone number of receiver (for MMS; can be null).
>     * |aDelivery| DOMString: the new delivery value to update (can be null).
>     * |aDeliveryStatus| DOMString: the new delivery status to update (can be null).
>     * |aCallback| nsIRilMobileMessageDatabaseCallback: an optional callback.
> +   * |aEnvelopeId| DOMString: to indentify the MMS for the delivery report.

For one time I can't really map this variable to MMS specs.  It should be "Message-ID" header field but that conflicts with "aMessageId" above.  Please have the same description as you have for |getMessageRecordByEnvelopeId|.

@@ +76,5 @@
>                            in DOMString aReceiver,
>                            in DOMString aDelivery,
>                            in DOMString aDeliveryStatus,
> +               [optional] in nsIRilMobileMessageDatabaseCallback aCallback,
> +               [optional] in DOMString aEnvelopeId);

Why not just make it null-able like aReceiver/aDelivery/aDeliveryStatus do?

Comment 25

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

Comment on attachment 774510 [details] [diff] [review]
Part 2, handle delivery report

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

I'm also worry about separating an operation to a get and a set query.  Maybe we should have a |setMessageDeliveryById()| and a |setMessageDeliveryByEnvelopeId()|.

::: dom/mobilemessage/src/ril/MmsService.js
@@ +1524,5 @@
> +      debug("handleDeliveryIndication: got delivery report" +
> +            JSON.stringify(aMsg));
> +    }
> +
> +    let headers = aMsg.headers;    

trailing ws.

@@ +1529,5 @@
> +    if (!headers || !headers["message-id"] || !headers["x-mms-status"] ||
> +        !headers.to || !headers.to.address) {
> +      if (DEBUG) debug("The delivery report is incomplete. Fail to update.");
> +      return;
> +    }

We have checked these mandatory fields in PDU helpers.

@@ +1544,5 @@
> +    if (!normalizedAddress) {
> +      if (DEBUG) debug("Normalized address is not valid. Return.");
> +      return;
> +    }
> +    let parsedAddress = PhoneNumberUtils.parseWithMCC(normalizedAddress, null);

Please don't do phone number specific parsing/matching outside MobileMessageDatabaseService.  Instead, just feed |headers.to.address| to the |setMessageDelivery()| call as the second argument.

@@ +1570,5 @@
> +      }
> +
> +      for (let i = 0; i < aMessageRecord.receivers.length; i++) {
> +        let receiver = aMessageRecord.receivers[i];
> +        let normalizedReceiver = PhoneNumberUtils.normalize(receiver, false);

The whole for-loop should be moved into database service.

@@ +1714,5 @@
>      message["timestamp"] = Date.now();
>      message["receivers"] = receivers;
> +    try {
> +      message["deliveryStatusRequested"] =
> +        Services.prefs.getBoolPref("dom.sms.requestStatusReport");

SMS & MMS are quite different thing.  I think we should have an independent setting for MMS here.

Comment 26

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

Comment on attachment 774509 [details] [diff] [review]
Part 1, provide and correct DB functions

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

::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
@@ +100,5 @@
> +   * |aCallback| nsIRilMobileMessageDatabaseRecordCallback: a callback which
> +   *   takes result flag and message record as parameters.
> +   */
> +  void getMessageRecordByEnvelopeId(in DOMString aEnvelopeId,
> +                                    in nsIRilMobileMessageDatabaseRecordCallback aCallback);

Second thought, please have a |setMessageDeliveryByEnvelopeId| instead.

Comment 29

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, provide and correct DB functions, V2

Comment 30

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, handle delivery report, V2

Comment 31

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

Comment on attachment 783619 [details] [diff] [review]
Part 1, provide and correct DB functions, V2

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

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +780,5 @@
> +      messageStore.deleteIndex("envelopeId");
> +    }
> +
> +    // Create new "envelopeId" indexes.
> +    messageStore.createIndex("envelopeId", "envelopeIdIndex", { unique: true });

We do save all MMS header fields since bug 845643, so I think we could set "envelopeIdIndex" values for existing outgoing MMS messages by reading their |messageRecord.headers["message-id"]|.

@@ +1364,5 @@
> +        getRequest = messageStore.index("envelopeId").get(id);
> +      } else {
> +        if (DEBUG) debug("ID type: " + type + " is invalid");
> +        notifyResult(Cr.NS_ERROR_FAILURE);
> +        return;

|updateMessageDeliveryById| is now an internal function.  There can't be an |else| case.

Comment 32

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

Comment on attachment 783620 [details] [diff] [review]
Part 2, handle delivery report, V2

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

::: dom/mobilemessage/src/ril/MmsService.js
@@ +1533,5 @@
> +    }
> +
> +    let headers = aMsg.headers;
> +    let envelopeId = headers["message-id"];
> +    let address = headers.to ? headers.to.address : null;

|headers.to| is a mandatory field in M-Delivery.ind PDU.  You can skip checking here.

@@ +1543,5 @@
> +
> +    if (!address) {
> +      if (DEBUG) debug("Address is invalid to update. Return.");
> +      return;
> +    }

ditto.

@@ +1554,5 @@
> +    } else if (mmsStatus === MMS.MMS_PDU_STATUS_DEFERRED) {
> +      deliverStatus = DELIVERY_STATUS_MANUAL;
> +    } else {
> +      if (DEBUG) debug("MMS status is invalid to update");
> +      return;

From OMA-TS-MMS_ENC-V1_3-20110913-A subclause 9.3 "X-Mms-Status", in the M-Delivery.ind the X-Mms-Status could be MMS.MMS_PDU_STATUS_{EXPIRED,RETRIEVED,REJECTED,DEFERRED,UNRECOGNIZED,INDETERMINATE,FORWARDED,UNREACHABLE}.

From bug 760065 attachment 732217 [details], we have delivery status values "success", "pending", "error" for sent messages with delivery status requisition, and "not-applicable" for those without delivery status requisition.

So, I think we should have:

  "success": RETRIEVED
  "error": EXPIRED, REJECTED, UNRECOGNIZED, UNREACHABLE
  "pending": DEFERRED
  "not-applicable": INDETERMINATE
  (ignored): FORWARDED or anything else.

@@ +1570,5 @@
> +
> +      // Notifying observers the delivery status is updated.
> +      Services.obs.notifyObservers(aDomMessage,
> +        deliverStatus === DELIVERY_STATUS_SUCCESS ?
> +          kSmsDeliverySuccessObserverTopic : kSmsDeliveryErrorObserverTopic,

Per offline discuss, let's notify kSmsDeliverySuccessObserverTopic only on DELIVERY_STATUS_SUCCESS and kSmsDeliveryErrorObserverTopic only on DELIVERY_STATUS_REJECTED.

Comment 33

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, provide and correct DB functions, V3

Comment 34

[Gene Lian (I already quit Mozilla)]

Attachment: Part 2, handle delivery report, V3

Comment 35

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

Comment on attachment 784315 [details] [diff] [review]
Part 1, provide and correct DB functions, V3

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

Thanks :)

Comment 36

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

Comment on attachment 784316 [details] [diff] [review]
Part 2, handle delivery report, V3

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1608,5 @@
> +        return;
> +      }
> +
> +      // Notifying observers the delivery status is updated.
> +      Services.obs.notifyObservers(aDomMessage, topic, null);      

trailing ws.

Comment 37

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/projects/birch/rev/0b43cdc93764
https://hg.mozilla.org/projects/birch/rev/c95c169d959e

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0b43cdc93764
https://hg.mozilla.org/mozilla-central/rev/c95c169d959e

Comment 39

[Gene Lian (I already quit Mozilla)]

Attachment: Part 3, new setting key

Please see bug 876746, comment #10. Gaia is hoping to merge SMS/MMS into one entry for delivery report.

Comment 40

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

Comment on attachment 786200 [details] [diff] [review]
Part 3, new setting key

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

Just like what we had discussed, MMS is quite different from SMS.  We provide two different keys so that content page may choose what fits it best.  Whether or not should MMS Delivery Indication be tied to SMS-DELIVER-REPORT is a policy to be decided in Gaia, not Gecko.  We don't commit policies into Gecko.  We commit only mechanisms.