849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

+++ This bug was initially created as a clone of Bug #844431 +++

After bug 844431 is done, we can have a |nsIDOMMobileMessageManager| interface to support firing an onreceived event whenever receiving an MMS notification. We also need to wait on the bug 845643 done.

Comment 2

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

Attachment: Deal with onreceived event v1

Comment 3

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

Attachment: Deal with onreceived event v1.1

Comment 4

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

Attachment: Deal with onreceived event v1.2

Comment 5

[Gene Lian (I already quit Mozilla)]

Comment on attachment 724828 [details] [diff] [review]
Deal with onreceived event v1.2

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

::: dom/mms/src/ril/MmsService.js
@@ +1028,5 @@
>            // for the resended notification indication.
>            return;
>          }
> +        // Notifing new comming notification indication through notifyObservers.
> +        let notifyMessage = this.createMmsMessageFromRecord(messageRecord);

Per off-line discussion, we need to be careful to use .createMmsMessageFromRecord(), which expects the content as a blob instead of a string. Two solutions:

1. Don't save a string for content into DB when receiving an MMS. Instead, always a blob.

2. During the sending process, we need to convert the blob to string to be saved into DB, when the content type is "text/...". If we do this, we need to restore the string back to a blob in the implementation of .createMmsMessageFromRecord().

@@ +1087,5 @@
>                  transaction.run();
>                  return;
>                }
> +              // Notifing new retrieved MMS message through notifyObservers.
> +              let retrievedMessage = this.createMmsMessageFromRecord(messageRecord);

ditto.

Comment 6

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

Attachment: Deal with onreceived event v1.3

Comment 7

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #6)
> Created attachment 725316 [details] [diff] [review]
> Deal with onreceived event v1.3

Modify according to Gene's comment.

Comment 8

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

Comment on attachment 725316 [details] [diff] [review]
Deal with onreceived event v1.3

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

Cancelling the review because the change in database service. Probably incorrect. :(

::: dom/mms/src/ril/MmsService.js
@@ +1027,5 @@
>            // response the notification. Hope the end user will clean some space
>            // for the resended notification indication.
>            return;
>          }
> +        // Notifing new comming notification indication through notifyObservers.

nit: new line

@@ +1086,5 @@
>                    transactionId, MMS.MMS_PDU_STATUS_DEFERRED, reportAllowed);
>                  transaction.run();
>                  return;
>                }
> +              // Notifing new retrieved MMS message through notifyObservers.

nit: new line

@@ +1089,5 @@
>                }
> +              // Notifing new retrieved MMS message through notifyObservers.
> +              let retrievedMessage = this.createMmsMessageFromRecord(messageRecord);
> +              Services.obs.notifyObservers(retrievedMessage, kMmsReceivedObserverTopic, null);
> +

