Last Comment Bug 720643 - B2G SMS: Notify SMS send success
: B2G SMS: Notify SMS send success
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla13
Assigned To: Philipp von Weitershausen [:philikon]
:
Mentors:
Depends on: 720632 728864
Blocks: b2g-sms
  Show dependency treegraph
 
Reported: 2012-01-24 02:18 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-02-20 10:20 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (15.64 KB, patch)
2012-02-07 09:47 PST, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
WIP-2 (16.19 KB, patch)
2012-02-10 02:06 PST, Fernando Jiménez Moreno [:ferjm] (PTO until August)
no flags Details | Diff | Splinter Review
v1 (15.45 KB, patch)
2012-02-10 18:19 PST, Philipp von Weitershausen [:philikon]
kyle: review+
Details | Diff | Splinter Review
v1.1 (15.15 KB, patch)
2012-02-16 14:50 PST, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2012-01-24 02:18:05 PST
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] (PTO until August) 2012-02-07 09:47:07 PST
Created attachment 595070 [details] [diff] [review]
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] 2012-02-07 10:26:23 PST
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] 2012-02-07 14:35:49 PST
(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] (PTO until August) 2012-02-08 01:58:34 PST
(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] (PTO until August) 2012-02-08 03:06:14 PST
(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] (PTO until August) 2012-02-10 02:06:44 PST
Created attachment 595995 [details] [diff] [review]
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] 2012-02-10 15:40:42 PST
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] 2012-02-10 15:55:07 PST
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] 2012-02-10 17:03:58 PST
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] 2012-02-10 17:13:51 PST
(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] 2012-02-10 18:19:03 PST
Created attachment 596248 [details] [diff] [review]
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 [:kmachulis] [:qdot] 2012-02-13 16:12:31 PST
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] 2012-02-14 17:45:07 PST
Morphing bug title since we punted failures (filed bug 727319 for those).
Comment 14 Philipp von Weitershausen [:philikon] 2012-02-16 14:50:01 PST
Created attachment 598009 [details] [diff] [review]
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] 2012-02-16 14:54:35 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1ded13556d
Comment 16 Ed Morley [:emorley] 2012-02-16 15:20:35 PST
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) 2012-02-16 23:53:08 PST
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] 2012-02-17 03:09:52 PST
> Push backed out for build failures on Android:

Also later failed on Windows.
Comment 19 Philipp von Weitershausen [:philikon] 2012-02-17 14:38:25 PST
(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] 2012-02-19 15:46:42 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/16c4770044a8
Comment 21 Ed Morley [:emorley] 2012-02-20 10:20:55 PST
https://hg.mozilla.org/mozilla-central/rev/16c4770044a8

Note You need to log in before you can comment on or make changes to this bug.