891242 - [MMI] Short Code MMI (length<3) does not work as expected - Gecko part

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[shawn ku [:sku]]

By following 3GPP 22.030, the dial string with length less than 3 should be treated as USSD, not call.

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


Steps:

0. Open the dialer app.
1. Send an short string USSD(ex: 21) message.

Expected:
Device will send out USSD command to network

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

Comment 1

[shawn ku [:sku]]

Attachment: patch for 891242 - RIL part

Comment 2

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

Comment on attachment 772510 [details] [diff] [review]
patch for 891242 - RIL part

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

Thanks for the patch, Shawn.
The code logic looks good to me, but I would like to have some code clean up per our offline discussion and my below comment. Also, please pay attention to mozilla coding style https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style. Thank you!

::: dom/system/gonk/ril_worker.js
@@ +2149,5 @@
>        if (mmiString.charAt(mmiString.length - 1) == MMI_END_OF_USSD) {
>          return {
>            fullMMI: mmiString
>          };
> +      } 

nit: trailing white space

@@ +2150,5 @@
>          return {
>            fullMMI: mmiString
>          };
> +      } 
> +      

ditto.

@@ +2379,3 @@
>      if (mmi.fullMMI &&
>          (mmiString.charAt(mmiString.length - 1) == MMI_END_OF_USSD)) {
>        options.ussd = mmi.fullMMI;

Hi Shawn, 

Since you are here, how about let's take the chance to correct this redundant code? 

We've done the same check in _parseMMI and don't have to do it again here. So, in _parseMMI, we could simply return {fullMMI: mmiString, mmiServiceCode: MMI_KS_SC_USSD}. And add a new case "MMI_KS_SC_USSD" for sending USSD. 

Then let your new code follow the style.

Sounds good? :)

Comment 3

[shawn ku [:sku]]

Attachment: updated patch for ril_worker.js

Comment 4

[Ken Chang[:kenkai]]

Comment on attachment 772510 [details] [diff] [review]
patch for 891242 - RIL part

Hsinyi is a review peer and can provide you many useful suggestions.

Comment 5

[Ken Chang[:kenkai]]

Comment on attachment 772564 [details] [diff] [review]
updated patch for ril_worker.js

Hsinyi is a review peer and can provide you many useful suggestions.

Comment 6

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

Comment on attachment 772564 [details] [diff] [review]
updated patch for ril_worker.js

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

Looks good! Just some minor changes needed.

Also, the patch fails the test test_ril_worker_mmi.js. Please modify the test accordingly and add new cases for the patch.

::: dom/system/gonk/ril_worker.js
@@ +2143,5 @@
>  
>      // If the regex does not apply over the MMI string, it can still be an MMI
>      // code. If the MMI String is a #-string (entry of any characters defined
>      // in the TS.23.038 Default Alphabet followed by #SEND) it shall be treated
>      // as a USSD code.

We need to update the comment to meet the code as well. So, let's remove "If the MMI String is a #-string (entry of any characters ...as a USSD code." And add some lines below.

@@ +2145,5 @@
>      // code. If the MMI String is a #-string (entry of any characters defined
>      // in the TS.23.038 Default Alphabet followed by #SEND) it shall be treated
>      // as a USSD code.
>      if (matches == null) {
>        if (mmiString.charAt(mmiString.length - 1) == MMI_END_OF_USSD) {

Add one comment line: // #-string, see TS 22.030, clause 6.5.3.2.

@@ +2153,5 @@
> +        };
> +      }
> +
> +      // For those mmiString.length < 3 (except length = 2 && '1' as prefix)
> +      // treat it as USSD, not call

I would suggest removing this as the code itself is clear enough. ;-)