remove this empty line, please

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +2292,5 @@
>          let octetArray = Octet.decodeMultiple(data, contentEnd);
>          let content = null;
>          if (octetArray) {
> +          content = new Blob([octetArray],
> +            {"type" : headers["content-type"].media});

feel like having:

  content = new Blob([octetArray],
                     { type : headers["content-type"].media});

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1112,5 @@
>      aMessage.readIndex = [FILTER_READ_UNREAD, timestamp];
>      if (aMessage.type == "sms") {
>        aMessage.delivery = DELIVERY_RECEIVED;
>      }
> +    aMessage.deliveryStatus = [DELIVERY_STATUS_NOT_APPLICABLE];

If this is the only change made to db service, then this patch would probably break the whole database. You should also modify setMessageDelivery(), createMessageFromRecord(), etc.

Comment 9

[Gene Lian (I already quit Mozilla)]

Don't need to call this.createMmsMessageFromRecord() anymore after Bug 847738 lands. Let's check in Bug 847738 first to make tasks simple here.

Comment 10

[Gene Lian (I already quit Mozilla)]

Comment on attachment 725316 [details] [diff] [review]
Deal with onreceived event v1.3

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

::: dom/mms/src/ril/MmsService.js
@@ +1014,5 @@
>  
>      notification = this.convertFromIntermediateToSavable(notification);
>  
>      gMobileMessageDatabaseService.saveReceivedMessage(notification,
>        (function (rv, messageRecord) {

s/messageRecord/domMessage

@@ +1029,5 @@
>            return;
>          }
> +        // Notifing new comming notification indication through notifyObservers.
> +        let notifyMessage = this.createMmsMessageFromRecord(messageRecord);
> +        Services.obs.notifyObservers(notifyMessage, kMmsReceivedObserverTopic, null);

Don't need to call this.createMmsMessageFromRecord() anymore. Just calling: 

  Services.obs.notifyObservers(domMessage, kMmsReceivedObserverTopic, null);

@@ +1072,5 @@
>            }
>  
>            messageRecord = this.mergeRetrievalConfirmationIntoRecord(retrievedMsg, messageRecord);
>            gMobileMessageDatabaseService.saveReceivedMessage(messageRecord,
>              (function (rv, messageRecord) {

ditto.

@@ +1088,5 @@
>                  return;
>                }
> +              // Notifing new retrieved MMS message through notifyObservers.
> +              let retrievedMessage = this.createMmsMessageFromRecord(messageRecord);
> +              Services.obs.notifyObservers(retrievedMessage, kMmsReceivedObserverTopic, null);

ditto.

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1112,5 @@
>      aMessage.readIndex = [FILTER_READ_UNREAD, timestamp];
>      if (aMessage.type == "sms") {
>        aMessage.delivery = DELIVERY_RECEIVED;
>      }
> +    aMessage.deliveryStatus = [DELIVERY_STATUS_NOT_APPLICABLE];

I don't think this is correct. We should save a single string for SMS's .deliveryStatus and save an array of strings for MMS's .deliveryStatus. Please refer to what I've done in the .saveSendingMessage().

I've already supported .setMessageDelivery() and .createDomMessageFromRecord() to properly interpret .deliveryStatus based on SMS/MMS branches. Don't need to worry about them I'd think. :)

Comment 11

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

Attachment: Deal with onreceived event v2.0

Comment 12

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

Attachment: Deal with onreceived event v2.0

Comment 13

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

Attachment: Deal with onreceived event v2.1

Comment 14

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

Attachment: Deal with onreceived event v2.1

Comment 15

[Gene Lian (I already quit Mozilla)]

Comment on attachment 725987 [details] [diff] [review]
Deal with onreceived event v2.0

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

::: dom/mms/src/ril/MmsService.js
@@ +1101,5 @@
>                  return;
>                }
> +
> +              // Notifing new retrieved MMS message through notifyObservers.
> +              Services.obs.notifyObservers(retrievedMessage, domMessage, null);

Oops, minor nit:

  Services.obs.notifyObservers(domMessage, kMmsReceivedObserverTopic, null);

Btw, I think you forget to remove another TODO comment above.

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +2292,5 @@
>          let octetArray = Octet.decodeMultiple(data, contentEnd);
>          let content = null;
>          if (octetArray) {
> +          content = new Blob([octetArray],
> +            {type : headers["content-type"].media});

Please align this as Vicamo suggested.

::: dom/mobilemessage/src/Constants.cpp
@@ +16,5 @@
>  
>  const char* kMmsSendingObserverTopic         = "mms-sending";
>  const char* kMmsSentObserverTopic            = "mms-sent";
>  const char* kMmsFailedObserverTopic          = "mms-failed";
> +const char* kMmsReceivedObserverTopic        = "mms-received";

Vicamo used to suggest we should share this with sms-received but not sure we should do that in this bug or address it in another one. I don't have strong opinion on that. Let's confirm with Vicamo.

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +461,5 @@
> +      NS_ERROR("Got a 'mms-received' topic without a valid message!");
> +      return NS_OK;
> +    }
> +
> +    DispatchTrustedMmsEventToSelf(FAILED_EVENT_NAME, message);

s/FAILED_EVENT_NAME/RECEIVED_EVENT_NAME

Comment 16

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

Attachment: Deal with onreceived event v2.2

Comment 17

[Gene Lian (I already quit Mozilla)]

Comment on attachment 725993 [details] [diff] [review]
Deal with onreceived event v2.2

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

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1171,5 @@
> +    // Set |aMessage.deliveryStatus|. Note that for MMS record
> +    // it must be an array of strings; For SMS, it's a string.
> +    let deliveryStatus = aMessage.deliveryStatusRequested
> +                       ? DELIVERY_STATUS_PENDING
> +                       : DELIVERY_STATUS_NOT_APPLICABLE;

Sorry, I missed this.

You don't need deliveryStatusRequested. That is for saveSendingMessage() only. Just directly using DELIVERY_STATUS_NOT_APPLICABLE.

@@ +1176,2 @@
>      if (aMessage.type == "sms") {
> +      aMessage.deliveryStatus = deliveryStatus;

aMessage.deliveryStatus = DELIVERY_STATUS_NOT_APPLICABLE;

@@ +1188,5 @@
> +        return;
> +      }
> +      aMessage.deliveryStatus = [];
> +      for (let i = 0; i < receivers.length; i++) {
> +        aMessage.deliveryStatus.push(deliveryStatus);

aMessage.deliveryStatus.push(DELIVERY_STATUS_NOT_APPLICABLE);

Comment 18

[Joe Cheng [:jcheng] (please needinfo)]

schung also need to do some work after this bug lands

Comment 19

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

Attachment: Deal with onreceived event v2.3

Comment 20

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

Attachment: Deal with onreceived event v2.3

Comment 21

[Gene Lian (I already quit Mozilla)]

Please see comment #17.

Comment 22

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

Attachment: Deal with onreceived event v2.4

Comment 23

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

Comment on attachment 726058 [details] [diff] [review]
Deal with onreceived event v2.4

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

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1135,5 @@
>      if (aMessage.type === undefined ||
>          aMessage.sender === undefined ||
>          (aMessage.type == "sms" && aMessage.messageClass === undefined) ||
>          (aMessage.type == "mms" && aMessage.delivery === undefined) ||
> +        (aMessage.type == "mms" && aMessage.deliveryStatus === undefined) ||

Please also help correct "===":

  if (aMessage.type == undefined ||
      aMessage.sender == undefined ||
      (aMessage.type == "sms" && aMessage.messageClass === undefined) ||
      (aMessage.type == "mms" &&
        (aMessage.delivery == undefined || aMessage.deliveryStatus == undefined)) ||
      aMessage.timestamp == undefined) {

Comment 24

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

Attachment: Deal with onreceived event v2.5

Comment 25

[Gene Lian (I already quit Mozilla)]

Conflicted with:

  const DEBUG = true;

Please provide a new patch and a=leo+. Thanks!

Comment 26

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

Attachment: Deal with onreceived event v2.6

Comment 27

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #25)
> Please provide a new patch and a=leo+. Thanks!
                                 Sorry, no this. Not yet leo+.

Comment 28

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48e028f2a5d7

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/48e028f2a5d7

Comment 30

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/0bc3eee6dfac

Comment 31

[Gene Lian (I already quit Mozilla)]

Attachment: [b2g18] Patch

Comment 32

[Gene Lian (I already quit Mozilla)]

Added [NO_UPLIFT] per recent commercial RIL compatibility issue. Waiting on further decision to keep the patch in b2g18 or to back it out.

------------------------------
If we really want to back them out, backing the following MMS bugs should be enough to make the commercial RIL compatible:

Bug 854422 - B2G MMS: should call .NotifyResponseTransaction() with MMS_PDU_STATUS_RETRIEVED after an MMS is retrieved under the RETRIEVAL_MODE_AUTOMATIC mode (a follow-up for bug 845643)
Bug 850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages
Bug 850530 - B2G MMS: Use the same attribute name for delivery (s/state/delivery) like SMS
Bug 852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments.
Bug 853725 - B2G MMS: fail to read nsIDOMMozMmsMessage.receivers for a received MMS (a follow-up of bug 849741).
Bug 853329 - B2G MMS: other Android phones cannot read attachments sent from FFOS
Bug 852471 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) (follow-up fix)
Bug 852460 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event (follow-up fix)
Bug 849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event
Bug 847756 - B2G MMS: provide nsIDOMMobileMessageManager.markMessageRead().
Bug 847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete().
Bug 847738 - B2G MMS: provide nsIDOMMobileMessageManager.getMessage(). 
Bug 844431 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)
Bug 845643 - B2G MMS: Save retrieved MMS into database.

Comment 33

[Gene Lian (I already quit Mozilla)]

Following the previous comment, some more needs to back out:

Bug 792321 - Check max values of MMS parameters in sendRequest.
Bug 833291 - B2G SMS & MMS: getMessages it's not working with PhoneNumberJS
Bug 844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS
Bug 839436 - B2G MMS: make DB be able to save MMS messages.

Comment 34

[Gene Lian (I already quit Mozilla)]

Confirming with Michael to see we should back out to Bug 839436 or just Bug 844431 (if we eventually decide to back out). Please see Bug 857632, comment #17.

Comment 35

[Anshul]

Please see bug 845643 comment #31.

Comment 36

[Gene Lian (I already quit Mozilla)]

Per off-line discussion with Michael, we decided not to back out the MMS bugs that have already been in mozilla-b2g18. Removing [NO_UPLIFT] to make the check-in status sync'ed.