935401 - B2G RIL: Add more ICC Record Helpers

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

Currently we have ICCRecordHelper and RuimRecordHelper in ril_worker.js, we should have more hierachies on these helpers.

- SIMRecordHelper is used to handle SIM/USIM
- RuimRecordHelper is used to handle CSIM/Ruim
- and a general or a more high level (U)ICCRecordHelper to handle common functionalities between those helpers above.

Comment 1

[Georgia Wang]

Attachment: Part1: Add SimRecordHelper in ril_worker.

Comment 2

[Georgia Wang]

Attachment: Part2: modify xpcshell test for SimRecordHelper.

Comment 3

[Georgia Wang]

Functions stay in "ICCRecordHelper"
  readICCID    
  readADNLike                                                                   
  updateADNLike
  readPBR                                   
  readIAP                                                               
  updateIAP                                                                     
  readEmail                                                               
  updateEmail                                  
  readANR                                                     
  updateANR                     
  findFreeRecordId        
                                                                  
Functions move to "SimRecordHelper"
   fetchICCRecord -> rename as "fetchSimRecord"                                                 
   readICCPhase   -> rename as "readSimPhase"                                                   
   readMSISDN                                       
   readAD                                                          
   readSPN                                                                 
   readSST                                                                
   readMBDN                                              
   readSPDI                                            
   readPNN                                                    
   readOPL                                                    
   readCBMI                                             
   readCBMID                                                                      
   readCBMIR                                                                                               
   readPLMNEntries

Comment 4

[Georgia Wang]

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

Comment 5

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

Comment on attachment 832817 [details] [diff] [review]
Part1: Add SimRecordHelper in ril_worker.

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

::: dom/system/gonk/ril_worker.js
@@ +11053,5 @@
>  
>  /**
>   * Helper for ICC records.
>   */
>  let ICCRecordHelper = {

Why not just rename this to SimRecordHelper?

And move those functions could be shared by SIM/USIM/CSIM/RUIM to a ICCRecordHelper?

Comment 6

[Georgia Wang]

Attachment: Part1: Add SimRecordHelper in ril_worker. v2

1. ICCRecordHelper -> SimRecordHelper, and move RuimRecordHelper upper.
2. All fetch card calls into fetchICCRecords, then separate by appType.

Comment 7

[Georgia Wang]

Attachment: Part2: modify xpcshell test for SimRecordHelper.

Add 2 test in test_ril_worker_icc.js, modify icc/ruim tests.

Comment 8

[Georgia Wang]

Attachment: Part1: Add SimRecordHelper in ril_worker. v2

Comment 9

[Georgia Wang]

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

Comment 10

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

Comment on attachment 8337513 [details] [diff] [review]
Part1: Add SimRecordHelper in ril_worker. v2

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

cancel r? for the readICCID.

::: dom/system/gonk/ril_worker.js
@@ +11649,5 @@
> + * Helper for (U)SIM Records.
> + */
> +let SimRecordHelper = {
> +  /**
> +   * Fetch (S)SIM records.

(U)

@@ +11652,5 @@
> +  /**
> +   * Fetch (S)SIM records.
> +   */
> +  fetchSimRecords: function fetchSimRecords() {
> +    ICCRecordHelper.readICCID();

I think Edgar has moved readICCID out of fetchXXXRecords in Bug 814637

@@ +11686,1 @@
>     * Read the MSISDN from the ICC.

from (U)SIM

Comment 11

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

Comment on attachment 8336683 [details] [diff] [review]
Part2: modify xpcshell test for SimRecordHelper.

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +2296,5 @@
> +      }
> +    }
> +  }
> +
> +  let expectCalled = ["readICCID", "getIMSI", "readAD", "readSST"];

same here.

Comment 12

[Georgia Wang]

Attachment: Part1: Add SimRecordHelper in ril_worker. v3

1. Remove readICCID in fetchSimRecords.
2. Modify naming, ICC -> (U)SIM or USIM/RUIM

Comment 13

[Georgia Wang]

Attachment: Part2: modify xpcshell test for SimRecordHelper. v2

Remove readICCID in fetchSimRecords test.

Comment 14

[Georgia Wang]

Patches are already re-based to last change.
TRY: https://tbpl.mozilla.org/?tree=Try&rev=ce6aeae2d64a

Comment 15

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

Comment on attachment 8338286 [details] [diff] [review]
Part2: modify xpcshell test for SimRecordHelper. v2

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

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +2311,5 @@
> +    fetchTag = 0x01;
> +  };
> +
> +  ruimRecord.fetchRuimRecords = function () {
> +    fetchTag = 0x10;

I am guessing you try to do bit-wise operation, in that case it should be 0x02 here.

0x01 = 0b00000001
0x10 = 0b00010000

@@ +2317,5 @@
> +
> +  RIL.appType = CARD_APPTYPE_SIM;
> +  iccRecord.fetchICCRecords();
> +  do_check_eq(fetchTag, 0x01);
> +

Add for USIM too

Comment 16

[Georgia Wang]

Attachment: Part2: modify xpcshell test for SimRecordHelper. v3

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

Comment 17

[Georgia Wang]

Part2, v3 change: 
1. fetchTag = 0x01 for "fetchRuimRecords", 0x02 for "fetchSimRecords".
2. Add test for RIL.appType = CARD_APPTYPE_SIM;

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Part 1 is bitrotted. Please rebase.

Comment 19

[Georgia Wang]

Attachment: Part1: Add SimRecordHelper in ril_worker. v4.

Rebase Part1.
TRY: https://tbpl.mozilla.org/?tree=Try&rev=62c9ec229303

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ca875ea4c03d
https://hg.mozilla.org/integration/b2g-inbound/rev/fc18d24e886c

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca875ea4c03d
https://hg.mozilla.org/mozilla-central/rev/fc18d24e886c