Closed Bug 823865 Opened 12 years ago Closed 12 years ago

B2G RIL: write AlphaId and DiallingNumber in GsmPDUHelper

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(2 files, 11 obsolete files)

Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v6
12.61 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
Part 2: xpcshell tests for writeAlphaIdDiallingNumber. v6
10.83 KB, patch
Details | Diff | Splinter Review 
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.
Assignee: nobody → allstars.chh
Attachment #695453 - Attachment is obsolete: true
use recordSize instead
Attachment #695951 - Attachment is obsolete: true
revised accordingly.
Attachment #695952 - Attachment is obsolete: true
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;
}
Attachment #697804 - Flags: review?(htsai)
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"
Attachment #697805 - Flags: review?(htsai) → review+
(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.
(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.
Addressed comments.
Attachment #697804 - Attachment is obsolete: true
Attachment #698582 - Flags: review?(htsai)
Comment on attachment 698582 [details] [diff] [review]
Part 1: GsmPDUHelper.writeAlphaIdDiallingNumber. v6

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

Great, thank you:)
Attachment #698582 - Flags: review?(htsai) → review+
https://hg.mozilla.org/mozilla-central/rev/d74a5a6e1aac
https://hg.mozilla.org/mozilla-central/rev/e005476eb9f6
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: