Closed Bug 734145 Opened 12 years ago Closed 12 years ago

B2G RIL: Support USSD codes

Categories

(Core :: DOM: Device Interfaces, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla16
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: mwu, Assigned: ferjm)

References

Details

(Whiteboard: [qa+])

Attachments

(4 files, 12 obsolete files)

6.62 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
6.58 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
7.21 KB, patch
Details | Diff | Splinter Review
12.33 KB, patch
Details | Diff | Splinter Review
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.
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.
Summary: Support USSD codes → B2G RIL: Support USSD codes
Assignee: nobody → ferjmoreno
Attached patch Part 1: IDL v1 (obsolete) — Splinter Review
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.
Attachment #622555 - Flags: feedback?(jonas)
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'
Attachment #622555 - Flags: feedback+
Attached patch Part 2: RIL v1 (obsolete) — Splinter Review
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.
Attachment #622555 - Attachment is obsolete: true
Attachment #623021 - Flags: feedback?(philipp)
Attachment #622555 - Flags: feedback?(jonas)
Attachment #622555 - Attachment is obsolete: false
Attachment #622555 - Flags: feedback+ → feedback?(jonas)
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.
Attachment #623021 - Flags: feedback?(philipp) → feedback+
Priority: -- → P1
Attached patch Part 1: IDLs v2 (obsolete) — Splinter Review
Added nsIDOMUSSDReceivedEvent.idl and some additions to nsIMobileConnectionProvider.idl.
Attachment #622555 - Attachment is obsolete: true
Attachment #625240 - Flags: feedback?(philipp)
Attached patch Part 3: DOMEvent v1 (obsolete) — Splinter Review
I am specially interested on your feedback about the addition of this event.
Attachment #625242 - Flags: feedback?(philipp)
Attached patch Part 4: MobileConnection v1 (obsolete) — Splinter Review
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?
Attachment #625244 - Flags: feedback?(philipp)
Attached patch Part 2: RIL v2 (obsolete) — Splinter Review
Attachment #623021 - Attachment is obsolete: true
Attachment #625245 - Flags: feedback?(philipp)
All this patches have been successfully tested with Gaia. Both USSD types, terminal initiated and network initiated works.
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.
Just FYI the DOM patches should have a DOM peer look over them. mounir or sicking or me, maybe?
No longer blocks: b2g-ril
(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.
Attached patch Part 1: IDLs v2 (obsolete) — Splinter Review
Minor change on comment
Attachment #625494 - Flags: feedback?(philipp)
Attachment #625240 - Attachment is obsolete: true
Attachment #625240 - Flags: feedback?(philipp)
Attached patch Part 2: RIL v3 (obsolete) — Splinter Review
DOMRequest support added
Attachment #625245 - Attachment is obsolete: true
Attachment #625495 - Flags: feedback?(philipp)
Attachment #625245 - Flags: feedback?(philipp)
Attached patch Part 4: MobileConnection v2 (obsolete) — Splinter Review
DOMRequest support added
Attachment #625244 - Attachment is obsolete: true
Attachment #625496 - Flags: feedback?(philipp)
Attachment #625244 - Flags: feedback?(philipp)
Attached patch Part 5: DOMRequestHelper (obsolete) — Splinter Review
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.
Attachment #625242 - Flags: feedback?(bent.mozilla)
Attachment #625494 - Flags: feedback?(bent.mozilla)
Attachment #625496 - Flags: feedback?(bent.mozilla)
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.
Attachment #625494 - Attachment is obsolete: true
Attachment #625494 - Flags: feedback?(philipp)
Attachment #625494 - Flags: feedback?(bent.mozilla)
Attachment #625495 - Attachment is obsolete: true
Attachment #625495 - Flags: feedback?(philipp)
Attachment #625497 - Attachment is obsolete: true
Attachment #625496 - Attachment is obsolete: true
Attachment #625496 - Flags: feedback?(philipp)
Attachment #625496 - Flags: feedback?(bent.mozilla)
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 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+
Attachment #625494 - Flags: review+
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.
Attachment #625495 - Flags: feedback+
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.
Attachment #625242 - Flags: review?(bent.mozilla)
Attachment #625242 - Flags: feedback?(philipp)
Attachment #625242 - Flags: feedback?(bent.mozilla)
Attachment #625242 - Flags: feedback+
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. :)
Attachment #625496 - Attachment is obsolete: false
Attachment #625496 - Flags: review?(bent.mozilla)
Attachment #625496 - Flags: feedback+
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.
Attachment #625242 - Flags: review?(bent.mozilla) → review+
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.
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.
(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.
Blocks: b2g-ril
(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?
Attached patch Part 1: IDLs v3 (obsolete) — Splinter Review
I´ve rebased the previous IDL part patch and make a few changes only to group functions and events.
Attached patch Part 2: RIL v4 (obsolete) — Splinter Review
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.
Ben's comments addressed
Attachment #625242 - Attachment is obsolete: true
This patch addresses most of Ben's feedback, except the comment regarding nsDependentString, which I couldn´t make it build :\
Attachment #625496 - Attachment is obsolete: true
Attachment #625496 - Flags: review?(bent.mozilla)
There's no reviewer specified for any of these patches. Are they ready to be reviewed?
Attachment #627200 - Flags: review?(philipp)
Attachment #627204 - Flags: review?(philipp)
Attachment #627207 - Flags: review?(bent.mozilla)
Attachment #627211 - Flags: review?(bent.mozilla)
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
Attachment #627200 - Flags: review?(philipp) → review+
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.
Attachment #627204 - Flags: review?(philipp) → review+
blocking-basecamp: --- → +
Attachment #627207 - Flags: review?(bent.mozilla) → review+
Attachment #627211 - Flags: review?(bent.mozilla) → review+
ferjm, can you rebase the patches so they apply on the latest m-c? Tjanks!
Keywords: checkin-needed
Sure! Sorry for the late reply. Rebasing now.
Attached patch Part 1: IDLs v3Splinter Review
Patch rebased and some extra blank spaces removed
Attachment #627200 - Attachment is obsolete: true
Attached patch Part 2: RIL v4Splinter Review
Patch rebased
Attachment #627204 - Attachment is obsolete: true
Keywords: checkin-needed
/me mumbles yet another way to create events :(
Depends on: 763326
blocking-kilimanjaro: --- → +
John, can you please find an owner who could possibly verify this?  Thanks!
Whiteboard: [qa+]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: