Closed Bug 720643 Opened 12 years ago Closed 12 years ago

B2G SMS: Notify SMS send success

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla13

People

(Reporter: philikon, Assigned: philikon)

References

Details

Attachments

(1 file, 3 obsolete files)

v1.1
15.15 KB, patch
Details | Diff | Splinter Review 
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).
Assignee: nobody → ferjmoreno
Attached patch WIP (obsolete) — — Splinter Review 
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 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});
(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.
(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!
(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.
Attached patch WIP-2 (obsolete) — — Splinter Review 
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.
Attachment #595070 - Attachment is obsolete: true
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 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 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!
(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.
Attached patch v1 (obsolete) — — Splinter Review 
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!
Assignee: ferjmoreno → philipp
Attachment #595995 - Attachment is obsolete: true
Attachment #596248 - Flags: review?(kyle)
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
Attachment #596248 - Flags: review?(kyle) → review+
Morphing bug title since we punted failures (filed bug 727319 for those).
Summary: B2G SMS: Notify sms-sent / sms-send-failed → B2G SMS: Notify SMS send success
Attached patch v1.1 — — Splinter Review 
Small fix was needed to make it work, due to some carelessness on my part. Will be pushing this shortly.
Attachment #596248 - Attachment is obsolete: true
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.
> Push backed out for build failures on Android:

Also later failed on Windows.
(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.
Depends on: 728864
https://hg.mozilla.org/mozilla-central/rev/16c4770044a8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: