921919 - B2G MMS: Notify Gaia SMS AP the MMS read report request and return the read result to the requester

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

As a receiver, FFOS is capable of returning MMS read report when the sender is requesting that.

Comment 1

[Gene Lian (I already quit Mozilla)]

IMO, we've already had markMessageRead() so we might not need new API.

Comment 2

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

Per talk with Gene, we still need to add an arguments in |markMessageRead| if we don't want to add a new API.

Comment 3

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

Attachment: bug-921919.patch v1.0

Comment 4

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

Attachment: bug-921919.patch v1.1

Comment 5

[Gene Lian (I already quit Mozilla)]

This final decision in agreement is:

DOMRequest markMessageRead(in long id, in boolean value
                           [optional] in boolean sendReadReport);

When |value| == TRUE && |sendReadReport| == TRUE, it will send the read port back to the requester in addition to marking the message to be read in the DB. If the parameter is not indicated, the send will apply the global setting.

Comment 6

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

We still need one more attribute in nsIDOMMozMmsMessage.idl.
The attribute is:
  readonly attribute boolean   isReadReportRequested;

We need this attribute to let gaia developer know is it necessary to ask end user return read report or not.

Comment 7

[Gene Lian (I already quit Mozilla)]

Thanks for catching this! Nice!

Comment 8

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

Attachment: bug-921919.patch v1.2

Comment 9

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

Comment on attachment 821496 [details] [diff] [review]
bug-921919.patch v1.2

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

::: dom/mobilemessage/interfaces/nsIMmsService.idl
@@ +21,5 @@
>    void retrieve(in long id,
>                  in nsIMobileMessageCallback request);
> +
> +  void sendReadReport(in DOMString messageID,
> +                      in jsval to);

Please use DOMString here.  Parse address type before sending read report out.

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +357,5 @@
>    NS_ENSURE_TRUE(mobileMessageDBService, NS_ERROR_FAILURE);
>  
>    nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner());
>    nsCOMPtr<nsIMobileMessageCallback> msgCallback = new MobileMessageCallback(request);
> +  nsresult rv = mobileMessageDBService->MarkMessageRead(aId, aValue, aSendReadReport, msgCallback);

nit: line wrap

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1206,5 @@
> +  // Mandatory fields
> +  headers["x-mms-message-type"] = MMS.MMS_PDU_TYPE_READ_REC_IND;
> +  headers["x-mms-mms-version"] = MMS.MMS_VERSION;
> +  headers["message-id"] = messageID;
> +  headers["to"] = to;

Parse |to| address here.

@@ +1335,5 @@
>        intermediate.sender = "anonymous";
>      }
>      intermediate.receivers = [];
>      intermediate.phoneNumber = this.getPhoneNumber();
> +    intermediate.sentReadReport = false;

Don't assign a default value in |convertIntermediateToSavable|.  It's for attributes critical to MobileMessageDatabaseService::saveReceivedMessage.  And since it has a default value, it's certainly not critical.  Please move this line into MobileMessageDatabaseService::saveReceivedMessage.  Besides, please also rename it to |isReadReportSent|.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +1140,5 @@
> +      }
> +
> +      let messageRecord = cursor.value;
> +      if (messageRecord.type == "sms") {
> +        messageRecord.sentReadReport = false;

Please update all "MMS" messages and set their 'isReadReportSent' attribute to false.

@@ +1208,5 @@
>          expiryDate = aMessageRecord.timestamp + headers["x-mms-expiry"] * 1000;
>        }
> +      let readReport = false;
> +      if (headers["x-mms-read-report"] != undefined) {
> +        readReport = headers["x-mms-read-report"];

'isReadReportRequested', and you can have:

  let isReadReportRequested = headers["x-mms-read-report"] || false;

@@ +1916,5 @@
>          (aMessage.type == "mms" && (aMessage.delivery == undefined ||
>                                      aMessage.transactionId == undefined ||
>                                      !Array.isArray(aMessage.deliveryStatus) ||
> +                                    !Array.isArray(aMessage.receivers) ||
> +                                    aMessage.sentReadReport == undefined)) ||

You don't need it.  Simply assign |aMessage.isReadReportSent| to false whenever it's a MMS.

@@ +2295,5 @@
>          }
>          messageRecord.read = value ? FILTER_READ_READ : FILTER_READ_UNREAD;
>          messageRecord.readIndex = [messageRecord.read, messageRecord.timestamp];
> +        if (messageRecord.type == "mms" && !messageRecord.sentReadReport &&
> +            sendReadReport) {

if (sendReadReport &&
    messageRecord.type == "mms" &&
    messageRecord.read == FILTER_READ_READ &&
    !messageRecord.isReadReportSend)

@@ +2299,5 @@
> +            sendReadReport) {
> +          messageRecord.sentReadReport = true;
> +          let messageID = messageRecord.headers["message-id"];
> +          let to = messageRecord.headers["from"];
> +          gMMSService.sendReadReport(messageID, to);

Please queue these things up and really send read report in:

  threadStore.put(threadRecord).onsuccess = function(event) {

Comment 10

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

Attachment: bug-921919.patch v1.3

Comment 11

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

Attachment: bug-921919.patch v1.4

Comment 12

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

Comment on attachment 823179 [details] [diff] [review]
bug-921919.patch v1.4

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1213,5 @@
> +  headers["to"] = to;
> +  headers["from"] = null;
> +  headers["x-mms-read-status"] = true;
> +
> +  this.istream = MMS.PduHelper.compose(null, {headers: headers});

throw |Cr.NS_ERROR_FAILURE| if |this.istream| is null and have a catch in MmsService::sendReadReport().

@@ +1222,5 @@
> +    if (callback) {
> +      requestCallback = function (httpStatus, data) {
> +        callback(httpStatus);
> +      };
> +    }

ReadRecTransaction::run is always called without a callback function.  So let skip this lines here.

@@ +2139,5 @@
>  
> +  sendReadReport: function sendReadReport(messageID, toAddress) {
> +    if (DEBUG) debug("messageID: " + messageID + " toAddress: " + JSON.stringify(toAddress));
> +    let readRecTransaction = new ReadRecTransaction(messageID, toAddress);
> +    readRecTransaction.run();

try {
  let transaction = new ReadRecTransaction(messageID, toAddress);
  transaction.run();
} catch (e) {}

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +2292,5 @@
>          }
>          messageRecord.read = value ? FILTER_READ_READ : FILTER_READ_UNREAD;
>          messageRecord.readIndex = [messageRecord.read, messageRecord.timestamp];
> +        let sendReadReportArguments = null;
> +        if (sendReadReport && 

nit: trailing ws

Comment 13

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

Attachment: bug-921919.patch v1.5

Fixed according to vyang's comments.

Comment 14

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

Attachment: bug-921919.patch v1.6

Rebased.

Comment 15

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

Attachment: bug-921919.patch v1.7

Rebased...

Comment 16

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

Attachment: bug-921919.patch v1.8

Remove empty line.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This has a lot of merge conflicts with b2g-inbound. What branch are you rebasing on top of?

Comment 18

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

Hi, Ryan,
I used b2g-inbound, but bug 887159 landed quicker than me.
I will rebase again.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> This has a lot of merge conflicts with b2g-inbound. What branch are you
> rebasing on top of?

Comment 19

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

Attachment: bug-921919.patch v1.9

Rebased on top of bug 866938 and bug 887159.

Comment 20

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

Comment on attachment 826582 [details] [diff] [review]
bug-921919.patch v1.9

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

Sorry, found some more things to be fixed before landing.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +237,4 @@
>              self.upgradeSchema15(event.target.transaction, next);
>              break;
>            case 16:
> +            if (DEBUG) debug("Upgrade to version 17. Add sentReadReport for incoming MMS.");

nit: isReadReportSent

@@ +1079,5 @@
>      };
>    },
>  
> +  /**
> +   * Add sentReadReport for incoming MMS.

ditto

@@ +2138,5 @@
> +          messageRecord.isReadReportSend = true;
> +          let messageID = messageRecord.headers["message-id"];
> +          let to = messageRecord.headers["from"];
> +          sendReadReportArguments = {messageID: messageID,
> +                                     toAddress : to.address}

Both MESSAGE-ID and FROM could be null because the former is a conditional parameter and the latter is an optional one.  Besides, we should have some more strict conditions that we should only send READ REPORT to retrieved MMS messages.

Comment 21

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

Attachment: bug-921919.patch v1.10

Thanks, vyang.

Comment 22

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

https://hg.mozilla.org/integration/b2g-inbound/rev/a876f43914f6

Comment 23

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

My bad, Backed out...
https://hg.mozilla.org/try/rev/b95c97c9617d

Full try:
https://tbpl.mozilla.org/?tree=Try&rev=1bd9c1f61911

Comment 24

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

Attachment: bug-921919.patch v1.11

Comment 25

[Carsten Book [:Tomcat]]

i had to back this out from b2g-inbound for build failure like https://tbpl.mozilla.org/php/getParsedLog.php?id=30064881&tree=B2g-Inbound

Comment 26

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

Thanks, sorry for the build failure.
(In reply to Carsten Book [:Tomcat] from comment #25)
> i had to back this out from b2g-inbound for build failure like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30064881&tree=B2g-Inbound

Comment 27

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

https://hg.mozilla.org/integration/b2g-inbound/rev/8e748d5a15ba

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e748d5a15ba

Comment 29

[Gene Lian (I already quit Mozilla)]

Although we've already had full discussions about the API changes to support read report, technically, we still need supuerreview+ before landing when it comes to DOM API changes. No worries. Just a reminder. :)

Comment 30

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

Thanks for reminder. Will be more careful next time. :)

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #29)
> Although we've already had full discussions about the API changes to support
> read report, technically, we still need supuerreview+ before landing when it
> comes to DOM API changes. No worries. Just a reminder. :)

Comment 31

[Andrew Overholt [:overholt]]

This landed before gecko 28 branched so will be in 1.3.