845643 - B2G MMS: Save retrieved MMS into database.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

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

Attachment: Save retrieved MMS to DB

Comment 2

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

Attachment: Save retrieved MMS to DB

Comment 3

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

Attachment: WIP-Save received MMS

Comment 4

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

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.

Comment 5

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

this is required to support MMS user stories. Leo+

Comment 6

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

Attachment: Save retrieved MMS to DB

Comment 7

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

Attachment: Save retrieved MMS to DB

Comment 8

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

Attachment: Save retrieved MMS to DB

Comment 9

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

Attachment: Save retrieved MMS to DB

Comment 10

[Vicamo Yang [:vicamo][: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.

Comment 11

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

Attachment: Save retrieved MMS to DB

Comment 12

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

(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.

Comment 13

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

expect to land on 3/11

Comment 14

[Kai-Chih Hu [:khu]]

Chia-Hung is implementing receiving part and Gene is implementing sending part.

Comment 15

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

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.

Comment 16

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

Attachment: Save retrieved MMS to DB

Comment 17

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

Attachment: Save retrieved MMS to DB

Comment 18

[Vicamo Yang [:vicamo][: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);
    }
  }

Comment 19

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

Attachment: Save retrieved MMS to DB v3.0

Comment 20

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

Attachment: Save retrieved MMS to DB v3.0

Comment 21

[Vicamo Yang [:vicamo][: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)|.

Comment 22

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

Attachment: Save retrieved MMS to DB v3.1

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/90fbe56a05b1

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Please remove checkin-needed when pushing to inbound.

Comment 25

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> Please remove checkin-needed when pushing to inbound.

Sorry for that. Just forgot :(

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90fbe56a05b1

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/baaa721f947f

Comment 28

[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 29

[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 30

[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 31

[Anshul]

Gene, no need to back out Bug 839436 or just Bug 844431 as commercial RIL is not compatible with the SMS interfaces changes.

Comment 32

[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.