Closed Bug 849741 Opened 11 years ago Closed 11 years ago

B2G MMS: provide nsIDOMMobileMessageManager.onreceived event

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22
blocking-b2g leo+
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: airpingu, Assigned: ctai)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 13 obsolete files)

13.78 KB, patch
Details | Diff | Splinter Review
13.79 KB, patch
Details | Diff | Splinter Review
+++ 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.
Attached patch Deal with onreceived event v1.1 (obsolete) — Splinter Review
Attachment #724825 - Attachment is obsolete: true
Attached patch Deal with onreceived event v1.2 (obsolete) — Splinter Review
Attachment #724827 - Attachment is obsolete: true
Attachment #724828 - Flags: review?(vyang)
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.
Blocks: 845173
Attached patch Deal with onreceived event v1.3 (obsolete) — Splinter Review
Attachment #724828 - Attachment is obsolete: true
Attachment #724828 - Flags: review?(vyang)
(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.
Attachment #725316 - Flags: review?(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.
Attachment #725316 - Flags: review?(vyang)
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 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. :)
Attached patch Deal with onreceived event v2.0 (obsolete) — Splinter Review
Attachment #725316 - Attachment is obsolete: true
Attachment #725987 - Flags: feedback?(gene.lian)
Attached patch Deal with onreceived event v2.0 (obsolete) — Splinter Review
Attachment #725987 - Attachment is obsolete: true
Attachment #725987 - Flags: feedback?(gene.lian)
Attached patch Deal with onreceived event v2.1 (obsolete) — Splinter Review
Attachment #725988 - Attachment is obsolete: true
Attachment #725989 - Flags: feedback?(gene.lian)
Attached patch Deal with onreceived event v2.1 (obsolete) — Splinter Review
Attachment #725989 - Attachment is obsolete: true
Attachment #725989 - Flags: feedback?(gene.lian)
Attachment #725990 - Flags: feedback?(gene.lian)
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
Attachment #725987 - Flags: feedback+
Attached patch Deal with onreceived event v2.2 (obsolete) — Splinter Review
Attachment #725990 - Attachment is obsolete: true
Attachment #725990 - Flags: feedback?(gene.lian)
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);
Attachment #725993 - Flags: feedback+
schung also need to do some work after this bug lands
Whiteboard: [target 3/22]
Attached patch Deal with onreceived event v2.3 (obsolete) — Splinter Review
Attachment #725993 - Attachment is obsolete: true
Attached patch Deal with onreceived event v2.3 (obsolete) — Splinter Review
Attachment #726017 - Attachment is obsolete: true
Attachment #726028 - Flags: feedback?(schung)
Please see comment #17.
Attached patch Deal with onreceived event v2.4 (obsolete) — Splinter Review
Attachment #726028 - Attachment is obsolete: true
Attachment #726028 - Flags: feedback?(schung)
Attachment #726058 - Flags: feedback?(schung)
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) {
Attachment #726058 - Flags: feedback?(schung) → review+
Attached patch Deal with onreceived event v2.5 (obsolete) — Splinter Review
Attachment #726058 - Attachment is obsolete: true
Conflicted with:

  const DEBUG = true;

Please provide a new patch and a=leo+. Thanks!
Keywords: checkin-needed
Attachment #726077 - Attachment is obsolete: true
(In reply to Gene Lian [:gene] from comment #25)
> Please provide a new patch and a=leo+. Thanks!
                                 Sorry, no this. Not yet leo+.
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/48e028f2a5d7
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 852460
Depends on: 852911
blocking-b2g: leo? → leo+
Depends on: 853725
Depends on: 854422
No longer depends on: 854422
Whiteboard: [target 3/22] → [NO_UPLIFT]
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.
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.
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.
Please see bug 845643 comment #31.
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.
Whiteboard: [NO_UPLIFT]
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: