867440 - B2G MMS: Add more delivery status for delivery state "not-downloaded" and send the dom message with right delivery status.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

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

In description of bug 840090.
"User Story:
As a user who has automatic downloads enabled, I will only be notified of a new MMS message after the entire message has been downloaded to my device to ensure that all content is available for viewing when I read the message."

We should only notify one multimedia message in automatic mode.

Comment 2

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

Attachment: Patch v1.0

Comment 3

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

Comment on attachment 743946 [details] [diff] [review]
Patch v1.0

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

It's true the User Story requires us to "show" only one notification to content when we're in automatic retrieval mode, but it doesn't follow that we should "emit" only one event to content. The reason we have to provide two events for MMS is to let Gaia know there is a new record in database. In other words, we try to acknowledge API users whenever a change made into database through either DOMRequest results or EventTargets.

And we have also a "never" and a "automatic-home" retrieval mode which are not taken into account in this patch. I suggest we add following new delivery/deliveryStatus pairs for MMS:

    +==================+=================================+
    | delivery         | delivery status                 |
    +==================+=================================+
    | "not-downloaded" | "rejected", "expired", "manual" |
    +------------------+---------------------------------+

So that "pending" always means "Gecko has started a retrieval transaction for the message notification." And Gaia skips onreceived events that has |deliveryStatus| set to "pending". This way, 1) we have a more clear delivery status for all "not-downloaded" MMS messages, 2) we still emit onreceived events for all incoming messages so cases under "never" and "automatic-home" are also covered, 3) we can proceed to implement onretrieving event (bug 810099) once it's necessary.

Comment 4

[Anshul]

This change doesn't effect commercial RIL so removing NO_UPLIFT

Comment 5

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

Attachment: WIP v1.1

Comment 6

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

Attachment: WIP v1.2

Comment 7

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

Attachment: Patch v1.3

Comment 8

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

Attachment: Patch v1.4

Comment 9

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

Comment on attachment 744717 [details] [diff] [review]
Patch v1.4

Based on the suggestion in comment 3, add more delivery status. Cover the situations including sending failure, expired, manual, automatic.

Comment 10

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

Comment on attachment 744717 [details] [diff] [review]
Patch v1.4

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

::: dom/mms/src/ril/MmsService.js
@@ +1468,5 @@
> +          .updateDeliveryStatus(id,
> +                                DELIVERY_STATUS_EXPIRED,
> +                                (function (rv, domMessage) {
> +          this.broadcastReceivedMessageEvent(domMessage);
> +          }).bind(this));

The message here is neither a new notification nor a just fetched message. You don't have to broadcast the state change. Instead, set appropriate deliveryStatus in convertIntermediateToSavable().

