793186 - MMI Codes: implement sendMMI() and cancelMMI()

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

Since MMI codes is a more general group of code types that includes USSD codes, IMO we should change the current |DOMRequest sendUSSD(in DOMString ussd);| function from the Mobile Connection API for |DOMRequest sendMMI(in DOMString mmi);|. The implementation of this function would parse and analize the given string, triggering a USSD request or any other RIL operation when corresponds.

Some MMI codes trigger radio operations, but unfortunately the RIL only supports cancelling USSD requests so far. Despite that, in order to keep the API uniformity, I would also suggest changing |DOMRequest cancelUSSD();| for |DOMRequest cancelMMI();|. Android also considers this situation (https://android.googlesource.com/platform/frameworks/opt/telephony/+/master/src/java/com/android/internal/telephony/gsm/GsmMmiCode.java line 437).

The |onussdreceived| event should not change and still be triggered when a new USSD message is received.

Philipp, can I have your feedback about this before starting any modification to the current code?

Comment 1

[Andrew Overholt [:overholt]]

Is this a P1 blockers or is it a nice-to-have?

Comment 2

[Philipp von Weitershausen [:philikon]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #0)
> Philipp, can I have your feedback about this before starting any
> modification to the current code?

This sounds good to me. The risk for this renaming seems pretty low.

(In reply to Andrew Overholt [:overholt] from comment #1)
> Is this a P1 blockers or is it a nice-to-have?

This bug would pave the way for other MMI-based features which are mandatory (= blockers0. See bug 793178 comment 0.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: Rename USSD - MMI

This patch only rename USSD for MMI where needed.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: RIL

This patch implements the basic RIL functionality to process MMI codes. I've been doing a few real tests on the device, but I'd like to review the algorithm and write some unit tests before asking for r?. I would appreciate some feedback in the meantime though :)

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 666230 [details] [diff] [review]
Part 2: RIL

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

I'm not a big fan of seeing this much logic on the main thread (RadioInterfaceLayer.js). I would prefer if we did this in ril_worker.js.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1436,5 @@
> +    //  D. Dialing number, if required.
> +    let pattern = "((\\*|#|\\*#|\\*\\*|##)(\\d{2,3})(\\*([^*#]*)(\\*([^*#]*)" +
> +                  "(\\*([^*#]*)(\\*([^*#]*))?)?)?)?#)([^#]*)";
> +    let regex = new RegExp(pattern);
> +    let matches = regex.exec(mmiString);

Every time a programmer uses a regular expression, God kills a kitten. :'(

Seriously though, where did this regex come from? Is it "battle proven" or did you make it up? Either case, we definitely need unit tests.

Also, please lazy-create those and keep them around. Compiling regexp's is pretty expensive.

::: dom/system/gonk/ril_consts.js
@@ +1809,5 @@
>  
> +// MMI match groups
> +const MMI_MATCH_GROUP_FULL_MMI = 1;
> +const MMI_MATCH_GROUP_MMI_PROCEDURE = 2;
> +const MMI_MATCH_GROUP_SERVICE_CODE = 3;

These constants are not "public" constants, they're just there to make the code easier to read. You should put them on the top of the file that they're being used in (as pointed out above, this should be ril_worker.js IMHO.)

Comment 6

[Philipp von Weitershausen [:philikon]]

Please request review from :marshall_law for subsequent patches, I will be afk until 2012-10-14. Thanks!

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Thanks Philipp!

(In reply to Philipp von Weitershausen [:philikon] (afk until 2012-10-14) from comment #5)
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +1436,5 @@
> > +    //  D. Dialing number, if required.
> > +    let pattern = "((\\*|#|\\*#|\\*\\*|##)(\\d{2,3})(\\*([^*#]*)(\\*([^*#]*)" +
> > +                  "(\\*([^*#]*)(\\*([^*#]*))?)?)?)?#)([^#]*)";
> > +    let regex = new RegExp(pattern);
> > +    let matches = regex.exec(mmiString);
> 
> Every time a programmer uses a regular expression, God kills a kitten. :'(
> 
> Seriously though, where did this regex come from? Is it "battle proven" or
> did you make it up? Either case, we definitely need unit tests.
>

I've taken the regular expression from Android implementation. So I guess it is "battle proven".

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: RIL v2

This patch addresses philikon's feedback:
- MMI logic and not public consts moved to ril_worker.js.
- Lazy-creation of MMI regexp.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Tests

xpcshell tests for the MMI parser.

I'll be adding more tests for each specific MMI functionality (Bug 793189, Bug 793192, etc.).

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #9)
> Created attachment 667345 [details] [diff] [review]
> Part 3: Tests
> 
> xpcshell tests for the MMI parser.
> 
> I'll be adding more tests for each specific MMI functionality (Bug 793189,
> Bug 793192, etc.).
I meant, I'll be adding more tests *in following bugs* :)

Comment 11

[Marshall Culpepper [:marshall_law]]

Comment on attachment 667342 [details] [diff] [review]
Part 2: RIL v2

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

This looks great, I highly appreciate the in depth comments :) I mainly just have nits, so r+. Will we need gaia follow up work for this?

::: dom/system/gonk/ril_worker.js
@@ +2203,5 @@
> +    //  SIA may comprise e.g. a PIN code or Directory Number, SIB may be used to
> +    //  specify the tele or bearer service and SIC to specify the value of the
> +    //  "No Reply Condition Timer". Where a particular service request does
> +    //  not require any SI, "*SI" is not entered. The use of SIA, SIB and SIC
> +    //  is optional andshall be entered in any of the following formats:

nit: "and shall"

@@ +2211,5 @@
> +    //    - *SIA#
> +    //    - **SIB*SIC#
> +    //    - ***SISC#
> +    if (this._mmiRegExp == null) {
> +      let pattern = "((\\*|#|\\*#|\\*\\*|##)(\\d{2,3})(\\*([^*#]*)(\\*([^*#]*)" +

nit: this regex is a little complicated, would you mind breaking up each group and commenting on it's purpose? you can probably remove the comment on line 2234 in that case..

also, the second regex group seems like it could be simplified..

(\\*|#|\\*#|\\*\\*|##)
becomes
(\\*[*#]?|##?)

@@ +2249,5 @@
> +    };
> +  },
> +
> +  sendMMI: function sendMMI(options) {
> +    debug("SendMMI " + JSON.stringify(options));

nit: make sure your debug statements are surrounded by |if (DEBUG)|

@@ +2257,5 @@
> +    let _sendMMIError = (function _sendMMIError(errorMsg) {
> +      options.rilMessageType = "sendMMI";
> +      options.errorMsg = errorMsg;
> +      this.sendDOMMessage(options);
> +    }).bind(this);

The .bind(this) shouldn't be necessary here, |this| is still in scope when you call _sendMMIError from with sendMMI

@@ +2276,5 @@
> +    // trigger the appropriate RIL request if possible.
> +    let sc = mmi.serviceCode;
> +
> +    // Call forwarding
> +    if (sc && (sc == MMI_SC_CFU || sc == MMI_SC_CF_BUSY ||

nit: all of these service code cases seem like they would be best handled in a switch/case statement. that would also let us get rid of the multiple "sc && .." statements.

@@ +4220,2 @@
>    }
> +  options.success = this._ussdSession = options.rilRequestError == 0 ?

nit: the ternary shouldn't be needed here..

|options.success = this._ussdSession = options.rilRequestError == 0;|

should suffice

Comment 12

[Marshall Culpepper [:marshall_law]]

Comment on attachment 667345 [details] [diff] [review]
Part 3: Tests

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

Looks good, just a few follow up questions.

::: dom/system/gonk/tests/test_ril_worker_mmi.js
@@ +37,5 @@
> +
> +add_test(function test_parseMMI_invalid() {
> +  let mmi = parseMMI("**");
> +
> +  do_check_null(mmi);

is there a way to also check your error condition NO_VALID_MMI_STRING here?

@@ +53,5 @@
> +
> +add_test(function test_parseMMI_USSD() {
> +  let mmi = parseMMI("*123#");
> +
> +  do_check_eq(mmi.fullMMI, "*123#");

should we also verify that a USSD was sent here?

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Thanks for the quick review Marshall! :)

(In reply to Marshall Culpepper [:marshall_law] from comment #11)
> @@ +2257,5 @@
> > +    let _sendMMIError = (function _sendMMIError(errorMsg) {
> > +      options.rilMessageType = "sendMMI";
> > +      options.errorMsg = errorMsg;
> > +      this.sendDOMMessage(options);
> > +    }).bind(this);
> 
> The .bind(this) shouldn't be necessary here, |this| is still in scope when
> you call _sendMMIError from with sendMMI
> 

If I remove it I'll get: 

I/Gecko   ( 5958): -*- RadioInterfaceLayer: Got an error: resource://gre/modules/ril_worker.js:2267: this is undefined

So I am afraid that I need to leave the .bind(this).

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: RIL v3

r=marshall_law in comment #11.

This patch should address all the feedback provided by Marshall in comment #11, except for the .bind(this) thing.

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Tests

I am asking for r? again cause, based on you review questions, I've added a few more tests to verify the |sendMMI| function, so now we check for the |NO_VALID_MMI_STRING| and other error messages :)

Answering your question from comment #12 about verifying that a USSD has been sent, if I am not wrong, it would required support for MMI and/or USSD in the android emulator, which I am afraid that it is not implemented (http://developer.android.com/tools/devices/emulator.html#telephony). I did a test with a fake sendUSSD function to test the related code faking a successfull and failed USSD.

Comment 16

[Marshall Culpepper [:marshall_law]]

Comment on attachment 667899 [details] [diff] [review]
Part 3: Tests

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

Very nice! r+ with just 2 minor nits.

::: dom/system/gonk/tests/test_ril_worker_mmi.js
@@ +309,5 @@
> +  run_next_test();
> +});
> +
> +add_test(function test_sendMMI_call_forwarding() {
> +  testSendMMI("*21#", "CALL_FORWARDING_NOT_SUPPORTED_VIA_MMI");

nit: Can you comment these not supported tests with "TODO: <bug link>" like you did in ril_worker.js? That way when they get implemented it's easier to spot why these tests might fail :)

@@ +357,5 @@
> +  }
> +  worker.RIL.sendMMI({mmi: "*123#"});
> +
> +  do_check_eq(ussdOptions.ussd, "*123#");
> +  do_check_eq (postedMessage.errorMsg, GECKO_ERROR_SUCCESS);

nit: extra space here, and below on line 387

Comment 17

[Marshall Culpepper [:marshall_law]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #13)
> If I remove it I'll get: 
> 
> I/Gecko   ( 5958): -*- RadioInterfaceLayer: Got an error:
> resource://gre/modules/ril_worker.js:2267: this is undefined
> 
> So I am afraid that I need to leave the .bind(this).

Apologies for leading you astray here.. thanks for letting me know :)

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e3e1e70afc
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc2fdb593a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad8fa8df0ab7

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a4e3e1e70afc
https://hg.mozilla.org/mozilla-central/rev/5dc2fdb593a7
https://hg.mozilla.org/mozilla-central/rev/ad8fa8df0ab7