Last Comment Bug 734145 - B2G RIL: Support USSD codes
: B2G RIL: Support USSD codes
Status: RESOLVED FIXED
[qa+]
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla16
Assigned To: Fernando Jiménez Moreno [:ferjm]
:
Mentors:
: 773577 (view as bug list)
Depends on: 763326
Blocks: b2g-ril
  Show dependency treegraph
 
Reported: 2012-03-08 09:29 PST by Michael Wu [:mwu]
Modified: 2012-09-25 03:08 PDT (History)
17 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Part 1: IDL v1 (1.73 KB, patch)
2012-05-09 15:42 PDT, Fernando Jiménez Moreno [:ferjm]
jonas: feedback+
Details | Diff | Review
Part 2: RIL v1 (9.98 KB, patch)
2012-05-10 20:06 PDT, Fernando Jiménez Moreno [:ferjm]
philipp: feedback+
Details | Diff | Review
Part 1: IDLs v2 (4.45 KB, patch)
2012-05-18 13:50 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Review
Part 3: DOMEvent v1 (6.62 KB, patch)
2012-05-18 13:51 PDT, Fernando Jiménez Moreno [:ferjm]
bent.mozilla: review+
philipp: feedback+
Details | Diff | Review
Part 4: MobileConnection v1 (6.70 KB, patch)
2012-05-18 13:53 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Review
Part 2: RIL v2 (12.14 KB, patch)
2012-05-18 13:54 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Review
Part 1: IDLs v2 (4.51 KB, patch)
2012-05-20 08:57 PDT, Fernando Jiménez Moreno [:ferjm]
philipp: review+
Details | Diff | Review
Part 2: RIL v3 (15.68 KB, patch)
2012-05-20 08:58 PDT, Fernando Jiménez Moreno [:ferjm]
philipp: feedback+
Details | Diff | Review
Part 4: MobileConnection v2 (6.72 KB, patch)
2012-05-20 08:59 PDT, Fernando Jiménez Moreno [:ferjm]
philipp: feedback+
Details | Diff | Review
Part 5: DOMRequestHelper (2.03 KB, patch)
2012-05-20 09:02 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Review
Part 1: IDLs v3 (5.59 KB, patch)
2012-05-25 05:47 PDT, Fernando Jiménez Moreno [:ferjm]
philipp: review+
Details | Diff | Review
Part 2: RIL v4 (12.28 KB, patch)
2012-05-25 05:53 PDT, Fernando Jiménez Moreno [:ferjm]
philipp: review+
Details | Diff | Review
Part 3: DOMEvent v2 (6.62 KB, patch)
2012-05-25 06:00 PDT, Fernando Jiménez Moreno [:ferjm]
bent.mozilla: review+
Details | Diff | Review
Part 4: MobileConnection v3 (6.58 KB, patch)
2012-05-25 06:13 PDT, Fernando Jiménez Moreno [:ferjm]
bent.mozilla: review+
Details | Diff | Review
Part 1: IDLs v3 (7.21 KB, patch)
2012-06-08 14:59 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Review
Part 2: RIL v4 (12.33 KB, patch)
2012-06-08 14:59 PDT, Fernando Jiménez Moreno [:ferjm]
no flags Details | Diff | Review

Description Michael Wu [:mwu] 2012-03-08 09:29:39 PST
On T-Mobile you can find out your number by dialing #686# and it displays a message with your phone number. This requires support for USSD codes. Other carriers have similar things.
Comment 1 Philipp von Weitershausen [:philikon] 2012-03-08 09:45:07 PST
Indeed, other carriers send you updates on your balance after each call or text, 3G settings, push notifications, etc.

This bug will cover the RIL side of things which we can implement right away. We will also have to figure out how to expose this to content. I will start this discussion on dev-webapi shortly.
Comment 2 Fernando Jiménez Moreno [:ferjm] 2012-05-09 15:42:28 PDT
Created attachment 622555 [details] [diff] [review]
Part 1: IDL v1

