889737 - [MMI] Unify both sendMMI() and dial() functions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[shawn ku [:sku]]

Attachment: 3GPP TS 22.030

STR:

0. Open the dialer app.
1. Send an short string USSD(ex: 21) message.
2. The MMI screen is opened.

Expected:
Device will send out USSD command to network

Current: 
Device will treat it as a call, not USSD.

Note:
 3GPP spec. 22.030 page 13 and 14.

4) Short string:
"Entry of 1 or 2 characters defined in the 3GPP TS 23.038 [8] Default Alphabet followed by SEND".

Comment 1

[Szu-Yu Chen [:aknow] (inactive)]

gecko provides two APIs
- dial()
- sendMMI()

Currently, the number string is parsed in gaia, and gaia determine the API to call. If gaia use dial(), gecko just dial out and make a call with the number given.

The problem of this bug is that gaia use the wrong dial() API. To fix it, gaia should follow the flow chart in spec (as attachment) to parse the string and make a correct choice of API usage.

As I know, current rule in gaia is as follows and it should be modified as I mentioned above.
- string ends with '#' => sendMMI
- others => dial

However, there are some problems:
1. I think that gaia should not have the knowledge of this part. We should not implement the domain related functionality in gaia layer.
2. We could provide a phone number utility function in gecko to make this decision.
3. However, how to handle other special code. ex: *#*#3646#*#* for android engineer mode. This kinds of number should not sent to gecko. It's neither dial() nor sendMMI(). From the view point. Number should be parsed and handled in gaia.

There are some conflict for 1. and 3.
I'm confuse about it. Does anyone has some idea?

Comment 2

[shawn ku [:sku]]

For this case, I think either gaia, or gecko should implement full mmi check logic according to 3GPP spec. definition,

However, there is one extra issue that may hit in the future, that is, emergecny call conflict with short code (ex: some region may need to treat 08 as ecc call). we need to consider if any better way to fix both of short code USSD, and, two digit ECC case.

Finally, *#*#4636#*#* should not collide with mmi/ussd code.

From google android design, dialer will monitor text change via TextWatcher. once hit pre-defined secrect code (*#*#nnnn#*#*) format, device will broadcast serect code intent, then start associated activity.

Comment 3

[Szu-Yu Chen [:aknow] (inactive)]

For ecc issue, we should determine whether the short number is an emergency number by checking properties "ril.ecclist", "ro.ril.ecclist", or default 112, 911.

If "08" has been written into "ril.ecclist" when the phone located in that region, the issue is resolved. If not, we still got the problem. This might be happend when 08 is not a defined ecc number, but when you make the call, the network will route it to the emergency center.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Thanks for starting this Szu-Yu.

(In reply to Szu-Yu Chen [:aknow] from comment #1)
> gecko provides two APIs
> - dial()
> - sendMMI()
> 
> Currently, the number string is parsed in gaia, and gaia determine the API
> to call. If gaia use dial(), gecko just dial out and make a call with the
> number given.
> 
> The problem of this bug is that gaia use the wrong dial() API. To fix it,
> gaia should follow the flow chart in spec (as attachment) to parse the
> string and make a correct choice of API usage.
> 
> As I know, current rule in gaia is as follows and it should be modified as I
> mentioned above.
> - string ends with '#' => sendMMI
> - others => dial
> 
> However, there are some problems:
> 1. I think that gaia should not have the knowledge of this part. We should
> not implement the domain related functionality in gaia layer.
> 2. We could provide a phone number utility function in gecko to make this
> decision.

FWIW we already have some code in Gecko for handling MMI strings [1]. We could probably expose a "isMMI()" function to content, so the UI can check if a string should be treated as an MMI code or as a regular dial number.

> 3. However, how to handle other special code. ex: *#*#3646#*#* for android
> engineer mode. This kinds of number should not sent to gecko. It's neither
> dial() nor sendMMI(). From the view point. Number should be parsed and
> handled in gaia.
> 
> There are some conflict for 1. and 3.
> I'm confuse about it. Does anyone has some idea?

Unfortunately, we don't support non-standard or vendor specific MMI codes in the reference RIL implementation so far. It would be great if we could start thinking about a plugin mechanism to allow vendors to add custom MMI codes associated with device custom actions. But that is probably not a priority right now as we won't be shipping any device with the reference RIL, at least for now.

In any case, the commercial RIL could probably add custom handlers for vendor specific MMI codes to the "isMMI()" function that I am proposing so the UI can still recognize if a vendor specific code should be treated as an MMI code. I wonder if the QC RIL is handling vendor specific MMI codes.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#2095

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Is this a certification requirement? It didn't appeared in 1.0.1 certification and it is not being reported as part of 1.1 neither, but it looks like an important thing to consider for leo.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

I've just had a quick IRC chat about this:

[18:41]  <ferjm> cyang, ok, no problem :) https://bugzilla.mozilla.org/show_bug.cgi?id=889737#c4 I guess that if we ever expose a "isMMI()" function, as suggested in that bug, the QCRIL will be able to say if a vendor specific code (if we are supporting them) is a valid MMI code or not. Am I right?
[18:51]  <cyang> ferjm: :) yeah, i think exposing isMMI() could work.. however, i'm still wondering why Gaia needs to know? shouldn't Gecko deal with the parsing completely whether its a for a vendor specific MMI or not?
[18:52]  <ferjm> cyang, Gaia needs to know if a string should be treated as an MMI or as a normal dial number to call either sendMMI() or dial(). Currently the logic is as poor as calling sendMMI() only if the string ends with '#', which is not correct
[18:53]  <ferjm> cyang, we could also unify sendMMI() and dial() and do the differentiation logic in Gecko
[18:54]  <ferjm> cyang, I mean, always use dial() and get rid of sendMMI
[18:55]  <cyang> ferjm: yeah, thats what i'm thinking.. Gaia wouldn't have to know the logic
[18:56]  <m1> cyang: are there not cases where a normal dial string could be transmogrified into a MMI code from down below too, unbeknownst to the user or dialer app?
[18:58]  <cyang> m1: none that i know of.. maybe anshulj would know?
[19:01]  <ferjm> cyang, the UI wouldn't know much of the MMI logic, it would only know if it needs to use one API or another (MobileConnection or Telephony). However the unification of all the logic within the Telephony API seems like more correct. The change might be quite big though… and certainly not for v1.1
[19:06]  <cyang> ferjm: i agree (all logic within Telephony API would be ideal)
[19:09]  <ferjm> cyang, ok, that's something that we'll need to discuss with the webAPI team. I don't know if the w3c Telephony API is considering the MMI/USSD scenario
[19:10]  <ferjm> marcosc, mounir ^
[19:10]  * marcosc catches up
[19:11]  <ferjm> marcosc, mounir we are considering the possibility of moving all the MMI/USSD exposed functionality from MobileConnection (https://developer.mozilla.org/en-US/docs/WebAPI/Mobile_Connection) to Telephony API 
[19:12]  <marcosc> I need to read up on that
[19:12]  <mounir> ferjm: why?
[19:12]  <ferjm> no rush :)
[19:13]  <marcosc> ferjm: Zoltan from intel and I talked about the same thing...
[19:13]  * marcosc continues reading 
[19:14]  <ferjm> mounir, mostly because the UI needs to do some nasty logic to know if a dial string is an MMI code or not. That feels like it should be part of the platform, not the UI. In the end you are using the dialer to "dial" an MMI/USSD request.
[19:14]   marcia marcosc
[19:14]  <ferjm> marcosc, mounir from http://telephony.sysapps.org/ "This API could be used to send USSD messages to the service provider and invoke functions such as wiping the handset or accessing security settings."
[19:18]  <marcosc> mounir, Zoltan and I discussed doing something like this... but we figured it was out of scope for V1
[19:19]  <marcosc> V1 is still in a sad state
[19:19]  <marcosc> (the spec, that is)
[19:19]  <marcosc> But it's getting better... I'm trying to get it done within 2-3months
[19:20]  <ferjm> FWIW, I'm also not considering this for firefoxOS v1.1
[19:20]  <marcosc> ok, great... we could then work towards a formal proposal
[19:20]  <marcosc> I don't mind branching the spec also to add this stuff in parallel in the future
[19:21]  <mounir> ferjm: what would that look like?
[19:21]  <mounir> do you have a pointer to the MMI/UUSD spec?
[19:21]  <marcosc> mounir: will send you a link.
[19:21]  <mounir> thanks


As you can see we are considering adding the MMI/USSD related functionality to the Telephony API, so every MMI request might also go through the dial() function and there will be no need to do any dial string differentiation logic in the UI. However... that's not an option for v1.1 and if this ends up being a blocker, we will probably need to expose and implement the "isMMI()" function.

Comment 7

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #6)
> I've just had a quick IRC chat about this:
> 
> [18:41]  <ferjm> cyang, ok, no problem :)
> https://bugzilla.mozilla.org/show_bug.cgi?id=889737#c4 I guess that if we
> ever expose a "isMMI()" function, as suggested in that bug, the QCRIL will
> be able to say if a vendor specific code (if we are supporting them) is a
> valid MMI code or not. Am I right?
> [18:51]  <cyang> ferjm: :) yeah, i think exposing isMMI() could work..
> however, i'm still wondering why Gaia needs to know? shouldn't Gecko deal
> with the parsing completely whether its a for a vendor specific MMI or not?
> [18:52]  <ferjm> cyang, Gaia needs to know if a string should be treated as
> an MMI or as a normal dial number to call either sendMMI() or dial().
> Currently the logic is as poor as calling sendMMI() only if the string ends
> with '#', which is not correct
> [18:53]  <ferjm> cyang, we could also unify sendMMI() and dial() and do the
> differentiation logic in Gecko
> [18:54]  <ferjm> cyang, I mean, always use dial() and get rid of sendMMI
> [18:55]  <cyang> ferjm: yeah, thats what i'm thinking.. Gaia wouldn't have
> to know the logic

Unifing sendMMI() and dial() is pretty good. It could also resolve the problem from ambiguous string, e.g., *31#<number> (temporary CLIR), which is a dial string w/ USSD modifier.

> As you can see we are considering adding the MMI/USSD related functionality
> to the Telephony API, so every MMI request might also go through the dial()
> function and there will be no need to do any dial string differentiation
> logic in the UI. However... that's not an option for v1.1 and if this ends
> up being a blocker, we will probably need to expose and implement the
> "isMMI()" function.

The parsing functionality in gecko is located in ril_worker.js. If we simply expose it as isMMI(), it needs an IPC and becomes an async function call.

It's better to have the function, which could be completed in only content process. Thus we could write the code easier.

if (isMMI(dialString)) {
  sendMMI(dialString);
} else {
  dial(dialString);
}

Comment 8

[Beatriz Rodríguez [:brg]]

This is not a certification requirement in our side. If we plan to modify the behaviour please keep in mind bug 813679. Thanks.

Comment 9

[Beatriz Rodríguez [:brg]]

Per previous comment, deleting ni flag.

Comment 10

[Anshul]

Fernando, since this is not a certification requirement for leo I would suggest we go with a unified approach of combining Dial and SendMMI request into one as we discussed on IRC the other day.

Comment 11

[Mounir Lamouri (:mounir)]

