Closed Bug 962995 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog, firefox40 fixed)

RESOLVED FIXED
2.2 S10 (17apr)
tracking-b2g backlog
Tracking Status
firefox40 --- fixed

People

(Reporter: allstars.chh, Assigned: jdai)

Details

(Whiteboard: [grooming])

Attachments

(2 files, 7 obsolete files)

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).
Put this bug into backlog.
blocking-b2g: --- → backlog
Assignee: allstars.chh → nobody
Assignee: nobody → jdai
Whiteboard: [grooming]
blocking-b2g: backlog → ---
Attachment #8580462 - Flags: review?(echen)
Attached patch Part 2: xpcshell tests. (obsolete) — Splinter Review
Attachment #8580463 - Flags: review?(echen)
Attachment #8580462 - Attachment is obsolete: true
Attachment #8580462 - Flags: review?(echen)
Attachment #8580469 - Flags: review?(echen)
Attached patch Part 2: xpcshell tests. (V2) (obsolete) — Splinter Review
Attachment #8580463 - Attachment is obsolete: true
Attachment #8580463 - Flags: review?(echen)
Attachment #8580471 - Flags: review?(echen)
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.
Attachment #8580469 - Flags: review?(echen)
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|.
Attachment #8580471 - Flags: review?(echen)
Attachment #8580469 - Attachment is obsolete: true
Attachment #8591595 - Flags: review?(echen)
Attached patch Part 2: xpcshell tests. (V3) (obsolete) — Splinter Review
Attachment #8580471 - Attachment is obsolete: true
Attachment #8591596 - Flags: review?(echen)
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.
Attachment #8591595 - Flags: review?(echen) → review+
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);
Attachment #8591596 - Flags: review?(echen)
Attached patch Part 2: xpcshell tests. (V4) (obsolete) — Splinter Review
Address comment 11.
Attachment #8591596 - Attachment is obsolete: true
Attachment #8592733 - Flags: review?(echen)
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/
Attachment #8592733 - Flags: review?(echen) → review+
Attachment #8592733 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e8dbd3b93ddc
https://hg.mozilla.org/mozilla-central/rev/2ad3b224ad1c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)
You need to log in before you can comment on or make changes to this bug.