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

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ctai, Assigned: ctai)

Tracking

({dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

43.81 KB, patch
Details | Diff | Splinter Review
As a receiver, FFOS is capable of returning MMS read report when the sender is requesting that.
IMO, we've already had markMessageRead() so we might not need new API.
Summary: B2G MMS: Notify Gaia SMS AP the MMS read report request when getting a MMS and open an API for return the result. → B2G MMS: Notify Gaia SMS AP the MMS read report request and return the read result to the requester
Assignee: nobody → ctai
Per talk with Gene, we still need to add an arguments in |markMessageRead| if we don't want to add a new API.
blocking-b2g: --- → 1.3?
Created attachment 812935 [details] [diff] [review]
bug-921919.patch v1.0
Created attachment 813424 [details] [diff] [review]
bug-921919.patch v1.1
Attachment #812935 - Attachment is obsolete: true
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.
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.
Thanks for catching this! Nice!
Created attachment 821496 [details] [diff] [review]
bug-921919.patch v1.2
Attachment #813424 - Attachment is obsolete: true
Attachment #821496 - Flags: feedback?(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) {
Attachment #821496 - Flags: feedback?(vyang)
Created attachment 823178 [details] [diff] [review]
bug-921919.patch v1.3
Attachment #821496 - Attachment is obsolete: true
Created attachment 823179 [details] [diff] [review]
bug-921919.patch v1.4
Attachment #823178 - Attachment is obsolete: true
Attachment #823179 - Flags: review?(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
Attachment #823179 - Flags: review?(vyang) → review+
Created attachment 823847 [details] [diff] [review]
bug-921919.patch v1.5

Fixed according to vyang's comments.
Attachment #823179 - Attachment is obsolete: true
Keywords: dev-doc-needed
Created attachment 824432 [details] [diff] [review]
bug-921919.patch v1.6

Rebased.
Attachment #823847 - Attachment is obsolete: true
Created attachment 825754 [details] [diff] [review]
bug-921919.patch v1.7

Rebased...
Attachment #824432 - Attachment is obsolete: true
Keywords: checkin-needed
Created attachment 825784 [details] [diff] [review]
bug-921919.patch v1.8

Remove empty line.
Attachment #825754 - Attachment is obsolete: true
This has a lot of merge conflicts with b2g-inbound. What branch are you rebasing on top of?
Keywords: checkin-needed
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?
Created attachment 826582 [details] [diff] [review]
bug-921919.patch v1.9

Rebased on top of bug 866938 and bug 887159.
Attachment #825784 - Attachment is obsolete: true
Attachment #826582 - Flags: review?(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.
Attachment #826582 - Flags: review?(vyang)
Created attachment 826611 [details] [diff] [review]
bug-921919.patch v1.10

Thanks, vyang.
Attachment #826582 - Attachment is obsolete: true
Attachment #826611 - Flags: review?(vyang)
Attachment #826611 - Flags: review?(vyang) → review+
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Created attachment 826635 [details] [diff] [review]
bug-921919.patch v1.11
Attachment #826611 - Attachment is obsolete: true
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
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
https://hg.mozilla.org/mozilla-central/rev/8e748d5a15ba
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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. :)
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. :)
Depends on: 942009
This landed before gecko 28 branched so will be in 1.3.
blocking-b2g: 1.3? → ---
You need to log in before you can comment on or make changes to this bug.