824611 - B2G RIL : Add ICC PDU Helper

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

In GsmPDUHelper, there are many functions only used by ICC, like readDiallingNumber, readAlphaIdentifier. We could create a helper for ICC PDU to simply/refactor GsmPDUHelper.

Comment 1

[Georgia Wang]

Attachment: Part1: Add ICC PDU Helper

Comment 2

[Georgia Wang]

Attachment: Part2: modify xpcshell test for ICCPDIHelper.

Comment 3

[Georgia Wang]

Try: https://tbpl.mozilla.org/?tree=Try&rev=0e9aa763e3bb

Comment 4

[Georgia Wang]

Attachment: Part1: Add ICC PDU Helper

Comment 5

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

Comment on attachment 830717 [details] [diff] [review]
Part1: Add ICC PDU Helper

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

::: dom/system/gonk/ril_worker.js
@@ +9049,5 @@
>    }
>  };
>  
> +/**
> + * Helper for ICC PDU

Helper for processing ICC PDUs.

@@ +9051,5 @@
>  
> +/**
> + * Helper for ICC PDU
> + *
> + * Move some ICC specific function from GsmPDUHelper to ICCPDUHelper.

remove this

Comment 6

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

Comment on attachment 830715 [details] [diff] [review]
Part2: modify xpcshell test for ICCPDIHelper.

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +245,4 @@
>  
>    run_next_test();
>  });
>  

It seems we're missing testing readAlphaIdDiallingNumber, readAlphaIdentifier here.

Maybe more are missing?

Comment 7

[Georgia Wang]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #6)
> Comment on attachment 830715 [details] [diff] [review]
> Part2: modify xpcshell test for ICCPDIHelper.
> 
> Review of attachment 830715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/tests/test_ril_worker_icc.js
> @@ +245,4 @@
> >  
> >    run_next_test();
> >  });
> >  
> 
> It seems we're missing testing readAlphaIdDiallingNumber,
> readAlphaIdentifier here.
> 
> Maybe more are missing?

For ICC part, seems there's no testing for below functions:
1.readAlphaIdDiallingNumber
2.readAlphaIdentifier
3.readNumberWithLength
4.writeNumberWithLength

Will implement in test_ril_worker_icc.js.

Comment 8

[Georgia Wang]

Attachment: Part1: Add ICC PDU Helper v2

Modify comment :)

Comment 9

[Georgia Wang]

Attachment: Part2: Add and modify tests for ICCPDUHelper. v2

Add tests for the four functions in ICCPDUHelper:
1.readAlphaIdDiallingNumber
2.readAlphaIdentifier
3.readNumberWithLength
4.writeNumberWithLength

Comment 10

[Georgia Wang]

TRY: https://tbpl.mozilla.org/?tree=Try&rev=a685a74c669a

Comment 11

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

Comment on attachment 832698 [details] [diff] [review]
Part2: Add and modify tests for ICCPDUHelper. v2

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +277,5 @@
> +  for (let i = 0; i < array2.length; i++) {
> +    helper.writeHexOctet(array2[i]);
> +  }
> +  do_check_eq(iccHelper.readAlphaIdentifier(array2.length), "Mozilla\u694a");
> +  

nit, ws

@@ +466,5 @@
> +  let helper = worker.GsmPDUHelper;
> +  let iccHelper = worker.ICCPDUHelper;
> +  let number = "123456789";
> +
> +  iccHelper.readDiallingNumber = function (numLen) {

You should check numLen here.

@@ +510,5 @@
> +
> +  // null
> +  let number_3;
> +  iccHelper.writeNumberWithLength(number_3);
> +  do_check_eq(0xff, helper.readHexOctet());

if the number is null,
writeNumberWithLength will write ADN_MAX_BCD_NUMBER_BYTES + 1 octets,
but you only read 1 octet here, it may cause problems when running xpcshell tests.

Comment 12

[Georgia Wang]

Attachment: Part2: Add and modify tests for ICCPDUHelper. v3

Diff between v2 and v3. now waiting for try server result.


@@ -278,7 +278,7 @@ add_test(function test_read_alpha_identifier() {
     helper.writeHexOctet(array2[i]);
   }
   do_check_eq(iccHelper.readAlphaIdentifier(array2.length), "Mozilla\u694a");
-  
+
   // GSM 8 Bit Unpacked
   const langTable = PDU_NL_LOCKING_SHIFT_TABLES[PDU_NL_IDENTIFIER_DEFAULT];
   for (let i = 0; i < PDU_NL_EXTENDED_ESCAPE; i++) {
@@ -468,7 +468,7 @@ add_test(function test_read_number_with_length() {
   let number = "123456789";
 
   iccHelper.readDiallingNumber = function (numLen) {
-    return number;
+    return number.Substring(0, numLen);
   };
 
   helper.writeHexOctet(number.length + 1);
@@ -511,7 +511,9 @@ add_test(function test_write_number_with_length() {
   // null
   let number_3;
   iccHelper.writeNumberWithLength(number_3);
-  do_check_eq(0xff, helper.readHexOctet());
+  for (let i = 0; i < (ADN_MAX_BCD_NUMBER_BYTES + 1); i++) {
+    do_check_eq(0xff, helper.readHexOctet());
+  }

Comment 13

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

Comment on attachment 832802 [details] [diff] [review]
Part2: Add and modify tests for ICCPDUHelper. v3

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

I think this is another patch submitting r? without a proper test first.
If you have run the test you should get a TypeError.

If you don't plan to run the test before sending r?
It would be *very very* hard for me to review your patch because I have to check each character, each constant, each word for you. 
And I would review your patch with minumum priority, like once in a week or a month.

You also made the same mistake just one or two days ago.
https://bugzilla.mozilla.org/show_bug.cgi?id=908554#c26
And there are more if we dig out the bugzilla history.

Next time I will review your patch *only* when you have attached the try-result.

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +467,5 @@
> +  let iccHelper = worker.ICCPDUHelper;
> +  let number = "123456789";
> +
> +  iccHelper.readDiallingNumber = function (numLen) {
> +    return number.Substring(0, numLen);

substring?

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/String/substring

Comment 14

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

(In reply to Georgia Wang from comment #12)
> Created attachment 832802 [details] [diff] [review]
> Part2: Add and modify tests for ICCPDUHelper. v3
> 
> Diff between v2 and v3. now waiting for try server result.
> 
> 
It's great that you can attach the diff.
But do ask r? only when the try-server is green.

Let's save both's time to work more efficiently.

Comment 15

[Georgia Wang]

Attachment: Part2: Add and modify tests for ICCPDUHelper. v4

>"<. Correct number.Substring as number.substring.

TRY: https://tbpl.mozilla.org/?tree=Try&rev=22b616d8fc7c

Comment 16

[Georgia Wang]

Comment on attachment 8333671 [details] [diff] [review]
Part2: Add and modify tests for ICCPDUHelper. v4

Dear Yoshi, 
Try link already provide in previous comment.
Thank you for all the trouble you've taken....

Comment 17

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

Comment on attachment 8333671 [details] [diff] [review]
Part2: Add and modify tests for ICCPDUHelper. v4

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +357,5 @@
> +  iccHelper.readNumberWithLength = function () {
> +    return contactW.number;
> +  };
> +
> +  helper.writeHexOctet(0x00); // fake length

Is this string length?
Then you miss the string delimiter.

@@ +364,5 @@
> +
> +  let contactR = iccHelper.readAlphaIdDiallingNumber(recordSize);
> +  do_check_eq(contactW.alphaId, contactR.alphaId);
> +  do_check_eq(contactW.number, contactR.number);
> +

When the alphaId and number are "", a null contact should be returned, please add this test.

@@ +473,5 @@
> +
> +  helper.writeHexOctet(number.length + 1);
> +  helper.writeHexOctet(PDU_TOA_ISDN);
> +  for (let i = 0; i < (ADN_MAX_BCD_NUMBER_BYTES - number.length); i++) {
> +    helper.writeHexOctet(0xff);

writing useless octets,

@@ +474,5 @@
> +  helper.writeHexOctet(number.length + 1);
> +  helper.writeHexOctet(PDU_TOA_ISDN);
> +  for (let i = 0; i < (ADN_MAX_BCD_NUMBER_BYTES - number.length); i++) {
> +    helper.writeHexOctet(0xff);
> +  }

also you didn't test when the numLen octet is 0xff.

Comment 18

[Georgia Wang]

Attachment: Part2: Add and modify tests for ICCPDUHelper. v5

Change:
1. In add_test(function test_read_alpha_id_dialling_number() {
   modify this test, add null test case. 

2. In add_test(function test_read_number_with_length() {
   remove unnecessary octets and add test for 0xff.
+  helper.writeHexOctet(number.length + 1);
+  helper.writeHexOctet(PDU_TOA_ISDN);
+  do_check_eq(iccHelper.readNumberWithLength(), number);
+  
+  helper.writeHexOctet(0xff);
+  do_check_eq(iccHelper.readNumberWithLength(),null);

Comment 19

[Georgia Wang]

TRY:: https://tbpl.mozilla.org/?tree=Try&rev=013d7ecac694

Comment 20

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

Comment on attachment 8335933 [details] [diff] [review]
Part2: Add and modify tests for ICCPDUHelper. v5

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +480,5 @@
> +
> +  helper.writeHexOctet(number.length + 1);
> +  helper.writeHexOctet(PDU_TOA_ISDN);
> +  do_check_eq(iccHelper.readNumberWithLength(), number);
> +  

nit, ws

@@ +482,5 @@
> +  helper.writeHexOctet(PDU_TOA_ISDN);
> +  do_check_eq(iccHelper.readNumberWithLength(), number);
> +  
> +  helper.writeHexOctet(0xff);
> +  do_check_eq(iccHelper.readNumberWithLength(),null);

nit, add a space after ,

Comment 21

[Georgia Wang]

Attachment: Part2: Add and modify tests for ICCPDUHelper. v6

modify nits base on comment 20.

Comment 22

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

Run try again before you're tring to push your patch.

Comment 23

[Georgia Wang]

TRY: https://tbpl.mozilla.org/?tree=Try&rev=8247a27f142a , result positive~

Comment 24

[Ken Chang[:kenkai]]

(In reply to Georgia Wang from comment #23)
> TRY: https://tbpl.mozilla.org/?tree=Try&rev=8247a27f142a , result positive~

Should you put "checkin-needed" again?

Comment 25

[Georgia Wang]

(In reply to Ken Chang from comment #24)
> (In reply to Georgia Wang from comment #23)
> > TRY: https://tbpl.mozilla.org/?tree=Try&rev=8247a27f142a , result positive~
> 
> Should you put "checkin-needed" again?

Oh, thanks for your reminder.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ba1fc388f146
https://hg.mozilla.org/integration/b2g-inbound/rev/7f8da0cc8b40

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ba1fc388f146
https://hg.mozilla.org/mozilla-central/rev/7f8da0cc8b40