Closed Bug 1199141 Opened 9 years ago Closed 6 years ago

Unify USSD and MMI promise behaviour

Categories

(Firefox OS Graveyard :: RIL, defect)

Product:
Firefox OS Graveyard
Component:
RIL
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: benoit, Assigned: pelloux)

References

Details

Attachments

(3 files, 9 obsolete files)

Patch v5
18.21 KB, patch
edgar
: review+
hsinyi
: review+
Details | Diff | Splinter Review
part2 v3
4.02 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
Patch v8
18.90 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review 
Steps to reproduce:
1. Use mozTelephony.dial([...]) to send a USSD code from a certified application
2. Wrap the call of the function with a promise solver, like so:
>      navigator.mozTelephony.dial(USSD_CODE, 0).then(obj => {
>        return obj.result.then(function(result) {
>          console.log('USSD response: ' + result);
>        });
>      }).catch(error => console.error(error));
This is what you can do for MMI codes (let's say pure MMI codes, that only use the phone's hardware and don't require any interaction with the operator)
3. Check out logcat to see what happens

Actual results:
The promise is solved almost instantly, when the USSD code is sent. An empty string is returned is the USSD code was sent successfully, and an error is thrown otherwise.
The real response comes later, in a system message that you can catch this way:
>      navigator.mozSetMessageHandler('ussd-received', e => {
>        console.log('USSD response: ' + e.message);
>      });
However, this system message can be caught by any application (even those that didn't send the original code) and Dialer always considers the response as an unsolicited message, and issues a notification.

Expected:
The promise should be solved later, along with the response of the USSD code.
This is how it works for pure MMI codes, and the API should be consistent whether the MMI code is a USSD one or not.
This way, developing USSD applications would be easier and could be open to operator-specific applications — which is not possible right now. Unsolicited messages should still be forwarded to Dialer, so that a notification is issued.
I already started working on this, and here is my work in progress :
https://github.com/mozilla/gecko-dev/pull/70
The first commit was made to fix bug 1191205.

You can also send USSD codes using an object named USSDSession, which contains two methods, send() and cancel(). This object used to be contained in the system message event, and should be returned in the promise response with this new API. Usind send() and cancel() should work in the same way as mozTelephony.dial() : you should be able to wrap them with a promise solver, and receive the response there and not in a system message.

This still need some work though, as I can't seem to convert this USSDSession object from C++ to Javascript. I tried using ToJSValue, but it just ends up being empty ({}) in Gaia.
(In reply to Benoit Chabod from comment #1)
> I already started working on this, and here is my work in progress :
> https://github.com/mozilla/gecko-dev/pull/70
> The first commit was made to fix bug 1191205.
> 
> You can also send USSD codes using an object named USSDSession, which
> contains two methods, send() and cancel(). This object used to be contained
> in the system message event, and should be returned in the promise response
> with this new API. Usind send() and cancel() should work in the same way as
> mozTelephony.dial() : you should be able to wrap them with a promise solver,
> and receive the response there and not in a system message.
> 
> This still need some work though, as I can't seem to convert this
> USSDSession object from C++ to Javascript. I tried using ToJSValue, but it
> just ends up being empty ({}) in Gaia.


Hi Ben,
Thank you very much for the WIP!

Here's some context about the current USSD API design. We need to rely on an additional way to notify gaia "ussd responses" because the responses are coming from distinct unsolicited events per android ril.h[1]. Hence, it lacks a determined link b/w the response and a request. So App needs to rely on "ussd-received" notification. Previous discussion here [2].

[1] http://androidxref.com/5.1.1_r6/xref/hardware/ril/include/telephony/ril.h#2195
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=897773#c7
> Hi Ben,
> Thank you very much for the WIP!

No problem, just trying to do my part :)

> Here's some context about the current USSD API design. We need to rely on an
> additional way to notify gaia "ussd responses" because the responses are
> coming from distinct unsolicited events per android ril.h[1]. Hence, it
> lacks a determined link b/w the response and a request. So App needs to rely
> on "ussd-received" notification. Previous discussion here [2].

Hmmmm, I understand. But this prevents any possibility of developing USSD applications, that could send USSD codes and receive the response without Dialer (or other USSD applications) catching it.

Even if all USSD messages come from unsolicited events at a low level (android ril.h), you can still link the response and the request : in normal circumstances, if you receive a message, it should be considered as unsolicited and dispatched to Dialer to issue a notification/dialog. But if an application just sent a USSD request, there is a very high probability that the next response is linked to this request, and it should be dispatched solely to the application that sent this USSD code.
Chiming in since I've worked on this stuff in the past. I was wondering if we could do something like this:

- An app sends a USSD code via mozTelephony.send()

- If the response code is USSD-Request then route it directly to the app (only one session can be active so it must be the one opened by the app)

- If not send the response to the last app that sent a USSD request, the app will inspect the message and tell gecko if it's the response it was waiting for or not
- If the app has not recognized the message as a response then the message is forwarded to the dialer to be treated as an unsollicited notification
- If the app has recognized the message it will "consume it" and the dialer won't be informed

It's a crude design that doesn't take into account multiple apps, but it might work provided two things:

- The calling app can detect its responses

- The content of the response are not confidential or otherwise security sensitive so it's OK to actually show them to more than one app
(In reply to Gabriele Svelto [:gsvelto] from comment #4)
> - If the app has not recognized the message as a response then the message
> is forwarded to the dialer to be treated as an unsollicited notification
> - If the app has recognized the message it will "consume it" and the dialer
> won't be informed

This idea seems interesting, if we can think of a good strategy to recognize if a message belongs to a given app. Maybe we can implement a naive version first (the last one to call mozTelephony.dial() gets the response, and if no one called dial() recently, dialer receives it), and then develop this ?

I'm currently stuck with a bug: I can't manage to get the USSDSession object in Gaia when solving the mozTelephony.dial() promise. This object used to be in the system message (if you did event.session, you could access to it), and I want it to be in the result of the promise. It's a WebIDL object that I instantiate in C++ in TelephonyDialCallback.cpp, and then I try to convert it to a Javascript object, but it ends up empty on Gaia side. Here's the code in Gecko: (/dom/telephony/TelephonyDialCallback.cpp, you can find it in my WIP) :

>  nsCOMPtr<nsITelephonyService> service = do_GetService(TELEPHONY_SERVICE_CONTRACTID);
>  nsRefPtr<USSDSession> session = new USSDSession(mWindow, service, aServiceId);
>  JSContext* cx = jsapi.cx();
>  JS::Rooted<JS::Value> jsAdditionalInformation(cx);
>  if (!ToJSValue(cx, session, &jsAdditionalInformation)) {
>    JS_ClearPendingException(cx);
>    return NS_ERROR_TYPE_ERR;
>  }
>  result.mAdditionalInformation.Construct().SetAsObject() = &jsAdditionalInformation.toObject();

And then on Gaia side I just do :

>  navigator.mozTelephony.dial(num, 0).then(obj => {
>    return obj.result.then(function(result) {
>      console.log("USSDSession object : " + JSON.stringify(result.additionalInformation));
>    });
>  }).catch(error => console.error(error));

I receive the text message of the USSD response, but for the USSDSession object I get {}.
Hsin-Yi, can you help me with this or ping someone who might ? :/
I might be wrong about how to use ToJSValue, nsRefPtr, and such things.
Flags: needinfo?(htsai)
(In reply to Benoit Chabod from comment #3)
> > Hi Ben,
> > Thank you very much for the WIP!
> 
> No problem, just trying to do my part :)
> 
> > Here's some context about the current USSD API design. We need to rely on an
> > additional way to notify gaia "ussd responses" because the responses are
> > coming from distinct unsolicited events per android ril.h[1]. Hence, it
> > lacks a determined link b/w the response and a request. So App needs to rely
> > on "ussd-received" notification. Previous discussion here [2].
> 
> Hmmmm, I understand. But this prevents any possibility of developing USSD
> applications, that could send USSD codes and receive the response without
> Dialer (or other USSD applications) catching it.
> 
> Even if all USSD messages come from unsolicited events at a low level
> (android ril.h), you can still link the response and the request : in normal
> circumstances, if you receive a message, it should be considered as
> unsolicited and dispatched to Dialer to issue a notification/dialog. But if
> an application just sent a USSD request, there is a very high probability
> that the next response is linked to this request, and it should be
> dispatched solely to the application that sent this USSD code.

As long as we could have a robust mechanism for binding responses with a correct request and causes no side effects, then your suggestion sounds interesting to me :)
Flags: needinfo?(htsai)
(In reply to Benoit Chabod from comment #5)
> (In reply to Gabriele Svelto [:gsvelto] from comment #4)
> > - If the app has not recognized the message as a response then the message
> > is forwarded to the dialer to be treated as an unsollicited notification
> > - If the app has recognized the message it will "consume it" and the dialer
> > won't be informed
> 
> This idea seems interesting, if we can think of a good strategy to recognize
> if a message belongs to a given app. Maybe we can implement a naive version
> first (the last one to call mozTelephony.dial() gets the response, and if no
> one called dial() recently, dialer receives it), and then develop this ?
> 
> I'm currently stuck with a bug: I can't manage to get the USSDSession object
> in Gaia when solving the mozTelephony.dial() promise. This object used to be
> in the system message (if you did event.session, you could access to it),
> and I want it to be in the result of the promise. It's a WebIDL object that
> I instantiate in C++ in TelephonyDialCallback.cpp, and then I try to convert
> it to a Javascript object, but it ends up empty on Gaia side. Here's the
> code in Gecko: (/dom/telephony/TelephonyDialCallback.cpp, you can find it in
> my WIP) :
> 
> >  nsCOMPtr<nsITelephonyService> service = do_GetService(TELEPHONY_SERVICE_CONTRACTID);
> >  nsRefPtr<USSDSession> session = new USSDSession(mWindow, service, aServiceId);
> >  JSContext* cx = jsapi.cx();
> >  JS::Rooted<JS::Value> jsAdditionalInformation(cx);
> >  if (!ToJSValue(cx, session, &jsAdditionalInformation)) {
> >    JS_ClearPendingException(cx);
> >    return NS_ERROR_TYPE_ERR;
> >  }
> >  result.mAdditionalInformation.Construct().SetAsObject() = &jsAdditionalInformation.toObject();
> 
> And then on Gaia side I just do :
> 
> >  navigator.mozTelephony.dial(num, 0).then(obj => {
> >    return obj.result.then(function(result) {
> >      console.log("USSDSession object : " + JSON.stringify(result.additionalInformation));
> >    });
> >  }).catch(error => console.error(error));
> 
> I receive the text message of the USSD response, but for the USSDSession
> object I get {}.
> Hsin-Yi, can you help me with this or ping someone who might ? :/
> I might be wrong about how to use ToJSValue, nsRefPtr, and such things.

In B2G there's a problem with printing out certain |JSON.stringify objects| but that doesn't mean you can't get correct attributes. Maybe you'd like to verify in another way again.
(In reply to Hsin-Yi Tsai (OOO Sep. 4 ~ Sep. 20) [:hsinyi] from comment #7)
> In B2G there's a problem with printing out certain |JSON.stringify objects|
> but that doesn't mean you can't get correct attributes. Maybe you'd like to
> verify in another way again.

Yes, only iterable objects (IIRC) can be printed out with JSON.stringify, otherwise you'll have to do it yourself with something like:

function dumpObj(obj) {
  for (var i in obj) dump('obj.' + i + ': ' + obj[i] + '\n');
}
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → benoit
Attached patch 0001-USSD-new-API.patch (obsolete) — — Splinter Review 
Thanks a lot Hsin-Yi and Gabriele, indeed it was just a logging mistake and the object was there :)

Here's a proposal that shows how USSD API could work in the future. It's a first draft, so please don't mind the coding style and the formatting. WIP !

It's based on the patch I made for bug 1191205, and there are two main changes :
- When a USSD message or a cancel request is sent, the callback is registered and kept for later. When a USSD response is received, we use the latest callback registered, and dispatch a system message if none is available.
- If there is an active USSD session, the callback is given an USSDSession object with send() and cancel() methods to shortcut mozTelephony.dial() and reply to the current session. This was already the case for system messages, but some refactoring had to be done to forward that object in the callback.

With this patch, any certified app (including Dialer) can now send USSD codes and retrieve the response itself by just doing :
>      navigator.mozTelephony.dial(num, 0).then(obj => {
>        return obj.result.then(function(result) {
>          console.log("USSD response: " + result.statusMessage);
>        });
>      }).catch(error => console.error(error));

No need of system message, just a simple promise corresponding to the callback.
And you can reply easily the same way :
>      var session = firstResponse.additionalInformation;
>      session.send(num).then(obj => {
>        return obj.result.then(function(result) {
>          console.log("USSD next response: " + result.statusMessage);
>        });
>      }).catch(error => console.error(error));

What is your opinion on this approach ? :)
Attachment #8656543 - Flags: feedback?(htsai)
Attachment #8656543 - Flags: feedback?(gsvelto)
Comment on attachment 8656543 [details] [diff] [review]
0001-USSD-new-API.patch

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

+1 from me, I do really like it this change and I doubt it's going to cause too many issues because of messages being delivered to the wrong app (in fact it might even fix some of the oddities we currently have in the dialer, especially when using dual SIMs). Since we're at it we might consider getting rid of the system message for unsolicited messages entirely and replace it with an event within mozTelephony (e.g. onussdunsolicitedmessage or something along the lines).
Attachment #8656543 - Flags: feedback?(gsvelto) → feedback+
Comment on attachment 8656543 [details] [diff] [review]
0001-USSD-new-API.patch

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

Please allow me to give you some feedback before Hsinyi is back. :)

Overall, the implementation is good, but will be NG if multiple access to initiate the USSD session is allowed and only one USSD allowed at a time.
(This is beyond the scope of this bug but is needed to be taken into consideration if we would like to support it.)

1. What's the expected behavior of 2nd USSD request when the 1st one is ongoing?
   - It will be better to reject the 2nd one instead of cancelling the 1st one.
2. If we need to reject the request from different *origin", 
   then we should also keep the *origin* info in TelephonyService::_ussdCallbacks as well.
3. What if no further response from the user for the 1st ongoing session?
   Currently, we reply on the USSD notification from network when the session is expired.
   Will that always be happened? 
   If not, there is no chance for the 2nd session if the 1st session was not cancelled by the user.
   This is the limitation according to the USSD Parcels provided by ril.h in AOSP. :(

Note: corresponding changes in Gaia is required if this patch is adopted.
Attachment #8656543 - Flags: feedback?(htsai) → feedback+
I'm taking over responsability for this bug since Benoit can't work on it anymore.

Thanks :btseng and :gsvelto for the reviews, I'll try to come up quickly with an updated patch.
Assignee: benoit → pierre-eric
I rebased Benoit's patch on master and retested it.

I'm working on updating the Dialer app to use the new API though I'd rather have this patch reviewed/merged in the first place (because if it doesn't land there's no point in modifying Dialer).
Attachment #8656543 - Attachment is obsolete: true
Attachment #8678837 - Flags: review?(htsai)
(In reply to pierre-eric from comment #13)
> Created attachment 8678837 [details] [diff] [review]
> 0001-Bug-1199141-Unify-MMI-USSD-API.patch
> 
> I rebased Benoit's patch on master and retested it.
> 
> I'm working on updating the Dialer app to use the new API though I'd rather
> have this patch reviewed/merged in the first place (because if it doesn't
> land there's no point in modifying Dialer).

Hi, just a soft reminder, to not break any existing feature, every time when we need to modify an existing webAPI, we need three bugs: 1) gecko bug for new API changes, 2) gaia bug which includes the usage of both new and old APIs, and 3) gaia bug to remove old API usage.

We usually start from working on bug 1), but be sure that bug 2) should always be landed prior to bug 1) to not prevent any regressions.

b.t.w. I'll start reviewing the patch tomorrow.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> (In reply to pierre-eric from comment #13)
> > Created attachment 8678837 [details] [diff] [review]
> > 0001-Bug-1199141-Unify-MMI-USSD-API.patch
> > 
> > I rebased Benoit's patch on master and retested it.
> > 
> > I'm working on updating the Dialer app to use the new API though I'd rather
> > have this patch reviewed/merged in the first place (because if it doesn't
> > land there's no point in modifying Dialer).
> 
> Hi, just a soft reminder, to not break any existing feature, every time when
> we need to modify an existing webAPI, we need three bugs: 1) gecko bug for
> new API changes, 2) gaia bug which includes the usage of both new and old
> APIs, and 3) gaia bug to remove old API usage.
> 
> We usually start from working on bug 1), but be sure that bug 2) should
> always be landed prior to bug 1) to not prevent any regressions.
> 

apparently, I typed an extra "not" - the correct sentence is "... to prevent any regressions."
Comment on attachment 8678837 [details] [diff] [review]
0001-Bug-1199141-Unify-MMI-USSD-API.patch

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

Hi Pierre-Eric, thank you very much for the patch! The direction this patch moves towards looks good but I am giving r- as there are details, shown as below, that need to be addressed. I'd also like to invite Bevis for the review, as he has been working on some ussd code refactoring recently. :)

