720643 - B2G SMS: Notify SMS send success

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

This means piping the requestId to the RIL where we need to keep a mapping of RIL request token and DOM requestId. And bubble success/error events back to SmsRequestManager (which requires bug 720632).

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

This WIP patch contains the code needed to keep track of the requestId and processId withing the RIL. I´ve finally used the Buff.tokenRequestMap for this as it may be needed for other DOM requests in the future.
The notification is still pending as I need to check what ackPDU and messageRef contains. Also I am not completely sure if I should notify directly via gSmsRequestManager.

Comment 2

[Philipp von Weitershausen [:philikon]]

Comment on attachment 595070 [details] [diff] [review]
WIP

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

Good patch! However, I'm not very happy with making the `Buf` object aware of SMS-specific stuff. Using `tokenRequestMap` is a good idea, but I would simply like to store the `options` object we get from the main thread. Of course, we can't do this right now because `Phone` decomposes `objects` into individual parameters, so the `RIL` object doesn't see it anymore. We should fix that (bug 725002). Until we get there, let's just construct an object when we need it.

::: dom/system/b2g/RadioInterfaceLayer.js
@@ +67,5 @@
> +const kNoSignalError  = 1;
> +const kNotFoundError  = 2;
> +const kUnknownError   = 3;
> +const kInternalError  = 4;
> +

These will be available from nsISmsRequestManager.

@@ +419,5 @@
>      // length in UCS2 encoding.
>      return Math.ceil(text.length / 160);
>    },
>  
> +  sendSMS: function sendSMS(number, message, requestId, processId) {

You're going to have to change nsIRadioInterfaceLayer.idl for this. (Don't forget to bump the UUID!)

::: dom/system/b2g/ril_consts.js
@@ +191,5 @@
>  
> +/**
> + * RIL Error number
> + */
> +const RIL_E_SUCCESS = 0,

We're within the RIL namespace, so please just use ERROR_ as the prefix instead of RIL_E_. Also, please do not copy any of the comments from the RIL header file. You do not have the copyright for that.

::: dom/system/b2g/ril_worker.js
@@ +449,5 @@
> +        request_id = this.tokenRequestMap[token].DOMRequestId;
> +      }
> +      if (this.tokenRequestMap[token].DOMProcessId) {
> +        process_id = this.tokenRequestMap[token].DOMProcessId;
> +      }

Ugh. I don't want to make processParcel and handleParcel aware of SMS-specific stuff. Please just store an `options` object.

@@ +488,5 @@
> +    this.tokenRequestMap[token] = { 
> +      type: type,
> +      DOMRequestId: DOMRequestId,
> +      DOMProcessId: DOMProcessId
> +    };

Where does DOMRequestId/DOMProcessId come from here? In either case, we should just be storing an additional `options` object here, without knowing the details. We can attach `type` as `rilRequestType` to the object.

@@ +744,5 @@
> +                            requestId,
> +                            processId,
> +                            dcs, 
> +                            bodyLengthInOctets) {
> +    let token = Buf.newParcel(REQUEST_SEND_SMS, requestId, processId);

Let's do this as

  let token = Buf.newParcel(REQUEST_SEND_SMS, {requestId: requestId, processId: processId});

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> > +const RIL_E_SUCCESS = 0,
> 
> We're within the RIL namespace, so please just use ERROR_ as the prefix
> instead of RIL_E_. Also, please do not copy any of the comments from the RIL
> header file. You do not have the copyright for that.

Also, I don't even see you using those constants in the patch. We should tackle error handling and reporting properly soon, but perhaps not in this patch. I filed bug 725099 for this. So maybe for now leave out these consts altogether.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> Comment on attachment 595070 [details] [diff] [review]
> WIP
> 
> Review of attachment 595070 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good patch! However, I'm not very happy with making the `Buf` object aware
> of SMS-specific stuff. Using `tokenRequestMap` is a good idea, but I would
> simply like to store the `options` object we get from the main thread. Of
> course, we can't do this right now because `Phone` decomposes `objects` into
> individual parameters, so the `RIL` object doesn't see it anymore. We should
> fix that (bug 725002). Until we get there, let's just construct an object
> when we need it.
> 
> ::: dom/system/b2g/RadioInterfaceLayer.js
> @@ +67,5 @@
> > +const kNoSignalError  = 1;
> > +const kNotFoundError  = 2;
> > +const kUnknownError   = 3;
> > +const kInternalError  = 4;
> > +
> 
> These will be available from nsISmsRequestManager.
> 
Ok, didn´t notice that.

> @@ +419,5 @@
> >      // length in UCS2 encoding.
> >      return Math.ceil(text.length / 160);
> >    },
> >  
> > +  sendSMS: function sendSMS(number, message, requestId, processId) {
> 
> You're going to have to change nsIRadioInterfaceLayer.idl for this. (Don't
> forget to bump the UUID!)
Oh, thanks for the reminder :)
 
> ::: dom/system/b2g/ril_consts.js
> @@ +191,5 @@
> >  
> > +/**
> > + * RIL Error number
> > + */
> > +const RIL_E_SUCCESS = 0,
> 
> We're within the RIL namespace, so please just use ERROR_ as the prefix
> instead of RIL_E_. Also, please do not copy any of the comments from the RIL
> header file. You do not have the copyright for that.
Done.

> ::: dom/system/b2g/ril_worker.js
> @@ +449,5 @@
> > +        request_id = this.tokenRequestMap[token].DOMRequestId;
> > +      }
> > +      if (this.tokenRequestMap[token].DOMProcessId) {
> > +        process_id = this.tokenRequestMap[token].DOMProcessId;
> > +      }
> 
> Ugh. I don't want to make processParcel and handleParcel aware of
> SMS-specific stuff. Please just store an `options` object.
Well, my intention was not to make them aware of SMS-specific stuff, but maybe aware of DOM-specific stuff. I was thinking about other possible requests ids comming from the DOM. Anyway, the options object seems to be much better for this :)

> @@ +488,5 @@
> > +    this.tokenRequestMap[token] = { 
> > +      type: type,
> > +      DOMRequestId: DOMRequestId,
> > +      DOMProcessId: DOMProcessId
> > +    };
> 
> Where does DOMRequestId/DOMProcessId come from here? In either case, we
> should just be storing an additional `options` object here, without knowing
> the details. We can attach `type` as `rilRequestType` to the object.
> 
Ok, I´ll do that.

> @@ +744,5 @@
> > +                            requestId,
> > +                            processId,
> > +                            dcs, 
> > +                            bodyLengthInOctets) {
> > +    let token = Buf.newParcel(REQUEST_SEND_SMS, requestId, processId);
> 
> Let's do this as
> 
>   let token = Buf.newParcel(REQUEST_SEND_SMS, {requestId: requestId,
> processId: processId});
Ok.

Thanks for the feedback!

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> (In reply to Philipp von Weitershausen [:philikon] from comment #2)
> > > +const RIL_E_SUCCESS = 0,
> > 
> > We're within the RIL namespace, so please just use ERROR_ as the prefix
> > instead of RIL_E_. Also, please do not copy any of the comments from the RIL
> > header file. You do not have the copyright for that.
> 
> Also, I don't even see you using those constants in the patch. We should
> tackle error handling and reporting properly soon, but perhaps not in this
> patch. I filed bug 725099 for this. So maybe for now leave out these consts
> altogether.
No, I am not using them yet. My intention was to use them within RadioInterfaceLayer.handleSmsSent and notify errors via gSmsRequestManager.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP-2

This patch should address all the feedback provided in the first revision.
I guess that I have to put this on hold until bug 720632 (which is also on hold) is resolved.

Comment 7

[Philipp von Weitershausen [:philikon]]

Comment on attachment 595995 [details] [diff] [review]
WIP-2

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

Looking good so far. I think we're going to need some rearchitecturing for error processing, but we might as well tie that in together with the RIL + Phone merger (bug 725002) and the general move to DOMRequest.

What was your plan for error handling?

::: dom/system/b2g/ril_worker.js
@@ +490,5 @@
>      this.writeUint32(token);
> +    this.tokenRequestMap[token] = {
> +      rilRequestType = type,
> +      options = options
> +    };

No, please reuse the `options` object itself and just attach the `rilRequestType` to it as a property.

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 595995 [details] [diff] [review]
WIP-2

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

::: dom/system/b2g/ril_worker.js
@@ +470,5 @@
>        if (DEBUG) debug("Unknown response type: " + response_type);
>        return;
>      }
>  
> +    RIL.handleParcel(request_type, length, options);

Hmm, I just realized, `request_type` is available as `options.rilRequestType`, so I think we can consolidate those parameters.

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 595995 [details] [diff] [review]
WIP-2

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

::: dom/system/b2g/nsIRadioInterfaceLayer.idl
@@ +187,5 @@
>    unsigned short getNumberOfMessagesForText(in DOMString text);
> +  void sendSMS(in DOMString number,
> +               in DOMString message,
> +               in DOMString requestId,
> +               in DOMString processId);

*cough* These are not strings!

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> > +    RIL.handleParcel(request_type, length, options);
> 
> Hmm, I just realized, `request_type` is available as
> `options.rilRequestType`, so I think we can consolidate those parameters.

Never mind, ignore me. This is only true for solicited parcels, not for unsolicited.

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Having fixed bug 720632, I wanted to see if I could get anywhere with this bug, based on ferjm's patch.

I decided to not tackle error reporting. I think in the interest of progress we should do that in a follow-up bug. Also I changed the way SMS passes around information, simply by passing around the message/options object. This is necessary for the success/error handler to know the original information, so that the right callback/event contexts (in this case the DOM message object) can be created. I think this should become the template for most message loops once we tackle bug 725002.

I haven't tested it yet because the navigator.mozSms permission check is being difficult and it's Friday night and MFBT etc. But I don't expect the overall patch to change much if at all, so it can't help to get review started on this.

Thanks, ferjm, for the initial ground work on this!

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 596248 [details] [diff] [review]
v1

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

Looks good, options addition makes things way cleaner. r=me

::: dom/system/b2g/ril_worker.js
@@ +769,5 @@
>     *        String containing the recipients address.
>     * @param body
>     *        String containing the message body.
> +   * @param requestId
> +   *        String used by the DOM to associate messages comming from RIL

sp coming

Comment 13

[Philipp von Weitershausen [:philikon]]

Morphing bug title since we punted failures (filed bug 727319 for those).

Comment 14

[Philipp von Weitershausen [:philikon]]

Attachment: v1.1

Small fix was needed to make it work, due to some carelessness on my part. Will be pushing this shortly.

Comment 15

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1ded13556d

Comment 16

[Ed Morley [:emorley]]

Push backed out for build failures on Android:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6c1ded13556d

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2e1b248f40d

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

philikon:

Is this bug the reason I didn't receive onsuccess callback when SMS is sent?

I discovered the problem when I re-styling the SMS app.
https://github.com/andreasgal/gaia/pull/456

If so I will reference the bug # in code. If not you've just got another bug.

Comment 18

[Ed Morley [:emorley]]

> Push backed out for build failures on Android:

Also later failed on Windows.

Comment 19

[Philipp von Weitershausen [:philikon]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TW) from comment #17)
> Is this bug the reason I didn't receive onsuccess callback when SMS is sent?

Yes.

Comment 20

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16c4770044a8

Comment 21

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/16c4770044a8