736697 - B2G SMS: Support SMS-STATUS-REPORT

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

According to 3GPP 23.040 section 3.2.9, "[The status report] offers to the SC the capabilities of informing the MS of the status of a previously sent mobile originated short message."

This feature is not yet implemented in b2g. We might reuse existing code in GsmPDUHelper.readMessage(), and also remove unreachable code regarding to SMS-SUBMIT (message reference, validity period are only appear in SMS-SUBMIT pdu).

Comment 1

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

Attachment: Part 1: Support SMS-STATUS-REPORT PDU: WIP

Comment 2

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

Attachment: Part 2: Request status report at SMS-SUBMIT : WIP

Comment 3

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

Attachment: Part 1: Remove SMS-SUBMIT-only property from SMS-DELIVER

Comment 4

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

Attachment: Part 2: Refactor to share methods

Comment 5

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

Attachment: Part 3: support SMS-STATUS-REPORT

Comment 6

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

Attachment: Part 4: handle SMS-STATUS-REPORT

Comment 7

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

Attachment: Part 5: add error handling

Comment 8

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

Attachment: DNS: debug only, for enable status report request in every SMS-SUBMIT

Comment 9

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

Attachment: Part 3: support SMS-STATUS-REPORT : V2

update comments in GsmPDUHelper.readMessage() only

Comment 10

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

verified with rev 9ff494dfc9b0 ;)

Comment 11

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

Comment on attachment 607489 [details] [diff] [review]
Part 3: support SMS-STATUS-REPORT : V2

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

::: dom/system/gonk/ril_worker.js
@@ +2806,5 @@
>     */
>    readMessage: function readMessage() {
>      // An empty message object. This gets filled below and then returned.
>      let msg = {
> +      /* D:DELIVER, DR:DELIVER-REPORT, S:SUBMIT, SR:SUBMIT-REPORT, ST:STATUS-REPORT

C:?

Comment 12

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

Comment on attachment 607486 [details] [diff] [review]
Part 4: handle SMS-STATUS-REPORT

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

::: dom/system/gonk/ril_worker.js
@@ +611,5 @@
>     */
>    _receivedSmsSegmentsMap: {},
>  
>    /**
> +   */

Documentation missed

@@ +1470,5 @@
>  
>      return options;
>    },
>  
> +  _processSentSmsSegment: function _processSentSmsSegment(options) {

Ditto.

Comment 13

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

Comment on attachment 607487 [details] [diff] [review]
Part 5: add error handling

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

::: dom/system/gonk/ril_worker.js
@@ +2096,5 @@
> +
> +  // Completed
> +  if ((status >>> 5) == 0x00) {
> +    this._processSentSmsSegment(options);
> +  }

Better have another line for "TODO: bug 727319 - Notify SMS send failures"

Comment 14

[Philipp von Weitershausen [:philikon]]

Thanks for those patches, Vicamo. I'll be afk for a few days, so I'll get to those reviews next week.

Comment 15

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

Attachment: Part 3: support SMS-STATUS-REPORT : V3

review comment #11

Comment 16

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

Attachment: Part 4: handle SMS-STATUS-REPORT : V2

review comment #12

Comment 17

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

Attachment: Part 5: add error handling : V2

review comment #13

Comment 18

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

Attachment: Part 4: handle SMS-STATUS-REPORT : V3

Add sms-delivered notification.

The behavior becomes:

0) send 1st segment, wait for rild response
1-1) get rild response, wait for SMSC status report
1-2) get SMSC status report, send 2nd segment, wait for rild response
2-1) get rild response, wait for SMSC status report
2-2) get SMSC status report, send 3rd segment, wait for rild response
...
n-1) get rild response, notify sms-sent, wait for SMSC status report
n-2) get SMSC status report, notify sms-delivered

The problem is I can't get message id in `RadioInterfaceLayer.handleSmsDelivered()`. In `RadioInterfaceLayer.handleSmsSent()`, the previous allocated message id was discard and there is no way to retrieve it back. I have to manually manage the mapping from requestId to messageId, but I don't really think it a good idea.

Comment 19

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

Attachment: Part 5: add error handling : V3

rebase to attachment 608665 [details] [diff] [review]

Comment 20

[Mounir Lamouri (:mounir)]

Vicamo, I'm not familiar at all with that code. What do you want me to give a feedback on?

Comment 21

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

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #20)
> Vicamo, I'm not familiar at all with that code. What do you want me to give
> a feedback on?

It's about SMS database.

I support the new handleSmsDelivered() should looks like:

  handleSmsDelivered: function handleSmsDelivered() {
    let id = HOW_DO_I_RETRIEVE_MESSAGE_ID_HERE;
    let sms = gSmsService.createSmsMessage(id,
                                           DOM_SMS_DELIVERY_SENT,
                                           message.sender || null,
                                           message.receiver || null,
                                           message.fullBody || null,
                                           message.timestamp);
    Services.obs.notifyObservers(sms, kSmsDeliveredObserverTopic, null);
  }

The RadioInterfaceLayer.handleSmsSent() creates a temporary SmsMessage object using a message id which is returned from saving the sent outgoing message into SMS database.

  handleSmsSent: function handleSmsSent() {
    let id = gSmsDatabaseService.saveSentMessage(...);
    let sms = gSmsService.createSmsMessage(id, ...);
    gSmsRequestManager.notifySmsSent(message.requestId, sms);
  }

I want do add sms-delivered notification in this issue, patch part 4. I'll have to create an temporary SmsMessage in order to invoke the notifyObservers() API. But I can't have the correct message-id as the one got in handleSmsSent(), which is a local variable. The only way to have a message-id is saving sent/received SMS message into database, but I don't want it. The sms-delivered notification is for the same SmsMessage that used to send sms-sent notification. Besides, the `message` variables in handleSmsXxxx functions are all temporary variables constructed by deserializing DOMMessage post from ril_worker. Saving `id` as a property of `message` in handleSmsSent() is meaningless.

I'm wondering whether should we move database operations up to SmsService or somewhere knows requestId <--> messageId mapping. There are still more error notifications to be sent in the future, and all of them will also have this problem. You may want to save a fail-to-send message as draft. When you want to send it again, you won't want to duplicate it, but instead, all errors thereafter should be associated with the draft message saved in previous failed sending attempt. Let RadioInterfaceLayer notify events, except sms-received, with requestId alone.

Comment 22

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

(In reply to Vicamo Yang [:vicamo] from comment #21)
> I support the new handleSmsDelivered() should looks like:

s/support/suppose/

Comment 23

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

Attachment: Part 2: Refactor to share methods : V2

Cache dcs property. It will be referenced many times in bug 736706 and/or other bugs.

Comment 24

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

Attachment: Part 2: Refactor to share methods : V3

Let process helper decide what ACK to reply

Comment 25

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

Attachment: Part 3: support SMS-STATUS-REPORT : V4

rebase due to changes in attachment #609252 [details] [diff] [review]

Comment 26

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

Attachment: Part 4: handle SMS-STATUS-REPORT : V4

1) rebase due to changes in attachment #609252 [details] [diff] [review]
2) rename `_sentPendingSms` to `_pendingSentSmsMap`

Comment 27

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

Attachment: Part 5: add error handling : V4

rebase due to changes in attachment #609252 [details] [diff] [review]

Comment 28

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

Comment on attachment 609252 [details] [diff] [review]
Part 2: Refactor to share methods : V3

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

::: dom/system/gonk/ril_consts.js
@@ +258,5 @@
>  const CALL_PRESENTATION_UNKNOWN = 2;
>  const CALL_PRESENTATION_PAYPHONE = 3;
>  
>  const SMS_HANDLED = 0;
> +const SMS_ERROR   = 1;

This should be "Unspecified error cause(0xFF)" as defined in 3GPP 23.040 9.2.3.22 TP-FCS.

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Vicamo Yang [:vicamo] from comment #21)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #20)
> > Vicamo, I'm not familiar at all with that code. What do you want me to give
> > a feedback on?
> 
> It's about SMS database.
> 
> I support the new handleSmsDelivered() should looks like:
> 
>   handleSmsDelivered: function handleSmsDelivered() {
>     let id = HOW_DO_I_RETRIEVE_MESSAGE_ID_HERE;
>     let sms = gSmsService.createSmsMessage(id,
>                                            DOM_SMS_DELIVERY_SENT,
>                                            message.sender || null,
>                                            message.receiver || null,
>                                            message.fullBody || null,
>                                            message.timestamp);
>     Services.obs.notifyObservers(sms, kSmsDeliveredObserverTopic, null);
>   }
> 
> The RadioInterfaceLayer.handleSmsSent() creates a temporary SmsMessage
> object using a message id which is returned from saving the sent outgoing
> message into SMS database.
> 
>   handleSmsSent: function handleSmsSent() {
>     let id = gSmsDatabaseService.saveSentMessage(...);
>     let sms = gSmsService.createSmsMessage(id, ...);
>     gSmsRequestManager.notifySmsSent(message.requestId, sms);
>   }
> 
> I want do add sms-delivered notification in this issue, patch part 4. I'll
> have to create an temporary SmsMessage in order to invoke the
> notifyObservers() API. But I can't have the correct message-id as the one
> got in handleSmsSent(), which is a local variable. The only way to have a
> message-id is saving sent/received SMS message into database, but I don't
> want it. The sms-delivered notification is for the same SmsMessage that used
> to send sms-sent notification. Besides, the `message` variables in
> handleSmsXxxx functions are all temporary variables constructed by
> deserializing DOMMessage post from ril_worker. Saving `id` as a property of
> `message` in handleSmsSent() is meaningless.
> 
> I'm wondering whether should we move database operations up to SmsService or
> somewhere knows requestId <--> messageId mapping. There are still more error
> notifications to be sent in the future, and all of them will also have this
> problem. You may want to save a fail-to-send message as draft. When you want
> to send it again, you won't want to duplicate it, but instead, all errors
> thereafter should be associated with the draft message saved in previous
> failed sending attempt. Let RadioInterfaceLayer notify events, except
> sms-received, with requestId alone.

I am not sure if I completely understand your needs, but you can always have a mapping from requestId and messageId at RadioInterfaceLayer.js, if that´s what you need. IIRC we already have a mapping of sms requestId and ril token at ril_worker.js. That mapped sms requestId is being passed within the options param of the sms-sent event, so you should have it available in RadioInterfaceLayer.js. The `message` object should contain a `requestId` member at https://hg.mozilla.org/mozilla-central/file/ba4983d9c1f9/dom/system/gonk/RadioInterfaceLayer.js#l426

Comment 30

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

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #29)
> I am not sure if I completely understand your needs, but you can always have
> a mapping from requestId and messageId at RadioInterfaceLayer.js, if that´s
> what you need. IIRC we already have a mapping of sms requestId and ril token
> at ril_worker.js. That mapped sms requestId is being passed within the
> options param of the sms-sent event, so you should have it available in
> RadioInterfaceLayer.js.

A request slot will be reset after being notified, error or success. So maybe I can't at all report both notifications on a request. The second notification can result in an assertion failure.

Besides, SmsRequestManager reuses smallest request slot whenever possible. A previous notified slot, also reset, is very likely to be reused in next transaction. So, if I cached the message-id, I will probably notify two different messages that coincidentally have the same message-id.

Comment 31

[Mounir Lamouri (:mounir)]

(In reply to Vicamo Yang [:vicamo] from comment #21)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #20)
> > Vicamo, I'm not familiar at all with that code. What do you want me to give
> > a feedback on?
> 
> It's about SMS database.
> 
> I support the new handleSmsDelivered() should looks like:
> 
>   handleSmsDelivered: function handleSmsDelivered() {
>     let id = HOW_DO_I_RETRIEVE_MESSAGE_ID_HERE;

For the Android backend, I'm keeping in memory an array that creates a relation between a unique id and an sms id. That way, when sms is sent, I fill the array and when the sms is delivered, I can just read it.

Anyhow, my POV is that you can do whatever you want as long as the id is set and correct ;)

Comment 32

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

Attachment: Part 2: Refactor to share methods : V4

1) review comment #28, use PDU_FCS_OK and PDU_FCS_UNSPECIFIED.
2) bug 736706 might need post-processing ACK error cause. 3GPP TS 31.111 7.1.1.1 `the ME shall wait for an acknowledgement from the UICC`.

Comment 33

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

Attachment: Part 3: support SMS-STATUS-REPORT : V5

rebase due to changes in attachment 609634 [details] [diff] [review]

Comment 34

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

Attachment: Part 4: handle SMS-STATUS-REPORT : V5

1) rebase due to changes in attachment 609634 [details] [diff] [review]
2) Thanks for Mounir's suggestion in review comment #31, now delivered event notification works :)

Comment 35

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

Attachment: Part 5: add error handling : V5

rebase due to changes in attachment 609634 [details] [diff] [review] and attachment 609691 [details] [diff] [review]

Comment 36

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

Attachment: DNS: debug only, to enable delivered event console log in gaia sms app

Comment 37

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

Attachment: Part 2: Refactor to share methods : V5

rebase

Comment 38

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

Attachment: Part 4: handle SMS-STATUS-REPORT : V6

rebase

Comment 39

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

Attachment: Part 2: Refactor to share methods : V6

Let BCD string length be a parameter. In parsing MSISDN number, the length might be set to 0xFF to indicate the invalidity of the following octets.

Comment 40

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

Attachment: Part 3: support SMS-STATUS-REPORT : V6

accommodate to changes made in attachment 610085 [details] [diff] [review]

Comment 41

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

Comment on attachment 610044 [details] [diff] [review]
Part 4: handle SMS-STATUS-REPORT : V6

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

::: dom/system/gonk/ril_worker.js
@@ +1899,5 @@
> +    if (!options.srr) {
> +      /* Status-Report requested, don't send next segment here. Instead, send
> +       * it in Status-Report handler.
> +       */
> +      this._processSentSmsSegment(options);

comments here says something totally in reverse.

Comment 42

[Philipp von Weitershausen [:philikon]]

Comment on attachment 610085 [details] [diff] [review]
Part 2: Refactor to share methods : V6

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

Nice. I like this. r=me with comments addressed.

::: dom/system/gonk/ril_consts.js
@@ +526,5 @@
>  
> +// FCS - Failure Cause
> +const PDU_FCS_OK          = 0x00;
> +const PDU_FCS_UNSPECIFIED = 0xFF;
> +

Are these standardized constants? If we just need a success/failure indicator, we could just make RIL._processSmsDeliver() return a boolean that we feed straight into RIL.acknowledgeSMS().

::: dom/system/gonk/ril_worker.js
@@ +1457,5 @@
>    /**
> +   * Helper for processing received SMS parcel data.
> +   *
> +   * @param length
> +   *        Length of SMS string in the incoming parcel.

Can you also please add a @return section to explain the return value, thanks!

@@ +1490,5 @@
> +  /**
> +   * Helper for processing SMS-DELIVER PDUs.
> +   *
> +   * @param length
> +   *        Length of SMS string in the incoming parcel.

Can you also please add a @return section to explain the return value, thanks!

@@ +2900,2 @@
>      // - Sender Address info -
> +    msg.sender = this.readAddress(this.readHexOctet());

For readability purposes, I suggest

  let senderAddressLength = this.readHexOctet();
  msg.sender = this.readAddress(senderAddressLength);

Comment 43

[Philipp von Weitershausen [:philikon]]

Comment on attachment 610087 [details] [diff] [review]
Part 3: support SMS-STATUS-REPORT : V6

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

r=me with questions/nits addressed.

::: dom/system/gonk/ril_consts.js
@@ +527,2 @@
>  const PDU_MTI_SMS_DELIVER         = 0x00;
> +const PDU_MTI_SMS_DELIVER_REPORT  = 0x00;

Why do we need a separate set of constants for report messages if they're the same anyway?

::: dom/system/gonk/ril_worker.js
@@ +1524,5 @@
> +    if (!message) {
> +      return PDU_FCS_UNSPECIFIED;
> +    }
> +
> +    return PDU_FCS_OK;

Same question about boolean return value vs. PDU_FCS_* constants as in previous patch.

@@ +2861,5 @@
> +   * @param msg
> +   *        message object for output.
> +   */
> +  readExtraParams: function readExtraParams(msg) {
> +    /* Becase each PDU octet is converted to two UCS2 char2, we should always

Typo: Because

@@ +2864,5 @@
> +  readExtraParams: function readExtraParams(msg) {
> +    /* Becase each PDU octet is converted to two UCS2 char2, we should always
> +     * get even messageStringLength in this#_processReceivedSms(). So, we'll
> +     * always need two delimitors at the end.
> +     */

Comment style nit: use //.

/**/ is only for javadoc-style comments on functions.

@@ +2876,5 @@
> +      /* `The most significant bit in octet 1 and any other TP-PI octets which
> +       * may be added later is reserved as an extension bit which when set to a
> +       * 1 shall indicate that another TP-PI octet follows immediately
> +       * afterwards.` ~ 3GPP TS 23.040 9.2.3.27
> +       */

Ditto

@@ +2883,5 @@
> +
> +    /* `If the TP-UDL bit is set to "1" but the TP-DCS bit is set to "0" then
> +     * the receiving entity shall for TP-DCS assume a value of 0x00, i.e. the
> +     * 7bit default alphabet.` ~ 3GPP 23.040 9.2.3.27
> +     */

Ditto.

Comment 44

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

(In reply to Philipp von Weitershausen [:philikon] from comment #42)
> Comment on attachment 610085 [details] [diff] [review]
> Part 2: Refactor to share methods : V6
> 
> Review of attachment 610085 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice. I like this. r=me with comments addressed.
> 
> ::: dom/system/gonk/ril_consts.js
> @@ +526,5 @@
> >  
> > +// FCS - Failure Cause
> > +const PDU_FCS_OK          = 0x00;
> > +const PDU_FCS_UNSPECIFIED = 0xFF;
> > +
> 
> Are these standardized constants? If we just need a success/failure
> indicator, we could just make RIL._processSmsDeliver() return a boolean that
> we feed straight into RIL.acknowledgeSMS().

Yes, they are. They are defined in 3GPP TS 23.040 9.2.3.22. And, Android rild does recognize the failure cause in PDU ack as values defined above.

> ::: dom/system/gonk/ril_worker.js
> @@ +1457,5 @@
> >    /**
> > +   * Helper for processing received SMS parcel data.
> > +   *
> > +   * @param length
> > +   *        Length of SMS string in the incoming parcel.
> 
> Can you also please add a @return section to explain the return value,
> thanks!

Of course.

> @@ +1490,5 @@
> > +  /**
> > +   * Helper for processing SMS-DELIVER PDUs.
> > +   *
> > +   * @param length
> > +   *        Length of SMS string in the incoming parcel.
> 
> Can you also please add a @return section to explain the return value,
> thanks!

Sure.

> @@ +2900,2 @@
> >      // - Sender Address info -
> > +    msg.sender = this.readAddress(this.readHexOctet());
> 
> For readability purposes, I suggest
> 
>   let senderAddressLength = this.readHexOctet();
>   msg.sender = this.readAddress(senderAddressLength);

Sweet!

Comment 45

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

(In reply to Philipp von Weitershausen [:philikon] from comment #43)
> Comment on attachment 610087 [details] [diff] [review]
> Part 3: support SMS-STATUS-REPORT : V6
> 
> Review of attachment 610087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with questions/nits addressed.
> 
> ::: dom/system/gonk/ril_consts.js
> @@ +527,2 @@
> >  const PDU_MTI_SMS_DELIVER         = 0x00;
> > +const PDU_MTI_SMS_DELIVER_REPORT  = 0x00;
> 
> Why do we need a separate set of constants for report messages if they're
> the same anyway?

PDU_MTI_SMS_DELIVER_REPORT and PDU_MTI_SMS_SUBMIT_REPORT will not get implemented unless we're going to turn b2g into an SMS gateway. These definitions can be removed with safe.

> ::: dom/system/gonk/ril_worker.js
> @@ +1524,5 @@
> > +    if (!message) {
> > +      return PDU_FCS_UNSPECIFIED;
> > +    }
> > +
> > +    return PDU_FCS_OK;
> 
> Same question about boolean return value vs. PDU_FCS_* constants as in
> previous patch.

No, same reason as comment #44.

> > +    /* Becase each PDU octet is converted to two UCS2 char2, we should always
> 
> Typo: Because
> 
> > +    /* Becase each PDU octet is converted to two UCS2 char2, we should always
> > +     * get even messageStringLength in this#_processReceivedSms(). So, we'll
> > +     * always need two delimitors at the end.
> > +     */
> 
> Comment style nit: use //.

OK, I'll follow this style and have re-examination on patches already uploaded.

Comment 46

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

Attachment: Part 2: Refactor to share methods : V7

update for comment #42

Comment 47

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

Attachment: Part 3: support SMS-STATUS-REPORT : V7

update from comment #43

Comment 48

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

Attachment: Part 2: Refactor to share methods : V8

fix comments

Comment 49

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

Attachment: Part 3: support SMS-STATUS-REPORT : V8

fix comments

Comment 50

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

Attachment: Part 4: handle SMS-STATUS-REPORT : V7

fix comments

Comment 51

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

Attachment: Part 5: add error handling : V6

fix comments

Comment 52

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

Attachment: Part 4: handle SMS-STATUS-REPORT : V8

Save created SmsMessage instead.

Comment 53

[Philipp von Weitershausen [:philikon]]

Comment on attachment 611417 [details] [diff] [review]
Part 4: handle SMS-STATUS-REPORT : V8

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

Some general comments:

* Throughout this patch, you're using the word "envelop". The correct spelling, however, is "envelope". That said, I'm not even sure it's a good name since there's not really any wrapping going on. But I'm willing to go with it, so long as you explain in a comment what an "envelope" means in this context. Though also consider my next point.

* It seems patch introduces a lot of complexity to support a feature that is currently unsupported by the WebSMS API. I would like to add code when necessary and not just to reach some theoretical 100% coverage of the spec. I would suggest we leave out the whole _sentSmsEnvelops and _pendingSentSmsMap stuff and all related complexity and postpone this stuff to a later time, when we have figured out how to hook it up to WebSMS.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +432,5 @@
>  
> +  /**
> +   * Local storage for sent SMS messages.
> +   */
> +  _sentSmsEnvelops: {},

This object gets created at prototype level which is bad form. Please create it as "null" here and init to an empty object in the constructor.

@@ +437,5 @@
> +  createSmsEnvelop: function createSmsEnvelop(options) {
> +    let i;
> +    for (i = 1; this._sentSmsEnvelops[i]; i++) {
> +      // Do nothing.
> +    }

What are you trying to do here? Just find a free envelope id? Seems very inefficient. Just make a counter?!?

@@ +483,5 @@
> +      return;
> +    }
> +    delete this._sentSmsEnvelops[message.envelopId];
> +
> +    Services.obs.notifyObservers(options.sms, kSmsDeliveredObserverTopic, null);

We don't have consumers for this. Seems pretty pointless to me to burn cycles like this for nothing.

::: dom/system/gonk/ril_worker.js
@@ +3081,5 @@
>     *        Table index used for normal 7-bit encoded character lookup.
>     * @param langShiftIndex
>     *        Table index used for escaped 7-bit encoded character lookup.
> +   * @param srr
> +   *        Request status report.

Why the cryptic "srr" name? Could just call it "requestStatusReport", no?

Comment 54

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

(In reply to Philipp von Weitershausen [:philikon] from comment #53)
> Comment on attachment 611417 [details] [diff] [review]
> Part 4: handle SMS-STATUS-REPORT : V8
> 
> Review of attachment 611417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some general comments:
> 
> * Throughout this patch, you're using the word "envelop". The correct
> spelling, however, is "envelope".

Well, I really need a spelling checker.

> That said, I'm not even sure it's a good name since there's not really
> any wrapping going on. But I'm willing to go with it, so long as you
> explain in a comment what an "envelope" means in this context. Though
> also consider my next point.

Please refer to embedding/android/GeckoSmsManager.java. That's the naming used in android backend. I use it for symmetric naming's sake.
 
> * It seems patch introduces a lot of complexity to support a feature that is
> currently unsupported by the WebSMS API.

No, it's already supported in WebSMS API, and that's why I can hook `ondelivery` event in attachment 609693 [details] [diff] [review]. The `ondelivery` notification is already implemented in mozSms.SmsManager, but never got fired in current RadioInterfaceLayer implementation. The Android backend can properly notify SMS app but Gonk backend can't. This patch completes the missing part.

> I would like to add code when necessary and not just to reach some
> theoretical 100% coverage of the spec. I would suggest we leave out the
> whole _sentSmsEnvelops and _pendingSentSmsMap stuff and all related
> complexity and postpone this stuff to a later time, when we have figured
> out how to hook it up to WebSMS.

No. That's not a theoretical feature, it's already implmented in Android backend. If you want an identical behavior for Android/Gonk backends, you'll have to implement SMS-STATUS-REPORT as Android does. Or, you can remove the Android backend. That's also an option.

> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +432,5 @@
> >  
> > +  /**
> > +   * Local storage for sent SMS messages.
> > +   */
> > +  _sentSmsEnvelops: {},
> 
> This object gets created at prototype level which is bad form. Please create
> it as "null" here and init to an empty object in the constructor.

Ok.

> @@ +437,5 @@
> > +  createSmsEnvelop: function createSmsEnvelop(options) {
> > +    let i;
> > +    for (i = 1; this._sentSmsEnvelops[i]; i++) {
> > +      // Do nothing.
> > +    }
> 
> What are you trying to do here? Just find a free envelope id? Seems very
> inefficient. Just make a counter?!?

Same logic applied in embedding/android/GeckoSmsManager.java. Outgoing SMS messages can't be too many. SMS messaging is slow, each one may take one or a few seconds in worst case. You won't expect this array to have 10*N elements.

> @@ +483,5 @@
> > +      return;
> > +    }
> > +    delete this._sentSmsEnvelops[message.envelopId];
> > +
> > +    Services.obs.notifyObservers(options.sms, kSmsDeliveredObserverTopic, null);
> 
> We don't have consumers for this. Seems pretty pointless to me to burn
> cycles like this for nothing.

Please refer to attachment 609693 [details] [diff] [review]. I've said that I can simply add a hook to this event, and it DOES get notified. That's not a pointless work. Please get updated what WebSMS API is already capable of.

> ::: dom/system/gonk/ril_worker.js
> @@ +3081,5 @@
> >     *        Table index used for normal 7-bit encoded character lookup.
> >     * @param langShiftIndex
> >     *        Table index used for escaped 7-bit encoded character lookup.
> > +   * @param srr
> > +   *        Request status report.
> 
> Why the cryptic "srr" name? Could just call it "requestStatusReport", no?

Ok.

Comment 55

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

Attachment: Part 4: handle SMS-STATUS-REPORT : V9

1) move instanciation of _sentSmsEnvelopes into constructor
2) s/envelop/envelope/
3) s/srr/requestStatusReport/

Comment 56

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang [:vicamo] from comment #54)
> > That said, I'm not even sure it's a good name since there's not really
> > any wrapping going on. But I'm willing to go with it, so long as you
> > explain in a comment what an "envelope" means in this context. Though
> > also consider my next point.
> 
> Please refer to embedding/android/GeckoSmsManager.java. That's the naming
> used in android backend. I use it for symmetric naming's sake.

I see! That makes sense then.

> > * It seems patch introduces a lot of complexity to support a feature that is
> > currently unsupported by the WebSMS API.
> 
> No, it's already supported in WebSMS API, and that's why I can hook
> `ondelivery` event in attachment 609693 [details] [diff] [review]. The
> `ondelivery` notification is already implemented in mozSms.SmsManager, but
> never got fired in current RadioInterfaceLayer implementation. The Android
> backend can properly notify SMS app but Gonk backend can't. This patch
> completes the missing part.

Oh! I wasn't even aware of that. I don't even understand how I could've missed that. I take it all back!

> > @@ +437,5 @@
> > > +  createSmsEnvelop: function createSmsEnvelop(options) {
> > > +    let i;
> > > +    for (i = 1; this._sentSmsEnvelops[i]; i++) {
> > > +      // Do nothing.
> > > +    }
> > 
> > What are you trying to do here? Just find a free envelope id? Seems very
> > inefficient. Just make a counter?!?
> 
> Same logic applied in embedding/android/GeckoSmsManager.java. Outgoing SMS
> messages can't be too many. SMS messaging is slow, each one may take one or
> a few seconds in worst case. You won't expect this array to have 10*N
> elements.

Hmm ok.

Thanks for the updated patch, I'll take a look at it in the morning.

Comment 57

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

Attachment: Part 4: handle SMS-STATUS-REPORT : V10

s/envelop/envelope/ in ril_worker as well

Comment 58

[Philipp von Weitershausen [:philikon]]

Comment on attachment 612483 [details] [diff] [review]
Part 4: handle SMS-STATUS-REPORT : V10

Great job!

Comment 59

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0abf53d9b3b
https://hg.mozilla.org/integration/mozilla-inbound/rev/dea06a99f0c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/572810096bb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e2a8ba65b5f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8646a8cfc01a

Comment 60

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

(In reply to Philipp von Weitershausen [:philikon] from comment #58)
> Comment on attachment 612483 [details] [diff] [review]
> Part 4: handle SMS-STATUS-REPORT : V10
> 
> Great job!

Thank you! I can finally forget it :)

Comment 61

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d0abf53d9b3b
https://hg.mozilla.org/mozilla-central/rev/dea06a99f0c6
https://hg.mozilla.org/mozilla-central/rev/572810096bb6
https://hg.mozilla.org/mozilla-central/rev/2e2a8ba65b5f
https://hg.mozilla.org/mozilla-central/rev/8646a8cfc01a