I guess I should give you some background on this, as USSD are not something usually known.
From Wikipedia: "USSD is a protocol used by GSM cellular telephones to communicate with the service provider's computers. USSD can be used for WAP browsing, prepaid callback service, mobile-money services, location-based content services, menu-based information services, and as part of configuring the phone on the network.
USSD messages are up to 182 alphanumeric characters in length. Unlike Short Message Service (SMS) messages, USSD messages create a real-time connection during a USSD session. The connection remains open, allowing a two-way exchange of a sequence of data".

The RIL currently exposes two functions to work with USSD codes:
- REQUEST_SEND_USSD: which creates a new session if none previously exists and sends the USSD in the context of that session. Only one session may exists at a time and it exists until the network sends an USSD with code 0 (no further action) or 2 (session terminated) or until a REQUEST_CANCEL_USSD parcel is sent to the network.
- REQUEST_CANCEL_USSD: Cancels the current USSD session if one exists.

The UNSOL_ON_USSD parcel is received when a new USSD is received by the RIL. It is followed by a type code and a chunk of data (the USSD message in UTF-8) if applicable.

As an example of use case, the following is the flow for a fictional menu-driven USSD application (operator side) enabling a mobile user to view his/her current mobile account balance and top up as needed:
1. A mobile user initiates the “Balance Enquiry and Top Up” service by dialing the USSD string defined by the service provider; for example, *#123#. (REQUEST_SEND_USSD)
1.1 The operator server responds with an "Accepted request" USSD message. (UNSOL_ON_USSD).
2. The USSD application receives the service request from the user and responds by sending the user a menu of options. (UNSOL_ON_USSD)
3. The user responds by selecting a “current balance” option. (REQUEST_SEND_USSD)
4. The USSD application sends back details of the mobile user’s current account balance and also gives the option to top up the balance. (UNSOL_ON_USSD)
5. The user selects to top up his/her account. (REQUEST_SEND_USSD)
6. The application responds by asking how much credit to
add? (UNSOL_ON_USSD)
7. The mobile user responds with the amount to add. (REQUEST_SEND_USSD)
8. The USSD application responds by sending an updated balance. (UNSOL_ON_USSD)
8.1 The USSD application responds ending the session. (UNSOL_ON_USSD)

So, given the previous explanation, after talking to Philipp, we agreed to expose this functionality via WebMobileConnection API. Here it goes the proposed modifications for its idl.
Comment 3 Philipp von Weitershausen [:philikon] 2012-05-09 16:14:45 PDT
Comment on attachment 622555 [details] [diff] [review]
Part 1: IDL v1

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +38,5 @@
>  
>    /**
> +   * Send a USSD message.
> +   *
> +   * The network reply should be reported via onussd event.

s/should/will/

Also worth noting that the 'success' event for the request means the USSD message has been sent successfully.

@@ +68,5 @@
>  
> +  /**
> +   * The 'ussd' event is notified whenever a new USSD message is received. 
> +   */
> +  attribute nsIDOMEventListener onussd;

bikeshed: I would suggest 'onussdreceived'
Comment 4 Fernando Jiménez Moreno [:ferjm] 2012-05-10 20:06:29 PDT
Created attachment 623021 [details] [diff] [review]
Part 2: RIL v1

I´ve tested the RIL part in Gaia. Please, ignore the DEBUG flags set to true as it will not be that way on the final patch.
Comment 5 Philipp von Weitershausen [:philikon] 2012-05-13 11:43:21 PDT
Comment on attachment 623021 [details] [diff] [review]
Part 2: RIL v1

Nice! Just make sure not to include parts from the IDL patch (part 1) in the final version.
Comment 6 Fernando Jiménez Moreno [:ferjm] 2012-05-18 13:50:20 PDT
Created attachment 625240 [details] [diff] [review]
Part 1: IDLs v2