Merging sendMMI() into dial() is probably a good idea but we should keep in mind security issues. What would happen if an application creates an Activity that calls a MMI that would wipe out your phone? Should we be okay with that because the user will anyway have to explicitly press CALL to make to commit that MMI? Another question would be whether MMI and Telephony represents the same security risks: should we allow MMI access to any application that has telephony access? (that question being in a world where it's not "Certified" only be bounded to permissions) We might want to create a 'mmi' permission in addition of the 'telephony' permission and have Gecko rejects the MMI calls from dial() if the 'mmi' permission isn't there.

It's just a brain dump and there might be some other issues I am missing, especially possible ones related to telephony/ril.

Comment 12

[shawn ku [:sku]]

We should divide the mmi code check into two types:
1. do not need call key (send button) behavior
   ex: *#06#, PIN/PUK etc...
2. need call key (send button) 
   ex: *#21#, *#43# etc.

For the first part, we should take security concern seriously. for example, Android side met an issue that a hacker issue a PIN (**04*PIN*New PIN*Confirm New PIN#) request via browser in a for loop, and, that result in SIM blocked forever.

For the 2nd part, most of them are network related (call forward, call barring, CLIR etc...) which should not have security issue (if my memory is correct).

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Anshul from comment #10)
> Fernando, since this is not a certification requirement for leo I would
> suggest we go with a unified approach of combining Dial and SendMMI request
> into one as we discussed on IRC the other day.

Sounds good to me, as long as the WebAPI team is ok with this change :)

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Mounir Lamouri (:mounir) from comment #11)
> Merging sendMMI() into dial() is probably a good idea but we should keep in
> mind security issues. What would happen if an application creates an
> Activity that calls a MMI that would wipe out your phone? Should we be okay
> with that because the user will anyway have to explicitly press CALL to make
> to commit that MMI?

We already force the user to press CALL before sending any MMI/USSD via sendMMI() and we'll be doing the same for dial(), as we already do for regular phone calls. Note that this security measure is done in the UI and not in the platform. I wonder if we should allow dial() usage only under user action. That would break the way the Settings app gets the device's IMEI, but we can still expose the IMEI number to content in a different way.

In any case, the USSD wipe case is a pretty edge one (afaik it is only valid for some Android Samsung devices) and it currently does not exist for any Firefox OS device. It is up to device vendors to implement it though.

> Another question would be whether MMI and Telephony
> represents the same security risks: should we allow MMI access to any
> application that has telephony access? (that question being in a world where
> it's not "Certified" only be bounded to permissions) We might want to create
> a 'mmi' permission in addition of the 'telephony' permission and have Gecko
> rejects the MMI calls from dial() if the 'mmi' permission isn't there.
>

I'm ok with the extra 'mmi' permission. Even if both will be granted only for certified apps.

> It's just a brain dump and there might be some other issues I am missing,
> especially possible ones related to telephony/ril.

I guess we need some help from our security team :)

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

(In reply to sku from comment #12)
> We should divide the mmi code check into two types:
> 1. do not need call key (send button) behavior
>    ex: *#06#, PIN/PUK etc...
> 2. need call key (send button) 
>    ex: *#21#, *#43# etc.
> 
> For the first part, we should take security concern seriously. for example,
> Android side met an issue that a hacker issue a PIN (**04*PIN*New
> PIN*Confirm New PIN#) request via browser in a for loop, and, that result in
> SIM blocked forever.
> 
> For the 2nd part, most of them are network related (call forward, call
> barring, CLIR etc...) which should not have security issue (if my memory is
> correct).

I don't think we should make such a distinction. Vendor specific MMI codes (like the USSD wipe one) might be as sensitive as the ones listed in type 1.

I think the safer option here would probably be requiring user action to trigger an MMI request in any case. Unless you can think about an use case where an MMI request is needed and user action is not possible.

Comment 16

[Antonio Manuel Amaya Calvo (:amac)]

The way I think it was working (and the way I think it should work) is that for every send MMI action (or dial, for different reasons) that originates outside of a certified app there should be an explicit user confirmation. That's currently done by requiring the user to press the 'Call' button for numbers received via a web activity.

The problem here is that while the risk for normal dial operations (monetary cost mainly) is well known, the actual risk for MMIs/USSD isn't known (and can't be known) beforehand. Gaia (nor Gecko) cannot know what a given MMI/USSD will do.

What would be desirable but somehow I don't see OEMs implementing is some kind of query interface by which Gecko could ask the underlying radio equipment what will happen if it submits a given MMI code. The answer could be just a enumerated type (something like UNKNOWN_PASSED_TO_NETWORK, SAFE_TO_EXECUTE, UNSAFE_TO_EXECUTE for example), and use that to at least warn the user. On the vaunted Samsung case, having the user press Call would have been a deterrent, but without further warning, how many users would just press the button anyway?

Barring that we could implement a list of well known MMIs codes and just warn the user when he's about to submit a unknown MMI.

Note also that if the OEM has followed the ETSI TS 122 030 specification, there's another thing we can do. According to [1] (pages 10 and following) MMIs are structured:

Activation : *SC*SI#
Deactivation : #SC*SI#
Interrogation : *#SC*SI#
Registration: *SC*SI# and **SC*SI#
Erasure: ##SC*SI#

So a MMI that starts with *# should be harmless. All the rest can be dangerous, with ## being definitely dangerous (but not only that one, registration is dangerous also since it's used to change the PIN/register a new PIN).

TL;DR: MMIs should require explicit user action. Just requiring the user to press Call might not be enough. We can think into adding an extra confirmation (Are you sure you want to send this code which can be dangerous?) for unknown codes. And while I think we can safely skip the user confirmation for interrogation codes, there's nothing on the spec that says interrogation operations have to be free.


[1] http://www.etsi.org/deliver/etsi_ts/122000_122099/122030/10.00.00_60/ts_122030v100000p.pdf

Comment 17

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> TL;DR: MMIs should require explicit user action. Just requiring the user to
> press Call might not be enough. We can think into adding an extra
> confirmation (Are you sure you want to send this code which can be
> dangerous?) for unknown codes. And while I think we can safely skip the user
> confirmation for interrogation codes, there's nothing on the spec that says
> interrogation operations have to be free.

Sounds fine to me. But any reason that we wouldn't just always show a confirmation screen so that user knows that number just dialed was a MMI code and not a regular phone number? And just show different text on that screen depending of what the code is i.e. if its known we give a confirmation screen with a description, and it's unknown then we show a confirmation screen with a warning described above. 

Or do you think that users will just become to accustomed to such a prompt and thus it has less effect as a security control?

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Mounir Lamouri (:mounir) from comment #11)
> Merging sendMMI() into dial() is probably a good idea but we should keep in
> mind security issues. What would happen if an application creates an

Another use case: in call MMI code (ref: Bug 890912)
It should be an MMI code (by spec) but triggered in a call and the behavior is related to telephony.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Antonio Manuel Amaya Calvo from comment #16)
> Barring that we could implement a list of well known MMIs codes and just
> warn the user when he's about to submit a unknown MMI.

Every USSD code is unknown for us and only known by the operator. Showing a confirmation screen for every USSD request sounds like a quite painful UX and IMO it doesn't really provide any extra security as we would not provide any valuable information to the user other than something like "Are you sure that you want to send this USSD code?".
 
> Note also that if the OEM has followed the ETSI TS 122 030 specification,
> there's another thing we can do. According to [1] (pages 10 and following)
> MMIs are structured:
> 
> Activation : *SC*SI#
> Deactivation : #SC*SI#
> Interrogation : *#SC*SI#
> Registration: *SC*SI# and **SC*SI#
> Erasure: ##SC*SI#
> 

Unfortunately, this does not really happen :( and you can find codes like #*#3264#*#* to get the RAM version.

IMHO requiring user action (i.e. press the dial button) should be enough to prevent issues like [1].

[1] http://www.youtube.com/watch?v=zEESPrE0Csw

Comment 20

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Szu-Yu Chen [:aknow] from comment #7)
> (In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC,

> Unifing sendMMI() and dial() is pretty good. It could also resolve the
> problem from ambiguous string, e.g., *31#<number> (temporary CLIR), which is
> a dial string w/ USSD modifier.

You were right man. We've run into this issue while working on temporay CLIR MMI command (bug 894871). IHMO Unifying both sendMMI() and dial() function is becoming a need (see bug 890912 please). The discussion started in this bug so IHMO we should continue it and work on it. I'll change the title of this bug. The change might be quite big, I guess not for being part of v1.2 release.

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

As we would like to support more and more MMI code (such as bug 894871 and bug 809192) in v1.2, I found that the urgency to combine dial() and sendMMI() seems more obvious. Summarizing the discussion above, the current separate design leads to a main problem, i.e. both gecko or gaia need to implement the whole MMI parsing algorithm.

Right now gaia determine an MMI string by checking if it ends with '#' character, which is totally not enough. If we don't want to unify dial() and sendMMI(), gaia would need to follow 3gpp spec to re-implement the MMI code checking part so that gaia can get the right API to call.

The other problem is the ambiguity between dial() and sendMMI(). Even gecko engineers all follow the 3gpp sepc, we still divergent opinions about when to call dial() or sendMMI().  For example, see bug 890912 comment 8. I feel like unifying them could be a way. If we don't agree to unify, then I think we'd definitely need to redefine the usage of dial() and sendMMI() since the definitions and distinctiveness aren't clear enough IMO.

To unify dial() and sendMMI(), the draft picture of the proposal on my mind is like:
1) discard mozMobileConnection.sendMMI()
2) mozTelephony.dial() returns a Promise. If the gecko determines the input string is a normal call number, then gecko makes a call and the Promise.result is a TelephonyCall. If the string is MMI code, gecko does corresponding request and Promise.result would be a any object or a string containing the information of the operation.

I am not sure whether we should do this change in v1.2 as the change might large but it's worth being discussed right now. So that we can have a better idea of how to keep bug 890912 on at the moment.

Any comment?

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> As we would like to support more and more MMI code (such as bug 894871 and
> bug 809192) in v1.2, I found that the urgency to combine dial() and
^^^^^^^^^^^^^ should be bug 890912

Comment 23

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> Right now gaia determine an MMI string by checking if it ends with '#'
> character, which is totally not enough. If we don't want to unify dial() and
> sendMMI(), gaia would need to follow 3gpp spec to re-implement the MMI code
> checking part so that gaia can get the right API to call.
> 
> The other problem is the ambiguity between dial() and sendMMI(). Even gecko
> engineers all follow the 3gpp sepc, we still divergent opinions about when
> to call dial() or sendMMI().  For example, see bug 890912 comment 8. I feel
> like unifying them could be a way. If we don't agree to unify, then I think
> we'd definitely need to redefine the usage of dial() and sendMMI() since the
> definitions and distinctiveness aren't clear enough IMO.

Thanks for summarizing current status and joining the discussion.

> I am not sure whether we should do this change in v1.2 as the change might
> large but it's worth being discussed right now. So that we can have a better
> idea of how to keep bug 890912 on at the moment.

IHMO we are not in a hurry so we should work on bug 890912 once this bug reaches an agreement on how to be implemented and gets landed. For 1.2 release we should be able to handle the multiparty call through the dialer UI so having the MMI commands for being able to handle the multiparty is a plus and shouldn't be a must.

Comment 24

[Anshul]

Hello Hsin-Yi, I had briefly discussed a proposal with ferjm on IRC sometime back regarding the design. Wondering if RIL sending OnMMIStarted event would be sufficient to tell Gaia that the dialed number is an MMI sequence and so Gaia should show the MMI popup.

Comment 25

[shawn ku [:sku]]

(In reply to Anshul from comment #24)
> Hello Hsin-Yi, I had briefly discussed a proposal with ferjm on IRC sometime
> back regarding the design. Wondering if RIL sending OnMMIStarted event would
> be sufficient to tell Gaia that the dialed number is an MMI sequence and so
> Gaia should show the MMI popup.

Hi Anshul:
  From AOSP, google will send out starting MMI callback to let UI show proper notification.
It should be do-able. 

However, there are some change needed in gecko layer since telephony.cpp will crate a TelephonyCall as return value for dial/dialEmergency request. this part need to get fixed at the same time as a new notificaiton is triggered. otherwise, there will be a dangling telephonyCall object.

sku.

// AOSP snippet, [e-mail]
        if (mmi == null) {
            return mCT.dial(newDialString, uusInfo);
        } else if (mmi.isTemporaryModeCLIR()) {
            return mCT.dial(mmi.mDialingNumber, mmi.getCLIRMode(), uusInfo);
        } else {
            mPendingMMIs.add(mmi);
            mMmiRegistrants.notifyRegistrants(new AsyncResult(null, mmi, null));
            mmi.processCode();

            // FIXME should this return null or something else?
            return null;
        }

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to shawn ku [:sku] from comment #25)
> (In reply to Anshul from comment #24)
> > Hello Hsin-Yi, I had briefly discussed a proposal with ferjm on IRC sometime
> > back regarding the design. Wondering if RIL sending OnMMIStarted event would
> > be sufficient to tell Gaia that the dialed number is an MMI sequence and so
> > Gaia should show the MMI popup.
> 
> Hi Anshul:
>   From AOSP, google will send out starting MMI callback to let UI show
> proper notification.
> It should be do-able. 
> 
> However, there are some change needed in gecko layer since telephony.cpp
> will crate a TelephonyCall as return value for dial/dialEmergency request.
> this part need to get fixed at the same time as a new notificaiton is
> triggered. otherwise, there will be a dangling telephonyCall object.
> 
> sku.

Totally agree with sku.

Without changing the current dial() API, it's gonna not work, as dial() now returns a TelephonyCall object immediately. If we simply call dial() for in-call MMI, we will have dangling TelephonyCall objects. 

Also, clearing ni due to bug 890912 is moved out from koi+.

> 
> // AOSP snippet, [e-mail]
>         if (mmi == null) {
>             return mCT.dial(newDialString, uusInfo);
>         } else if (mmi.isTemporaryModeCLIR()) {
>             return mCT.dial(mmi.mDialingNumber, mmi.getCLIRMode(), uusInfo);
>         } else {
>             mPendingMMIs.add(mmi);
>             mMmiRegistrants.notifyRegistrants(new AsyncResult(null, mmi,
> null));
>             mmi.processCode();
> 
>             // FIXME should this return null or something else?
>             return null;
>         }

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> To unify dial() and sendMMI(), the draft picture of the proposal on my mind
> is like:
> 1) discard mozMobileConnection.sendMMI()
> 2) mozTelephony.dial() returns a Promise. If the gecko determines the input
> string is a normal call number, then gecko makes a call and the
> Promise.result is a TelephonyCall. If the string is MMI code, gecko does
> corresponding request and Promise.result would be a any object or a string
> containing the information of the operation.
>

That looks good to me. I would also add an event to let the consumer know that an MMI is being sent (as Anshul mentions in comment 24) so a "Sending"-like UI can be shown as we currently do in Gaia.
 
> I am not sure whether we should do this change in v1.2 as the change might
> large but it's worth being discussed right now. So that we can have a better
> idea of how to keep bug 890912 on at the moment.
> 
> Any comment?

An alternative for an easier solution to face v1.2 requirements would be exposing an "isMMI()" function as suggested in comment 4. I could work on that right away.

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Fernando Jiménez Moreno [:ferjm] (PTO from 30/7 to 26/8) from comment #27)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> > To unify dial() and sendMMI(), the draft picture of the proposal on my mind
> > is like:
> > 1) discard mozMobileConnection.sendMMI()
> > 2) mozTelephony.dial() returns a Promise. If the gecko determines the input
> > string is a normal call number, then gecko makes a call and the
> > Promise.result is a TelephonyCall. If the string is MMI code, gecko does
> > corresponding request and Promise.result would be a any object or a string
> > containing the information of the operation.
> >
> 
> That looks good to me. I would also add an event to let the consumer know
> that an MMI is being sent (as Anshul mentions in comment 24) so a
> "Sending"-like UI can be shown as we currently do in Gaia.

Welcome back, Fernando! 

Isn't Promise.result is enough? If we are sending MMI, then gaia could tell from the Promise.result. We need to determine the format for the result so that Gaia could use it easily.

>  
> > I am not sure whether we should do this change in v1.2 as the change might
> > large but it's worth being discussed right now. So that we can have a better
> > idea of how to keep bug 890912 on at the moment.
> > 
> > Any comment?
> 
> An alternative for an easier solution to face v1.2 requirements would be
> exposing an "isMMI()" function as suggested in comment 4. I could work on
> that right away.

Bug 890912 isn't in v1.2 now. May I ask which user story needs isMMI() function?

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #28)
> (In reply to Fernando Jiménez Moreno [:ferjm] (PTO from 30/7 to 26/8) from
> comment #27)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> > > To unify dial() and sendMMI(), the draft picture of the proposal on my mind
> > > is like:
> > > 1) discard mozMobileConnection.sendMMI()
> > > 2) mozTelephony.dial() returns a Promise. If the gecko determines the input
> > > string is a normal call number, then gecko makes a call and the
> > > Promise.result is a TelephonyCall. If the string is MMI code, gecko does
> > > corresponding request and Promise.result would be a any object or a string
> > > containing the information of the operation.
> > >
> > 
> > That looks good to me. I would also add an event to let the consumer know
> > that an MMI is being sent (as Anshul mentions in comment 24) so a
> > "Sending"-like UI can be shown as we currently do in Gaia.
> 
> Welcome back, Fernando!

Thanks! :)

> 
> Isn't Promise.result is enough? If we are sending MMI, then gaia could tell
> from the Promise.result. We need to determine the format for the result so
> that Gaia could use it easily.

If I understand it correctly, the Promise won't be resolved until the MMI is already sent, am I right? That means that we won't be able to show the appropriate "Sending"-like screen while the MMI operation is being done. Some MMI related functionality might take a while to complete (i.e: Query call forwarding status), so we need to be able to know that there is an MMI function on going so we can provide the appropriate feedback to the user.

> >  
> > > I am not sure whether we should do this change in v1.2 as the change might
> > > large but it's worth being discussed right now. So that we can have a better
> > > idea of how to keep bug 890912 on at the moment.
> > > 
> > > Any comment?
> > 
> > An alternative for an easier solution to face v1.2 requirements would be
> > exposing an "isMMI()" function as suggested in comment 4. I could work on
> > that right away.
> 
> Bug 890912 isn't in v1.2 now. May I ask which user story needs isMMI()
> function?

Per https://bugzilla.mozilla.org/show_bug.cgi?id=890912#c15 it seems that the main reason for taking Bug 890912 out of v1.2 was the uncertainty of this sendMMI/dial issue being solved in time for the v1.2 deadline. So I was just wondering if a more straight and temporary approach, like the isMMI() function, could be considered for v1.2. Even if I fully agree that the right way to do it is by unifying sendMMI() and dial() as suggested before, I think that the product team needs to know that there are alternatives to what the eng team considers the right choice so they can evaluate if its worth the time and effort of implementing a temporary solution.

Bug 857522 which is koi? also seems to be blocked by this issue.

Comment 30

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Okay... the above discussion is all based on gsm networks. The thing is totally different if we are considering cdma networks.

Per my understanding, there is no standard for cdma MMI/USSD format. The service code is defined by operators. And as the service code is defined by operators, I don't think we should maintain the parsing rules for variant operators in Gecko code. It would be nice to have a separate helper utility, such as PhoneNumberUtility, as a plugin to check if the string is MMI/service code for that. In this way, no matter how we have a unified API or not, the utility is needed. In this situation, I am wondering the necessity and advantage of unifying dial() and sendMMI().

Comment 31

[Anshul]

If there is no blocking requirement from carriers I suggest we fix it the right way by unifying the SendMMI and Dial requests rather than introducing the IsMmi check and then redesign later.

Comment 32

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Anshul from comment #31)
> If there is no blocking requirement from carriers I suggest we fix it the
> right way by unifying the SendMMI and Dial requests rather than introducing
> the IsMmi check and then redesign later.

Taking cdma into account, I am not sure whether unifying sendMMI() and dial() APIs is the right way...

Comment 33

[Anshul]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #32)
> (In reply to Anshul from comment #31)
> > If there is no blocking requirement from carriers I suggest we fix it the
> > right way by unifying the SendMMI and Dial requests rather than introducing
> > the IsMmi check and then redesign later.
> 
> Taking cdma into account, I am not sure whether unifying sendMMI() and
> dial() APIs is the right way...
Hsin-Yi, for CDMA we can skip checking for MMI altogether in RIL. Cases likes in call supplemental services and short code USSD would need hacks (like a call to IsMMI()) to get to work without unifying them.

Comment 34

[Noemí Freire (:noemi)]

Hi,

Is there any decision about the way we should proceed?. Thanks!

Comment 35

[Joe Cheng [:jcheng] (please needinfo)]

this is about whether we want to support "in-call" mmi?
1.3? into roadmap discussions

Comment 36

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Etienne,

Due to the difficulties in parsing MMI in gaia, here you see a proposal (comment 27) to merge sendMMI() and dial(). It would be great to have more feedbacks from dialer developers. Does it make sense to you? Thank you.

Comment 37

[Etienne Segonzac (:etienne)]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #36)
> Hi Etienne,
> 
> Due to the difficulties in parsing MMI in gaia, here you see a proposal
> (comment 27) to merge sendMMI() and dial(). It would be great to have more
> feedbacks from dialer developers. Does it make sense to you? Thank you.

Sounds good.

My only concern is to keep the delay between the press of the call button and the opening of the call screen as short as possible.
And we need to know that we're handling a TelephonyCall (and not a mmi code) before opening the call screen.

Currently we start loading/opening the call screen as soon as we get a truthy value from |telephony.dial()| [1].

I guess we're hiding a bunch of IPC stuff behind the sync telephony.dial() already, and that waiting for the promise to be fulfilled with a result of type TelephonyCall should be equivalent.

But I just want to make sure this hypothesis is correct :)

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/dialer.js#L294

Comment 38

[Joe Cheng [:jcheng] (please needinfo)]

triage: add to backlog bug 891754

Comment 39

[Anshul]

Can we please address this issue in 1.4?

Comment 40

[Hsin-Yi Tsai (she/her) [:hsinyi]]

1.4? per comment 39.

Note this bug is for gecko part and gaia needs according work to use the newly introduced API, otherwise dialer app will be definitely broken.

Comment 41

[Michael Schwartz [:m4]]

I've filed a bug that may be a dup of this issue https://bugzilla.mozilla.org/show_bug.cgi?id=958733  Basically CDMA needs a fix that could be addressed by this.

One thing that hasn't been mentioned in this conversation is the wrench that Call Control from Sim Application Tookit (3GPP 31.111) mucks with things.  Basically that feature allows for toolkit to change a dial string from MMI to a regular call, vice versa, and a number of other combinations.

The recommendation by Hsin-Yi in https://bugzilla.mozilla.org/show_bug.cgi?id=889737#c21 would work if the promise would return an object representing the resultant operation (please correct me if that wasn't the recommendation).  Gaia would need to know the type of those objects though so it could determine which transition to initiate and how to display subsequent states.

Also mentioned above is that a minimal amount of call classification mus be performed on every key entry when the primary subscription is a 3gpp flavour of service.  So yes, at least a two part problem.

Comment 42

[Ken Chang[:kenkai]]

Aknow, please help to fix this bug.

Comment 43

[Szu-Yu Chen [:aknow] (inactive)]

Hsinyi,

In addition to Comment 21, the unified function could either return (1) Promise or (2) DOMRequest.
For (1), the promise could be resolved by different type of object.
For (2), similar, we put different type of object in request.result.
Do you have any preference?

For both of them, gaia could use (result instanceof TelephonyCall) to distinguish the call from mmi code.

Comment 44

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #43)
> Hsinyi,
> 
> In addition to Comment 21, the unified function could either return (1)
> Promise or (2) DOMRequest.
> For (1), the promise could be resolved by different type of object.
> For (2), similar, we put different type of object in request.result.
> Do you have any preference?
> 
> For both of them, gaia could use (result instanceof TelephonyCall) to
> distinguish the call from mmi code.

For newly introduced API, I think we should return Promise.

Comment 45

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (WIP) promise dial

Hsinyi,
Just a prototype to illustrate the concept and didn't considering any memory handling. The real unified logic is not yet handled. However it provide some different result hard coded in telephnyProvider.js. You may check test_superdial.js for the use case.

Comment 46

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (WIP) #2 promise dial

Sorry, I forgot to include the test file in previous attachment.

Comment 47

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Michael Schwartz [:m4] from comment #41)
> I've filed a bug that may be a dup of this issue
> https://bugzilla.mozilla.org/show_bug.cgi?id=958733  Basically CDMA needs a
> fix that could be addressed by this.
> 
> One thing that hasn't been mentioned in this conversation is the wrench that
> Call Control from Sim Application Tookit (3GPP 31.111) mucks with things. 
> Basically that feature allows for toolkit to change a dial string from MMI
> to a regular call, vice versa, and a number of other combinations.
> 
MozRIL doesn't support call control now, but if I understand the STK call control flow correctly, the flow will look like: before we parse the number string, we will ask STK to see if the number needs to be modified. If yes, we will get a new number from STK. We parse the new number and then return the corresponding result object to gaia.

There should be no problem with STK call control as well.

> The recommendation by Hsin-Yi in
> https://bugzilla.mozilla.org/show_bug.cgi?id=889737#c21 would work if the
> promise would return an object representing the resultant operation (please
> correct me if that wasn't the recommendation).  Gaia would need to know the
> type of those objects though so it could determine which transition to
> initiate and how to display subsequent states.
> 
> Also mentioned above is that a minimal amount of call classification mus be
> performed on every key entry when the primary subscription is a 3gpp flavour
> of service.  So yes, at least a two part problem.

Comment 48

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8364904 [details] [diff] [review]
(WIP) #2 promise dial

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

Hi Aknow,

Thanks for the patches.
I haven't reviewed all of them but I left some notes on interface parts. Please take a look.
I'd like to see if there's anything we miss from gaia perspective. So, please separate the patches into different parts; it's easier to invite more people to discuss the API. Thank you again :)

::: dom/telephony/Telephony.cpp
@@ +16,5 @@
>  #include "nsCharSeparatedTokenizer.h"
>  #include "nsContentUtils.h"
>  #include "nsCxPusher.h"
>  #include "nsNetUtil.h"
> +#include "nsRefPtrHashtable.h"

We don't need this here as it is in Telephony.h.

@@ +405,5 @@
> +{
> +  uint32_t serviceId = ProvidedOrDefaultServiceId(aServiceId);
> +  if (!IsValidNumber(aNumber) || !IsValidServiceId(serviceId)) {
> +    aRv.Throw(NS_ERROR_INVALID_ARG);
> +    return nullptr;

For these error cases, shouldn't we call promise.reject()? Of course, we need to dispatch the task to the next tick.

::: dom/telephony/nsITelephonyProvider.idl
@@ +130,5 @@
>     */
>    void notifyConferenceError(in AString name,
>                               in AString message);
> +
> +  void notifyDialStart(in boolean isCall);

This is the notification of mmi being sending, right?

And, I know we agreed this architecture before, but I'd prefer to a 1-to-1 communication after second thought. Sorry for that :(

We could refer to 'nsIMmsService' and 'nsIMobileMessageCallback.' Here we might need one more callback interface, and move 'notifyDialStart' and 'notifyDialComplete' to the callback interface.

And I think once we have a 1-1 communication, we won't need gTelephonyList anymore...

@@ +131,5 @@
>    void notifyConferenceError(in AString name,
>                               in AString message);
> +
> +  void notifyDialStart(in boolean isCall);
> +  void notifyDialComplete(in boolean isCall, in AString json);

This is for notifying the Promise is ready to resolve, right?
And what about "reject"?

::: dom/telephony/test/marionette/test_superdial.js
@@ +23,5 @@
> +
> +startTest(function() {
> +  test_mmi()
> +  .then(test_call)
> +  .then(finish);

Thanks for the use case. Yup, it's what I imagine. :)

::: dom/webidl/Telephony.webidl
@@ +16,5 @@
>     * |navigator.mozMobileConnections.length|.
>     */
>  
>    [Throws]
> +  Promise superDial(DOMString number, optional unsigned long serviceId);

This is just a temp name, right...

@@ +25,5 @@
>    [Throws]
>    TelephonyCall dialEmergency(DOMString number, optional unsigned long serviceId);
>  
>    [Throws]
>    void startTone(DOMString tone, optional unsigned long serviceId);

We need one more event to indicate MMI is sending, requested from comment 27.

Comment 49

[Michael Schwartz [:m4]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #47)
Looking at the ril.h API, call control is actually performed automatically by the modem and then we are notified of the change.  So the flow might look like this:

1. Gaia send the dial string to Gecko
2. Gaia waits for notification from Gecko on whether a SS or call was initiated

Comment 50

[shawn ku [:sku]]

(In reply to Michael Schwartz [:m4] from comment #49)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #47)
> Looking at the ril.h API, call control is actually performed automatically
> by the modem and then we are notified of the change.  So the flow might look
> like this:
> 
> 1. Gaia send the dial string to Gecko
> 2. Gaia waits for notification from Gecko on whether a SS or call was
> initiated

Yes, it should be as what Michael said.
Call Control by SIM is done by modem side, Gecko/Gaia only perform call request as normal call do.

The only different should be extra call state change to inform informatin (ex: number) changed.

Comment 51

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to shawn ku [:sku] from comment #50)
> (In reply to Michael Schwartz [:m4] from comment #49)
> > (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #47)
> > Looking at the ril.h API, call control is actually performed automatically
> > by the modem and then we are notified of the change.  So the flow might look
> > like this:
> > 
> > 1. Gaia send the dial string to Gecko
> > 2. Gaia waits for notification from Gecko on whether a SS or call was
> > initiated
> 

Yes, this is exactly the flow the bug is trying to provide. We are on the same page for sure.

> Yes, it should be as what Michael said.
> Call Control by SIM is done by modem side, Gecko/Gaia only perform call
> request as normal call do.
> 
> The only different should be extra call state change to inform informatin
> (ex: number) changed.

Thanks for clarification! Yup, I know this is QC's solution (call control done by modem), but I am not very sure if done by modem side is defined in specification, applied to all modem vendors. Per my understanding call control can also be done via STK command, REQUEST_STK_SEND_ENVELOPE_WITH_STATUS, based on TS 11.14.

However, given the fact that call control doesn't modify a normal dial number into MMI code, no matter call control is done by modem or by RIL/gecko, it's not problem with the API Aknow is working on. MozRIL and DOM code has already addressed the case of number modification, and the modification is reflected on gaia.

Comment 52

[Michael Schwartz [:m4]]

(In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment #51)
> However, given the fact that call control doesn't modify a normal dial
> number into MMI code

Why do you say that?  That case is supported although perhaps rarely used.

Comment 53

[shawn ku [:sku]]

(In reply to Michael Schwartz [:m4] from comment #52)
> (In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment
> #51)
> > However, given the fact that call control doesn't modify a normal dial
> > number into MMI code
> 
> Why do you say that?  That case is supported although perhaps rarely used.

Hi Michael:
 It's my bad to pass the wrong information to HsinYi.
And, thanks for your correction here.


"For example, a call request can be replaced by a supplementary service operation
or a USSD operation, and vice-versa."


TS 151 014 Clause 4.5
4.5 Call control by SIM
When this service is activated by the SIM, all dialled digit strings, supplementary service control strings and USSD
strings are first passed to the SIM before the ME sets up the call, the supplementary service operation or the USSD
operation. The ME shall also pass to the SIM at the same time its current serving cell. The SIM has the ability to allow,
bar or modify the call, the supplementary service operation or the USSD operation. The SIM also has the ability to
replace a call request, a supplementary service operation or a USSD operation by another call request or supplementary
service operation or USSD operation. For example, a call request can be replaced by a supplementary service operation
or a USSD operation, and vice-versa.

Comment 54

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to shawn ku [:sku] from comment #53)
> (In reply to Michael Schwartz [:m4] from comment #52)
> > (In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment
> > #51)
> > > However, given the fact that call control doesn't modify a normal dial
> > > number into MMI code
> > 
> > Why do you say that?  That case is supported although perhaps rarely used.
> 
> Hi Michael:
>  It's my bad to pass the wrong information to HsinYi.
> And, thanks for your correction here.
> 
> 
> "For example, a call request can be replaced by a supplementary service
> operation
> or a USSD operation, and vice-versa."
> 
> 
> TS 151 014 Clause 4.5
> 4.5 Call control by SIM
> When this service is activated by the SIM, all dialled digit strings,
> supplementary service control strings and USSD
> strings are first passed to the SIM before the ME sets up the call, the
> supplementary service operation or the USSD
> operation. The ME shall also pass to the SIM at the same time its current
> serving cell. The SIM has the ability to allow,
> bar or modify the call, the supplementary service operation or the USSD
> operation. The SIM also has the ability to
> replace a call request, a supplementary service operation or a USSD
> operation by another call request or supplementary
> service operation or USSD operation. For example, a call request can be
> replaced by a supplementary service operation
> or a USSD operation, and vice-versa.

Hi Michael and Shawn,

Thanks for the correction. Great to learn more :)

Given the fact modem would manage call control and the dial number might be changed from a normal call number to a USSD operation, I'd like to know if modem will send out any unsolicited event to RIL, informing a USSD instead of a normal call is sending out. Do you have any idea? Thanks again!

Comment 55

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment #54)
> (In reply to shawn ku [:sku] from comment #53)
> > (In reply to Michael Schwartz [:m4] from comment #52)
> > > (In reply to Hsin-Yi Tsai (OOO Jan. 29 ~ Feb. 5th) [:hsinyi] from comment
> > > #51)
> > > > However, given the fact that call control doesn't modify a normal dial
> > > > number into MMI code
> > > 
> > > Why do you say that?  That case is supported although perhaps rarely used.
> > 
> > Hi Michael:
> >  It's my bad to pass the wrong information to HsinYi.
> > And, thanks for your correction here.
> > 
> > 
> > "For example, a call request can be replaced by a supplementary service
> > operation
> > or a USSD operation, and vice-versa."
> > 
> > 
> > TS 151 014 Clause 4.5
> > 4.5 Call control by SIM
> > When this service is activated by the SIM, all dialled digit strings,
> > supplementary service control strings and USSD
> > strings are first passed to the SIM before the ME sets up the call, the
> > supplementary service operation or the USSD
> > operation. The ME shall also pass to the SIM at the same time its current
> > serving cell. The SIM has the ability to allow,
> > bar or modify the call, the supplementary service operation or the USSD
> > operation. The SIM also has the ability to
> > replace a call request, a supplementary service operation or a USSD
> > operation by another call request or supplementary
> > service operation or USSD operation. For example, a call request can be
> > replaced by a supplementary service operation
> > or a USSD operation, and vice-versa.
> 
> Hi Michael and Shawn,
> 
> Thanks for the correction. Great to learn more :)
> 
> Given the fact modem would manage call control and the dial number might be
> changed from a normal call number to a USSD operation, I'd like to know if
> modem will send out any unsolicited event to RIL, informing a USSD instead
> of a normal call is sending out. Do you have any idea? Thanks again!

More question, in the case of changing normal dial number to USSD/MMI, will modem return success for REQUEST_DIAL? If the REQUEST_DIAL fails, could we get a corresponding fail cause by querying "RIL_REQUEST_LAST_CALL_FAIL_CAUSE"? Thank you.

Comment 56

[Michael Schwartz [:m4]]

Hi Hsin-Yi and Szu-Yu, I need to verify the exact call flow scenarios.  Please standy by.  Thanks, M

Comment 57

[Michael Schwartz [:m4]]

Hi Hsin-Yi and Szu-Yu,

When a dial request is made, RIL can notify Gecko if a CC has occurred and what kind of change has happened.  After that, we can notify the result of the operation as normal.

M

Comment 58

[Michael Schwartz [:m4]]

To be more specific, the notification of CC can be sent as part of the result to the dial request.

Comment 59

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Michael Schwartz [:m4] from comment #58)
> To be more specific, the notification of CC can be sent as part of the
> result to the dial request.

Thanks, Michael.

Yeah, the request response is specified according to ril.h.

/**
 * RIL_REQUEST_DIAL
 *
 * Initiate voice call
 *
 * "data" is const RIL_Dial *
 * "response" is NULL
 *
 * This method is never used for supplementary service codes
 *
 * Valid errors:
 *  SUCCESS
 *  RADIO_NOT_AVAILABLE (radio resetting)
 *  DIAL_MODIFIED_TO_USSD
 *  DIAL_MODIFIED_TO_SS
 *  DIAL_MODIFIED_TO_DIAL
 *  GENERIC_FAILURE
 */
#define RIL_REQUEST_DIAL 10

Comment 60

[Joe Cheng [:jcheng] (please needinfo)]

No longer blocks: 960372, please reconsider blocking status

Comment 61

[Ken Chang[:kenkai]]

This isn't in blocker list provided by partner.

Comment 62

[Hsin-Yi Tsai (she/her) [:hsinyi]]

We provide a workaround for bug 958733 which is targeting on 2/28. This issue doesn't block that.

Comment 63

[Szu-Yu Chen [:aknow] (inactive)]

We have some requirements:

1. could use telephony to dial an MMI code
2. know the dialling number is an MMI code ASAP so that we could show a different UI
3. know the response of that MMI code request


My proposal of unified dial():

telephony.dial("some number here") will return a promise and there are 3 cases.
1. rejected when error occur.
2. resolved as a TelephonyCall => it's a call 
3. resolved as an MMICodeRequest => it's an MMI code request, and gecko is currently handle the request.

Focus on item3. When the request finished, 'success' or 'error' event will be trigger on MMICodeRequest object. There will be two attributes 'result' and 'error' (DOMError) in MMICodeRequest showing the corresponding response.

Some example test code.

    telephony.dial(MMICode).then(request => {
      log("MMI code request start");
      is(typeof(request), "object");
      ok(request instanceof MMICodeRequest);

      request.onsuccess = function(event) {
        //log("  success: [" + event.target.result + "]");
      };

      request.onerror = function(event) {
        log("  error: [" + event.target.error.name + "]");
      };
    });

Any suggestion? Let's discuss the web API interface for dialling an MMI code.

Comment 64

[Anthony Ricaud (:rik)]

Flagging myself and Francisco to see if comment 63 works for Dialer and Contacts. I won't be able to look into it before next week though.

Comment 65

[Hsin-Yi Tsai (she/her) [:hsinyi]]

One question: how are we going to do with "cancelMMI()"?

Comment 66

[Szu-Yu Chen [:aknow] (inactive)]

We could add a 'cancel' method on MMICodeRequest.

telephony.dial(MMICode).then(request => {
  ...
  request.cancel();
});

Comment 67

[Francisco Jordano [:arcturus] [:francisco]]

In the case of contacts this will work and will simplify a lot the code:

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/details.js#L470-L488

My only concern is about the resolution, right now we are asking if we have a mmi number, since in contacts we open a web activity for dialer with the mmi code (that just presents the number into the dialer). Perhaps in dialer we want to send the mmi code directly.

Anyway, nothing that we cannot encapsulate in the TelephonyHelper, just passing a callback in case of specific action when having a mmi code.

Comment 68

[Michael Schwartz [:m4]]

(In reply to Francisco Jordano [:arcturus] from comment #67)

Why should contacts have any understanding of mmi?  It should send the string to the dialer and forget about it.

Comment 69

[Francisco Jordano [:arcturus] [:francisco]]

(In reply to Michael Schwartz [:m4] from comment #68)
> (In reply to Francisco Jordano [:arcturus] from comment #67)
> 
> Why should contacts have any understanding of mmi?  It should send the
> string to the dialer and forget about it.

Cause of product requirements contacts must call directly (without going through the dialer) when we have a phone number, we do use the TelephonyHelper (created for sharing the functionality of performing the call).

With MMI is different, we use the web activity to display the phone number in the dialer. So the user can perform the action of calling, to execute the mmi code.

Comment 70

[Anthony Ricaud (:rik)]

I think Contacts can just use telephony.dial and remove MMI specific code with this API in place. Dialer listens to the ussd-received system event. So when contact "calls" a MMI number, Dialer should be launched by the system message.

This API should then be cool to work with. Also note that no code change is needed on the Gaia side to make this "work". We'll need code changes to remove the Gaia side that currently checks if a number is a MMI. We can do that a few days/weeks after the Gecko patch lands. That will help not break any bisection work.

Comment 71

[Gabriele Svelto [:gsvelto]]

(In reply to Anthony Ricaud (:rik) from comment #70)
> This API should then be cool to work with. Also note that no code change is
> needed on the Gaia side to make this "work". We'll need code changes to
> remove the Gaia side that currently checks if a number is a MMI.

And we could finally clean up the MMI unit-tests which are quite messy at the moment with most of them being commented out :-/

Comment 72

[Francisco Jordano [:arcturus] [:francisco]]

Hei!

(In reply to Anthony Ricaud (:rik) from comment #70)
> I think Contacts can just use telephony.dial and remove MMI specific code
> with this API in place. Dialer listens to the ussd-received system event. So
> when contact "calls" a MMI number, Dialer should be launched by the system
> message.

\o/ happy to know that dialer will take care of that then.

Will simplify contacts code too.

Comment 73

[Gabriele Svelto [:gsvelto]]

Do we have a rough idea of when this will land? Currently the MMI code in the dialer is in pretty bad shape and I've been itching to refactor it. This won't be needed if this lands, but if these changes won't be implemented soon (say within the next 3 months) it might still be worth it.

Comment 74

[Szu-Yu Chen [:aknow] (inactive)]

I think it could be landed in 3 months. We are now waiting for a dependent bug (Bug 898445)

Comment 75

[Szu-Yu Chen [:aknow] (inactive)]

Well... I haven't finished the sentences in my previous comment.

What's the effort of refactoring?

This bug won't be implemented very soon (within 1 month), but for 2 or 3 months is probably a good estimation. We are planing to accomplish the following steps.

1. Bug 898445, which rewrite the MobileConnection by webidl (as Telephony's usage)
2. Bug 843452, which rewrite the ipc part of MobileConnection by ipdl (as Telephony's usage)
3. After 1 and 2, all the back-end architecture of MobileConnection and Telephony are the same. We could easily move the API and related code between them (ex: MobileConnection.sendMMI to Telephony.sendMMI)
4. Then, combining Telephony.dial and Telephony.sendMMI is our last step. I have already done some work on this part.

For the shipping date,
- Step 1 seems almost ready.
- Step 2 will be a big effort.
- Steps 3 and 4 could be done within several days.

So, in my opinion, it might required 2 or 3 months.

Comment 76

[Gabriele Svelto [:gsvelto]]

(In reply to Szu-Yu Chen [:aknow] from comment #75)
> What's the effort of refactoring?

We'd need to rethink how we handle USSD and MMI messages in the dialer and re-implement the relevant code. This is not trivial work as that part of the dialer is quite complicated. On the other hand some of this work might be beneficial for later if we manage to better encapsulate the MMI functionality so it might still be worth doing.

Anyway thanks for the clarification; I think 3 months are enough for some refactoring to take place.

Comment 77

[Hsin-Yi Tsai (she/her) [:hsinyi]]

We spread the API discussion to different threads. Here's the collection of the conclusion:
1) unify sendMMI() and dial() as comment 63
2) remove cancelMMI(), instead we do: https://bugzilla.mozilla.org/show_bug.cgi?id=1031193#c8

Thanks!

Comment 78

[Eric Chang [:ericcc] [:echang]]

Dailer cases https://moztrap.mozilla.org/manage/suites/?pagenumber=1&pagesize=100&sortfield=created_on&sortdirection=desc&filter-name=%5Bcomms%5D&filter-name=dialer&filter-name=small
MMI sending cases
https://moztrap.mozilla.org/manage/cases/?pagenumber=1&pagesize=100&sortfield=created_on&sortdirection=desc&filter-tag=2982

Comment 79

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Gecko working branch

Comment 80

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Example showing the progress

I'll keep updating the test case while working on the bug. The test case tried to demonstrate the current ability of API.

Comment 81

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 03: Telephony interface changed

How about dialEmergency? Could it accept an MMI code?

Comment 82

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 01: Add MMIRequest (webidl)

Do we want to expose some attribute in MMIRequest?

Comment 83

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8474450 [details] [diff] [review]
Part 01: Add MMIRequest (webidl)

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

Based on the current UI behaviour, I don't see a need to expose attributes so this looks good to me. Though MMIRequest simply inherits DOMRequest without adding any new attributes, it indeed provides a clear interface type that looks good too. Thank you.

Comment 84

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8474449 [details] [diff] [review]
Part 03: Telephony interface changed

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

Regarding dialEmergency()... I was thinking to restrict dialEmergency() to accepting only call numbers. But in offline discussion, a use case was raised:  what if App wants to query some specific MMI code when phone is in emergencyCallsOnly mode? Then the APP will need to parse the input string before determining which is the right API to call. But that is exactly what we want to improve and discard in this bug... So, I think in this bug we will need to have dialEmergency() accept MMI as well. Does that make sense?

In the meanwhile, I do feel confused about the expected behaviour of dialEmergency... :(

::: dom/webidl/Telephony.webidl
@@ +23,2 @@
>    [Throws]
> +  Promise<any> dial(DOMString number, optional unsigned long serviceId);

Could we use a union type (TelephonyCall or MMIRequest) here? Not sure if it's already supported by codegen.

Just to make sure the bug and the patch scope: you will be providing another patch for covering ussdreceived webidl changes, right?

Comment 85

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #84)
> Comment on attachment 8474449 [details] [diff] [review]
> Part 3: Telephony interface changed
> 
> Review of attachment 8474449 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Regarding dialEmergency()... I was thinking to restrict dialEmergency() to
> accepting only call numbers. But in offline discussion, a use case was
> raised:  what if App wants to query some specific MMI code when phone is in
> emergencyCallsOnly mode? Then the APP will need to parse the input string
> before determining which is the right API to call. But that is exactly what
> we want to improve and discard in this bug... So, I think in this bug we
> will need to have dialEmergency() accept MMI as well. Does that make sense?
> 

Agree.

> In the meanwhile, I do feel confused about the expected behaviour of
> dialEmergency... :(
> 
> ::: dom/webidl/Telephony.webidl
> @@ +23,2 @@
> >    [Throws]
> > +  Promise<any> dial(DOMString number, optional unsigned long serviceId);
> 
> Could we use a union type (TelephonyCall or MMIRequest) here? Not sure if
> it's already supported by codegen.

It's not supported, thus I have to use 'any'.

> Just to make sure the bug and the patch scope: you will be providing another
> patch for covering ussdreceived webidl changes, right?

Yes.

Comment 86

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8474449 [details] [diff] [review]
Part 03: Telephony interface changed

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

Looks good based on comment 85. :)

r=me with the assumption that separate patches for ussdreceived and dialEmergency api change will be provided later.

Comment 87

[Gabriele Svelto [:gsvelto]]

I've started looking into bug 1031175 and I've immediately hit an issue that I'm not sure how we're going to address. What happens when I dial() an MMI code while I'm already in a call (or conference call)? I'm asking because our current flow is the following:

if (the number is an MMI code) {
  Call sendMMI()
  // Do MMI-related stuff
} else {
  if (we're already in a call) {
    1. Set the onheld handler of the current call to invoke dial()
    2. Put the current call on hold
    3. When the current call has been held call dial()
  } else {
    Call dial()
  }
}

With the current implementation when dialing an MMI code during a call the existing call is kept active. With the new interface the first part of the code will go away since it's impossible to know beforehand if we're going to call an MMI number and so we'd put the active call on hold anyway. Is that acceptable UX? Or should we immediately reconnect the call to kind-of simulate the old behavior? Or even better, can we dial() before putting the active call on hold? IIRC the last option is not possible but I might remember incorrectly.

Comment 88

[Szu-Yu Chen [:aknow] (inactive)]

> Or even better, can we dial() before putting the
> active call on hold? IIRC the last option is not possible but I might
> remember incorrectly.

Originally, we put the call on hold in gaia. How about move the same logic into gecko? Gecko has more information to handle the case appropriately. Then, for gaia, it could simply dial out the number.

Comment 89

[Gabriele Svelto [:gsvelto]]

(In reply to Szu-Yu Chen [:aknow] from comment #88)
> Originally, we put the call on hold in gaia. How about move the same logic
> into gecko?

This sounds like a reasonable follow up since not being able to place a new call w/o putting the first one on hold is really an implementation detail. In the meantime are we OK with changing the UX so that sending an MMI code during a call puts it on hold?

Comment 90

[Michael Schwartz [:m4]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #84)
> Regarding dialEmergency()... I was thinking to restrict dialEmergency() to
> accepting only call numbers. But in offline discussion, a use case was
> raised:  what if App wants to query some specific MMI code when phone is in
> emergencyCallsOnly mode? Then the APP will need to parse the input string
> before determining which is the right API to call. But that is exactly what
> we want to improve and discard in this bug... So, I think in this bug we
> will need to have dialEmergency() accept MMI as well. Does that make sense?

Could there be some confusion between emergency mode, also known as limited service from the network, and the UI created limited dialing state?  dialEmergency is an API used for cases when the caller does not have access to make calls but needs access to make only emergency calls ie. the lockscreen is locked so the user doesn't have access to make calls until the unlock code has been entered.  The only entity needing dialEmergency should be the lockscreen, right?  That means only emergency calls are allowed and most MMI would be rejected.

(In reply to Gabriele Svelto [:gsvelto] from comment #89)
> In the meantime are we OK with changing the UX so that sending an MMI code
> during a call puts it on hold?

In short, we cannot do that as it would be a GCF violation for in-call SS cases (#4 below).  Please see Figure 3.5.3.2 from ETSI TS 122 030 V11.0.0.  To summarize, we need to handle:

1. Out of call SS not requiring SEND (03, 04, 05, 06)
2. Out of call SS requiring SEND (CFX, CLIP, CLIR, etc)
3. #-String
4. Short string (3GPP 22.030 section 6.5.5 Handling of supplementary services within a call)
5. Normal dial string

UX for #4 would be different than other MMI so not sure if we need a differentiation in the messaging to accommodate that.

Comment 91

[Szu-Yu Chen [:aknow] (inactive)]

> Could there be some confusion between emergency mode, also known as limited
> service from the network, and the UI created limited dialing state? 
> dialEmergency is an API used for cases when the caller does not have access
> to make calls but needs access to make only emergency calls ie. the
> lockscreen is locked so the user doesn't have access to make calls until the
> unlock code has been entered.  The only entity needing dialEmergency should
> be the lockscreen, right?  That means only emergency calls are allowed and
> most MMI would be rejected.

I remembered that in current design, gaia also uses dialEmergency when mobileconnection said that the phone is in emergencyOnly mode, e.g, flight mode. Thinking about checking IMEI during flight mode or w/o sim card, the dialEmergency will be called with an MMI code. It should be workable, so maybe we shall only use dialEmergency in lock screen and use dial in other cases. Then in dialEmergency, we could safely reject all numbers except emergency.

Comment 92

[Gabriele Svelto [:gsvelto]]

(In reply to Michael Schwartz [:m4] from comment #90)
> In short, we cannot do that as it would be a GCF violation for in-call SS
> cases (#4 below).  Please see Figure 3.5.3.2 from ETSI TS 122 030 V11.0.0. 

Nasty, then we have to make it so that gaia doesn't need to put a call on hold first before being able to make a second one. We should be able to call dial() from Gaia directly under all conditions and have Gecko decide what needs to happen for it to work (e.g. put the first call on hold if there's one already). I'll post a WIP of my current work on bug 1031175 but I'm starting to get the feeling that this won't be completed very soon due to the Gecko dependency.

Comment 93

[Gabriele Svelto [:gsvelto]]

(In reply to Szu-Yu Chen [:aknow] from comment #91)
> I remembered that in current design, gaia also uses dialEmergency when
> mobileconnection said that the phone is in emergencyOnly mode, e.g, flight
> mode.

Correct.

Comment 94

[Michael Schwartz [:m4]]

(In reply to Gabriele Svelto [:gsvelto] from comment #93)
> (In reply to Szu-Yu Chen [:aknow] from comment #91)
> > I remembered that in current design, gaia also uses dialEmergency when
> > mobileconnection said that the phone is in emergencyOnly mode, e.g, flight
> > mode.
> 
> Correct.

That's a bug.  Gecko should do that check.

Comment 95

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [proposal] ussd interface

Originally, we design to add a new object ussdSession in ussdReceivedEvent so that we could use it to send the continued ussd code on the same session.

However, it looks like gaia is using system message for ussd and I don't know how to add this kind of object in the system message. 

So I think that a simple solution is to create a special api for the purpose (sending ussd code on the existed session). Now, I called it "replyUSSD" though I don't really like the name.

All the message passed into the function will always be treated as ussd code and sent to the network. Also, it will check whether a existed ussd session is existed. Calling the api w/o a existing session results an error (promise reject).

Any idea for the better API design?

Comment 96

[Gabriele Svelto [:gsvelto]]

(In reply to Szu-Yu Chen [:aknow] from comment #95)
> Originally, we design to add a new object ussdSession in ussdReceivedEvent
> so that we could use it to send the continued ussd code on the same session.
> 
> However, it looks like gaia is using system message for ussd and I don't
> know how to add this kind of object in the system message. 

You'll have to modify the USSDReceivedEvent object to hold the new session object:

http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/webidl/USSDReceivedEvent.webidl

The currently existing fields (message, sessionEnded) could be moved into the session object for extra cleanliness. Then you'll need to modify the code dealing with the object:

http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/RadioInterfaceLayer.js#l3472
http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/ril_worker.js#l6908
http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/RILContentHelper.js#l1751

> So I think that a simple solution is to create a special api for the purpose
> (sending ussd code on the existed session). Now, I called it "replyUSSD"
> though I don't really like the name.
> 
> All the message passed into the function will always be treated as ussd code
> and sent to the network. Also, it will check whether a existed ussd session
> is existed. Calling the api w/o a existing session results an error (promise
> reject).

No, please. The idea here is to get rid of the MMI-specific functions (i.e. mozMobileConnection.sendMMI) to provide a better abstraction for this functionality. The original proposal would have had these two effects:

- User code doesn't need to whitelist MMI numbers anymore to send them with a separate function but can just rely on dial

- The USSD session that is currently hidden becomes a visible object so that its life-cycle can be followed by the Gaia/user code. Note that this is one of the biggest problems right now, since the USSD session state is hidden from gaia one never knows what effect sendMMI() is going to have: will it start a new session or continue an existing one? Is there only one available at the same time or not? Etc... Exposing the session object in the ussd-received event solves this problem by giving the user code an object that can be used to manage the session properly (reply/cancel). In this proposal the hidden session would be reintroduced since replyUSSD() can be called only if there's an active session but there's no way to tell if there is one in the first place.

Comment 97

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Gabriele Svelto [:gsvelto] from comment #96)
> (In reply to Szu-Yu Chen [:aknow] from comment #95)
> > Originally, we design to add a new object ussdSession in ussdReceivedEvent
> > so that we could use it to send the continued ussd code on the same session.
> > 
> > However, it looks like gaia is using system message for ussd and I don't
> > know how to add this kind of object in the system message. 
> 
> You'll have to modify the USSDReceivedEvent object to hold the new session
> object:
> 
> http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/webidl/
> USSDReceivedEvent.webidl
> 
> The currently existing fields (message, sessionEnded) could be moved into
> the session object for extra cleanliness. Then you'll need to modify the
> code dealing with the object:
> 
> http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/
> RadioInterfaceLayer.js#l3472
> http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/
> ril_worker.js#l6908
> http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/
> RILContentHelper.js#l1751

Yes, I could do this. However, I believe that current gaia relies on system message to wake up the app. If I put the ussdSession in the event, the app might miss that event because it doesn't have enough time to register the event listener.

Here is the problem I don't know how to handle.
http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/RadioInterfaceLayer.js#l3471
How to include a session object in the system message?

> > So I think that a simple solution is to create a special api for the purpose
> > (sending ussd code on the existed session). Now, I called it "replyUSSD"
> > though I don't really like the name.
> > 
> > All the message passed into the function will always be treated as ussd code
> > and sent to the network. Also, it will check whether a existed ussd session
> > is existed. Calling the api w/o a existing session results an error (promise
> > reject).
> 
> No, please. The idea here is to get rid of the MMI-specific functions (i.e.
> mozMobileConnection.sendMMI) to provide a better abstraction for this
> functionality. The original proposal would have had these two effects:
> 
> - User code doesn't need to whitelist MMI numbers anymore to send them with
> a separate function but can just rely on dial
> 
> - The USSD session that is currently hidden becomes a visible object so that
> its life-cycle can be followed by the Gaia/user code. Note that this is one
> of the biggest problems right now, since the USSD session state is hidden
> from gaia one never knows what effect sendMMI() is going to have: will it
> start a new session or continue an existing one? Is there only one available
> at the same time or not? Etc... Exposing the session object in the
> ussd-received event solves this problem by giving the user code an object
> that can be used to manage the session properly (reply/cancel). In this
> proposal the hidden session would be reintroduced since replyUSSD() can be
> called only if there's an active session but there's no way to tell if there
> is one in the first place.

There will be at most one session at a time. The gaia/user could only choose either staring a new session or sending on an existing one.

The flow in my mind is that:
1. Always use telephony.dial(). Don't worry about the number.
2. If it is an ussd code, network will send back the response. Then we get ussd-receive event (system message).
3. There is a flag sessionEnd in the content indicating whether the session is finished.
4. If it is not finished, we use replyUSSD to keep the interactive communication between network and phone.

Comment 98

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Szu-Yu Chen [:aknow] from comment #97)
> (In reply to Gabriele Svelto [:gsvelto] from comment #96)
> > (In reply to Szu-Yu Chen [:aknow] from comment #95)
> > > Originally, we design to add a new object ussdSession in ussdReceivedEvent
> > > so that we could use it to send the continued ussd code on the same session.
> > > 
> > > However, it looks like gaia is using system message for ussd and I don't
> > > know how to add this kind of object in the system message. 
> > 
> > You'll have to modify the USSDReceivedEvent object to hold the new session
> > object:
> > 
> > http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/webidl/
> > USSDReceivedEvent.webidl
> > 
> > The currently existing fields (message, sessionEnded) could be moved into
> > the session object for extra cleanliness. Then you'll need to modify the
> > code dealing with the object:
> > 
> > http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/
> > RadioInterfaceLayer.js#l3472
> > http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/
> > ril_worker.js#l6908
> > http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/
> > RILContentHelper.js#l1751
> 
> Yes, I could do this. However, I believe that current gaia relies on system
> message to wake up the app. If I put the ussdSession in the event, the app
> might miss that event because it doesn't have enough time to register the
> event listener.
> 
> Here is the problem I don't know how to handle.
> http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/
> RadioInterfaceLayer.js#l3471
> How to include a session object in the system message?
> 

Oh, I just learned that it seems we could take advantage of nsISystemMessagesWrapper?! I am not 100% sure, more study needed :)

Otherwise, if gaia still relies on the system message, and if there's no current framework for us to include a session object in a system message, attachment 8477350 [details] [diff] [review] looks fine for me.

Comment 99

[Szu-Yu Chen [:aknow] (inactive)]

> > Here is the problem I don't know how to handle.
> > http://hg.mozilla.org/mozilla-central/file/0b9dd32d1e16/dom/system/gonk/
> > RadioInterfaceLayer.js#l3471
> > How to include a session object in the system message?
> > 
> 
> Oh, I just learned that it seems we could take advantage of
> nsISystemMessagesWrapper?! I am not 100% sure, more study needed :)

Thanks. I have traced the related code and it seems possible. I am working on the solution based on it.

Comment 100

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8477350 [details] [diff] [review]
[proposal] ussd interface

I filed a new Bug 1058397 - [B2G][Telephony] add UssdSession API. Let's move ussd session discussion and implementation there, thank you.

Comment 101

[Kai-Chih Hu [:khu]]

Confirmed with EM/EPM, and this can be landed before FL.

Comment 102

[Gabriele Svelto [:gsvelto]]

Szu-Yu, Hsin-Yi, can we take a step back here and start with writing down a full proposal with all the API changes to see if it fits the bill? I think we've rushed too fast to try and implement something before having had a good view of the full picture. For example, why would we need a separate replyMMI() method? Once an USSD session has started why can't we just use dial() to reply?

Comment 103

[Anshul]

(In reply to Gabriele Svelto [:gsvelto] from comment #102)
> Szu-Yu, Hsin-Yi, can we take a step back here and start with writing down a
> full proposal with all the API changes to see if it fits the bill? I think
> we've rushed too fast to try and implement something before having had a
> good view of the full picture. For example, why would we need a separate
> replyMMI() method? Once an USSD session has started why can't we just use
> dial() to reply?
Taking liberty to reply on Hsin-Yi's behalf. This is because when you are in a USSD session the rules of MMI don't apply to the data being exchanged in that session.

Comment 104

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Anshul from comment #103)
> (In reply to Gabriele Svelto [:gsvelto] from comment #102)
> > Szu-Yu, Hsin-Yi, can we take a step back here and start with writing down a
> > full proposal with all the API changes to see if it fits the bill? I think
> > we've rushed too fast to try and implement something before having had a
> > good view of the full picture. For example, why would we need a separate
> > replyMMI() method? Once an USSD session has started why can't we just use
> > dial() to reply?
> Taking liberty to reply on Hsin-Yi's behalf. This is because when you are in
> a USSD session the rules of MMI don't apply to the data being exchanged in
> that session.

Thanks, Anshul :)
This is definitely the answer. No rules or specified patterns for data in a ussd session. Gecko does need additional information to differentiate the request.

Comment 105

[Gabriele Svelto [:gsvelto]]

OK, thanks Anshul, Hsin-Yi. Then putting replyMMI() inside mozTelephony doesn't feel right since it can be used only within an active USSD session. As discussed before we could have an UssdSession object returned via the ussd-received system message; I wouldn't mind if the method was within the ussd-received event object, either works for me. The point is, let's make sure replyMMI() is in a place where calling it makes sense.

Comment 106

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Gabriele Svelto [:gsvelto] from comment #105)
> OK, thanks Anshul, Hsin-Yi. Then putting replyMMI() inside mozTelephony
> doesn't feel right since it can be used only within an active USSD session.
> As discussed before we could have an UssdSession object returned via the
> ussd-received system message; I wouldn't mind if the method was within the
> ussd-received event object, either works for me. 

Does that mean Dialer doesn't need to rely on system message to be waken up in ussd-received case? 
Is the concern (comment 97) below valid?
"However, I believe that current gaia relies on system message to wake up the app. If I put the ussdSession in the event, the app might miss that event because it doesn't have enough time to register the event listener."

> The point is, let's make
> sure replyMMI() is in a place where calling it makes sense.

Comment 107

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 02: Add MMIRequest (dom)

Comment 108

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 04: Test case

Copy the test cases from mobileconnection.
There are some modifications to let them work in telephony.

Comment 109

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 05: Refactoring: Rename DialSuccess to DialCallSuccess

Add 'Call' because we'll add some cases for 'MMI' later

Comment 110

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 06: Refactoring: Change Telephony::Callback to TelephonyCallback

Comment 111

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 07: Refactoring: Embed the response in delete

Comment 112

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 08: Dial MMI (internal interface)

Comment 113

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 09: Dial MMI (callback)

Comment 114

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 10: Dial MMI (ipc)

Comment 115

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 11: Dial MMI (ril)

Comment 116

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8474449 [details] [diff] [review]
Part 03: Telephony interface changed

Kyle, Could you help us review the webidl changes? (DOM peer needed)

Comment 117

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8474450 [details] [diff] [review]
Part 01: Add MMIRequest (webidl)

Kyle, Could you help us review the webidl changes? (DOM peer needed)

It's a DOMRequest but we create another type to make it more meaningful.

Comment 118

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8474449 [details] [diff] [review]
Part 03: Telephony interface changed

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

::: dom/webidl/Telephony.webidl
@@ +23,2 @@
>    [Throws]
> +  Promise<any> dial(DOMString number, optional unsigned long serviceId);

I believe we do support Promise<(This or That)>.  r=me with that change.

Comment 119

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8474450 [details] [diff] [review]
Part 01: Add MMIRequest (webidl)

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

Why do we want to do this?  Has this been discussed on dev-webapi or somewhere similar?

Comment 120

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #119)
> Comment on attachment 8474450 [details] [diff] [review]
> Part 01: Add MMIRequest (webidl)
> 
> Review of attachment 8474450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why do we want to do this?  Has this been discussed on dev-webapi or
> somewhere similar?

Has been discussed in comment 63.

Comment 121

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8479828 [details] [diff] [review]
Part 05: Refactoring: Rename DialSuccess to DialCallSuccess

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

r=me with uuid change

::: dom/telephony/ipc/TelephonyChild.h
@@ +87,1 @@
>                          const nsString& aNumber) MOZ_OVERRIDE;

nit: align the lines

::: dom/telephony/ipc/TelephonyParent.cpp
@@ +540,1 @@
>                                            const nsAString& aNumber)

nit: align the lines.

::: dom/telephony/nsITelephonyService.idl
@@ +188,5 @@
>  
>    /**
>     * Called when a dial request succeeds.
>     */
> +  void notifyDialCallSuccess(in unsigned long callIndex, in AString number);

Don't forget to change the uuid.

Comment 122

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8479825 [details] [diff] [review]
Part 02: Add MMIRequest (dom)

Hi Vicamo, please help review this patch, thank you!

Comment 123

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 08#2: Dial MMI (internal interface)

Comment 124

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 09#2: Dial MMI (callback)

Comment 125

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8479829 [details] [diff] [review]
Part 06: Refactoring: Change Telephony::Callback to TelephonyCallback

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

::: dom/telephony/Telephony.cpp
@@ +24,5 @@
>  
>  #include "CallsList.h"
>  #include "TelephonyCall.h"
>  #include "TelephonyCallGroup.h"
>  #include "TelephonyCallId.h"

nit: super nits, but if I were you, I'd like to use "mozilla/dom/CallsList.h" instead.

::: dom/telephony/TelephonyCallback.cpp
@@ +4,5 @@
> +
> +#include "TelephonyCallback.h"
> +
> +#include "nsJSUtils.h"
> +#include "nsServiceManagerUtils.h"

Why do we need the two headers?

::: dom/telephony/TelephonyCallback.h
@@ +4,5 @@
> +
> +#ifndef mozilla_dom_TelephonyCallback_h
> +#define mozilla_dom_TelephonyCallback_h
> +
> +#include "Telephony.h"

Use "mozilla/dom/Telephony.h" in headers as possible. Or that implies dom/telephony has to be in the inclusion path of a foreign component. I think forwarded declarations for Telephony and Promise will be enough here.

@@ +11,5 @@
> +
> +class nsPIDOMWindow;
> +
> +namespace mozilla {
> +namespace dom {

This is not an implementation of some DOM interface. Please move it into mozilla::dom::telephony.

@@ +23,5 @@
> +  TelephonyCallback(nsPIDOMWindow* aWindow, Telephony* aTelephony,
> +                    Promise* aPromise, uint32_t aServiceId);
> +
> +private:
> +  virtual ~TelephonyCallback() {}

You don't need 'virtual' here because of MOZ_FINAL.

@@ +25,5 @@
> +
> +private:
> +  virtual ~TelephonyCallback() {}
> +
> +  nsCOMPtr<nsPIDOMWindow> mWindow;

mWindow is assigned but never referenced.

::: dom/telephony/moz.build
@@ +14,5 @@
>      'CallsList.h',
>      'MMIRequest.h',
>      'Telephony.h',
>      'TelephonyCall.h',
> +    'TelephonyCallback.h',

I don't think we have to export it.

Comment 126

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 06#2: Refactoring: Change Telephony::Callback to TelephonyCallback

Comment 127

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 09#3: Dial MMI (callback)

Comment 128

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 10#2: Dial MMI (ipc)

Comment 129

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: (Mobileconnection ipdl dependency)

This part of changes come from
Bug 843452 - B2G RIL: use ipdl as IPC in MozMobileConnection

Comment 130

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8480356 [details] [diff] [review]
Part 06#2: Refactoring: Change Telephony::Callback to TelephonyCallback

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

::: dom/telephony/TelephonyCallback.h
@@ +34,5 @@
> +};
> +
> +} // namespace dom
> +} // namespace mozilla
> +} // namespace telephony

nit: wrong order.

Comment 131

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8474450 [details] [diff] [review]
Part 01: Add MMIRequest (webidl)

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

After some discuss with Aknow & HsinYi, we don't have obvious benefit to create a new MMIRequest. So let's remove it for now.

Comment 132

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8479825 [details] [diff] [review]
Part 02: Add MMIRequest (dom)

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

So we don't need this, either.

Comment 133

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #131)
> Comment on attachment 8474450 [details] [diff] [review]
> Part 01: Add MMIRequest (webidl)
> 
> Review of attachment 8474450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After some discuss with Aknow & HsinYi, we don't have obvious benefit to
> create a new MMIRequest. So let's remove it for now.

Yes, I am convinced. Thanks for the feedback and discussion.

Comment 134

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8480330 [details] [diff] [review]
Part 08#2: Dial MMI (internal interface)

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

r=me with that NO_INFORMATION removed.

This patch introduces new methods with jsval as one of its arguments. Considering that jsval-removal is of bug 1047196, I think that's good enough for now. Thank you!

::: dom/telephony/nsITelephonyService.idl
@@ +177,5 @@
>  };
>  
> +%{C++
> +#define NO_INFORMATION 0
> +%}

This IDL is supposed to be parsed and transformed in to C++ header file. This means this macro will appear in every entity that includes "nsITelephonyService.h". I'm afraid that it may cause some problems in the future because it's not retained in any scope. If you really have to do this, please make it a constant attribute of nsITelephonyCallback or so.

But actually I don't think we need this. See below.

@@ +239,5 @@
> +%{C++
> +  // non-virtual so it won't affect the vtable
> +  inline nsresult NotifyDialMMIError(const nsAString& aError)
> +  {
> +    return NotifyDialMMIError(aError, NO_INFORMATION, 0 /* ARGC = 0 */);

NO_INFORMATION is previously defined as 0, but 0 is still a valid value for aInfo. In fact, I think you may simply put a numeric 0 here. Just have:

  return NotifyDialMMIError(aError, 0 /* Don't care */, 0 /* ARGC */);

Comment 135

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8480359 [details] [diff] [review]
Part 10#2: Dial MMI (ipc)

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

::: dom/telephony/ipc/PTelephonyRequest.ipdl
@@ +45,5 @@
>    // dial
>    DialResponseError;
>    DialResponseCallSuccess;
> +  DialResponseMMISuccess;
> +  DialResponseMMIError;

nit: maybe rename DialResponseError to DialResponseCallError in previous patch to be more symmetric?

::: dom/telephony/ipc/TelephonyChild.cpp
@@ +196,4 @@
>  TelephonyRequestChild::DoResponse(const DialResponseError& aResponse)
>  {
>    MOZ_ASSERT(mCallback);
>    mCallback->NotifyDialError(aResponse.name());

And NotifyDialCallError here.

Comment 136

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #135)
> Comment on attachment 8480359 [details] [diff] [review]
> Part 10#2: Dial MMI (ipc)
> 
> Review of attachment 8480359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/ipc/PTelephonyRequest.ipdl
> @@ +45,5 @@
> >    // dial
> >    DialResponseError;
> >    DialResponseCallSuccess;
> > +  DialResponseMMISuccess;
> > +  DialResponseMMIError;
> 
> nit: maybe rename DialResponseError to DialResponseCallError in previous
> patch to be more symmetric?
> 
> ::: dom/telephony/ipc/TelephonyChild.cpp
> @@ +196,4 @@
> >  TelephonyRequestChild::DoResponse(const DialResponseError& aResponse)
> >  {
> >    MOZ_ASSERT(mCallback);
> >    mCallback->NotifyDialError(aResponse.name());
> 
> And NotifyDialCallError here.

Actually, I have no idea which one might be better. "Error" just means the dial request fails. Maybe the dialing string is incorrect. It's neither a call setup nor an MMI code.

Hierarchy is like this.

- notifyError       => promise rejects
- notifyCallSuccess => promise resolves as TelephonyCall
- notifyMMI         => promise resolves as MMI (DOMRequest)
--- notifyMMIError
--- notifyMMISuccess

Comment 137

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 09#4: Dial MMI (callback)

Comment 138

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 04#2: Test case

Comment 139

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8480330 [details] [diff] [review]
Part 08#2: Dial MMI (internal interface)

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

::: dom/telephony/nsITelephonyService.idl
@@ +237,5 @@
> +  void notifyDialMMIError(in AString error, [optional] in unsigned short info);
> +
> +%{C++
> +  // non-virtual so it won't affect the vtable
> +  inline nsresult NotifyDialMMIError(const nsAString& aError)

Just find you don't need the two overloading functions if you have following lines:

  bool
  TelephonyRequestChild::DoResponse(const DialResponseMMIError& aResponse)
  {
    ...
    switch (info.type()) {
      case AdditionalInformation::Tvoid_t:
        mCallback->NotifyDialMMIError(name, 0, 0);
        break;
      case AdditionalInformation::Tuint16_t:
        mCallback->NotifyDialMMIError(name, info.get_uint16_t(), 1);
        break;
      default:
        MOZ_CRASH("Received invalid type!");
        break;
    }

    return true;
  }

Comment 140

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8480359 [details] [diff] [review]
Part 10#2: Dial MMI (ipc)

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

::: dom/telephony/ipc/TelephonyChild.cpp
@@ +245,5 @@
> +  AdditionalInformation info(aResponse.additionalInformation());
> +
> +  switch (info.type()) {
> +    case AdditionalInformation::Tvoid_t:
> +      mCallback->NotifyDialMMIError(name);

mCallback->NotifyDialMMIError(name, 0, 0);

@@ +248,5 @@
> +    case AdditionalInformation::Tvoid_t:
> +      mCallback->NotifyDialMMIError(name);
> +      break;
> +    case AdditionalInformation::Tuint16_t:
> +      mCallback->NotifyDialMMIError(name, info.get_uint16_t());

mCallback->NotifyDialMMIError(name, info.get_uint16_t(), 1);

Comment 141

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8480359 [details] [diff] [review]
Part 10#2: Dial MMI (ipc)

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

::: dom/telephony/ipc/TelephonyChild.cpp
@@ +212,5 @@
> +TelephonyRequestChild::DoResponse(const DialResponseMMISuccess& aResponse)
> +{
> +  MOZ_ASSERT(mCallback);
> +
> +  nsRefPtr<TelephonyCallback> callback = static_cast<TelephonyCallback*>(mCallback.get());

Sorry, jumping back and forth between these patches. But this is not really safe because all that we know is that mCallback is a nsITelephonyCallback. Please place a FIXME before this line. This can only be fixed when we've overloaded NotifyDialMMISuccess in the IDL.

Comment 142

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8480497 [details] [diff] [review]
Part 09#4: Dial MMI (callback)

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

::: dom/telephony/TelephonyCallback.h
@@ +55,5 @@
> +  }
> +
> +  nsresult
> +  NotifyDialMMISuccess(const nsAString& aStatusMessage,
> +                       JS::Handle<JS::Value> aInfo);

nsresult
NotifyDialMMISuccess(JSContext* aCx,
                     const nsAString& aStatusMessage,
                     JS::Handle<JS::Value> aInfo);

@@ +58,5 @@
> +  NotifyDialMMISuccess(const nsAString& aStatusMessage,
> +                       JS::Handle<JS::Value> aInfo);
> +
> +  nsresult
> +  NotifyDialMMISuccess(const MozMMIResult& aResult);

nsresult
NotifyDialMMISuccess(JSContext* aCx,
                     const MozMMIResult& aResult);

Then you don't initialize AutoJSAPI so many times.

Comment 143

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8480499 [details] [diff] [review]
Part 04#2: Test case

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

::: dom/telephony/test/marionette/test_mmi_call_forwarding.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

I don't understand why do we need this. All the tests here have been covered by dom/mobileconnection/tests/marionette/test_mobile_mmi_call_forwarding.js. If all the tests here are testing mozTelephony::dial, then it makes sense, but they're not.

@@ +70,5 @@
> + *        A MozCallForwardingOptions.
> + *
> + * @return A deferred promise.
> + */
> + function setCallForwardingOption(aOptions) {

nit: extra ^\s

Comment 144

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Szu-Yu Chen [:aknow] from comment #136)
> Actually, I have no idea which one might be better. "Error" just means the
> dial request fails. Maybe the dialing string is incorrect. It's neither a
> call setup nor an MMI code.
> 
> Hierarchy is like this.
> 
> - notifyError       => promise rejects
> - notifyCallSuccess => promise resolves as TelephonyCall
> - notifyMMI         => promise resolves as MMI (DOMRequest)
> --- notifyMMIError
> --- notifyMMISuccess

Ok, the original naming makes more sense.

Comment 145

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #143)
> Comment on attachment 8480499 [details] [diff] [review]
> Part 04#2: Test case
> 
> Review of attachment 8480499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/test/marionette/test_mmi_call_forwarding.js
> @@ +1,2 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> I don't understand why do we need this. All the tests here have been covered
> by dom/mobileconnection/tests/marionette/test_mobile_mmi_call_forwarding.js.
> If all the tests here are testing mozTelephony::dial, then it makes sense,
> but they're not.

Yes, they are testing telephony.dial
Please see head.js for the difinition of sendMMI.

Comment 146

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 08#3: Dial MMI (internal interface)

Comment 147

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 09#5: Dial MMI (callback)

Comment 148

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 10#3: Dial MMI (ipc)

Comment 149

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8480499 [details] [diff] [review]
Part 04#2: Test case

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

Szu-Yu points out the most cases are using mozTelephony.dial (via sendMMI in head.js). mozMobileConnection is only used in the beginning and ending.

Comment 150

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8481030 [details] [diff] [review]
Part 09#5: Dial MMI (callback)

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

::: dom/telephony/TelephonyCallback.h
@@ +57,5 @@
> +  NotifyDialMMISuccess(JSContext* aCx, const nsAString& aStatusMessage,
> +                       JS::Handle<JS::Value> aInfo);
> +
> +  nsresult
> +  NotifyDialMMISuccess(JSContext* aCx, const MozMMIResult& aResult);

nit: please also move the two into private section.

@@ +69,5 @@
> +      if (!NS_WARN_IF(jsapi.Init(mWindow))) {
> +        return false;
> +      }
> +
> +      aCx = jsapi.cx();

This will probably bring disasters because that 'Auto'JSAPI should have been destructed at the time you return a pointer via aCx.

Please just do:

  template<typename T>
  nsresult
  NotifyDialMMISuccess(const nsAString& aStatusMessage, const T& aInfo)
  {
    AutoJSAPI jsapi;
    ...
    JSContext* cx = jsapi.cx();
    ...
    return NotifyDialMMISuccess(cx, aStatusMessage, info);
  }

Comment 151

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8481031 [details] [diff] [review]
Part 10#3: Dial MMI (ipc)

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

::: dom/telephony/ipc/TelephonyChild.cpp
@@ +212,5 @@
> +TelephonyRequestChild::DoResponse(const DialResponseMMISuccess& aResponse)
> +{
> +  MOZ_ASSERT(mCallback);
> +
> +  // FIXME: Need to overload NotifyDialMMISuccess in the IDL.

nit: please also mention that mCallback is not necessarily an instance of TelephonyCallback.

Comment 152

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 09#6: Dial MMI (callback)

Comment 153

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8479836 [details] [diff] [review]
Part 11: Dial MMI (ril)

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2320,5 @@
>          if (DEBUG) this.debug("cdma-info-rec-received: " + JSON.stringify(message));
>          gSystemMessenger.broadcastMessage("cdma-info-rec-received", message);
>          break;
> +      case "notifyCFStateChanged":
> +        this._sendCfStateChanged(message);

I didn't find any function named "_sendCfStateChanged", even with bug 843452 patched.

::: dom/system/gonk/ril_worker.js
@@ +6018,5 @@
>      }
>    }
> +
> +  // It's a dial request from telephony.
> +  if (options.parsedMMI) {

But why do we have to sendChromeMessage twice?

@@ +6021,5 @@
> +  // It's a dial request from telephony.
> +  if (options.parsedMMI) {
> +    let notifyOptions = {
> +      rilMessageType: "notifyCFStateChanged",
> +      success: !options.errorMsg,

options.success

::: dom/telephony/gonk/TelephonyService.js
@@ +331,5 @@
> +  _rulesToCallForwardingOptions: function(aRules) {
> +    for (let i = 0; i < aRules.length; i++) {
> +      let info = new CallForwardingOptions(aRules[i]);
> +      aRules[i] = info;
> +    }

I really don't like reassigning a variable with something of a different type. Please do:

  return aRules.map(function(aRule) {
    return new CallForwardingOptions(aRule);
  });

@@ +678,5 @@
>      });
>    },
>  
> +  _dialMMI: function(aClientId, aOptions, aCallback) {
> +    this._sendToRilWorker(aClientId, "sendMMI", {parsedMMI: aOptions.mmi},

Just take a string as 2nd argument for |this._dialMMI|.

@@ +683,5 @@
> +                          response => {
> +      if (DEBUG) debug("MMI response: " + JSON.stringify(response));
> +
> +      if (!response.success) {
> +        if (response.additionalInformation) {

nit: if (response.additionalInformation != null)

@@ +689,5 @@
> +                                       response.additionalInformation);
> +        } else {
> +          aCallback.notifyDialMMIError(response.errorMsg);
> +        }
> +      } else {

if (...) {
  return;
}

...

Comment 154

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #153)
> Comment on attachment 8479836 [details] [diff] [review]
> Part 11: Dial MMI (ril)
> 
> Review of attachment 8479836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +2320,5 @@
> >          if (DEBUG) this.debug("cdma-info-rec-received: " + JSON.stringify(message));
> >          gSystemMessenger.broadcastMessage("cdma-info-rec-received", message);
> >          break;
> > +      case "notifyCFStateChanged":
> > +        this._sendCfStateChanged(message);
> 
> I didn't find any function named "_sendCfStateChanged", even with bug 843452
> patched.

http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#3737

> ::: dom/system/gonk/ril_worker.js
> @@ +6018,5 @@
> >      }
> >    }
> > +
> > +  // It's a dial request from telephony.
> > +  if (options.parsedMMI) {
> 
> But why do we have to sendChromeMessage twice?

One is for original sendMMI result.
And another one is to trigger cfstatechange event on mozMobileConnection

http://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileConnection.webidl?from=MozMobileConnection.webidl#541

We could set the call forwarding options by 3 methods
- mozMobileConnection.setCallForwardingOptions
- mozMobileConnection.sendMMI
- mozTelephony.dial

Comment 155

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 11#2: Dial MMI (ril)

Comment 156

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Szu-Yu Chen [:aknow] from comment #154)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #153)
> > Comment on attachment 8479836 [details] [diff] [review]
> > Review of attachment 8479836 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > I didn't find any function named "_sendCfStateChanged", even with bug 843452
> > patched.
> 
> http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RadioInterfaceLayer.js?from=RadioInterfaceLayer.js&case=true#3737

Will be removed in Bug 843452 - Part 4-5.

> > ::: dom/system/gonk/ril_worker.js
> > @@ +6018,5 @@
> > >      }
> > >    }
> > > +
> > > +  // It's a dial request from telephony.
> > > +  if (options.parsedMMI) {
> > 
> > But why do we have to sendChromeMessage twice?
> 
> One is for original sendMMI result.
> And another one is to trigger cfstatechange event on mozMobileConnection
> 
> http://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozMobileConnection.
> webidl?from=MozMobileConnection.webidl#541
> 
> We could set the call forwarding options by 3 methods
> - mozMobileConnection.setCallForwardingOptions
> - mozMobileConnection.sendMMI
> - mozTelephony.dial

Then, sorry, please call to nsIMobileConnectionService::sendMMI instead.

Comment 157

[Ken Chang[:kenkai]]

After off-line discussion, remove this from 2.1. ni? Sandip as well.

Comment 158

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 11#3: Dial MMI (ril)

I don't want to redirect telephony.dial to nsIMobileConnectionService::sendMMI. After gaia migrating their use to telephony.dial, all related code for sendMMI will be removed. So telephonyService should have its on path to ril_worker instead of relying on mobileConnectionService.

Comment 159

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8484012 [details] [diff] [review]
Part 11#3: Dial MMI (ril)

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

I'll only buy anything with one single path for MMI handling.

Comment 160

[Sandip Kamat]

Timing wise, OK for making it 2.2nom

Comment 161

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 11#4: Dial MMI (ril)

Comment 162

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #130)
> Comment on attachment 8480356 [details] [diff] [review]
> Part 06#2: Refactoring: Change Telephony::Callback to TelephonyCallback

Need remade.

Comment 163

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Szu-Yu Chen [:aknow] from comment #111)
> Created attachment 8479831 [details] [diff] [review]
> Part 07: Refactoring: Embed the response in delete

Need remade.

Comment 164

[Vicamo Yang [:vicamo][:vyang]]

Hi, the patches here just can't be applied easily. Please revise them. I'd really want to know what's the problem that we can't simply call nsIMobileConnectionService::sendMMI in telephony service.

Comment 165

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 03#2: Telephony interface changed. r=khuey,hsinyi

Comment 166

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 04#3: Test case. r=vicamo

Comment 167

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 05#2: Refactoring: Rename DialSuccess to DialCallSuccess. r=hsinyi

Comment 168

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 06#3: Refactoring: Change Telephony::Callback to TelephonyCallback. r=vicamo

Comment 169

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 07#2: Refactoring: Embed the response in delete. r=vicamo

Comment 170

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 08#4: Dial MMI (internal interface)

re-r?

1. Add dialMMI in gonk interface.
2. Separate notifyDialMMIError() into two interfaces because the original version has [optional_argc] which cannot be implemented by JS.
   Ref: http://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedJSClass.cpp#905

Comment 171

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 09#7: Dial MMI (callback)

re-r? for the modification according to callback interface change

Comment 172

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 10#4: Dial MMI (ipc)

re-r? for the modification according to callback interface change

Comment 173

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 11#5: Dial MMI (ril)

Comment 174

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

updating the target milestone to reflect fact.

Comment 175

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8486951 [details] [diff] [review]
Part 11#5: Dial MMI (ril)

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

::: dom/mobileconnection/gonk/MobileConnectionGonkService.js
@@ +543,5 @@
>    },
>  
> +  notifyCFStateChanged: function(aInfo) {
> +    this.deliverListenerEvent("notifyCFStateChanged",
> +                              [!aInfo.errorMsg, aInfo.action,

Whenever we have to call |notifyCFStateChanged|, it's always a successful response and |aInfo.errorMsg| is always undefined. That means we should remove that field in some follow-up.

::: dom/mobileconnection/interfaces/nsIMobileConnectionGonkService.idl
@@ +46,5 @@
>  
>    void notifyLastHomeNetworkChanged(in unsigned long clientId,
>                                      in DOMString network);
> +
> +  void notifyCFStateChanged(in unsigned long clientId, in jsval info);

Please don't use jsval in new interfaces whenever possible. r+ with this addressed.

Comment 176

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 12: Fix xpcshell test

xpcshell test is broke.
- fix test_ril_worker_mmi.js
- fix test_ril_worker_mmi_cf.js
- move test_ril_worker_mmi_parseMMI.js to telephony/test/xpcshell

Comment 177

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8491253 [details] [diff] [review]
Part 12: Fix xpcshell test

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

::: dom/system/gonk/tests/test_ril_worker_mmi_parseMMI.js
@@ -1,1 @@
> -/* Any copyright is dedicated to the Public Domain.

Hi, could you use `hg mv` to rename files? It would be easier to track the differences other than renaming itself.

Comment 178

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 12: Fix xpcshell test (rename)

Vicamo, 

Sorry, I work on git repo, not hg. The rename info seems lost in generated patch.
So I separate the patch into two part for your review.

1. pure rename: dom/system/gonk/tests/test_ril_worker_mmi_parseMMI.js -> dom/telephony/test/xpcshell/test_parseMMI.js

2. rest

Comment 179

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: Part 12-2: Fix xpcshell test

Comment 180

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8491295 [details] [diff] [review]
Part 12: Fix xpcshell test (rename)

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

I don't know whether I should r+ this patch because it's supposed to be covered by hg. Anyway...

Comment 181

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8491296 [details] [diff] [review]
Part 12-2: Fix xpcshell test

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

r+ with that 'equal' renamings explained.

::: dom/system/gonk/tests/test_ril_worker_mmi.js
@@ +6,5 @@
>  function run_test() {
>    run_next_test();
>  }
>  
> +function MMIOptions(procedure, serviceCode, sia, sib, sic) {

nit: if that's not a instantiable class, and indeed it's not, please name it as other ordinary functions, for example, 'createMMIOptions'.

::: dom/telephony/test/xpcshell/test_parseMMI.js
@@ +17,5 @@
>  
>  add_test(function test_parseMMI_empty() {
>    let mmi = parseMMI("");
>  
> +  equal(mmi, null);

Did I miss something? I don't see a |equal| utility function was added?

Comment 182

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #181)
> Comment on attachment 8491296 [details] [diff] [review]
> Part 12-2: Fix xpcshell test
> 
> Review of attachment 8491296 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with that 'equal' renamings explained.
> 
> ::: dom/system/gonk/tests/test_ril_worker_mmi.js
> @@ +6,5 @@
> >  function run_test() {
> >    run_next_test();
> >  }
> >  
> > +function MMIOptions(procedure, serviceCode, sia, sib, sic) {
> 
> nit: if that's not a instantiable class, and indeed it's not, please name it
> as other ordinary functions, for example, 'createMMIOptions'.
> 
> ::: dom/telephony/test/xpcshell/test_parseMMI.js
> @@ +17,5 @@
> >  
> >  add_test(function test_parseMMI_empty() {
> >    let mmi = parseMMI("");
> >  
> > +  equal(mmi, null);
> 
> Did I miss something? I don't see a |equal| utility function was added?

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

"""
xpcshell tests have access to the following functions. They are defined in testing/xpcshell/head.js and testing/modules/Assert.jsm if you want to see how they work.

Assertions and logging

ok, equal, notEqual, deepEqual, notDeepEqual, strictEqual, notStrictEqual
    These assertion methods are provided by Assert.jsm. It implements the CommonJS Unit Testing specification version 1.1, which provides a basic, standardized interface for performing in-code logical assertions with optional, customizable error reporting. It is highly recommended to use these assertion methods, instead of the ones mentioned below.

do_check_eq(a, b)Deprecated since Gecko 32.0
"""

Therefore, I changed do_check_eq() to equal()

Comment 183

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 01: Telephony interface changed. r=khuey,hsinyi

Comment 184

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 02: Test case. r=vicamo

Comment 185

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 03: Refactoring: Rename DialSuccess to DialCallSuccess. r=hsinyi

Comment 186

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 04: Refactoring: Change Telephony::Callback to TelephonyCallback. r=vicamo

Comment 187

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 05: Refactoring: Embed the response in delete. r=vicamo

Comment 188

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 06: Dial MMI (internal interface). r=vicamo

Comment 189

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 07: Dial MMI (callback). r=vicamo

Comment 190

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 08: Dial MMI (ipc). r=vicamo

Comment 191

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 09: Dial MMI (ril). r=vicamo

Comment 192

[Szu-Yu Chen [:aknow] (inactive)]

Attachment: [final] Part 10: Fix xpcshell test. r=vicamo

Comment 193

[Szu-Yu Chen [:aknow] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=b48bd51fd516

Comment 194

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/61e26d5e8f1e
https://hg.mozilla.org/integration/b2g-inbound/rev/3b5788be9578
https://hg.mozilla.org/integration/b2g-inbound/rev/f397ab9d50b3
https://hg.mozilla.org/integration/b2g-inbound/rev/361dd2e9b028
https://hg.mozilla.org/integration/b2g-inbound/rev/a212208a5821
https://hg.mozilla.org/integration/b2g-inbound/rev/fbdf1e8930a9
https://hg.mozilla.org/integration/b2g-inbound/rev/0e6e5e5ac9aa
https://hg.mozilla.org/integration/b2g-inbound/rev/12f4e6d6454a
https://hg.mozilla.org/integration/b2g-inbound/rev/5ee85506486b
https://hg.mozilla.org/integration/b2g-inbound/rev/923dbd546a96

Comment 195

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/61e26d5e8f1e
https://hg.mozilla.org/mozilla-central/rev/3b5788be9578
https://hg.mozilla.org/mozilla-central/rev/f397ab9d50b3
https://hg.mozilla.org/mozilla-central/rev/361dd2e9b028
https://hg.mozilla.org/mozilla-central/rev/a212208a5821
https://hg.mozilla.org/mozilla-central/rev/fbdf1e8930a9
https://hg.mozilla.org/mozilla-central/rev/0e6e5e5ac9aa
https://hg.mozilla.org/mozilla-central/rev/12f4e6d6454a
https://hg.mozilla.org/mozilla-central/rev/5ee85506486b
https://hg.mozilla.org/mozilla-central/rev/923dbd546a96

Comment 196

[Bobby Holley (:bholley)]

Hi Szu-Yu and Vicamo,

Part 9 adds new usage of __exposedProps__, which is forbidden per [1]. Shall we back this out? I'd personally be ok with a prompt (i.e. this week) followup patch to convert this code to WebIDL, but jst is the ultimate authority here, so I'll defer to him.

[1] https://groups.google.com/forum/#!msg/mozilla.dev.platform/yNKS1Z6UYQo/qloD9G0AdkcJ

Comment 197

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Bobby Holley (:bholley) from comment #196)
> Hi Szu-Yu and Vicamo,
> 
> [1]
> https://groups.google.com/forum/#!msg/mozilla.dev.platform/yNKS1Z6UYQo/
> qloD9G0AdkcJ

We are already working on deprecating those jsval for other reason, so that MMIResult has to go eventually. Since new usage of __exposedProps__ has been marked as forbidden right now, we'll provide a follow-up to remove it asap.

Comment 198

[Vicamo Yang [:vicamo][:vyang]]

Follow-up filed in bug 1071469.