B2G MMS: Save retrieved MMS into database.

RESOLVED FIXED in Firefox 22

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ctai, Assigned: ctai)

Tracking

unspecified
mozilla22
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

Attachments

(1 attachment, 12 obsolete attachments)

13.95 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
Assignee: nobody → ctai
blocking-b2g: --- → leo?
Depends on: 839436
Summary: B2G MMS: Save retrieved MM into database. → B2G MMS: Save retrieved MMS into database.
Created attachment 719795 [details] [diff] [review]
Save retrieved MMS to DB
Created attachment 719796 [details] [diff] [review]
Save retrieved MMS to DB
Attachment #719795 - Attachment is obsolete: true
Attachment #719796 - Flags: feedback?(vyang)
Whiteboard: [by 3/8]
Created attachment 720549 [details] [diff] [review]
WIP-Save received MMS
Attachment #719796 - Attachment is obsolete: true
Attachment #719796 - Flags: feedback?(vyang)
Attachment #720549 - Flags: feedback?(vyang)
Attachment #720549 - Flags: feedback?(gene.lian)
Comment on attachment 720549 [details] [diff] [review]
WIP-Save received MMS

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

Thank you :)

::: dom/mms/src/ril/MmsService.js
@@ +824,5 @@
>  
> +    notification.type = "mms";
> +    notifyMsg.pduType = "NOTIFICATION_IND";
> +    gMobileMessageDatabaseService.saveReceivedMessage(notifyMsg,
> +      function (rv, record) {

Use |messageRecord| instead.

@@ +850,5 @@
> +            // should not store into database.
> +            if (MMS.MMS_PDU_STATUS_RETRIEVED == mmsStatus) {
> +              retrievedMsg.type = "mms";
> +              retrievedMsg.pduType = "RETRIEVE_CONF";
> +              retrievedMsg.id = record.id;

for (let field in retrievedMsg.headers) {
    messageRecord.headers[field] = retrievedMsg.headers[field];
  }
  if (retrievedMsg.parts) {
    messageRecord.parts = retrievedMsg.parts;
  }
  if (retrievedMsg.content) {
    messageRecord.content = retrievedMsg.content;
  }

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +647,5 @@
>      return false;
>    },
>  
>    saveRecord: function saveRecord(aRecord, aCallback) {
> +    if (aMessage.pduType !== "RETRIEVE_CONF") {

if (aRecord.id === null) {
  // Assign a new id.
}

@@ +740,5 @@
>     * nsIRilMobileMessageDatabaseService API
>     */
>  
>    saveReceivedMessage: function saveReceivedMessage(aMessage, aCallback) {
> +    if (aMessage.type == "mms") {

Since we handle all MMS specific attributes in one if-block, let's open another interface method for saving mms stuff.

@@ +741,5 @@
>     */
>  
>    saveReceivedMessage: function saveReceivedMessage(aMessage, aCallback) {
> +    if (aMessage.type == "mms") {
> +      if (aMessage.timestamp === undefined) {

If we're going to create another interface method, then checkings here might also include "type", "sender", ...

@@ +748,5 @@
> +        }
> +        return;
> +      }
> +      aMessage.deliveryStatus = (aMessage.pduType == "NOTIFICATION_IND") ?
> +        DELIVERY_STATUS_PENDING:DELIVERY_STATUS_SUCCESS;

In MMS, deliveryStatus should be an array, which has the size of the number of receivers, which is one in this (received) case.

@@ +751,5 @@
> +      aMessage.deliveryStatus = (aMessage.pduType == "NOTIFICATION_IND") ?
> +        DELIVERY_STATUS_PENDING:DELIVERY_STATUS_SUCCESS;
> +      aMessage.sender = "";
> +      if (aMessage.headers && aMessage.headers.from && aMessage.headers.from.address) {
> +        aMessage.sender = aMessage.headers.from.address;

Fill necessary attributes outside first.  If header field 'From' is omitted, set to null.

@@ +755,5 @@
> +        aMessage.sender = aMessage.headers.from.address;
> +      }
> +      aMessage.receiver = "";
> +      if (aMessage.headers && aMessage.headers.to && aMessage.headers.to.address) {
> +        aMessage.receiver = aMessage.headers.from.address;

Fill necessary attributes outside first.  And, this attribute should be an array of combined values of To & Cc header fields.

@@ +757,5 @@
> +      aMessage.receiver = "";
> +      if (aMessage.headers && aMessage.headers.to && aMessage.headers.to.address) {
> +        aMessage.receiver = aMessage.headers.from.address;
> +      }
> +      aMessage.timestamp = (aMessage.pduType == "NOTIFICATION_IND") ?

Fill necessary attributes outside first.  Set to now at saving indication pdu.

@@ +760,5 @@
> +      }
> +      aMessage.timestamp = (aMessage.pduType == "NOTIFICATION_IND") ?
> +          new Date():aMessage.headers.date;
> +      // Adding needed indexes and extra attributes for internal use.
> +      aMessage.deliveryIndex = [DELIVERY_RECEIVED, aMessage.timestamp];

We need another DELIVERY_NOT_DOWNLOADED for MMS notification.

@@ +762,5 @@
> +          new Date():aMessage.headers.date;
> +      // Adding needed indexes and extra attributes for internal use.
> +      aMessage.deliveryIndex = [DELIVERY_RECEIVED, aMessage.timestamp];
> +      aMessage.numberIndex = [[aMessage.sender, aMessage.timestamp],
> +                              [aMessage.receiver, aMessage.timestamp]];

There could be multiple receivers.

@@ +763,5 @@
> +      // Adding needed indexes and extra attributes for internal use.
> +      aMessage.deliveryIndex = [DELIVERY_RECEIVED, aMessage.timestamp];
> +      aMessage.numberIndex = [[aMessage.sender, aMessage.timestamp],
> +                              [aMessage.receiver, aMessage.timestamp]];
> +      aMessage.readIndex = [FILTER_READ_UNREAD, aMessage.timestamp];

There could be multiple read status reports, one for each recipient. But maybe open as another bug :(

@@ +764,5 @@
> +      aMessage.deliveryIndex = [DELIVERY_RECEIVED, aMessage.timestamp];
> +      aMessage.numberIndex = [[aMessage.sender, aMessage.timestamp],
> +                              [aMessage.receiver, aMessage.timestamp]];
> +      aMessage.readIndex = [FILTER_READ_UNREAD, aMessage.timestamp];
> +      aMessage.delivery = DELIVERY_RECEIVED;

We need another DELIVERY_NOT_DOWNLOADED for MMS notification.
Attachment #720549 - Flags: feedback?(vyang)
Attachment #720549 - Flags: feedback?(gene.lian)
this is required to support MMS user stories. Leo+
blocking-b2g: leo? → leo+
Created attachment 722091 [details] [diff] [review]
Save retrieved MMS to DB
Attachment #720549 - Attachment is obsolete: true
Created attachment 722097 [details] [diff] [review]
Save retrieved MMS to DB
Attachment #722091 - Attachment is obsolete: true
Created attachment 722098 [details] [diff] [review]
Save retrieved MMS to DB
Attachment #722097 - Attachment is obsolete: true
Attachment #722098 - Flags: feedback?(vyang)
Attachment #722098 - Flags: feedback?(gene.lian)
Created attachment 722618 [details] [diff] [review]
Save retrieved MMS to DB
Attachment #722098 - Attachment is obsolete: true
Attachment #722098 - Flags: feedback?(vyang)
Attachment #722098 - Flags: feedback?(gene.lian)
Attachment #722618 - Flags: feedback?(vyang)
Comment on attachment 722618 [details] [diff] [review]
Save retrieved MMS to DB

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

Please rebase after bug 833291.
Attachment #722618 - Flags: feedback?(vyang)
Created attachment 722699 [details] [diff] [review]
Save retrieved MMS to DB
Attachment #722618 - Attachment is obsolete: true
Attachment #722699 - Flags: review?(vyang)
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #11)
> Created attachment 722699 [details] [diff] [review]
> Save retrieved MMS to DB

rebased patch.
expect to land on 3/11

Comment 14

6 years ago
Chia-Hung is implementing receiving part and Gene is implementing sending part.
No longer blocks: 849742
Comment on attachment 722699 [details] [diff] [review]
Save retrieved MMS to DB

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

::: dom/mms/src/ril/MmsService.js
@@ +827,5 @@
> +    notification.sender = null;
> +    if (notification.headers && notification.headers.from) {
> +      notification.sender = notification.headers.from;
> +    }
> +    notification.receiver = [];

Have a |convertFromIntermediateToSavable()|.

@@ +844,5 @@
> +          // Because MMSC will resend the notification indication once we don't
> +          // response the notification. Hope the end user will clean some space
> +          // for the resended notification indication.
> +          return;
> +        }

new line.

@@ +859,5 @@
> +
> +            // If the mmsStatus is still MMS_PDU_STATUS_DEFERRED after retry, we
> +            // should not store into database.
> +            if (MMS.MMS_PDU_STATUS_RETRIEVED == mmsStatus) {
> +              messageRecord.delivery = MMS.DELIVERY_RECEIVED;

Move all the converting process into |convertFromIntermediateToSavable()|

::: dom/mms/src/ril/mms_consts.js
@@ +98,5 @@
> +this.DELIVERY_RECEIVED = "received";
> +this.DELIVERY_NOT_DOWNLOADED = "not-downloaded";
> +this.DELIVERY_SENDING = "sending";
> +this.DELIVERY_SENT = "sent";
> +this.DELIVERY_ERROR = "error";

Please move to MmsService.
Attachment #722699 - Flags: review?(vyang)
Created attachment 723754 [details] [diff] [review]
Save retrieved MMS to DB
Attachment #722699 - Attachment is obsolete: true
Created attachment 723755 [details] [diff] [review]
Save retrieved MMS to DB
Attachment #723754 - Attachment is obsolete: true
Attachment #723755 - Flags: review?(vyang)
Comment on attachment 723755 [details] [diff] [review]
Save retrieved MMS to DB

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

Thank you :)

::: dom/mms/src/ril/MmsService.js
@@ +808,5 @@
> +   * @param pervousRecord
> +   *        Pervous record store in the database.
> +   */
> +  convertFromIntermediateToSavable: function convertFromIntermediateToSavable(intermediateMsg,
> +                                                                              pervousRecord) {

Spelling: previous

@@ +818,5 @@
> +    if (intermediateMsg.headers && intermediateMsg.headers.from) {
> +      intermediateMsg.sender = intermediateMsg.headers.from.address;
> +    }
> +    intermediateMsg.receivers = [];
> +    return intermediateMsg;

Indentation.

@@ +825,5 @@
> +    if (pervousRecord == null) {
> +      return null;
> +    }
> +    if (MMS.MMS_PDU_TYPE_RETRIEVE_CONF == intermediateMsg.headers["x-mms-message-type"]) {
> +      pervousRecord.delivery = DELIVERY_RECEIVED;

Let's have split retrieval confirmation handling into another function, named "mergeRetrievalConfirmationIntoRecord"?

@@ +859,5 @@
> +          }
> +        } else {
> +          pervousRecord.receivers.push(intermediateMsg.headers.cc.address);
> +        }
> +      }

for each (let type in ["cc", "to"]) {
  // ...
}

And please also have a comment why we ignore BCC here.

@@ +964,5 @@
> +            }
> +
> +            let transaction =
> +              new NotifyResponseTransaction(transactionId, mmsStatus, reportAllowed);
> +            transaction.run();

Please move these lines into:

  if (MMS.MMS_PDU_STATUS_RETRIEVED != mmsStatus) {
    ...
    return;
  }

  messageRecord = this.convertFromIntermediateToSavable(retrievedMsg, messageRecord);
  ...

@@ +967,5 @@
> +              new NotifyResponseTransaction(transactionId, mmsStatus, reportAllowed);
> +            transaction.run();
> +          }).bind(this));
> +          return;
> +        }

new line

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1087,5 @@
>      }
> +    if (aMessage.type == "sms") {
> +      aMessage.receiver = this.getRilIccInfoMsisdn();
> +    }
> +    if (aMessage.type == "mms" && aMessage.receivers.length == 0) {

|else if|

@@ +1104,4 @@
>      aMessage.deliveryStatus = DELIVERY_STATUS_SUCCESS;
>      aMessage.read = FILTER_READ_UNREAD;
>  
>      return this.saveRecord(aMessage, [aMessage.sender], aCallback);

We have to take care |[aMessage.sender]| here. Actually that array means "all participants other than myself". So, for SMS, it's |aMessage.sender| only; for MMS, it's |aMessage.sender| plus all receivers besides myself. You might have:

  let threadParticipants = [aMessage.sender];
  if (aMessage.type == "mms") {
    aMessage.receiver = this.getRilIccInfoMsisdn();
  } else if (aMessage.type == "mms") {
    let self = this.getRilIccInfoMsisdn();
    if (!aMessage.receivers.length) {
      if (self) {
        aMessage.receivers.push(self);
      }
    } else {
      let xxx = aMessage.receivers.slice();
      if (self) {
        xxx.splice(xxx.indexOf(self));
      }
      threadParticipants.concat(xxx);
    }
  }
Attachment #723755 - Flags: review?(vyang)
Created attachment 723799 [details] [diff] [review]
Save retrieved MMS to DB v3.0
Attachment #723755 - Attachment is obsolete: true
Attachment #723799 - Flags: review?(vyang)
Attachment #723799 - Flags: review?(vyang)
Created attachment 723804 [details] [diff] [review]
Save retrieved MMS to DB v3.0
Attachment #723799 - Attachment is obsolete: true
Attachment #723804 - Flags: review?(vyang)
Comment on attachment 723804 [details] [diff] [review]
Save retrieved MMS to DB v3.0

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

Great! Lunch time!

::: dom/mms/src/ril/MmsService.js
@@ +812,5 @@
> +    intermediate.timestamp = Date.now();
> +    intermediate.sender = null;
> +    if (intermediate.headers.from) {
> +      intermediate.sender = intermediate.headers.from.address;
> +    }

We may have undefined sender here.

@@ +828,5 @@
> +   */
> +  mergeRetrievalConfirmationIntoRecord: function mergeRetrievalConfirmationIntoRecord(intermediate, record) {
> +    if (record == null) {
> +      return null;
> +    }

Do we need it?

@@ +835,5 @@
> +      record.timestamp = Date.parse(intermediate.headers["Date"]);
> +    }
> +    if (intermediate.headers.from) {
> +      record.sender = intermediate.headers.from.address;
> +    }

Possible undefined sender, too.

@@ +855,5 @@
> +    for (let field in intermediate.headers) {
> +      record.headers[field] = intermediate.headers[field];
> +    }
> +    if (intermediate.parts) {
> +    record.parts = intermediate.parts;

Indentation.

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1096,5 @@
> +        }
> +      } else {
> +        let slicedReceivers = aMessage.receivers.slice();
> +        if (self && (slicedReceivers.indexOf(self) !== -1)) {
> +          slicedReceivers.splice(slicedReceivers.indexOf(self), 1);

cache |slicedReceivers.indexOf(self)|.
Attachment #723804 - Flags: review?(vyang) → review+
Created attachment 723813 [details] [diff] [review]
Save retrieved MMS to DB v3.1
Attachment #723804 - Attachment is obsolete: true
Keywords: checkin-needed
Please remove checkin-needed when pushing to inbound.
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> Please remove checkin-needed when pushing to inbound.

Sorry for that. Just forgot :(
https://hg.mozilla.org/mozilla-central/rev/90fbe56a05b1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
https://hg.mozilla.org/releases/mozilla-b2g18/rev/baaa721f947f
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → fixed
Depends on: 854422
Whiteboard: [by 3/8] → [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.

Comment 31

5 years ago
Gene, no need to back out Bug 839436 or just Bug 844431 as commercial RIL is not compatible with the SMS interfaces changes.
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]

Updated

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