962995 - B2G RIL: write ICC UCS2 characters for 0x81 and 0x82 cases for SIM Contact

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Currently when writing ICC UCS2 characters for SIM, we only support 0x80 cases.
0x81, 0x82 also needed for special characters. (Chinese, Japanese, some Eurpean characters).

Comment 1

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 2

[John Dai[:jdai]]

Attachment: Part 1: Write 0x81 and 0x82 UCS2 String on UICC

Comment 3

[John Dai[:jdai]]

Attachment: Part 2: xpcshell tests.

Comment 4

[John Dai[:jdai]]

Attachment: Part 1: Write 0x81 and 0x82 UCS2 String on UICC.(V2)

Comment 5

[John Dai[:jdai]]

Attachment: Part 2: xpcshell tests. (V2)

Comment 6

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Comment on attachment 8580469 [details] [diff] [review]
Part 1: Write 0x81 and 0x82 UCS2 String on UICC.(V2)

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

Please see my inline comments, thank you.

::: dom/system/gonk/ril_worker.js
@@ +10174,5 @@
> +   *        alphaId and the length of trailing unused octets(0xff).
> +   * @param alphaId
> +   *        Alpha Identifier to be written.
> +   */
> +  writeICCUCS2String: function(numOctets, alphaId) {

s/alphaId/str/ and please also remember to update above comments.

@@ +10180,5 @@
> +    let min = 0x7FFF;
> +    let max = 0;
> +
> +    if (alphaId.length > 2) {
> +      for(let i = 0; i < alphaId.length; i++) {

nit: space between `for` and `(`.

@@ +10182,5 @@
> +
> +    if (alphaId.length > 2) {
> +      for(let i = 0; i < alphaId.length; i++) {
> +        let code = alphaId.charCodeAt(i);
> +        if (code & 0xFF00) {

This check seems for filtering out the GSM Default Alphabet character, please add more comments about it. And should the mask be `0xFF80`? (GSM Default Alphabet is a 7 bit coding)

@@ +10186,5 @@
> +        if (code & 0xFF00) {
> +          // 0x81 format can't compute 0x8000~0xFFFF, set range over 128
> +          if(code & 0x8000) {
> +            max = min + 128;
> +            break;

Since we parser all character of the target string in this for-loop, we already collect enough information to decide which coding scheme we should use.

And for the encoding part, we could just use switch-case which makes the logic clearer.

switch (scheme) {
  case 0x80: {
    // do encoding ...
    break;
  }
  case 0x81: {
    // do encoding ...
    break;
  }
  case 0x82: {
    // do encoding ...
    break;
  }
}

@@ +10200,5 @@
> +    }
> +
> +    // 0x81 and 0x82 only support 'half-page', 128 characters.
> +    if ((max - min) >= 0 && (max - min) < 128) {
> +      let base_pointer;

s/base_pointer/basePointer/

@@ +10203,5 @@
> +    if ((max - min) >= 0 && (max - min) < 128) {
> +      let base_pointer;
> +      // since 0x81 only support 128 characters,
> +      // either XX00~XX7f(bit 8 are 0) or XX80~XXff(bit 8 are 1)
> +      if ((min & 0x80) == (max & 0x80)) {

The checking here seems cannot cover all cases, the base pointer of 0x81 is 0x0hhhhhhhh0000000, we have to make sure the bits 15 to 8 of each character is the base pointer.

Comment 7

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Comment on attachment 8580471 [details] [diff] [review]
Part 2: xpcshell tests. (V2)

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

::: dom/system/gonk/tests/test_ril_worker_icc_ICCPDUHelper.js
@@ +56,5 @@
> +  let helper = context.GsmPDUHelper;
> +  let iccHelper = context.ICCPDUHelper;
> +  let alphaLen = 18;
> +  let len = 17;
> +  let test_data = [// 0x80

Looks good, but please help to add more test data for the corner cases.
For example,
- Some strings cannot be encoded as 0x81, but 0x82 
- If the characters are not located in the same "half-page" of the UCS2 code space, it could only be encoding as 0x80.

@@ +70,5 @@
> +  for (let i = 0; i < test_data.length; i++) {
> +    let test = test_data[i];
> +    do_print("Debug:test_write_icc_ucs2_string num:"+i);
> +    iccHelper.writeICCUCS2String(alphaLen, test);
> +    let encode = helper.readHexOctet();

We should also check the string are encoded by a coding scheme that we expect to use in |writeICCUCS2String|.

Comment 8

[John Dai[:jdai]]

Attachment: Part 1: Write 0x81 and 0x82 UCS2 String on UICC.(V3)

Comment 9

[John Dai[:jdai]]

Attachment: Part 2: xpcshell tests. (V3)

Comment 10

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Comment on attachment 8591595 [details] [diff] [review]
Part 1: Write 0x81 and 0x82 UCS2 String on UICC.(V3)

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

r=me with comments addressed. Thank you.

::: dom/system/gonk/ril_worker.js
@@ +9390,5 @@
> +    let max = 0;
> +    let scheme = 0x80;
> +    let basePointer;
> +
> +    if (str.length > 2) {

Move `let min ...` and `let max ...` here.
After deciding which |scheme| we should use, we also know what value of |basePointer| we should be set to.
So it seems we don't have to expose |min| and |max| outside this closure.

@@ +9411,5 @@
> +        // therefore it can't compute 0x8000~0xFFFF.
> +        // Since 0x81 only support 128 characters,
> +        // either XX00~XX7f(bit 8 are 0) or XX80~XXff(bit 8 are 1)
> +        if (((min & 0x7f80) == (max & 0x7f80)) &&
> +            ((min & 0x80) == (max & 0x80)) &&

It seems the check, "(min & 0x7f80) == (max & 0x7f80)", already cover this?

@@ +9413,5 @@
> +        // either XX00~XX7f(bit 8 are 0) or XX80~XXff(bit 8 are 1)
> +        if (((min & 0x7f80) == (max & 0x7f80)) &&
> +            ((min & 0x80) == (max & 0x80)) &&
> +            ((max & 0x8000) == 0)) {
> +          scheme = 0x81;

basePointer = min & 0x7f80;

@@ +9415,5 @@
> +            ((min & 0x80) == (max & 0x80)) &&
> +            ((max & 0x8000) == 0)) {
> +          scheme = 0x81;
> +        } else {
> +          scheme = 0x82;

basePointer = min;

@@ +9440,5 @@
> +        // trailing 0xff
> +        for (let i = str.length * 2; i < numOctets; i++) {
> +          GsmPDUHelper.writeHexOctet(0xff);
> +        }
> +        break;

All the data is written, it seems we could just return here.

@@ +9462,5 @@
> +        if (str.length > (numOctets - 3)) {
> +          str = str.substring(0, numOctets - 3);
> +        }
> +
> +        // len

This comment is meaningless, please make is clearer or remove it.

@@ +9465,5 @@
> +
> +        // len
> +        GsmPDUHelper.writeHexOctet(str.length);
> +        // 0x81 base pointer: 0hhh hhhh h000 0000
> +        basePointer = min & 0x7f80;

Move this to line#9417.

@@ +9488,5 @@
> +        if (str.length > (numOctets - 4)) {
> +          str = str.substring(0, numOctets - 4);
> +        }
> +
> +        // len

Ditto.

@@ +9491,5 @@
> +
> +        // len
> +        GsmPDUHelper.writeHexOctet(str.length);
> +
> +        basePointer = min;

Ditto.

Comment 11

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Comment on attachment 8591596 [details] [diff] [review]
Part 2: xpcshell tests. (V3)

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

::: dom/system/gonk/tests/test_ril_worker_icc_ICCPDUHelper.js
@@ +55,5 @@
> +  let context = worker.ContextPool._contexts[0];
> +  let helper = context.GsmPDUHelper;
> +  let iccHelper = context.ICCPDUHelper;
> +  let alphaLen = 18;
> +  let len = 17;

|len| can be replaced by `alphaLen - 1`.

@@ +63,5 @@
> +      // 2 UCS2 character not located in the same half-page.
> +      data: "Fire \u82b3\u8233"
> +    }, {
> +      encode: 0x80,
> +      data: "\u694a\u704a"

Also add comment for this test data.

And please help add one more case for 0x80: string only contains one character.

@@ +78,5 @@
> +      // 2 UCS2 character within same half-page, but bit 8 is different.
> +      data: "Fire \u0514\u0593"
> +    }, {
> +      encode: 0x82,
> +      data: "Fire \u8000\u8001"

Also add comment for this test data.

@@ +81,5 @@
> +      encode: 0x82,
> +      data: "Fire \u8000\u8001"
> +    }, {
> +      encode: 0x82,
> +      data: "Fire \ufffd\ufffe"

Ditto.

@@ +88,5 @@
> +  for (let i = 0; i < test_data.length; i++) {
> +    let test = test_data[i];
> +    iccHelper.writeICCUCS2String(alphaLen, test.data);
> +    let encode = helper.readHexOctet();
> +    equal(encode, test.encode);

equal(helper.readHexOctet(), test.encode);

Comment 12

[John Dai[:jdai]]

Attachment: Part 1: Write 0x81 and 0x82 UCS2 String on UICC. (Final), r=echen

Address comment10.

Comment 13

[John Dai[:jdai]]

Attachment: Part 2: xpcshell tests. (V4)

Address comment 11.

Comment 14

[Edgar Chen [:edgar] (OOO ~ 07/13, 07/16 ~ 07/20)]

Comment on attachment 8592733 [details] [diff] [review]
Part 2: xpcshell tests. (V4)

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

Thank you.

::: dom/system/gonk/tests/test_ril_worker_icc_ICCPDUHelper.js
@@ +95,5 @@
> +  for (let i = 0; i < test_data.length; i++) {
> +    let test = test_data[i];
> +    iccHelper.writeICCUCS2String(alphaLen, test.data);
> +    equal(helper.readHexOctet(), test.encode);
> +    equal(iccHelper.readICCUCS2String(test.encode, len), test.data);

Nit: s/len/alphaLen - 1/

Comment 15

[John Dai[:jdai]]

Attachment: Part 2: xpcshell tests. (Final), r=echen

Comment 16

[John Dai[:jdai]]

Try Server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a72f376d645b

Comment 17

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/integration/b2g-inbound/rev/e8dbd3b93ddc
https://hg.mozilla.org/integration/b2g-inbound/rev/2ad3b224ad1c

Comment 18

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/e8dbd3b93ddc
https://hg.mozilla.org/mozilla-central/rev/2ad3b224ad1c