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

RESOLVED FIXED in Firefox 23

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ctai, Assigned: ctai)

Tracking

unspecified
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

(Whiteboard: [fixed-in-birch])

Attachments

(1 attachment, 12 obsolete attachments)

15.43 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
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.
Attachment #743946 - Flags: review?(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.
Attachment #743946 - Flags: review?(vyang) → review-
Summary: B2G MMS: Only send one multimedia message in automatic mode. → B2G MMS: Add more delivery status in delivery "not-downloaded" and send the dom message with right delivery status.
Whiteboard: [NO_UPLIFIT], [INTERFACE_CHANGE]

Comment 4

6 years ago
This change doesn't effect commercial RIL so removing NO_UPLIFT
Whiteboard: [NO_UPLIFIT], [INTERFACE_CHANGE] → [INTERFACE_CHANGE]
Created attachment 744312 [details] [diff] [review]
WIP v1.1
Attachment #743946 - Attachment is obsolete: true
Created attachment 744317 [details] [diff] [review]
WIP v1.2
Attachment #744312 - Attachment is obsolete: true
Blocks: 744684
Summary: B2G MMS: Add more delivery status in delivery "not-downloaded" and send the dom message with right delivery status. → B2G MMS: Add more delivery status for delivery state "not-downloaded" and send the dom message with right delivery status.
Created attachment 744703 [details] [diff] [review]
Patch v1.3
Attachment #744317 - Attachment is obsolete: true
Created attachment 744717 [details] [diff] [review]
Patch v1.4
Attachment #744703 - Attachment is obsolete: true
Attachment #744717 - Attachment is patch: true
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.
Attachment #744717 - Flags: feedback?(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().
Attachment #744717 - Flags: feedback?(vyang)
Created attachment 745952 [details] [diff] [review]
Patch v1.5
Attachment #744717 - Attachment is obsolete: true
Created attachment 745960 [details] [diff] [review]
Patch v1.6
Attachment #745952 - Attachment is obsolete: true
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.
Attachment #745960 - Flags: feedback?(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".
Attachment #745960 - Flags: feedback?(vyang)
Created attachment 746523 [details] [diff] [review]
Patch v1.7
Attachment #745960 - Attachment is obsolete: true
Comment on attachment 746523 [details] [diff] [review]
Patch v1.7

Deal with autohome & roamming.
Attachment #746523 - Flags: review?(vyang)
Blocks: 840089
No longer blocks: 840090
Blocks: 840087
No longer blocks: 840089
Blocks: 840090
No longer blocks: 840087
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).
Whiteboard: [INTERFACE_CHANGE]
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.
Attachment #746523 - Flags: review?(vyang)
Created attachment 747024 [details] [diff] [review]
Patch v1.8
Attachment #746523 - Attachment is obsolete: true
Attachment #747024 - Flags: review?(vyang)
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).
blocking-b2g: --- → leo?

Updated

6 years ago
Blocks: 868218
Attachment #747024 - Attachment is obsolete: true
Attachment #747024 - Flags: review?(vyang)
Comment on attachment 747156 [details] [diff] [review]
Patch v1.9

Add one line for updating the delivery status to pending when calling retriveMMS.
Attachment #747156 - Flags: review?
Attachment #747156 - Flags: review? → review?(vyang)
Blocks a blocker.
blocking-b2g: leo? → leo+
Whiteboard: [NO_UPLIFT]
Erp, Gecko. Can uplift as long as doesn't cause compat problems with existing SMS functionality.
Whiteboard: [NO_UPLIFT]
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.
Attachment #747156 - Flags: review?(vyang)
Created attachment 747631 [details] [diff] [review]
Patch v1.10
Attachment #747156 - Attachment is obsolete: true
Created attachment 747671 [details] [diff] [review]
Patch v1.11
Attachment #747631 - Attachment is obsolete: true
Attachment #747671 - Flags: review?(vyang)
Whiteboard: [NO_UPLIFT]
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 :)
Attachment #747671 - Flags: review?(vyang) → review+
Created attachment 748129 [details] [diff] [review]
Patch v1.12
Attachment #747671 - Attachment is obsolete: true
Keywords: checkin-needed
Anshul, Could you please check the NO_UPLIFT flag? This bug has big change after you check last time.
Flags: needinfo?(anshulj)
https://hg.mozilla.org/projects/birch/rev/4d6289816a95
Keywords: checkin-needed
Whiteboard: [NO_UPLIFT] → [NO_UPLIFT] [fixed-in-birch]
https://hg.mozilla.org/mozilla-central/rev/4d6289816a95
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Flags: needinfo?(anshulj)
Anshul, Could you please check the NO_UPLIFT flag? This bug has big change after you check last time. Thanks.
Flags: needinfo?(anshulj)
No any idl change, should remove NO_UPLIFT.
Flags: needinfo?(anshulj)
Whiteboard: [NO_UPLIFT] [fixed-in-birch] → [fixed-in-birch]
https://hg.mozilla.org/releases/mozilla-b2g18/rev/58e5ff2fcfce
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → wontfix
status-firefox23: --- → fixed

Updated

6 years ago
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.