Added nsIDOMUSSDReceivedEvent.idl and some additions to nsIMobileConnectionProvider.idl.
Comment 7 Fernando Jiménez Moreno [:ferjm] 2012-05-18 13:51:44 PDT
Created attachment 625242 [details] [diff] [review]
Part 3: DOMEvent v1

I am specially interested on your feedback about the addition of this event.
Comment 8 Fernando Jiménez Moreno [:ferjm] 2012-05-18 13:53:45 PDT
Created attachment 625244 [details] [diff] [review]
Part 4: MobileConnection v1

I am missing the DOMRequest part, some error handling and the addition of some tests for this. Could I fill follow-up bugs for any of this?
Comment 9 Fernando Jiménez Moreno [:ferjm] 2012-05-18 13:54:57 PDT
Created attachment 625245 [details] [diff] [review]
Part 2: RIL v2
Comment 10 Fernando Jiménez Moreno [:ferjm] 2012-05-18 13:55:58 PDT
All this patches have been successfully tested with Gaia. Both USSD types, terminal initiated and network initiated works.
Comment 11 Fernando Jiménez Moreno [:ferjm] 2012-05-18 13:58:32 PDT
Comment on attachment 625240 [details] [diff] [review]
Part 1: IDLs v2

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

::: dom/network/interfaces/nsIDOMMobileConnection.idl
@@ +38,5 @@
>  
>    /**
> +   * Send a USSD message.
> +   *
> +   * The network reply will be reported via onussd event.

Sorry, I´ve just noticed this error on the comment. s/onussd/onussdreceived/. I´ll change it on the next version.
Comment 12 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-20 08:42:01 PDT
Just FYI the DOM patches should have a DOM peer look over them. mounir or sicking or me, maybe?
Comment 13 Fernando Jiménez Moreno [:ferjm] 2012-05-20 08:49:48 PDT
(In reply to ben turner [:bent] from comment #12)
> Just FYI the DOM patches should have a DOM peer look over them. mounir or
> sicking or me, maybe?
Didn´t know that. Thanks :). Jonas was on CC though.
Comment 14 Fernando Jiménez Moreno [:ferjm] 2012-05-20 08:57:28 PDT
Created attachment 625494 [details] [diff] [review]
Part 1: IDLs v2

Minor change on comment
Comment 15 Fernando Jiménez Moreno [:ferjm] 2012-05-20 08:58:39 PDT
Created attachment 625495 [details] [diff] [review]
Part 2: RIL v3

DOMRequest support added
Comment 16 Fernando Jiménez Moreno [:ferjm] 2012-05-20 08:59:54 PDT
Created attachment 625496 [details] [diff] [review]
Part 4: MobileConnection v2

DOMRequest support added
Comment 17 Fernando Jiménez Moreno [:ferjm] 2012-05-20 09:02:56 PDT
Created attachment 625497 [details] [diff] [review]
Part 5: DOMRequestHelper

This patch is probably not going to land with the others as it was taken from Bug 731786. I´ve only added here to let you know that I have the intention of using that part of code. When bug 731786 lands, I´ll need to rebase my patches.
Comment 18 Fernando Jiménez Moreno [:ferjm] 2012-05-20 09:11:18 PDT
I´ve successfully tested the DOMRequest part with a modification of Gaia dialer app. 
I am also asking Ben for feedback for the DOM related parts.
Comment 19 Fernando Jiménez Moreno [:ferjm] 2012-05-22 09:41:48 PDT
Bug 731786 has already landed in m-c, so I am marking my previous patches as obsolete and rebasing it. I´ll be uploading new patches soon.
Comment 20 Philipp von Weitershausen [:philikon] 2012-05-23 19:20:12 PDT
Comment on attachment 625494 [details] [diff] [review]
Part 1: IDLs v2

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

Yeah it needs to be rebased, but that's trivial. r+
Comment 21 Philipp von Weitershausen [:philikon] 2012-05-23 19:26:28 PDT
Comment on attachment 625495 [details] [diff] [review]
Part 2: RIL v3

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

Needs rebasing like you said and a few nits below.

::: dom/system/gonk/RILContentHelper.js
@@ +117,5 @@
>  
> +  sendUSSD: function sendUSSD(window, ussd) {
> +    debug("Sending USSD " + ussd);
> +    if (!window) {
> +      throw Components.Exception("Can´t get window object",

Use apostrophe ('), not inverse backtick (´)

@@ +129,5 @@
> +
> +  cancelUSSD: function cancelUSSD(window) {
> +    debug("Cancel USSD");
> +    if (!window) {
> +      throw Components.Exception("Can´t get window object",

Ditto

@@ +279,5 @@
> +                                     msg.json.message);
> +        break;
> +      case "RIL:SendUssdOK":
> +      case "RIL:CancelUssdOK":
> +        request = this.getRequest(msg.json.requestId);

s/getRequest/takeRequest/! Otherwise you leak the request.

@@ +286,5 @@
> +        }
> +        break;
> +      case "RIL:SendUssdKO":
> +      case "RIL:CancelUssdKO":
> +        request = this.getRequest(msg.json.requestId);

Ditto.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +694,5 @@
> +  },
> +
> +  handleSendUSSD: function handleSendUSSD(message) {
> +    debug("handleSendUSSD " + JSON.stringify(message));
> +    let messageType = message.success ? "RIL:SendUssdOK" : "RIL:SendUssdKO";

Please add a colon before OK and KO. Or, just do the error checking in the content process?

@@ +700,5 @@
> +  },
> +
> +  handleCancelUSSD: function handleCancelUSSD(message) {
> +    debug("handleCancelUSSD " + JSON.stringify(message));
> +    let messageType = message.success ? "RIL:CancelUssdOK" : "RIL:CancelUssdKO";

Ditto.

::: dom/system/gonk/ril_worker.js
@@ +2560,5 @@
> +  }
> +  this.sendDOMMessage({type: "sendussd",
> +                       success: options.rilRequestError == 0 ? true : false,
> +                       errorMsg: options.errorMsg,
> +                       requestId: options.requestId});

Just reuse the `options` object:

  options.success = options.rilRequestError == 0 ? true : false;
  this.sendDOMMessage(options);

(`options.type` will be "sendUSSD".)

@@ +2569,5 @@
> +  }
> +  this.sendDOMMessage({type: "cancelussd",
> +                       success: options.rilRequestError == 0 ? true : false,
> +                       errorMsg: options.errorMsg,
> +                       requestId: options.requestId});

Ditto.
Comment 22 Philipp von Weitershausen [:philikon] 2012-05-23 19:28:31 PDT
Comment on attachment 625242 [details] [diff] [review]
Part 3: DOMEvent v1

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

I'd r+, but I'm not a DOM peer.
Comment 23 Philipp von Weitershausen [:philikon] 2012-05-23 19:33:06 PDT
Comment on attachment 625496 [details] [diff] [review]
Part 4: MobileConnection v2

I don't think this particular patch is obsolete and it looks good to me, although I'm not a DOM peer, so I'm flinging the review over to bent.

>--- a/dom/network/src/MobileConnection.h
>+++ b/dom/network/src/MobileConnection.h
>@@ -32,20 +32,28 @@ public:
>   void Shutdown();
> 
>   NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(MobileConnection,
>                                            nsDOMEventTargetHelper)
> 
> private:
>   nsCOMPtr<nsIMobileConnectionProvider> mProvider;
> 
>+  nsIDOMEventTarget*
>+  ToIDOMEventTarget() const
>+  {
>+    return static_cast<nsDOMEventTargetHelper*>(
>+           const_cast<MobileConnection*>(this));
>+  }

I'm assuming there's a reason why you had to add this? I'm curious to know what that is. :)
Comment 24 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-23 20:42:25 PDT
Comment on attachment 625242 [details] [diff] [review]
Part 3: DOMEvent v1

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

::: dom/network/src/USSDReceivedEvent.cpp
@@ +32,5 @@
> +  NS_INTERFACE_MAP_ENTRY(nsIDOMUSSDReceivedEvent)
> +  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(USSDReceivedEvent)
> +NS_INTERFACE_MAP_END_INHERITING(nsDOMEvent)
> +
> +

Nit: extra newline here

::: dom/network/src/USSDReceivedEvent.h
@@ +6,5 @@
> +#define mozilla_dom_network_ussdreceivedevent_h
> +
> +#include "nsIDOMUSSDReceivedEvent.h"
> +#include "nsDOMEvent.h"
> +#include "nsIDOMEventTarget.h"

Nit: Not sure, but it doesn't look like this is needed.

@@ +48,5 @@
> +    return NS_OK;
> +  }
> +
> +private:
> +  USSDReceivedEvent()

You should do the destructor too.
Comment 25 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-05-23 21:03:33 PDT
Comment on attachment 625496 [details] [diff] [review]
Part 4: MobileConnection v2

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

::: dom/network/src/MobileConnection.cpp
@@ +123,5 @@
>  
> +  if (!strcmp(aTopic, kUssdReceivedTopic)) {
> +    nsString ussd;
> +    ussd.Assign(aData);
> +    nsRefPtr<USSDReceivedEvent> event = USSDReceivedEvent::Create(ussd);

Nit: You can do this:

  USSDReceivedEvent::Create(nsDependentString(aData));

@@ +185,5 @@
> +NS_IMETHODIMP
> +MobileConnection::SendUSSD(const nsAString& aUSSDString,
> +                           nsIDOMDOMRequest** request)
> +{
> +  *request = nsnull;

This is unnecessary. XPConnect will not inspect any out-params until you return a success code.

@@ +193,5 @@
> +  }
> +
> +  if (aUSSDString.IsEmpty()) {
> +    NS_WARNING("Empty USSD string will be ignored");
> +    return NS_OK;

Hm... I don't think this is right. It means that a JS caller that passes a bad value won't get a request object returned. I feel like you should either throw an exception (if this is really something that developers should never do) or return a DOMRequest that later gets a success callback if this is supposed to be ok.

@@ +202,5 @@
> +
> +NS_IMETHODIMP
> +MobileConnection::CancelUSSD(nsIDOMDOMRequest** request)
> +{
> +  *request = nsnull;

Remove.
Comment 26 David Lawrence [:dkl] 2012-05-24 07:58:27 PDT
Over the weekend an issue occurred with the BMO database which resulted in duplication of dependencies. The dependency issue may have resulted in "Depends On" and "Blocks" values being removed while updating a bug. This issue should now be resolved, however dependencies may need to be manually restored to some bugs.

This bug had dependencies removed during the failure period and will need verification that the dependency removal(s) were intentional. Please help out by taking a look at this bug and adding anything back that was mistakenly removed.
Comment 27 Philipp von Weitershausen [:philikon] 2012-05-24 11:55:11 PDT
(In reply to David Lawrence [:dkl] from comment #26)
> Over the weekend an issue occurred with the BMO database which resulted in
> duplication of dependencies. The dependency issue may have resulted in
> "Depends On" and "Blocks" values being removed while updating a bug. This
> issue should now be resolved, however dependencies may need to be manually
> restored to some bugs.
> 
> This bug had dependencies removed during the failure period and will need
> verification that the dependency removal(s) were intentional. Please help
> out by taking a look at this bug and adding anything back that was
> mistakenly removed.

Yup, comment 12 was the culprit. Fixed.
Comment 28 Philipp von Weitershausen [:philikon] 2012-05-24 11:55:41 PDT
(In reply to ben turner [:bent] from comment #12)
> Just FYI the DOM patches should have a DOM peer look over them. mounir or
> sicking or me, maybe?

Wait, you're *not* a DOM peer?
Comment 29 Fernando Jiménez Moreno [:ferjm] 2012-05-25 05:47:25 PDT
Created attachment 627200 [details] [diff] [review]
Part 1: IDLs v3

I´ve rebased the previous IDL part patch and make a few changes only to group functions and events.
Comment 30 Fernando Jiménez Moreno [:ferjm] 2012-05-25 05:53:45 PDT
Created attachment 627204 [details] [diff] [review]
Part 2: RIL v4

This patch addresses Philipp's corrections and adds a simple check to avoid progressing empty USSD messages (replies only with TypeCode, like the ones that closes the USSD session) to the DOM.
Comment 31 Fernando Jiménez Moreno [:ferjm] 2012-05-25 06:00:38 PDT
Created attachment 627207 [details] [diff] [review]
Part 3: DOMEvent v2

Ben's comments addressed
Comment 32 Fernando Jiménez Moreno [:ferjm] 2012-05-25 06:13:28 PDT
Created attachment 627211 [details] [diff] [review]
Part 4: MobileConnection v3

This patch addresses most of Ben's feedback, except the comment regarding nsDependentString, which I couldn´t make it build :\
Comment 33 Dietrich Ayala (:dietrich) 2012-05-25 12:18:03 PDT
There's no reviewer specified for any of these patches. Are they ready to be reviewed?
Comment 34 Philipp von Weitershausen [:philikon] 2012-05-29 15:26:13 PDT
Comment on attachment 627200 [details] [diff] [review]
Part 1: IDLs v3

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

::: dom/network/interfaces/nsIMobileConnectionProvider.idl
@@ +19,5 @@
>    readonly attribute nsIDOMMozMobileConnectionInfo voiceConnectionInfo;
>    readonly attribute nsIDOMMozMobileConnectionInfo dataConnectionInfo;
>  
>    nsIDOMDOMRequest getNetworks(in nsIDOMWindow window);
> +  

tsk tsk, trailing whitespace
Comment 35 Philipp von Weitershausen [:philikon] 2012-05-29 15:29:15 PDT
Comment on attachment 627204 [details] [diff] [review]
Part 2: RIL v4

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

::: dom/system/gonk/ril_worker.js
@@ +2872,5 @@
> +RIL[REQUEST_SEND_USSD] = function REQUEST_SEND_USSD(length, options) {
> +  if (DEBUG) {
> +    debug("REQUEST_SEND_USSD " + JSON.stringify(options)); 
> +  }
> +  options.type = "sendussd";

Seems pointless to me to rewrite this from "sendUSSD" to "sendussd", but whatever.
Comment 36 Philipp von Weitershausen [:philikon] 2012-06-06 10:28:50 PDT
ferjm, can you rebase the patches so they apply on the latest m-c? Tjanks!
Comment 37 Fernando Jiménez Moreno [:ferjm] 2012-06-08 00:24:47 PDT
Sure! Sorry for the late reply. Rebasing now.
Comment 38 Fernando Jiménez Moreno [:ferjm] 2012-06-08 14:59:15 PDT
Created attachment 631539 [details] [diff] [review]
Part 1: IDLs v3

Patch rebased and some extra blank spaces removed
Comment 39 Fernando Jiménez Moreno [:ferjm] 2012-06-08 14:59:50 PDT
Created attachment 631540 [details] [diff] [review]
Part 2: RIL v4

Patch rebased
Comment 42 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-06-10 11:10:27 PDT
/me mumbles yet another way to create events :(
Comment 43 Stephen Donner [:stephend] 2012-06-25 16:21:11 PDT
John, can you please find an owner who could possibly verify this?  Thanks!
Comment 44 Philipp von Weitershausen [:philikon] 2012-07-13 16:23:22 PDT
*** Bug 773577 has been marked as a duplicate of this bug. ***

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