823865 - B2G RIL: write AlphaId and DiallingNumber in GsmPDUHelper

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

To read EF_ADN, we have a utility function called 'GsmPDUHelper.readAlphaIdDiallingNumber' to parse the record. Now for exporting contact to SIM, we also need an utility function to write alphaId and dialling number.

Comment 1

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

Attachment: Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber.

Comment 2

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

Attachment: Part 2: xpcshell tests for writeAlphaIdDiallingNumber

Comment 3

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

Attachment: Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v2

Comment 4

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

Attachment: Part 2: xpcshell tests for writeAlphaIdDiallingNumber. v2

Comment 5

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

Attachment: Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v3

Comment 6

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

Attachment: Part 2: xpcshell tests for writeAlphaIdDiallingNumber. v3

Comment 7

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

Attachment: Part 2: xpcshell tests for writeAlphaIdDiallingNumber. v3

Comment 8

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

Attachment: Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v4

Comment 9

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

Attachment: Part 2: xpcshell tests for writeAlphaIdDiallingNumber. v4

Comment 10

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

Attachment: Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v5

use recordSize instead

Comment 11

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

Attachment: Part 2: xpcshell tests for writeAlphaIdDiallingNumber. v5

revised accordingly.

Comment 12

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

Comment on attachment 697804 [details] [diff] [review]
Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v5

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

It looks good overall, but please address the comments. Thank you:)

::: dom/system/gonk/ril_worker.js
@@ +5972,5 @@
> +    let contact = null;
> +    if (alphaId || number) {
> +      contact = {alphaId: alphaId,
> +                 number: number};
> +    }

contact = {alphaId: alphaId || null,
                number: number || null};

@@ +6071,5 @@
> +      return;
> +    }
> +
> +    // If alphaId is GSM 8 bit or it's empty.
> +    if (ICCUtilsHelper.isGsm8BitAlphabet(alphaId) || !alphaId) {

if (!alphaId || ICCUtilsHelper.isGsm8BitAlphabet(alphaId)) {
    ... ...
}

@@ +9419,5 @@
> +      let octet = langTable.indexOf(c);
> +      if (octet == -1) {
> +        octet = langShiftTable.indexOf(c);
> +        if (octet == -1) {
> +          octet = langTable.indexOf(' ');

We can just 'return false' when char 'c' isn't found in langShiftTable, right? If so, then we don't need the following
if (c != ' ' && octet == langTable.indexOf(' ')) {
    return false;
}

Comment 13

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

Comment on attachment 697805 [details] [diff] [review]
Part 2: xpcshell tests for writeAlphaIdDiallingNumber. v5

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

Tests are great! However, there seem some grammatical errors in comments. Would you please review them again? Thank you.

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +169,5 @@
> +
> +  // 2 octets * 2 = 4 octets for 2 gsm extended alphabets,
> +  // 1 octet for 1 gsm alphabet,
> +  // 2 octes for trailing 0xff.
> +  // Total octets written is 7.

"Totally 7 octets are to be written."

@@ +187,5 @@
> +  const langTable = PDU_NL_LOCKING_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT];
> +  const langShiftTable = PDU_NL_SINGLE_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT];
> +
> +  // Maximum 3 octets can be written, but the string is of length 4.
> +  // So only 3 characters shall be written.

I rewrote the comments to correct the grammar, and hopefully make it more clear. :)
"The maximum of the number of octets that can be written is 3. Only 3 characters shall be written even the length of the string is 4."

@@ +234,5 @@
> +  helper.writeAlphaIdentifier(str.length + ffLen, str);
> +  do_check_eq(helper.readAlphaIdentifier(str.length + ffLen), str);
> +
> +  // UCS2
> +  str = "Mozilla\u694a"

Forgot ";"

@@ +241,5 @@
> +  do_check_eq(helper.readAlphaIdentifier(str.length * 2 + ffLen), str);
> +
> +  // Test with maximum octets written.
> +  // 1 coding scheme (0x80) and 1 UCS2 character, total 3 octets.
> +  str = "\u694a"

ditto.

@@ +246,5 @@
> +  helper.writeAlphaIdentifier(3, str);
> +  do_check_eq(helper.readAlphaIdentifier(3), str);
> +
> +  // 1 coding scheme (0x80) and 2 UCS2 characters, total 5 octets.
> +  // But numOctets is limited to 4, so only 1 UCS2 character can be written.

Grammar: remove "But"

Comment 14

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> Comment on attachment 697804 [details] [diff] [review]
> Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v5
> 
> Review of attachment 697804 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good overall, but please address the comments. Thank you:)
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +5972,5 @@
> > +    let contact = null;
> > +    if (alphaId || number) {
> > +      contact = {alphaId: alphaId,
> > +                 number: number};
> > +    }
> 
> contact = {alphaId: alphaId || null,
>                 number: number || null};
> 

But in this way, an object will always be returned, 
the origin code will return an object only when we got 'alphaId' or 'number', otherwise return null.

Comment 15

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

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #14)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #12)
> > Comment on attachment 697804 [details] [diff] [review]
> > Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v5
> > 
> > Review of attachment 697804 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It looks good overall, but please address the comments. Thank you:)
> > 
> > ::: dom/system/gonk/ril_worker.js
> > @@ +5972,5 @@
> > > +    let contact = null;
> > > +    if (alphaId || number) {
> > > +      contact = {alphaId: alphaId,
> > > +                 number: number};
> > > +    }
> > 
> > contact = {alphaId: alphaId || null,
> >                 number: number || null};
> > 
> 
> But in this way, an object will always be returned, 
> the origin code will return an object only when we got 'alphaId' or
> 'number', otherwise return null.

Sounds reasonable as well. Fine by that.

Comment 16

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

Attachment: Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v6

Addressed comments.

Comment 17

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

Attachment: Part 2: xpcshell tests for writeAlphaIdDiallingNumber. v6

Addressed review comments.

Comment 18

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

Comment on attachment 698582 [details] [diff] [review]
Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v6

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

Great, thank you:)

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d74a5a6e1aac
https://hg.mozilla.org/integration/mozilla-inbound/rev/e005476eb9f6

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d74a5a6e1aac
https://hg.mozilla.org/mozilla-central/rev/e005476eb9f6