Closed Bug 925618 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: sku, Assigned: sku)

Details

Attachments

(2 files, 4 obsolete files)

Both MSISDN (0x6f40) and MBDN (0x6fc7) are optional EFs, these two files are better to be handled by the callback of readSST().
Attachment #815711 - Flags: review?(allstars.chh)
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
Attachment #815711 - Flags: review?(allstars.chh)
Attachment #817018 - Flags: review?(allstars.chh)
Attachment #817019 - Flags: review?(allstars.chh)
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.
Attachment #817019 - Flags: review?(allstars.chh) → review-
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
Attachment #817018 - Flags: review?(allstars.chh) → review+
Summary: B2g RIL: move MBDN/MSISDN reading to the callback of readSST() → B2G RIL: move reading MBDN/MSISDN into readSST()
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Attachment #817641 - Flags: review?(allstars.chh)
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?
Attachment #817641 - Flags: review?(allstars.chh)
(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 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);
Attachment #817641 - Flags: review+
update try server link - https://tbpl.mozilla.org/?tree=Try&rev=44d54e61471f
Attachment #817018 - Attachment is obsolete: true
Attachment #817641 - Attachment is obsolete: true
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.