Closed Bug 935401 Opened 6 years ago Closed 6 years ago

B2G RIL: Add more ICC Record Helpers

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: allstars.chh, Assigned: gwang)

References

Details

Attachments

(2 files, 7 obsolete files)

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.
Depends on: 821611
Depends on: 824611
No longer depends on: 821611
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
Attachment #832817 - Flags: review?(allstars.chh)
Attachment #832818 - Flags: review?(allstars.chh)
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?
Attachment #832817 - Flags: review?(allstars.chh) → review-
Attachment #832818 - Flags: review?(allstars.chh)
1. ICCRecordHelper -> SimRecordHelper, and move RuimRecordHelper upper.
2. All fetch card calls into fetchICCRecords, then separate by appType.
Attachment #832817 - Attachment is obsolete: true
Add 2 test in test_ril_worker_icc.js, modify icc/ruim tests.
Attachment #832818 - Attachment is obsolete: true
Attachment #8336682 - Attachment is obsolete: true
Attachment #8337513 - Flags: review?(allstars.chh)
Attachment #8336683 - Flags: review?(allstars.chh)
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
Attachment #8337513 - Flags: review?(allstars.chh)
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.
Attachment #8336683 - Flags: review?(allstars.chh)
1. Remove readICCID in fetchSimRecords.
2. Modify naming, ICC -> (U)SIM or USIM/RUIM
Attachment #8337513 - Attachment is obsolete: true
Remove readICCID in fetchSimRecords test.
Attachment #8336683 - Attachment is obsolete: true
Patches are already re-based to last change.
TRY: https://tbpl.mozilla.org/?tree=Try&rev=ce6aeae2d64a
Attachment #8338284 - Flags: review?(allstars.chh)
Attachment #8338286 - Flags: review?(allstars.chh)
Attachment #8338284 - Flags: review?(allstars.chh) → review+
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
Attachment #8338286 - Flags: review?(allstars.chh)
Part2, v3 change: 
1. fetchTag = 0x01 for "fetchRuimRecords", 0x02 for "fetchSimRecords".
2. Add test for RIL.appType = CARD_APPTYPE_SIM;
Attachment #8340951 - Flags: review?(allstars.chh)
Attachment #8340951 - Flags: review?(allstars.chh) → review+
Keywords: checkin-needed
Part 1 is bitrotted. Please rebase.
Keywords: checkin-needed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.