@@ +2154,5 @@
> +      }
> +
> +      // For those mmiString.length < 3 (except length = 2 && '1' as prefix)
> +      // treat it as USSD, not call
> +      if (mmiString.length < 3 && !(mmiString.length == 2 && mmiString.charAt(0) === '1')) {

Let's rewrite the condition a little bit:
if (mmiString.length < 3 &&
    (mmiString.length != 2 || mmiString.charAt(0) !== '1'))

Also, please add one comment line: // Short string, see TS 22.030, clause 6.5.3.2.

@@ +2377,5 @@
> +
> +      // USSD
> +      // If the MMI code is not a known code and is a recognized USSD request or
> +      // a #-string or short string which length < 3 (except length = 2 && '1' as prefix),
> +      // it shall still be sent as a USSD request.

nit: Let's just keep the line "// USSD" and get rid of others as we already explained that in _parseMMI.

Comment 7

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

Kind reminder: always mark the invalid attachments as 'obsolete' and indicate the version of an attached patch clearly.

Comment 8

[shawn ku [:sku]]

Attachment: 3rd revised patch for 891242

1. change code logic to fit 3GPP spec. (for in call ss part, still more work)
2. test cases are revised to compy the changes.

Comment 9

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

Comment on attachment 776131 [details] [diff] [review]
3rd revised patch for 891242

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

Just provide some suggestions. You need hsinyi's formal review.

::: dom/system/gonk/ril_worker.js
@@ +2174,5 @@
>      // If the regex does not apply over the MMI string, it can still be an MMI
>      // code. If the MMI String is a #-string (entry of any characters defined
>      // in the TS.23.038 Default Alphabet followed by #SEND) it shall be treated
>      // as a USSD code.
>      if (matches == null) {

if (!matches)

@@ +2175,5 @@
>      // code. If the MMI String is a #-string (entry of any characters defined
>      // in the TS.23.038 Default Alphabet followed by #SEND) it shall be treated
>      // as a USSD code.
>      if (matches == null) {
> +      // refer to TS.22.030 clause 6.3.5.2 for not-implemented supplementary services check

nit:
1. Begin with upper case => 'R'efer
2. Better to end with a period '.'
3. Exceed 80 chars. Please wrap the line.

should be clause 6.5.3.2

@@ +2186,5 @@
> +
> +      if (mmiString.length < 3 && !(mmiString.length == 2 && mmiString.charAt(0) === '1')) {
> +        return {
> +          fullMMI: mmiString,
> +          serviceCode: MMI_KS_SC_USSD

I would suggest:

let isPoundString = (mmiString.charAt(mmiString.length - 1) == MMI_END_OF_USSD);  // #-String.
let isShortString = (mmiString.length < 3 && !(mmiString.length == 2 && mmiString.charAt(0) === '1'))

if (isPoundString || isShortString) {
  return ...
}

@@ +2427,5 @@
>  
> +    // If the MMI code is not a known code and is a recognized USSD request,
> +    // it shall still be sent as a USSD request.
> +    if (mmi.fullMMI ||
> +        (sc == MMI_KS_SC_USSD)) {

I would like to move this part into the above switch case.
switch (sc) {
  ...
  case MMI_KS_SC_USSE:
    ...
}


Another question. After the if block, there are ... (not shown in the diff)

  // At this point, the MMI string is considered as not valid MMI code and
  // not valid USSD code.                                                 
  _sendMMIError(MMI_ERROR_KS_ERROR);                                      

When will code reach here?
_parseMMI() guarnatees return either null or an object with 'serviceCode' property.
The null case is handled and return earlier. All 'serviceCode' are also handled in the switch case. So... what's the remaining case?

Comment 10

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

Comment on attachment 776131 [details] [diff] [review]
3rd revised patch for 891242

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

Thanks for the patch and careful check! It indeed complements the lack of short mmi code. 
But I would want to take the opportunity here to refactor mmi part so that we don't maintain the logic of checking if it's ussd in two places.

::: dom/system/gonk/ril_worker.js
@@ +2174,5 @@
>      // If the regex does not apply over the MMI string, it can still be an MMI
>      // code. If the MMI String is a #-string (entry of any characters defined
>      // in the TS.23.038 Default Alphabet followed by #SEND) it shall be treated
>      // as a USSD code.
>      if (matches == null) {

Even we try to examine USSD here, we still need additional check in line #2428. So, in stead of checking ussd here, I would suggest moving the determination logic to line #2428 since we don't really get benefit from do the checking stuff here. 

So, here, we just do:
if (!matches) { // Thanks for Aknow's suggestion
  return {
    fullMMI: mmiString
  };
}

Kind reminder: we need corresponding xpcshell test modification.

@@ +2424,5 @@
>          _sendMMIError(MMI_ERROR_KS_NOT_SUPPORTED);
>          return;
>      }
>  
> +    // If the MMI code is not a known code and is a recognized USSD request,

Let's check USSD here (and only here) by doing:

  1 // If the MMI code is not a known code, then check if it is a recognized USSD request.                
  2 if (mmi.fullMMI) {                                                                     
  3   if (!_isRadioAvailable(MMI_KS_SC_USSD)) {                                            
  4     return;
  5   } 
  6   
  7   // #-String, see TS.22.030 clause 6.5.3.2.                                           
  8   let isPoundString = (mmiString.charAt(mmiString.length - 1) == MMI_END_OF_USSD);     
  9   // Short string, see TS 22.030, clause 6.5.3.2.
 10   let isShortString = (mmiString.length < 3 &&
 11                        (mmiString.length != 2 || mmiString.charAt(0) !== '1'));        
 12                        
 13   if (isPoundString || isShortString) {                                                
 14     options.ussd = mmi.fullMMI;
 15     options.mmiServiceCode = MMI_KS_SC_USSD;                                           
 16     this.sendUSSD(options);
 17     return;
 18   } 
 19   
 20   if (this._ussdSession) {                                                             
 21     // Even we could neither parse the MMI code nor recogniz the USSD request,         
 22     // we'll send it as an USSD request when there's already a USSD session.
 23     if (!_isRadioAvailable(MMI_KS_SC_USSD)) {                                          
 24       return;
 25     } 
 26     options.ussd = mmi.fullMMI;                                                          
 27     this.sendUSSD(options);
 28     return;
 29   } 
 30   
 31   _sendMMIError(MMI_ERROR_KS_ERROR);                                                   
 32   return;
 33 }

We need to change the code a little bit in [1].
Just do:
if (mmi === null) {
  _sendMMIError(MMI_ERROR_KS_ERROR);
  return;
}


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

::: dom/system/gonk/tests/test_ril_worker_mmi.js
@@ +69,5 @@
>    run_next_test();
>  });
>  
>  add_test(function test_parseMMI_invalid() {
> +  let mmi = parseMMI("11");

I would want to see more invalid string tests.

@@ +332,5 @@
> +    worker.RIL[REQUEST_SEND_USSD](0, {
> +      rilRequestError: ERROR_SUCCESS
> +    });
> +  }
> +

Please add one line to get the test pass.

worker.RIL.radioState = GECKO_RADIOSTATE_READY;

@@ +333,5 @@
> +      rilRequestError: ERROR_SUCCESS
> +    });
> +  }
> +
> +  worker.RIL.sendMMI({mmi: "**"});

Could we add more string cases here, such as string with length 1?

Comment 11

[shawn ku [:sku]]

Attachment: 4th revised patch for 891242

Revised the 4th patch to make more comply to 3GPP spec.
Test cases has been passed.

Comment 12

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

Comment on attachment 778243 [details] [diff] [review]
4th revised patch for 891242

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

::: dom/system/gonk/ril_worker.js
@@ +2257,5 @@
>  
>        return true;
>      }
>  
> +    function _isRadioAvailable(mmiServiceCode) {

This function exists already. Why in your patch? Please check whether you attached the correct patch. Thank you.

Comment 13

[shawn ku [:sku]]

Attachment: 5th revised patch for 891242

It seems my central trunk did not update to the latest one.
re-upload the patch

Comment 14

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

Comment on attachment 778368 [details] [diff] [review]
5th revised patch for 891242

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

The whole structure looks good to me, thank you for the improvement. But there are some minor issues, such as coding style and namings, that I would like to improve. Once they are well addressed, I believe the patch is ready.

::: dom/system/gonk/ril_worker.js
@@ +2178,5 @@
>  
>    /**
> +   * Helper to parse MMI/USSD string. TS.22.030 Figure 3.5.3.2
> +   */
> +  parseMMI: function parseMMI(mmiString) {

Having parseMMI for parsing MMI/USSD and _parseMMI for parsing MMI only is kinda confusing. Let's move parseMMI back to _parseMMI, and rename _parseMMI in line#2222 to _matchMMIRegexp.

@@ +2184,5 @@
>        return null;
>      }
>  
> +    let matches = this._parseMMI(mmiString);
> +    if(matches) {

Put space between if and (

@@ +2205,5 @@
> +        sic: matches[MMI_MATCH_GROUP_SIC],
> +        pwd: matches[MMI_MATCH_GROUP_PWD_CONFIRM],
> +        dialNumber: matches[MMI_MATCH_GROUP_DIALING_NUMBER]
> +      };
> +    } else if(this._parsePoundString(mmiString) ||

ditto.

Remove else. Just move the following if (this._...) to the next new line.

@@ +2218,5 @@
> +
> +  /**
> +   * Helper to parse MMI string. TS.22.030 Figure 3.5.3.2
> +   */
> +  _parseMMI: function _parseMMI(mmiString) {

Rename it '_matchMMIRegexp'

@@ +2262,5 @@
>  
>        this._mmiRegExp = new RegExp(pattern);
>      }
> +
> +    // Regex only apply for those well-defined MMI string (refer to TS.22.030

nit: s/apply/applys, s/string/strings

@@ +2263,5 @@
>        this._mmiRegExp = new RegExp(pattern);
>      }
> +
> +    // Regex only apply for those well-defined MMI string (refer to TS.22.030
> +    // Annex B), otherwise, null should be expected return value

nit: s/expected/the expected

And remember to put a period at the end of a line.

@@ +2270,5 @@
> +
> +  /**
> +   * Helper to parse # string. TS.22.030 Figure 3.5.3.2
> +   */
> +  _parsePoundString: function _parsePoundString(mmiString) {

s/_parsePoundString/_isPoundString

@@ +2271,5 @@
> +  /**
> +   * Helper to parse # string. TS.22.030 Figure 3.5.3.2
> +   */
> +  _parsePoundString: function _parsePoundString(mmiString) {
> +    if(mmiString.charAt(mmiString.length - 1) === MMI_END_OF_USSD) {

Simply do 

"return mmiString.charAt(mmiString.length - 1) === MMI_END_OF_USSD;"

@@ +2281,5 @@
> +
> +  /**
> +   * Helper to parse short string. TS.22.030 Figure 3.5.3.2
> +   */
> +  _parseShortString: function _parseShortString(mmiString) {

s/_parseShortString/_isMMIShortString

@@ +2282,5 @@
> +  /**
> +   * Helper to parse short string. TS.22.030 Figure 3.5.3.2
> +   */
> +  _parseShortString: function _parseShortString(mmiString) {
> +    if(this._isEmergencyNumber(mmiString)) {

ditto.

@@ +2286,5 @@
> +    if(this._isEmergencyNumber(mmiString)) {
> +      return false;
> +    }
> +
> +    if(mmiString.length < 3) {

Let's use "early bailout" style.

if (mmiString.length > 2) {
  return false;
}

@@ +2287,5 @@
> +      return false;
> +    }
> +
> +    if(mmiString.length < 3) {
> +      // has call(s) alive case

nit: Let's use the convention the spec uses. "In a call"

@@ +2288,5 @@
> +    }
> +
> +    if(mmiString.length < 3) {
> +      // has call(s) alive case
> +      if(Object.getOwnPropertyNames(this.currentCalls).length>0) {

ditto.

Put space between "length and >" and one more between "> and 0."

@@ +2290,5 @@
> +    if(mmiString.length < 3) {
> +      // has call(s) alive case
> +      if(Object.getOwnPropertyNames(this.currentCalls).length>0) {
> +        return true
> +      } else if((mmiString.length != 2) || (mmiString.charAt(0) !== '1')) {

No "else" here. 

Just start from a new line, and 
if ((mmiString.length ...

Still, space right after 'if' please.

@@ +2363,5 @@
>          this.sendUSSD(options);
>          return;
>        }
> +      // There should be no more null mmi after _parseMMI() unless something
> +      // unexpected.

I'm not sure I got your idea about the comment here.

@@ +2520,5 @@
>      }
>  
> +    // If the MMI code is not a known code and is a recognized USSD request,
> +    // it shall still be sent as a USSD request.
> +    if (mmi.fullMMI) {

Great to see no more redundancy here.

::: dom/system/gonk/tests/test_ril_worker_mmi.js
@@ +381,5 @@
>  
>  add_test(function test_sendMMI_dial_string() {
>    testSendMMI("123", MMI_ERROR_KS_ERROR);
>  
>    run_next_test();

I would like to see the test cases for strings with length 1.

Comment 15

[shawn ku [:sku]]

Attachment: the 6th revised patch

Hi HsinYi:
 Thanks for your patient, Please review the 6th revised patch.
Any problems, please kindly let me know.

Thanks again.
sku

Comment 16

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

Comment on attachment 779065 [details] [diff] [review]
the 6th revised patch

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

Great! r=me with the following addressed.

Before we push this patch, would you please test it on the try server and provide the try link? Thank you.

::: dom/system/gonk/ril_worker.js
@@ +2208,5 @@
> +      };
> +    }
> +
> +    if (this._isPoundString(mmiString) ||
> +         this._isMMIShortString(mmiString)) {

nit: indention

@@ +2289,5 @@
> +    if (this._isEmergencyNumber(mmiString)) {
> +      return false;
> +    }
> +
> +    if (mmiString.length < 3) {

Hey, we don't need this anymore as we already bail out for the wrong length case in the beginning, right?

@@ +2292,5 @@
> +
> +    if (mmiString.length < 3) {
> +      // In a call case.
> +      if (Object.getOwnPropertyNames(this.currentCalls).length > 0) {
> +        return true

Oops, we miss a semicolon here. Dangerous javascript... :\

::: dom/system/gonk/tests/test_ril_worker_mmi.js
@@ +70,5 @@
>    run_next_test();
>  });
>  
> +add_test(function test_parseMMI_one_digit_short_code() {
> +  let mmi = parseMMI("1");

Thank you.

Comment 17

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

Comment on attachment 779065 [details] [diff] [review]
the 6th revised patch

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

Thx.
Looks good to me.

Comment 18

[shawn ku [:sku]]

Attachment: the 7th revised patch

Oooops, thanks for double check the patch again.
Will update try server link later today.

Thanks again!!!

1. add missed semicolon for "In a call case"
+    // In a call case.
+    if (Object.getOwnPropertyNames(this.currentCalls).length > 0) {
+      return true;
+    }

2. adjust indent
+    if (this._isPoundString(mmiString) ||
+        this._isMMIShortString(mmiString)) {
+      return {
+        fullMMI: mmiString
+      };
+    }

Comment 19

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

Comment on attachment 779549 [details] [diff] [review]
the 7th revised patch

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

You don't have to ask for review again since the patch is review granted. But, of course, I am happy to help out if you would like me to confirm again. :)

Comment 20

[shawn ku [:sku]]

update try server link:
https://tbpl.mozilla.org/?tree=Try&rev=070b6df34f28

Comment 21

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

(In reply to sku from comment #20)
> update try server link:
> https://tbpl.mozilla.org/?tree=Try&rev=070b6df34f28

The try result looks great! 

Please make sure the bug commit follows mozilla style, e.g. should have  r=xxx.
If the patch is ready to land, just add 'checkin-needed' in keyword.

Comment 22

[shawn ku [:sku]]

Attachment: Final Patch: Bug 891242 - [MMI] Short Code MMI (length<3) does not work as expected - Gecko part, r=HsinYi

Comment 23

[shawn ku [:sku]]

Attachment: Final Patch: Bug 891242 - [MMI] Short Code MMI (length<3) does not work as expected - Gecko part, r=HsinYi

Comment 24

[Gina Yeh [:gyeh] [:ginayeh]]

http://hg.mozilla.org/projects/birch/rev/0ebf0fabe24d

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0ebf0fabe24d