A few general comments:
1) please provide a hg format patch. you may want to refer to [1] to know how to do that.
2) One thing is missing - remember to modify [2], so that the type of |additionalInformation| is (short or object or USSDSession). Please also revise the comment accordingly.


[1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[2] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileConnection.webidl?from=MozMobileConnection.webidl#649

::: dom/telephony/gonk/TelephonyService.js
@@ +1082,4 @@
>            this._cancelUSSDInternal(aClientId, aResponse => {
>              // Fail to cancel ussd session, report error instead of sending ussd
>              // request.
> +            if (aResponse && aResponse.errorMsg) {

Can you point me that why we need a new condition for aResponse? I think our protocol within TelephonyService and ril_worker guarantees the existence of aResponse..

@@ +1086,2 @@
>                aCallback.notifyDialMMIError(aResponse.errorMsg);
> +            } else {

We usually prefer "bail-out eariler" coding style. Please leave it as what it has been.

@@ +1091,2 @@
>            });
> +        } else {

ditto.

@@ +1692,4 @@
>     *        The response from ril_worker.
>     */
>    _defaultCallbackHandler: function(aCallback, aResponse) {
> +    if (aResponse && aResponse.errorMsg) {

ditto: Can you point me that why we need a new condition for aResponse? I think our protocol within TelephonyService and ril_worker guarantees the existence of aResponse..

@@ +1698,5 @@
>        aCallback.notifySuccess();
>      }
>    },
>  
> +  _defaultMMICallbackHandler: function(aCallback, aResponse, aClientId, aSessionEnded) {

Please do not do this. As the name says, it's used  for "default" behaviour, i.e. for only notifyDialMMISuccess() and notifyDialMMIError(). If you ever need a more specific or complicated function, please create a new one when you need.

@@ +2193,4 @@
>    },
>  
>    _cancelUSSDInternal: function(aClientId, aCallback) {
> +    this._ussdCallbacks[aClientId] = aCallback;

Put this line after |this._ussdSessions[aClientId] = USSD_SESSION_CANCELLING;|.

@@ +2482,5 @@
>  
> +    // If there is a callback registered, call it
> +    if (this._ussdCallbacks[aClientId]) {
> +      let tempCallback = this._ussdCallbacks[aClientId];
> +      this._ussdCallbacks[aClientId] = null;

Do we need to have a timeout for clearing the _ussdCallbacks so it's protected from a dangling callback when there's no network unsolicited response coming back?

::: dom/telephony/ipc/TelephonyTypes.ipdlh
@@ +22,4 @@
>  union AdditionalInformation {
>    void_t;
>    uint16_t;
> +  uint32_t;

According to my comment on nsITelephonyService.idl, I think we won't need this.

::: dom/telephony/nsITelephonyService.idl
@@ +124,4 @@
>    void notifyDialMMISuccess(in AString statusMessage);
>    void notifyDialMMISuccessWithInteger(in AString statusMessage,
>                                         in unsigned short aAdditionalInformation);
> +  void notifyDialMMISuccessWithSession(in AString statusMessage, in unsigned long aAdditionalInformation);

From your implementation in TelephonyService, what you need is a callback to distinguish if a session is ended or not and to know which client id the session is on. Therefore, I'd suggest we make the callback and argument name explicit, that is, |notifyDialMMISuccessWithSession(in unsigned long clientId, in AString statusMessage)|. Please also provide comments.

@@ +234,3 @@
>     * Otherwise, callback.notifyError() will be called.
>     */
>    void sendUSSD(in unsigned long clientId, in DOMString ussd,

The comments about the callback parts are wrong. You will need to revise the comments, e.g. callback.notifyDialMMISuccessWithSession() will be called. Please check again.

@@ +242,4 @@
>     * If successful, callback.notifySuccess() will be called.
>     * Otherwise, callback.notifyError() will be called.
>     */
> +  void cancelUSSD(in unsigned long cliendId, in nsITelephonyDialCallback callback);

ditto.
Attachment #8678837 - Flags: review?(htsai)
Attachment #8678837 - Flags: review?(btseng)
Attachment #8678837 - Flags: review-
(In reply to Gabriele Svelto [:gsvelto] from comment #10)
> Comment on attachment 8656543 [details] [diff] [review]
> 0001-USSD-new-API.patch
> 
> Review of attachment 8656543 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> +1 from me, I do really like it this change and I doubt it's going to cause
> too many issues because of messages being delivered to the wrong app (in
> fact it might even fix some of the oddities we currently have in the dialer,
> especially when using dual SIMs). Since we're at it we might consider
> getting rid of the system message for unsolicited messages entirely and
> replace it with an event within mozTelephony (e.g. onussdunsolicitedmessage
> or something along the lines).

Hello Gabriele, 
I think we still need system messages even with Ben's patch, given that there might be network initiated unsolicited USSD coming, and at that time Dialer app is possible not being launched at all. But if in gaia, we are going to let the system app listen to onussdunsolicitedmessage, then we are probably fine to get rid of the system message.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #18)
> I think we still need system messages even with Ben's patch, given that
> there might be network initiated unsolicited USSD coming, and at that time
> Dialer app is possible not being launched at all. But if in gaia, we are
> going to let the system app listen to onussdunsolicitedmessage, then we are
> probably fine to get rid of the system message.

Ah yes, good point. I keep forgetting that we don't have any other way to wake up an app but using a system message :(
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> Comment on attachment 8678837 [details] [diff] [review]
> 0001-Bug-1199141-Unify-MMI-USSD-API.patch
> 
> Review of attachment 8678837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Pierre-Eric, thank you very much for the patch! The direction this patch
> moves towards looks good but I am giving r- as there are details, shown as
> below, that need to be addressed. I'd also like to invite Bevis for the
> review, as he has been working on some ussd code refactoring recently. :)

Thanks for the review. I'm addressing most of the comments and will submit a new version soon.

I still have a few questions:

> >  
> > +  _defaultMMICallbackHandler: function(aCallback, aResponse, aClientId, aSessionEnded) {
> 
> Please do not do this. As the name says, it's used  for "default" behaviour,
> i.e. for only notifyDialMMISuccess() and notifyDialMMIError(). If you ever
> need a more specific or complicated function, please create a new one when
> you need.

Actually it only seems used for USSD stuff, so I'll rename it |_USSDCallbackHandler| 

> 
> @@ +2482,5 @@
> >  
> > +    // If there is a callback registered, call it
> > +    if (this._ussdCallbacks[aClientId]) {
> > +      let tempCallback = this._ussdCallbacks[aClientId];
> > +      this._ussdCallbacks[aClientId] = null;
> 
> Do we need to have a timeout for clearing the _ussdCallbacks so it's
> protected from a dangling callback when there's no network unsolicited
> response coming back?

Good question. Isn't there already some timeout handled at the ril level?

> ::: dom/telephony/nsITelephonyService.idl
> @@ +124,4 @@
> >    void notifyDialMMISuccess(in AString statusMessage);
> >    void notifyDialMMISuccessWithInteger(in AString statusMessage,
> >                                         in unsigned short aAdditionalInformation);
> > +  void notifyDialMMISuccessWithSession(in AString statusMessage, in unsigned long aAdditionalInformation);
> 
> From your implementation in TelephonyService, what you need is a callback to
> distinguish if a session is ended or not and to know which client id the
> session is on. Therefore, I'd suggest we make the callback and argument name
> explicit, that is, |notifyDialMMISuccessWithSession(in unsigned long
> clientId, in AString statusMessage)|. Please also provide comments.

We need to be able to return the USSDSession to the javascript code that initiated the ussd session.
Are you sure that your proposed solution can fullfil this requirement?
(In reply to pierre-eric from comment #20)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #16)
> > Comment on attachment 8678837 [details] [diff] [review]
> > 0001-Bug-1199141-Unify-MMI-USSD-API.patch
> > 
> > Review of attachment 8678837 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hi Pierre-Eric, thank you very much for the patch! The direction this patch
> > moves towards looks good but I am giving r- as there are details, shown as
> > below, that need to be addressed. I'd also like to invite Bevis for the
> > review, as he has been working on some ussd code refactoring recently. :)
> 
> Thanks for the review. I'm addressing most of the comments and will submit a
> new version soon.
> 
> I still have a few questions:
> 
> > >  
> > > +  _defaultMMICallbackHandler: function(aCallback, aResponse, aClientId, aSessionEnded) {
> > 
> > Please do not do this. As the name says, it's used  for "default" behaviour,
> > i.e. for only notifyDialMMISuccess() and notifyDialMMIError(). If you ever
> > need a more specific or complicated function, please create a new one when
> > you need.
> 
> Actually it only seems used for USSD stuff, so I'll rename it
> |_USSDCallbackHandler| 
> 

That sounds good.

> > 
> > @@ +2482,5 @@
> > >  
> > > +    // If there is a callback registered, call it
> > > +    if (this._ussdCallbacks[aClientId]) {
> > > +      let tempCallback = this._ussdCallbacks[aClientId];
> > > +      this._ussdCallbacks[aClientId] = null;
> > 
> > Do we need to have a timeout for clearing the _ussdCallbacks so it's
> > protected from a dangling callback when there's no network unsolicited
> > response coming back?
> 
> Good question. Isn't there already some timeout handled at the ril level?


Ah, you are right. According to [1] I think we are fine with your patch.

[1] http://androidxref.com/4.4.4_r1/xref/hardware/ril/include/telephony/ril.h#3689

> 
> > ::: dom/telephony/nsITelephonyService.idl
> > @@ +124,4 @@
> > >    void notifyDialMMISuccess(in AString statusMessage);
> > >    void notifyDialMMISuccessWithInteger(in AString statusMessage,
> > >                                         in unsigned short aAdditionalInformation);
> > > +  void notifyDialMMISuccessWithSession(in AString statusMessage, in unsigned long aAdditionalInformation);
> > 
> > From your implementation in TelephonyService, what you need is a callback to
> > distinguish if a session is ended or not and to know which client id the
> > session is on. Therefore, I'd suggest we make the callback and argument name
> > explicit, that is, |notifyDialMMISuccessWithSession(in unsigned long
> > clientId, in AString statusMessage)|. Please also provide comments.
> 
> We need to be able to return the USSDSession to the javascript code that
> initiated the ussd session.
> Are you sure that your proposed solution can fullfil this requirement?

Yes, we need to return the USSDSession to the Gaia via WebAPI, and I agree on what you've done in |TelephonyDialCallback::NotifyDialMMISuccessWithSession|. 

That said, here I was talking about that the internal interface b/w TelephonyService.js and TelephonyDialCallback.cpp. Actually my suggestion takes the same information as your original patch. The only difference is the name and order of the arguments.
> That said, here I was talking about that the internal interface b/w
> TelephonyService.js and TelephonyDialCallback.cpp. Actually my suggestion
> takes the same information as your original patch. The only difference is
> the name and order of the arguments.

Actually I though there was only 1 interface, both used internally and externally: nsITelephonyDialCallback.
So are you recommending me adding another method here and use it for communication between TelephonyService.js <-> TelephonyDialCallback.cpp? Or is there another way?
(In reply to pierre-eric from comment #22)
> > That said, here I was talking about that the internal interface b/w
> > TelephonyService.js and TelephonyDialCallback.cpp. Actually my suggestion
> > takes the same information as your original patch. The only difference is
> > the name and order of the arguments.
> 
> Actually I though there was only 1 interface, both used internally and
> externally: nsITelephonyDialCallback.
> So are you recommending me adding another method here and use it for
> communication between TelephonyService.js <-> TelephonyDialCallback.cpp? Or
> is there another way?

Sorry that seems I still didn't express myself clear enough. :(

The external APIs, i.e. USSDSession.webidl and MMIResult dictionay, in your patch look great to me. :)

My comment is around the internal part. 
My recommendation is that we won't have 
  notifyDialMMISuccessWithSession(in AString statusMessage, in unsigned long aAdditionalInformation);
but we will have 
  notifyDialMMISuccessWithSession(in unsigned long clientId, in AString statusMessage);

The only difference is around the names and order of the function arguments.

The reason I suggest this way is because |aAdditionalInformation| sounds vague to me, especially given that in this case |cliendId| is the only data that will be assigned to this attribute. Then why not simply make it clear? Also in our coding style, we usually put |clientId| at the 1st place; thus triggers my opinion. Hopefully this time I answered your questions well. :)
> Sorry that seems I still didn't express myself clear enough. :(

Ah, no worries - I'm the one having trouble understanding how all the pieces fit together.

I've attached a wip progress patch - it's not yet tested nor built - but it'll ease the discussion.
Hopefully you can give me a quick review of this one today in order for me to update it later today.

Thanks!
Attachment #8678837 - Attachment is obsolete: true
Attachment #8678837 - Flags: review?(btseng)
Flags: needinfo?(htsai)
Comment on attachment 8681215 [details] [diff] [review]
0001-Bug-1199141-Unify-MMI-USSD-API.patch

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

Hi Pierre-Eric,

You got my idea about the new change to nsITelephonyService.idl. :) Based on that, we need corresponding modifications for the ipdl protocol (see my comments in details inline).

One thing I missed to point out is that, whenever you modify .idl interfaces, you will need to update their uuid.

Thank you!

::: dom/telephony/ipc/TelephonyChild.cpp
@@ +205,4 @@
>      case AdditionalInformation::Tvoid_t:
>        callback->NotifyDialMMISuccess(statusMessage);
>        break;
> +    case AdditionalInformation::Tuint32_t:

We need to modify this according to my comment on TelephonyTypes.ipdl below.

::: dom/telephony/ipc/TelephonyParent.cpp
@@ +472,5 @@
>  NS_IMETHODIMP
> +TelephonyRequestParent::DialCallback::NotifyDialMMISuccessWithSession(uint32_t aClientId,
> +                                                                      const nsAString& aStatusMessage)
> +{
> +  return SendResponse(DialResponseMMISuccess(nsAutoString(aStatusMessage),

According to my comment on TelephonyTypes.ipdlh below, we need to replace DialResponseMMISuccess with DialResponseSessionSuccess.

::: dom/telephony/ipc/TelephonyTypes.ipdlh
@@ +22,4 @@
>  union AdditionalInformation {
>    void_t;
>    uint16_t;
> +  uint32_t;

Let's not add uint32_t here. Instead, I'd suggest we modify |PTelephonyRequest.ipdl| to add a new structure, e.g. DialResponseSessionSuccess. 

So you will add below into the file:
struct DialResponseSessionSuccess {
  uint32_t clientId;
  nsString statusMessage;
};

Then add |DialResponseSessionSuccess| into the block of |union IPCTelephonyResponse|.

The reason for this is that  the nature of |notifyDialMMISuccessWithSession|is different from other notifyDialMMISuccessWith* that every other notifyDialMMISuccessWith* carries an argument with the exact type of *, instead of _WithSession which doesn't really carry a real |Session| object. If we take your solution, then one day when we need another callback notifyDialMMISuccessWithLongInteger, we will get into trouble of telling the difference b/w _WithLongInteger and _WithSession. So, I prefer we separate the usage in the Ussd session case (though I admit that your solution should work well for now, before we see a need for _WithLongInteger).

::: dom/telephony/nsITelephonyService.idl
@@ +124,4 @@
>    void notifyDialMMISuccess(in AString statusMessage);
>    void notifyDialMMISuccessWithInteger(in AString statusMessage,
>                                         in unsigned short aAdditionalInformation);
> +  void notifyDialMMISuccessWithSession(in unsigned long clientId,

Please add a comment explaining when this is being called and the meanings of the arguments.

nit: as the nature of this function is a bit different from other notifyDialMMISuccessWith*  it would be good to move this after the block of notifyDialMMISuccessWith* functions.

@@ +235,4 @@
>     * Otherwise, callback.notifyError() will be called.
>     */
>    void sendUSSD(in unsigned long clientId, in DOMString ussd,
> +                in nsITelephonyDialCallback callback);

Kind reminder: don't forget to revise the comments based on the final implementation.

@@ +243,4 @@
>     * If successful, callback.notifySuccess() will be called.
>     * Otherwise, callback.notifyError() will be called.
>     */
> +  void cancelUSSD(in unsigned long cliendId, in nsITelephonyDialCallback callback);

ditto.
b.t.w. whenever you attach a new version of patch, please put the version number, e.g. v2, on the attachment description. It does help reviewers for comparing the differences b/w identified versions.
Flags: needinfo?(htsai)
Going back to an earlier comment:

> 
> ::: dom/telephony/gonk/TelephonyService.js
> @@ +1082,4 @@
> >            this._cancelUSSDInternal(aClientId, aResponse => {
> >              // Fail to cancel ussd session, report error instead of sending ussd
> >              // request.
> > +            if (aResponse && aResponse.errorMsg) {
> 
> Can you point me that why we need a new condition for aResponse? I think our
> protocol within TelephonyService and ril_worker guarantees the existence of
> aResponse..
> 

I'm experiencing cases where aResponse is null. Adding logs in RadioInterfaceLayer.js handleUnsolicitedWorkerMessage method shows that while 'message' is not null, message.message can be null.
So I'll re-add the null checks - except if you believe that this is anormal and should be fixed elsewhere?
 
> One thing I missed to point out is that, whenever you modify .idl
> interfaces, you will need to update their uuid.

Is this change documented? (what new uuid should I use etc)
Got some more questions.

Enabling RIL logs, we can see that ril_worker actually knows if a message is an answer ("RIL Worker: [1] Solicited response [...]" aka type code = 0) or if it's unsolicited (type code = 1).

This information is very valuable, because it would help to properly forward  all unsolicited message to the gTelephonyMessenger.notifyUssdReceived.
Another usage would be to filter out duplicate messages we get when cancelling a session. Currently when cancelling we can see this (from TelephonyService.js perspective):

   11-02 14:46:53.182 Gecko: TelephonyService: notifyUssdReceived for 1: null (sessionEnded : true)
   11-02 14:46:53.192 Gecko: TelephonyService: notifyUssdReceived for 1: null (sessionEnded : true)

But these 2 messages are actually different: one of them is the reply to the cancel we did ask for, the 2nd is an unsolicited msg from the network informing us that the session is finished. And IMHO the reply should be sent to the application that initiated the cancel while the second one shouldn't.

I'm going to pass the type_code information up the stack so TelephonyService.js can do wiser choices.

Any objections?
Flags: needinfo?(htsai)
Attached patch Patch v3 (obsolete) — — Splinter Review 
Updated patch.
Introducing the |DialResponseSessionSuccess| struct improved greatly the patch structure, thanks for the hint :)
Attachment #8681215 - Attachment is obsolete: true
(In reply to pierre-eric from comment #29)
> 
> 
>    11-02 14:46:53.182 Gecko: TelephonyService: notifyUssdReceived for 1:
> null (sessionEnded : true)
>    11-02 14:46:53.192 Gecko: TelephonyService: notifyUssdReceived for 1:
> null (sessionEnded : true)
> 
> [...]
> 
> I'm going to pass the type_code information up the stack so
> TelephonyService.js can do wiser choices.
> 

Replying to myself:
The typeCode information (from https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#5160) could be used to replace the test at https://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js#2467 by something like:
   |if (typeCode == 2 && !message)|
Comment on attachment 8682147 [details] [diff] [review]
Patch v3

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

Hi Pierre-eric,
I know we will need to figure out how we could address interference b/w the responses of consequent canselUssd and sendUssd requests, so some comments might not be valid afterwards.

::: dom/telephony/TelephonyDialCallback.cpp
@@ +5,4 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "TelephonyDialCallback.h"
> +#include "mozilla/dom/USSDSession.h"

add an empty line here

@@ +125,5 @@
> +    JS_ClearPendingException(cx);
> +    return NS_ERROR_TYPE_ERR;
> +  }
> +
> +  result.mAdditionalInformation.Construct().SetAsObject() = &jsAdditionalInformation.toObject();

I haven't reviewed this part carefully because I'd like to see the change along with my comments on MozMobileconnection.webidl first.

::: dom/telephony/USSDSession.cpp
@@ +98,4 @@
>      return nullptr;
>    }
>  
> +  nsCOMPtr<nsITelephonyDialCallback> callback = new TelephonyDialCallback(mWindow, nullptr, promise);

After the patch, it's possible that TelephonyDialCallback::mTelephony is null, so please have a null-check first before [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/TelephonyDialCallback.cpp?from=TelephonyDialCallback.cpp#59

::: dom/telephony/gonk/TelephonyService.js
@@ +1082,4 @@
>            this._cancelUSSDInternal(aClientId, aResponse => {
>              // Fail to cancel ussd session, report error instead of sending ussd
>              // request.
> +            if (aResponse && aResponse.errorMsg) {

Regarding your comment 27, you are right I missed that part.  :(

That said, according to my comment on line#1702 and #2488, we will be safe without the check of aResponse. Could you take a look again?

@@ +1086,5 @@
>                aCallback.notifyDialMMIError(aResponse.errorMsg);
>                return;
>              }
>              this._sendUSSDInternal(aClientId, aMmi.fullMMI,
> +                                   this._USSDCallbackHandler.bind(this, aCallback));

It's very likely that aClientId is going to be used in the callback function, how about we also bind aClientId into _USSDCallbackHandler so that we will have |this._USSDCallbackHandler.bind(this, aClientId, aCallback, aResponse);|?

Please note the order of aClientId and aCallback.

@@ +1699,4 @@
>      }
>    },
>  
> +  _USSDCallbackHandler: function(aCallback, aResponse, aClientId, aSessionEnded) {

As aSessionEnded info is definitely part of aResponse, let's simply put them together. So, let's rewrite it as 
    _USSDCallbackHandler: function (aClientId, aCallback, aResponse)
and make sure |sessionEnded| info is part of aResponse. See my comment on line#2488, too.

@@ +1699,5 @@
>      }
>    },
>  
> +  _USSDCallbackHandler: function(aCallback, aResponse, aClientId, aSessionEnded) {
> +    if (aResponse && aResponse.errorMsg) {

Based on above, we won't need aResponse here.

@@ +1704,3 @@
>        aCallback.notifyDialMMIError(aResponse.errorMsg);
>      } else {
> +      if (!aSessionEnded) {

We need corresponding revision here.

@@ +1704,4 @@
>        aCallback.notifyDialMMIError(aResponse.errorMsg);
>      } else {
> +      if (!aSessionEnded) {
> +        aCallback.notifyDialMMISuccessWithSession(aClientId, aResponse);

ditto.

@@ +1705,5 @@
>      } else {
> +      if (!aSessionEnded) {
> +        aCallback.notifyDialMMISuccessWithSession(aClientId, aResponse);
> +      } else {
> +        aCallback.notifyDialMMISuccess(aResponse);

ditto.

@@ +2183,3 @@
>      this._sendToRilWorker(aClientId, "sendUSSD", { ussd: aUssd }, aResponse => {
>        if (aResponse.errorMsg) {
>          this._ussdSessions[aClientId] = USSD_SESSION_DONE;

Sorry I missed to point this out in previous review rounds.

Since the parcel request fails, we should just notifyError right here because there supposedly will never an binding unsolicited message coming. Please remember to do the following in the body of if clause:

let tempCallback = this._ussdCallbacks[aClientId];
this._ussdCallbacks[aClientId] = null;
ttempCallback(aResponse);

@@ +2200,3 @@
>      this._sendToRilWorker(aClientId, "cancelUSSD", {}, aResponse => {
>        if (aResponse.errorMsg) {
>          this._ussdSessions[aClientId] = USSD_SESSION_ONGOING;

Sorry I missed to point this out in previous review rounds.

Since the parcel request fails, we should just notifyError. Please remember to do the following in the body of if clause:

let tempCallback = this._ussdCallbacks[aClientId];
this._ussdCallbacks[aClientId] = null;
tempCallback(aResponse);

@@ +2481,4 @@
>        return;
>      }
>  
> +    // If there is a callback registered, call it

nit: place a period at the end of a line.

@@ +2484,5 @@
> +    // If there is a callback registered, call it
> +    if (this._ussdCallbacks[aClientId]) {
> +      let tempCallback = this._ussdCallbacks[aClientId];
> +      this._ussdCallbacks[aClientId] = null;
> +      tempCallback(aMessage, aClientId, aSessionEnded);

tempCallback({message: aMessage,
                           sessionEnded: aSessionEnded});

::: dom/telephony/ipc/TelephonyTypes.ipdlh
@@ +27,5 @@
>  };
>  
> +struct DialResponseSessionSuccess {
> +  uint32_t clientId;
> +  nsString statusMessage;

Looks much better, no?  ;)

::: dom/telephony/nsITelephonyService.idl
@@ +133,5 @@
>  
>    /**
> +   * Called when a USSD code request succeeds.
> +   * @param clientId
> +   *        Indicate the RIL client. Used later to build a USSDSession

nit: place a period at the end of the line.

@@ +135,5 @@
> +   * Called when a USSD code request succeeds.
> +   * @param clientId
> +   *        Indicate the RIL client. Used later to build a USSDSession
> +   * @param statusMessage
> +   *        Response content

ditto.

We need  to change the uuid for nsITelephonyDialCallback [1].
May refer to [2] to get some uuid generators.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/nsITelephonyService.idl#93
[2] https://developer.mozilla.org/en-US/docs/Generating_GUIDs

@@ +254,1 @@
>     * If successful, callback.notifySuccess() will be called.

s/callback.notifySuccess()/callback.notifyDialMMISuccess()/

Change the uuid https://dxr.mozilla.org/mozilla-central/source/dom/telephony/nsITelephonyService.idl#153

::: dom/webidl/MozMobileConnection.webidl
@@ +645,3 @@
>     *
>     * And it should be
>     * (unsigned short or sequence<DOMString> or sequence<MozCallForwardingOptions>)

You last patch did have |(unsigned short or object or USSDSession) additionalInformation;| but why did you remove this in v3? Any implementation problem with the suggestion? I didn't really try this but I think it could work. Could you please try?
 
> You last patch did have |(unsigned short or object or USSDSession)
> additionalInformation;| but why did you remove this in v3? Any
> implementation problem with the suggestion? I didn't really try this but I
> think it could work. Could you please try?

This is the build error I get if I use this:
    WebIDL.WebIDLError: error: Flat member types of a union should be distinguishable,
    Object is not distinguishable from USSDSession (Wrapper)
    (unsigned short or object or USSDSession) additionalInformation;
Blocks: 1221154
Attached patch Patch v4 (obsolete) — — Splinter Review 
- addressed comments (except that I didn't bind aResponse to _USSDCallbackHandler)
- added mechanism to filter spurious null messages being received after session cancelling is complete
Attachment #8682147 - Attachment is obsolete: true
Flags: needinfo?(htsai)
Attached patch Patch v5 — — Splinter Review 
Updated patch.

I dropped the buggy workaround to try to fix the problem occuring when you issue a sendUSSD request just after a cancelUSSD one. This bug existed before and is out of the scope of this change.
Attachment #8683014 - Attachment is obsolete: true
Attachment #8683124 - Flags: review?(htsai)
I also added a PR to bug 1221154 so that Dialer is compatible with both the promise API and the 'ussd-received' message API.
(In reply to pierre-eric from comment #36)
> I also added a PR to bug 1221154 so that Dialer is compatible with both the
> promise API and the 'ussd-received' message API.

As your PR to bug 1221154 covers two versions of gecko APIs, we should land the gaia part fist. Change the bug dependency to reflect the relation.
No longer blocks: 1221154
Depends on: 1221154
Comment on attachment 8683124 [details] [diff] [review]
Patch v5

Hi Edgar,
Would you please help review the jsapi usage in TelephonyDialCallback::NotifyDialMMISuccessWithSession, in case I missed something? Thank you.
Attachment #8683124 - Flags: review?(echen)
Comment on attachment 8683124 [details] [diff] [review]
Patch v5

I just thought about that there seems some errors in my last review comments. Let me review it again.
Attachment #8683124 - Flags: review?(htsai)
Comment on attachment 8683124 [details] [diff] [review]
Patch v5

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

Great job! We are almost there :) 
This patch deserves r+. r=me with comments addressed or answered. Thank you!

However, I'd ask for an extra Part 2 patch to deal with timeout issues. I was corrected that I seemed misinterpreted ril.h that no guarantee there will be timeout notifications coming. That means, we need to take care of clearing callbacks if waiting for no upcoming unsol responses when time expired.


Besides, I tried both your gecko and gaia patches. Most cases work quite well except the case of sending an arbitrary USSD code that carrier doesn't support.
Without the new changes, Dialer will display "session expired" shortly. With the new changes, the Dialer screen is hang on at "Sending..." even I could tell from the gecko log that ussdcallback is returned. You might want to check the gaia patch again.

::: dom/telephony/TelephonyDialCallback.cpp
@@ +107,4 @@
>  }
>  
>  NS_IMETHODIMP
> +TelephonyDialCallback::NotifyDialMMISuccessWithSession(uint32_t aClientId, const nsAString& aStatusMessage)

nit: wrap at 80th character, make |const nsAString& aStatusMessage| aligned with |uint32_t aClientId|

@@ +129,5 @@
> +    JS_ClearPendingException(cx);
> +    return NS_ERROR_TYPE_ERR;
> +  }
> +
> +  result.mAdditionalInformation.Construct().SetAsObject() = &jsAdditionalInformation.toObject();

nit: wrap at 80th

::: dom/telephony/gonk/TelephonyService.js
@@ +1703,5 @@
> +  /**
> +   * Callback handler for USSD messages.
> +   *
> +   * @param aResponse
> +   *        An object that can hold a message, an errorMsg and a sessionEnded flag

nit: place a period at the end of a line. The comment is applied to the whole file.

@@ +2213,3 @@
>      this._sendToRilWorker(aClientId, "cancelUSSD", {}, aResponse => {
>        if (aResponse.errorMsg) {
>          this._ussdSessions[aClientId] = USSD_SESSION_ONGOING;

Add this: this._ussdCallbacks[aClientId] = null;

::: dom/telephony/ipc/TelephonyTypes.ipdlh
@@ +28,5 @@
>  
> +struct DialResponseSessionSuccess {
> +  uint32_t clientId;
> +  nsString statusMessage;
> +};

Please move this to the right file |PTelephonyRequest.ipdl| as what I suggested in comment 25.

::: dom/telephony/nsITelephonyService.idl
@@ +255,3 @@
>     * Otherwise, callback.notifyError() will be called.
>     */
> +  void cancelUSSD(in unsigned long cliendId, in nsITelephonyDialCallback callback);

Please remember to modify the uuid for the interface nsITelephonyService [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/nsITelephonyService.idl#153
Attachment #8683124 - Flags: review?(htsai) → review+
Quick thoughts about Part 2:
Basically we need two new functions _setUssdCb() and _clearUssdTimer(), new flags/attributes _ussdTimers[].

Rough code looks like below.

_setUssdCb: function (aClientId, aCallback) {
  this._ussdCallbacks[aClientId] = aCallback;
  if (!this._ussdTimers[aClientId]) {
    this._ussdTimers[aClientId] = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
  }
  this._ussdTimers[aClientId].initWithCallback(
    this._clearUssdTimer.bind(this, aClientId), USSD_TIMEOUT_MS, Ci.nsITimer.TYPE_ONE_SHOT);
},

_clearUssdTimer: function(aClientId) {
  if (this._ussdCallbacks[aClientId]) {
     this._ussdCallbacks[aClientId]({errorMsg: XXX});
     this._ussdCallbacks[aClientId] = null;
  }
  if (this._ussdTimers[aClientId]) {
    this._ussdTimers[aClientId].cancel();
  }
},

Then call _setUssdCb() properly in _sendUssdInternal and _cancelUssdInternal. And call _clearUssdTimer properly when a callback is returned.

Hope this helps.
Attached patch Patch v6 (obsolete) — — Splinter Review 
Updated revision (didn't mark v5 as 'Obseletes' to not lose the r+ flag)
> 
> Besides, I tried both your gecko and gaia patches. Most cases work quite
> well except the case of sending an arbitrary USSD code that carrier doesn't
> support.
> Without the new changes, Dialer will display "session expired" shortly. With
> the new changes, the Dialer screen is hang on at "Sending..." even I could
> tell from the gecko log that ussdcallback is returned. You might want to
> check the gaia patch again.

I've updated the gaia patch to fix this.
Now an empty page (because null message) is displayed when you dial a non-supported USSD code.
Comment on attachment 8683124 [details] [diff] [review]
Patch v5

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #39)
> Comment on attachment 8683124 [details] [diff] [review]
> Patch v5
> 
> Hi Edgar,
> Would you please help review the jsapi usage in
> TelephonyDialCallback::NotifyDialMMISuccessWithSession, in case I missed
> something? Thank you.

The jsapi usage looks good to me. Thank you.
Attachment #8683124 - Flags: review?(echen) → review+
Hi Pierre-Eric, 
As this is a significant API change, could you please help add tests to make sure we don't cause regressions and to make sure the new API behavior is as expected? I've written down some ideas about the test scenarios in Bug 1083688.
Here's a patch built upon comment 42.

Two questions:
  * what value should be used for USSD_TIMEOUT_MS (I picked 60 sec) ?
  * what error message in case of timeout (I used the rather uninspired "Timeout") ?
Attachment #8684915 - Flags: feedback?(htsai)
When I was doing more tests for making sure the API behaviour, I learned that the patch current behaviour doesn't meet my expectation.

When the underlying REQUEST_SEND_USSD or REQUEST_CANCEL_USSD fails, according to USSDSession.webidl, I'd expect that the send/cancel promise is rejected. However, the patch v6 behaves that the promise is resolved even those fail. Gaia tells the failure from |MMIResult.success|. I think this is a flaw we need to fix.
Comment on attachment 8683647 [details] [diff] [review]
Patch v6

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

It's my bad to not capture this earlier. I am sorry about that. :(
The lesson I learned is that we do need automation tests along with this new API change. Please add tests on bug 1083688. Once we are making sure test coverage looks good and test results looks as expected, then we will be confident enough to land this new API.

::: dom/telephony/gonk/TelephonyService.js
@@ +2182,2 @@
>      this._sendUSSDInternal(aClientId, aUssd,
> +                           this._USSDCallbackHandler.bind(this, aClientId, aCallback));

We can't use this._USSDCallbackHandler here because the new USSDSession.send() API expects promise rejected in an error case. The current _USSDCallbackHandler() calls aCallback.notifyDialMMIError() [1] but what we need is aCallback.notifyError() [2] which triggers promise rejected. That means, we need another ussd callback handler.

Therefore, please have another callback doing the following:
if (aResponse.errorMsg) {
  aCallback.notifyError(aResponse.errorMsg);
} else {
  if (!aResponse.sessionEnded) {
    aCallback.notifyDialMMISuccessWithSession(aClientId, aResponse.message); 
  } else {
    aCallback.notifyDialMMISuccess(aResponse.message);
  }
}


[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/TelephonyDialCallback.cpp#223
[2] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/TelephonyCallback.cpp#32

@@ +2204,2 @@
>      this._cancelUSSDInternal(aClientId,
> +                             this._USSDCallbackHandler.bind(this, aClientId, aCallback));

ditto: same as sendUSSD
Attachment #8683647 - Flags: review-
(In reply to pierre-eric from comment #47)
> Created attachment 8684915 [details] [diff] [review]
> part2 v1: add timers to clean callbacks
> 
> Here's a patch built upon comment 42.
> 
> Two questions:
>   * what value should be used for USSD_TIMEOUT_MS (I picked 60 sec) ?

Looks good, that's use this.

>   * what error message in case of timeout (I used the rather uninspired
> "Timeout") ?

Following the convention, "TimeoutError" is better.
Comment on attachment 8684915 [details] [diff] [review]
part2 v1: add timers to clean callbacks

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

This part looks good to me. r=me with comments addressed, thank you.

::: dom/telephony/gonk/TelephonyService.js
@@ +373,4 @@
>    this._audioStates = [];
>    this._ussdSessions = [];
>    this._ussdCallbacks = [];
> +  this._ussdTimers = [];

Don't for get to add |this._ussdTimers[i] = null;| in [1].

And add |_ussdTimers: null,| in [2]

[1] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js?from=TelephonyService.js#388
[2] https://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js?from=TelephonyService.js#417

@@ +2234,5 @@
> +  },
> +
> +  _clearUssdTimer: function(aClientId) {
> +    if (this._ussdCallbacks[aClientId]) {
> +       this._ussdCallbacks[aClientId]({errorMsg: "Timeout"});

s/Timeout/TimeoutError/
Attachment #8684915 - Flags: feedback?(htsai) → review+
Attached patch Patch v7 (obsolete) — — Splinter Review 
Hopefully I understood properly your suggestion.
I added a parameter to _USSDCallbackHandler to choose the proper error function to call depending on the context.
Attachment #8683647 - Attachment is obsolete: true
Attachment #8685539 - Flags: review?(htsai)
Attached patch part2 v2 (obsolete) — — Splinter Review 
Updated part2 with proper error message
Attachment #8684915 - Attachment is obsolete: true
Comment on attachment 8685539 [details] [diff] [review]
Patch v7

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

Thank you!

r=me with comments addressed.

Albeit this is review granted, please hold the landing until the test cases are done. Thank you for the work. :)

::: dom/telephony/gonk/TelephonyService.js
@@ +1715,5 @@
> +   *        to get promise rejected.
> +   * @param aResponse
> +   *        An object that can hold a message, an errorMsg and a sessionEnded flag.
> +   */
> +  _USSDCallbackHandler: function(aIsDial, aClientId, aCallback, aResponse) {

Let's switch the order of aIsDial and aClientId.
Attachment #8685539 - Flags: review?(htsai) → review+
Attached patch part2 v3 — — Splinter Review 
:hsinyi while testing the patch I had a issue when starting a new session after a previous one timeout because the session state was never reset.
Here's an updated patch that takes care of this.
Attachment #8685540 - Attachment is obsolete: true
Attachment #8695460 - Flags: review?(htsai)
Comment on attachment 8695460 [details] [diff] [review]
part2 v3

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

::: dom/telephony/gonk/TelephonyService.js
@@ +2254,5 @@
> +    this._ussdTimers[aClientId].initWithCallback(
> +      this._clearUssdTimer.bind(this, aClientId, true), USSD_TIMEOUT_MS, Ci.nsITimer.TYPE_ONE_SHOT);
> +  },
> +
> +  _clearUssdTimer: function(aClientId, clearSession) {

Nice catch! I am glad that tests do help here!
Attachment #8695460 - Flags: review?(htsai) → review+
What would you think of removing |MMIResult.success|, renaming |MMICall::NotifyResult| -> |MMICall::NotifySuccess| and add a |MMICall::NotifyError| method?

Because using the current code there's no way to fail the |MMIcall.result| promise (see https://dxr.mozilla.org/mozilla-central/source/dom/telephony/MMICall.cpp#63)
Flags: needinfo?(htsai)
Attached patch Patch v8 — — Splinter Review 
In this iteration I removed the aIsDial boolean in order to always use |notifyDialMMIError| on failure/timeout.
So we get a consistent behavior, and the MMICall.result promise is always resolved and the calling site needs to check |message.success| before using the result.

As a later change, I could implement my suggestion from comment 57 but this needs some tweaking as in some cases failure returns more than just an error message (see |notifyDialMMIErrorWithInfo|).
Attachment #8685539 - Attachment is obsolete: true
Attachment #8696952 - Flags: review?(htsai)
Comment on attachment 8696952 [details] [diff] [review]
Patch v8

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

r=me with comments addressed. Thank you for the work!

::: dom/telephony/nsITelephonyService.idl
@@ +255,1 @@
>     * Otherwise, callback.notifyError() will be called.

s/notifyError/notifyDialMMIError/  right?

::: dom/webidl/USSDSession.webidl
@@ +10,4 @@
>   Constructor(unsigned long serviceId)]
>  interface USSDSession {
>    [NewObject]
> +  Promise<MMICall> send(DOMString ussd);

Please add a comment saying that the promise is always resolved. The status can be retrieved through MMICall.result.

@@ +14,3 @@
>  
>    [NewObject]
> +  Promise<MMICall> cancel();

ditto
Attachment #8696952 - Flags: review?(htsai) → review+
Flags: needinfo?(htsai)
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: