925618 - B2G RIL: move reading MBDN/MSISDN into readSST()

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[shawn ku [:sku]]

Both MSISDN (0x6f40) and MBDN (0x6fc7) are optional EFs, these two files are better to be handled by the callback of readSST().

Comment 1

[shawn ku [:sku]]

Attachment: Bug 925618 - B2g RIL: move MBDN/MSISDN reading to the callback of readSST()

Comment 2

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

Comment on attachment 815711 [details] [diff] [review]
Bug 925618 - B2g RIL: move MBDN/MSISDN reading to the callback of readSST()

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

The patch looks good to me, 
but I'd like to see a test case for this.

Thanks

Comment 3

[shawn ku [:sku]]

Attachment: Bug 925618 Part 1: RIL implementation on moving MBDN/MSISDN reading to the callback of readSST().

Comment 4

[shawn ku [:sku]]

Attachment: Bug 925618 Part 2: Add test for moving MBDN/MSISDN reading to the callback of readSST().

Comment 5

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

Comment on attachment 817019 [details] [diff] [review]
Bug 925618 Part 2: Add test for moving MBDN/MSISDN reading to the callback of readSST().

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1895,5 @@
>  
>    run_next_test();
>  });
> +
> +add_test(function test_reading_optional_efs_msisdn_and_mbdn() {

remove msisdn_and_mbdn
Some day we could extend yours tests to test more optional EFs.

@@ +1898,5 @@
> +
> +add_test(function test_reading_optional_efs_msisdn_and_mbdn() {
> +  let worker = newUint8Worker();
> +  let record = worker.ICCRecordHelper;
> +  let helper = worker.GsmPDUHelper;

gsmPdu, or gsm
There are many helpers here.

@@ +1902,5 @@
> +  let helper = worker.GsmPDUHelper;
> +  let ril    = worker.RIL;
> +  let buf    = worker.Buf;
> +  let io     = worker.ICCIOHelper;
> +  let utils  = worker.ICCUtilsHelper;

Where did you use this?

@@ +1908,5 @@
> +  // TODO: only enable MSISDN and MBDN for testing currently.
> +  let SST_EF_ON = [0x00, 0x00, 0x10, 0x00, 0x00,
> +                   0x40, 0x00, 0x00, 0x00, 0x00,
> +                   0x00, 0x00, 0x00, 0x00, 0x00];
> +  let SST_ALL_OFF = [0x00, 0x00, 0x00, 0x00, 0x00,

Don't need to test this case.

@@ +1913,5 @@
> +                     0x00, 0x00, 0x00, 0x00, 0x00,
> +                     0x00, 0x00, 0x00, 0x00, 0x00];
> +
> +  // TODO: add all necessary optional EFs eventually
> +  let supported_efs = [ICC_EF_MSISDN, ICC_EF_MBDN];

supportedEf
Also if there's a function could covert this EFs to SST Table, then you don't need to manually write SST_EF_ON above.

@@ +1914,5 @@
> +                     0x00, 0x00, 0x00, 0x00, 0x00];
> +
> +  // TODO: add all necessary optional EFs eventually
> +  let supported_efs = [ICC_EF_MSISDN, ICC_EF_MBDN];
> +  let counter = 0;

move this before do_test

@@ +1916,5 @@
> +  // TODO: add all necessary optional EFs eventually
> +  let supported_efs = [ICC_EF_MSISDN, ICC_EF_MBDN];
> +  let counter = 0;
> +
> +  ril.appType = CARD_APPTYPE_USIM;

Can you test SIM also?

@@ +1920,5 @@
> +  ril.appType = CARD_APPTYPE_USIM;
> +  function do_test(sstTable, availability) {
> +    ril.updateCellBroadcastConfig = function fakeUpdateCellBroadcastConfig() {
> +      // Ignore updateCellBroadcastConfig after reading SST
> +    };

Move this out of do_test

@@ +1948,5 @@
> +      if (options.callback) {
> +        options.callback(options);
> +      }
> +
> +      do_check_true(counter == 0);

That seems not so correct to me.
You are counting the total times,
so if one day one of the functions isn't called, say we accidentally remove readMSISDN from readSST,
we only know this test fails but don't know which function isn't called.

Also if I try to only enable MSISDN, but it turned out it's MBN being called, this test cannot tell that.

@@ +1957,5 @@
> +
> +  counter = supported_efs.length;
> +  do_test(SST_EF_ON, true);
> +  counter = 0;
> +  do_test(SST_ALL_OFF, false);

no need this.

Comment 6

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

Comment on attachment 817018 [details] [diff] [review]
Bug 925618 Part 1: RIL implementation on moving MBDN/MSISDN reading to the callback of readSST().

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

Add r=me

Comment 7

[shawn ku [:sku]]

Attachment: Bug 925618 Part 2: Add test for moving MBDN/MSISDN reading to the callback of readSST(). v2.

Comment 8

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

Comment on attachment 817641 [details] [diff] [review]
Bug 925618 Part 2: Add test for moving MBDN/MSISDN reading to the callback of readSST(). v2.

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

This patch is much better now.
But I have some questions.
Please see below.


Also the patch title needs to be updated, it should be test case.
Please send r? to me again once questions are answered and comments are addressed.

Thanks

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1903,5 @@
> +  let ril    = worker.RIL;
> +  let buf    = worker.Buf;
> +  let io     = worker.ICCIOHelper;
> +
> +  function buildupSST(supportedEf) {

buildSST

@@ +1904,5 @@
> +  let buf    = worker.Buf;
> +  let io     = worker.ICCIOHelper;
> +
> +  function buildupSST(supportedEf) {
> +    let sstTable = [];

SST stands for SIM Service Table,
so adding another Table in the end seems not neccesary.

@@ +1949,5 @@
> +      buf.writeInt32(sstTable.length * 2);
> +
> +      // Write data
> +      for (let i = 0; i < sstTable.length; i++) {
> +         gsmPdu.writeHexOctet(sstTable[i] ? sstTable[i] : 0);

Why use ? to check?
Will it become undefined?

@@ +1963,5 @@
> +      if (testEf.length !== 0) {
> +        do_print("Un-handled EF: " + JSON.stringify(testEf));
> +        do_check_true(false);
> +      }
> +      do_check_true(availability);

availability is always true?

Comment 9

[shawn ku [:sku]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #8)
> Comment on attachment 817641 [details] [diff] [review]
> Bug 925618 Part 2: Add test for moving MBDN/MSISDN reading to the callback
> of readSST(). v2.
> 
> Review of attachment 817641 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch is much better now.
> But I have some questions.
> Please see below.
> 
> 
> Also the patch title needs to be updated, it should be test case.
> Please send r? to me again once questions are answered and comments are
> addressed.
> 
> Thanks

Will do.

> 
> ::: dom/system/gonk/tests/test_ril_worker_icc.js
> @@ +1903,5 @@
> > +  let ril    = worker.RIL;
> > +  let buf    = worker.Buf;
> > +  let io     = worker.ICCIOHelper;
> > +
> > +  function buildupSST(supportedEf) {
> 
> buildSST

Will change function name in next patch.

> 
> @@ +1904,5 @@
> > +  let buf    = worker.Buf;
> > +  let io     = worker.ICCIOHelper;
> > +
> > +  function buildupSST(supportedEf) {
> > +    let sstTable = [];
> 
> SST stands for SIM Service Table,
> so adding another Table in the end seems not neccesary.

Thanks, will revise this in next patch.

> 
> @@ +1949,5 @@
> > +      buf.writeInt32(sstTable.length * 2);
> > +
> > +      // Write data
> > +      for (let i = 0; i < sstTable.length; i++) {
> > +         gsmPdu.writeHexOctet(sstTable[i] ? sstTable[i] : 0);
> 
> Why use ? to check?
> Will it become undefined?

Yes, we will got sst = [undefined, undefined, 0x40, undefined, ....] as an example after buildupSST (will rename to buildSST).
sstTable[i] may be undefined, and will got an excpetion by gsmPdu.writeHexOctet.
That's why I add one more protection here.

> 
> @@ +1963,5 @@
> > +      if (testEf.length !== 0) {
> > +        do_print("Un-handled EF: " + JSON.stringify(testEf));
> > +        do_check_true(false);
> > +      }
> > +      do_check_true(availability);
> 
> availability is always true?

yes, it is always true, I will remove this check in next patch.


Finally, thanks for your comment/suggection.
sku

Comment 10

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

Comment on attachment 817641 [details] [diff] [review]
Bug 925618 Part 2: Add test for moving MBDN/MSISDN reading to the callback of readSST(). v2.

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

Thanks for answering the question.
Then this patch looks good to me.

Add r=me.

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1949,5 @@
> +      buf.writeInt32(sstTable.length * 2);
> +
> +      // Write data
> +      for (let i = 0; i < sstTable.length; i++) {
> +         gsmPdu.writeHexOctet(sstTable[i] ? sstTable[i] : 0);

gsmPdu.writeHexOctet(sstTable[i] || 0);

Comment 11

[shawn ku [:sku]]

Attachment: [Final Patch] Bug 925618 Part 1: RIL implementation on moving MBDN/MSISDN reading to the callback of readSST(). r=yoshi

Comment 12

[shawn ku [:sku]]

Attachment: [Final Patch] Bug 925618 Part 2: Add test case for moving reading MBDN/MSISDN into readSST(). r=yoshi

Comment 13

[shawn ku [:sku]]

update try server link - https://tbpl.mozilla.org/?tree=Try&rev=44d54e61471f

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/63d9aa9e39a4
https://hg.mozilla.org/integration/b2g-inbound/rev/11d3e13a8144

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63d9aa9e39a4
https://hg.mozilla.org/mozilla-central/rev/11d3e13a8144