::: dom/mobilemessage/interfaces/nsIRilMobileMessageDatabaseService.idl
@@ +96,5 @@
> +   * |aMessageId| Number: the message's DB record ID.
> +   * |aDeliveryStatus| DOMString: the new delivery status to update (can be null).
> +   * |aCallback| nsIRilMobileMessageDatabaseCallback: an optional callback.
> +   */
> +  void updateDeliveryStatus(in long aMessageId,

We have already setMessageDelivery().

Comment 11

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

Attachment: Patch v1.5

Comment 12

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

Attachment: Patch v1.6

Comment 13

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

Comment on attachment 745960 [details] [diff] [review]
Patch v1.6

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

::: dom/mms/src/ril/MmsService.js
@@ +1152,5 @@
> +                                                       null,
> +                                                       null,
> +                                                       DELIVERY_STATUS_ERROR,
> +                                                       (function (rv, domMessage) {
> +        this.broadcastReceivedMessageEvent(domMessage);

Need broadcast the error domMessage in the situation of auto-retrieving on but can't retrieve it. Or the icon of getting message will spin forever.

Comment 14

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

Comment on attachment 745960 [details] [diff] [review]
Patch v1.6

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

Other parts looks fine to me. But still have to deal with automatic-home & roaming.

::: dom/mms/src/ril/MmsService.js
@@ +980,5 @@
> +        intermediate.deliveryStatus = [DELIVERY_STATUS_REJECTED];
> +        break;
> +      case RETRIEVAL_MODE_AUTOMATIC:
> +      case RETRIEVAL_MODE_AUTOMATIC_HOME:
> +        intermediate.deliveryStatus = [DELIVERY_STATUS_PENDING];

When we're in "automatic-home" retrieval mode, we actually skip message retrieval and NotifyResp when we're also in roaming. In this situation, the deliveryStatus should also be "manual".

Comment 15

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

Attachment: Patch v1.7

Comment 16

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

Comment on attachment 746523 [details] [diff] [review]
Patch v1.7

Deal with autohome & roamming.

Comment 17

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

This bug fixes the situation "When the auto retrieve on, sometimes we can't download the message because of server problem. Before that bug, we don't notify Messaging AP in this situation." So I nominate this bug because of blocking leo+ bug(840090).

Comment 18

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

Comment on attachment 746523 [details] [diff] [review]
Patch v1.7

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

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +155,5 @@
> +      status = eDeliveryStatus_Error;
> +    } else if (statusStr.Equals(DELIVERY_STATUS_EXPIRED)) {
> +      status = eDeliveryStatus_Error;
> +    } else if (statusStr.Equals(DELIVERY_STATUS_MANUAL)) {
> +      status = eDeliveryStatus_Error;

I ran into a problem in handling expired messages and found the statuses are all assigned to eDeliveryStatus_Error here. A few other places like MmsMessage::GetDeliveryStatus, |enum DeliveryStatus| in Types.h have to accommodate to these changes as well.

Comment 19

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

Attachment: Patch v1.8

Comment 20

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

This bug fixes the situation "When the auto retrieve on, sometimes we can't download the message because of server problem. Before that bug, we don't notify Messaging AP in this situation." So I nominate this bug because of blocking leo+ bug(840090).

Comment 21

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

Attachment: Patch v1.9

Comment 22

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

Comment on attachment 747156 [details] [diff] [review]
Patch v1.9

Add one line for updating the delivery status to pending when calling retriveMMS.

Comment 23

[Dietrich Ayala (:dietrich)]

Blocks a blocker.

Comment 24

[Dietrich Ayala (:dietrich)]

Erp, Gecko. Can uplift as long as doesn't cause compat problems with existing SMS functionality.

Comment 25

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

Comment on attachment 747156 [details] [diff] [review]
Patch v1.9

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

That "expired" thing seems to cause more problems. Correcting expired delivery status at retrieving a message raise a problem that "the fact that message is expired" and "the value of delivery status" may not always match. A more complete solution would be checking every message record retrieval in MobileMessageDatabaseService, but that's so ugly and would just probably mess up the database.

Indeed there is something I had in mind for the expiryDate feature. Actually the spec doesn't forbid you from trying to retrieve a already expired message because there is no way to know whether a message is really expired and removed on MMSC. Should we really return an error or correct delivery status when Gecko found a message seems to be expired? We have had expiryDate exported to content. Such policy can be easily implemented in Gaia if UX want it.

So, back to original problem, we don't really need a "expired" delivery status. Thank you for digging out this problem. Without correcting it in retrieve(), we keep the original delivery status and returns an error.

::: dom/mms/src/ril/MmsService.js
@@ +62,5 @@
> +const DELIVERY_STATUS_SUCCESS  = "success";
> +const DELIVERY_STATUS_PENDING  = "pending";
> +const DELIVERY_STATUS_ERROR    = "error";
> +const DELIVERY_STATUS_REJECTED = "rejected";
> +const DELIVERY_STATUS_EXPIRED  = "expired";

Please remove DELIVERY_STATUS_EXPIRED.

@@ +1500,5 @@
> +        gMobileMessageDatabaseService.setMessageDelivery(id,
> +                                                         null,
> +                                                         null,
> +                                                         DELIVERY_STATUS_EXPIRED,
> +                                                         null);

Ditto.

@@ +1518,1 @@
>        this.retrieveMessage(url, (function responseNotify(mmsStatus, retrievedMsg) {

Should make |this.retrieveMessage()| in the callback of |setMessageDelivery()|.

@@ +1519,5 @@
>          // If the mmsStatus is still MMS_PDU_STATUS_DEFERRED after retry,
>          // we should not store it into database.
>          if (MMS.MMS_PDU_STATUS_RETRIEVED !== mmsStatus) {
>            if (DEBUG) debug("RetrieveMessage fail after retry.");
>            aRequest.notifyGetMessageFailed(Ci.nsIMobileMessageCallback.INTERNAL_ERROR);

Have to setMessageDelivery(DELIVERY_STATUS_ERROR);

::: dom/mobilemessage/src/Constants.h
@@ +34,5 @@
>  #define DELIVERY_STATUS_PENDING        NS_LITERAL_STRING("pending")
>  #define DELIVERY_STATUS_ERROR          NS_LITERAL_STRING("error")
> +#define DELIVERY_STATUS_REJECTED       NS_LITERAL_STRING("rejected")
> +#define DELIVERY_STATUS_MANUAL         NS_LITERAL_STRING("manual")
> +#define DELIVERY_STATUS_EXPIRED        NS_LITERAL_STRING("expired")

ditto

::: dom/mobilemessage/src/MmsMessage.cpp
@@ +157,5 @@
>        status = eDeliveryStatus_Error;
> +    } else if (statusStr.Equals(DELIVERY_STATUS_REJECTED)) {
> +      status = eDeliveryStatus_Reject;
> +    } else if (statusStr.Equals(DELIVERY_STATUS_EXPIRED)) {
> +      status = eDeliveryStatus_Expired;

ditto

::: dom/mobilemessage/src/Types.h
@@ +32,5 @@
>    eDeliveryStatus_Success,
>    eDeliveryStatus_Pending,
>    eDeliveryStatus_Error,
> +  eDeliveryStatus_Reject,
> +  eDeliveryStatus_Expired,

ditto.

Comment 26

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

Attachment: Patch v1.10

Comment 27

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

Attachment: Patch v1.11

Comment 28

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

Comment on attachment 747671 [details] [diff] [review]
Patch v1.11

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

We still have to make sure retrieve() only applies to messages that 1) is a mms, 2) delivery state is "not-downloaded", 3) delivery status is not "pending".  We have already 1), but with this patch we can enforce 2) and 3).  Thank you :)

Comment 29

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

Attachment: Patch v1.12

Comment 30

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

Anshul, Could you please check the NO_UPLIFT flag? This bug has big change after you check last time.

Comment 31

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/projects/birch/rev/4d6289816a95

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4d6289816a95

Comment 33

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

Anshul, Could you please check the NO_UPLIFT flag? This bug has big change after you check last time. Thanks.

Comment 34

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

No any idl change, should remove NO_UPLIFT.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/58e5ff2fcfce