Last Comment Bug 736941 - B2G RIL: Fetch SIM record
: B2G RIL: Fetch SIM record
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
:
:
Mentors:
Depends on: 736697 740300
Blocks: 715788 729440 736706 738132
  Show dependency treegraph
 
Reported: 2012-03-18 23:51 PDT by Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
Modified: 2014-09-17 11:43 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
[WIP]fetch SIM records. v1 (12.35 KB, patch)
2012-03-22 00:16 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
[WIP]fetch SIM records. v2 (12.28 KB, patch)
2012-03-22 00:35 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
[WIP]fetch SIM records. v3 (12.35 KB, patch)
2012-03-22 01:38 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
[WIP]fetch SIM records. v3 (12.42 KB, patch)
2012-03-22 01:44 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
[WIP]fetch SIM records. v4 (13.90 KB, patch)
2012-03-22 03:55 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
[WIP]fetch SIM records. v5 (14.51 KB, patch)
2012-03-26 02:39 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
[WIP]fetch SIM records. v6 (15.01 KB, patch)
2012-03-26 21:11 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Part 1: add/sort constants (3.17 KB, patch)
2012-03-28 05:56 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 2: Add Buf.skip() (1.49 KB, patch)
2012-03-28 05:56 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 3: Refactor _processMSISDNResponse (6.70 KB, patch)
2012-03-28 05:57 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 4: Fetch IMSI (2.58 KB, patch)
2012-03-28 05:57 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 5: Add read/writeHexOctetArray() (1.23 KB, patch)
2012-03-28 05:58 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 6: Fetch Administrative Data (6.00 KB, patch)
2012-03-28 05:58 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 7: Fetch UST(USIM Service Table) (3.59 KB, patch)
2012-03-28 05:58 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 8: Fetch ICC Identification (2.52 KB, patch)
2012-03-28 05:59 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 5: Add read/writeHexOctetArray() : V2 (1.23 KB, patch)
2012-03-28 21:33 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 6: Fetch Administrative Data : V2 (6.00 KB, patch)
2012-03-28 21:34 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 1: Add/Sort constants (3.63 KB, patch)
2012-03-30 00:22 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Part 2: Add Buf.skip (1.57 KB, patch)
2012-03-30 00:23 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Part 3: Add GsmPDUHelper.read/writeHexOctetArray (1.32 KB, patch)
2012-03-30 00:24 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Part 4: processICCResponse (6.80 KB, patch)
2012-03-30 00:25 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Part 5: Get MSISDN (3.97 KB, patch)
2012-03-30 00:28 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Part 6: Get IMSI (1.10 KB, patch)
2012-03-30 00:29 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Part 7: Get AD and retrieve MCC, MNC from IMSI (4.62 KB, patch)
2012-03-30 00:30 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Part 8: Get USST (3.48 KB, patch)
2012-03-30 00:31 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Part 2: Add Buf seekIncoming/seekOutgoing (6.25 KB, patch)
2012-04-10 21:34 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 4: processICCResponse (6.92 KB, patch)
2012-04-10 21:36 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 5: Get MSISDN (1.96 KB, patch)
2012-04-10 21:37 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
Part 1: Add/Sort constants (3.85 KB, patch)
2012-04-12 02:20 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review+
Details | Diff | Splinter Review
Part 2: Add Buf.seekIncoming/seekOutgoing (6.53 KB, patch)
2012-04-12 02:21 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review-
Details | Diff | Splinter Review
Part 3: Add GsmPDUHelper.read/writeHexOctetArray (1.50 KB, patch)
2012-04-12 02:23 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review+
Details | Diff | Splinter Review
Part 4: processICCResponse (7.08 KB, patch)
2012-04-12 02:24 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: feedback+
Details | Diff | Splinter Review
Part 5: Get MSISDN (2.24 KB, patch)
2012-04-12 02:25 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review+
Details | Diff | Splinter Review
Part 6: Get IMSI (1.28 KB, patch)
2012-04-12 02:26 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review+
Details | Diff | Splinter Review
Part 7: Get AD and retrieve MCC, MNC from IMSI (4.97 KB, patch)
2012-04-12 02:29 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review+
Details | Diff | Splinter Review
Part 8: Get USST (3.73 KB, patch)
2012-04-12 02:30 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review+
Details | Diff | Splinter Review
[WIP] Fetch SIM records. v2 (25.60 KB, patch)
2012-04-18 03:11 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
no flags Details | Diff | Splinter Review
Fetch Sim Records. v2 (23.90 KB, patch)
2012-04-18 23:28 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review+
Details | Diff | Splinter Review
Fetch Sim Records. v3 (23.69 KB, patch)
2012-04-22 20:30 PDT, Yoshi Huang[:allstars.chh] (OOO ~ 9.30)
philipp: review+
Details | Diff | Splinter Review

Description Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-18 23:51:06 PDT
Currently we need some information in the simcard. i.e.
EF_AD: To get the length of MNC, for looking up apn. 
EF_SPN, EF_SPDI, EF_PNN: To get the service provider information.
EF_SST: To get the SIM service table.
Comment 1 Vicamo Yang [:vicamo][:vyang] 2012-03-20 23:36:26 PDT
bug 736706 depends on SST(SIM Service Table) that will be fetched in this issue.
Comment 2 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-22 00:16:08 PDT
Created attachment 608245 [details] [diff] [review]
[WIP]fetch SIM records. v1

Get AD, SST and IMSI from simcard.
Comment 3 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-22 00:35:50 PDT
Created attachment 608251 [details] [diff] [review]
[WIP]fetch SIM records. v2

Revised by Vicamo's suggestions.
Comment 4 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-22 01:38:10 PDT
Created attachment 608261 [details] [diff] [review]
[WIP]fetch SIM records. v3

rebase to latest m-c.
note that need to apply patch of Bug 733990 first
Comment 5 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-22 01:44:54 PDT
Created attachment 608265 [details] [diff] [review]
[WIP]fetch SIM records. v3
Comment 6 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-22 03:55:25 PDT
Created attachment 608291 [details] [diff] [review]
[WIP]fetch SIM records. v4

Add send 'siminfo' event to RadioInterfaceLayer
Comment 7 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-26 02:39:53 PDT
Created attachment 609263 [details] [diff] [review]
[WIP]fetch SIM records. v5

Revised based on latest patch of Bug 733990.
Comment 8 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-26 21:11:14 PDT
Created attachment 609604 [details] [diff] [review]
[WIP]fetch SIM records. v6

Update constants.
I'll make a formal review request when Bug 733990 is reviewed.
thanks.
Comment 9 Vicamo Yang [:vicamo][:vyang] 2012-03-28 02:16:10 PDT
Depends on bug 736697 for GsmPDUHelper.readAddress(). The MSISDN number shares the same format with SMS sender address.
Comment 10 Vicamo Yang [:vicamo][:vyang] 2012-03-28 05:56:17 PDT
Created attachment 610089 [details] [diff] [review]
Part 1: add/sort constants

1) tear down to smaller patches
2) s/SST/UST/
Comment 11 Vicamo Yang [:vicamo][:vyang] 2012-03-28 05:56:48 PDT
Created attachment 610090 [details] [diff] [review]
Part 2: Add Buf.skip()
Comment 12 Vicamo Yang [:vicamo][:vyang] 2012-03-28 05:57:14 PDT
Created attachment 610091 [details] [diff] [review]
Part 3: Refactor _processMSISDNResponse
Comment 13 Vicamo Yang [:vicamo][:vyang] 2012-03-28 05:57:39 PDT
Created attachment 610092 [details] [diff] [review]
Part 4: Fetch IMSI
Comment 14 Vicamo Yang [:vicamo][:vyang] 2012-03-28 05:58:04 PDT
Created attachment 610093 [details] [diff] [review]
Part 5: Add read/writeHexOctetArray()
Comment 15 Vicamo Yang [:vicamo][:vyang] 2012-03-28 05:58:30 PDT
Created attachment 610094 [details] [diff] [review]
Part 6: Fetch Administrative Data
Comment 16 Vicamo Yang [:vicamo][:vyang] 2012-03-28 05:58:53 PDT
Created attachment 610095 [details] [diff] [review]
Part 7: Fetch UST(USIM Service Table)
Comment 17 Vicamo Yang [:vicamo][:vyang] 2012-03-28 05:59:21 PDT
Created attachment 610096 [details] [diff] [review]
Part 8: Fetch ICC Identification
Comment 18 Vicamo Yang [:vicamo][:vyang] 2012-03-28 06:16:03 PDT
(In reply to Vicamo Yang [:vicamo] from comment #11)
> Created attachment 610090 [details] [diff] [review]
> Part 2: Add Buf.skip()

Maybe turn this into seek()? So that we can also rewind data pointer back.
Comment 19 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-28 19:15:35 PDT
Comment on attachment 610093 [details] [diff] [review]
Part 5: Add read/writeHexOctetArray()

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

::: dom/system/gonk/ril_worker.js
@@ +2458,5 @@
> +
> +  /**
> +   * Write an array of hex-encoded octets.
> +   */
> +  writeHexOctetArray: function readHexOctetArray(array) {

should be writeHexOctetArray.
Comment 20 Vicamo Yang [:vicamo][:vyang] 2012-03-28 19:52:18 PDT
Comment on attachment 610094 [details] [diff] [review]
Part 6: Fetch Administrative Data

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

::: dom/system/gonk/ril_worker.js
@@ +1341,5 @@
>              break;
>            }
> +          case EF_TYPE_TRANSPARENT: {
> +            Buf.skip(RESPONSE_DATA_FILE_SIZE * 4);
> +            let fileSize = GsmPDUHelper.readHexOctet() << 8 \

illegal character: backslash
Comment 21 Vicamo Yang [:vicamo][:vyang] 2012-03-28 21:33:39 PDT
Created attachment 610424 [details] [diff] [review]
Part 5: Add read/writeHexOctetArray() : V2

comment #19, should be writeHexOctetArray.
Comment 22 Vicamo Yang [:vicamo][:vyang] 2012-03-28 21:34:52 PDT
Created attachment 610425 [details] [diff] [review]
Part 6: Fetch Administrative Data : V2

comment #20, illegal backslash character
Comment 23 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-30 00:22:39 PDT
Created attachment 610817 [details] [diff] [review]
Part 1: Add/Sort constants
Comment 24 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-30 00:23:21 PDT
Created attachment 610818 [details] [diff] [review]
Part 2: Add Buf.skip
Comment 25 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-30 00:24:44 PDT
Created attachment 610819 [details] [diff] [review]
Part 3: Add GsmPDUHelper.read/writeHexOctetArray
Comment 26 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-30 00:25:42 PDT
Created attachment 610820 [details] [diff] [review]
Part 4: processICCResponse
Comment 27 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-30 00:28:58 PDT
Created attachment 610821 [details] [diff] [review]
Part 5: Get MSISDN

Revised getMSISDN.
Noted that I use the function 'GsmPDUHelper.readAddress(len)" suggested by Vicamo, but his patch is still waiting for be reviewed.
Bug 736697, patch Part 2: Refactor to use share methods
https://bugzilla.mozilla.org/attachment.cgi?id=610442
Comment 28 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-30 00:29:33 PDT
Created attachment 610822 [details] [diff] [review]
Part 6: Get IMSI
Comment 29 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-30 00:30:44 PDT
Created attachment 610823 [details] [diff] [review]
Part 7: Get AD and retrieve MCC, MNC from IMSI
Comment 30 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-30 00:31:30 PDT
Created attachment 610825 [details] [diff] [review]
Part 8: Get USST
Comment 31 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-30 00:37:52 PDT
So far provide patches to get MSISDN, IMSI, AD, MCC, MNC and USST.

Thanks for Vicamo for providing suggestions and some patches.

I haven't provided patch for SPN, PNN and SPDI. Because I cannot get those values from my SIM card.(All 0xff)
Comment 32 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-03-30 02:18:30 PDT
Depends on Bug 740300, 
Since getICCStatus won't be called.
Comment 33 Vicamo Yang [:vicamo][:vyang] 2012-04-10 21:34:20 PDT
Created attachment 613869 [details] [diff] [review]
Part 2: Add Buf seekIncoming/seekOutgoing

refactor to support complete seek in both incoming & outgoing
Comment 34 Vicamo Yang [:vicamo][:vyang] 2012-04-10 21:36:18 PDT
Created attachment 613870 [details] [diff] [review]
Part 4: processICCResponse

s/skip/seekIncoming/
Comment 35 Vicamo Yang [:vicamo][:vyang] 2012-04-10 21:37:00 PDT
Created attachment 613871 [details] [diff] [review]
Part 5: Get MSISDN

s/skip/seekIncoming/
Comment 36 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-12 02:20:02 PDT
Created attachment 614305 [details] [diff] [review]
Part 1: Add/Sort constants

Rebase to current m-c
Comment 37 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-12 02:21:59 PDT
Created attachment 614306 [details] [diff] [review]
Part 2: Add Buf.seekIncoming/seekOutgoing

Rebase to current m-c
Thanks for Vicamo for providing this util function.
Comment 38 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-12 02:23:00 PDT
Created attachment 614307 [details] [diff] [review]
Part 3: Add GsmPDUHelper.read/writeHexOctetArray

Thanks for Vicamo providing this.
Comment 39 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-12 02:24:12 PDT
Created attachment 614308 [details] [diff] [review]
Part 4: processICCResponse

Rebase to latest m-c
Comment 40 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-12 02:25:06 PDT
Created attachment 614309 [details] [diff] [review]
Part 5: Get MSISDN

Rebase to latest m-c
Comment 41 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-12 02:26:26 PDT
Created attachment 614310 [details] [diff] [review]
Part 6: Get IMSI

Rebase to latest m-c
Comment 42 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-12 02:29:00 PDT
Created attachment 614311 [details] [diff] [review]
Part 7: Get AD and retrieve MCC, MNC from IMSI

Rebase to latest m-c
Also I found in Bug 729173 has separated voice and data from radioState,
so I did the same for icc.
Comment 43 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-12 02:30:45 PDT
Created attachment 614313 [details] [diff] [review]
Part 8: Get USST

Rebase to latest m-c
Comment 44 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-12 02:31:56 PDT
Patches uploaded finish!
Thanks for Vicamo for rebasing my patches.
Comment 45 Philipp von Weitershausen [:philikon] 2012-04-13 01:28:29 PDT
Comment on attachment 614305 [details] [diff] [review]
Part 1: Add/Sort constants

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

::: dom/system/gonk/ril_consts.js
@@ +427,5 @@
>  const EF_TYPE_CYCLIC = 3;
>  
>  // For retriveing MSISDN
>  const FOOTER_SIZE_BYTES = 14;
> +const MAX_NUMBER_SIZE_BYTES = 10;

Both of those constants should be prefixed so that it's clear what they apply to. I don't even know -- do they apply to all ICC I/O? Or just the MSISDN? Please clarify by updating their name. Thanks!

r=me with that.
Comment 46 Philipp von Weitershausen [:philikon] 2012-04-17 19:39:16 PDT
Comment on attachment 614306 [details] [diff] [review]
Part 2: Add Buf.seekIncoming/seekOutgoing

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

r- for creating dead code.

::: dom/system/gonk/ril_worker.js
@@ +77,5 @@
>  
> +const SEEK_SET = 0;
> +const SEEK_CUR = 1;
> +const SEEK_END = 2;
> +

You only use SEEK_CUR so far, so let's only implement that for now. Simplifies the code a lot.

@@ +354,5 @@
> +   *
> +   * @param offset
> +   * @param whence SEEK_SET, SEEK_CUR or SEEK_END
> +   */
> +  seekOutgoing: function seekOutgoing(offset, whence) {

Where do we need this? I don't see it used in your patches, not here and not in bug 738132 either. I don't like adding dead code, especially if it adds more complexity to existing code (the checked/unchecked business). Please remove.
Comment 47 Philipp von Weitershausen [:philikon] 2012-04-17 20:01:10 PDT
Comment on attachment 614308 [details] [diff] [review]
Part 4: processICCResponse

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

This definitely goes in the right direction, I like it! I think the code could be a bit clearer here and there. Please see below for my detailed review.

::: dom/system/gonk/ril_worker.js
@@ +876,2 @@
>     */
> +  fetchSimRecords: function fetchSimRecords() {

Better name: fetchICCRecords().

@@ +1332,5 @@
>                           cardState: this.cardState});
>    },
>  
>    /**
> +   * Process ICC GET_RESPONSE command

This is imprecise (and missing a period). I suggest something like:

  /**
   * Process a ICC_COMMAND_GET_RESPONSE type response for REQUEST_SIM_IO.
   */

@@ +1337,2 @@
>     */
> +  _ICCRspGetResponse: function _ICCRspGetResponse(options) {

I would like to keep the convention of prefixing these with _process, so how about _processICCIOGetResponse().

@@ +1341,5 @@
> +
> +    // The format is from TS 51.011, clause 9.2.1
> +
> +    // Skip RFU, data[0] data[1]
> +    Buf.seekIncoming((2 << 2), SEEK_CUR);

Eeek, magic numbers! You want to skip two hex octets, so I suggest you define the size of a hex octet at the top of ril_worker.js:

  const PDU_HEX_OCTET_SIZE = 4;

and then use that constant:

  Buf.seekIncoming(2 * PDU_HEX_OCTET_SIZE);

That way anybody can understand where these numbers are coming from.

@@ +1345,5 @@
> +    Buf.seekIncoming((2 << 2), SEEK_CUR);
> +
> +    // File size, data[2], data[3]
> +    let fileSize = GsmPDUHelper.readHexOctet() << 8
> +         | GsmPDUHelper.readHexOctet();

In consistency with Buf.readUint* I suggested placing the | on the first line and aligning `GsmPDUHelper`.

@@ +1349,5 @@
> +         | GsmPDUHelper.readHexOctet();
> +
> +    // 2 bytes File id. data[4], data[5]
> +    let fileId = GsmPDUHelper.readHexOctet() << 8
> +         | GsmPDUHelper.readHexOctet();

Ditto.

@@ +1351,5 @@
> +    // 2 bytes File id. data[4], data[5]
> +    let fileId = GsmPDUHelper.readHexOctet() << 8
> +         | GsmPDUHelper.readHexOctet();
> +    if (fileId !== options.fileid) {
> +      debug("Invalid file id!!!");

Missing an `if (DEBUG)`. This message is also not very helpful for debugging and three exclamation marks are really not necessary. Please print what you got and what you expected, e.g.:

  if (DEBUG) {
    debug("Expected file ID " + options.fileid + " but read " + fileId);
  }

Also, there's some casing inconsistency here: fileId vs options.fileid. Please fix.

@@ +1357,5 @@
> +    }
> +
> +    // Type of file, data[6]
> +    if (GsmPDUHelper.readHexOctet() !== TYPE_EF) {
> +      debug("Invalid response type!!");

Missing `if DEBUG`, unhelpful error message (see above).

@@ +1366,5 @@
> +    //      3 bytes Access conditions, data[8] data[9] data[10]
> +    //      1 byte File status, data[11]
> +    //      1 byte Length of the following data, data[12]
> +    Buf.seekIncoming(((RESPONSE_DATA_STRUCTURE - RESPONSE_DATA_FILE_TYPE - 1) << 2),
> +                     SEEK_CUR);

Magic numbers (see above)

@@ +1370,5 @@
> +                     SEEK_CUR);
> +
> +    // Read Structure of EF, data[13]
> +    if (GsmPDUHelper.readHexOctet() !== options.type) {
> +      debug("Invalid EF type!!");

Missing `if DEBUG`, unhelpful error message (see above).

@@ +1390,5 @@
>            p3:      recordSize,
>            data:    null,
>            pin2:    null,
> +          type:    options.type,
> +          cb:      options.cb,

We can spare the few extra bytes and call it `callback`

@@ +1407,5 @@
> +          type:    options.type,
> +          cb:      options.cb,
> +        };
> +        break;
> +    }

So in this whole `switch` statement, we're copying over a lot of values from `options`. Why not just simply modify `options` in place and pass that to `this.iccIO()`?

@@ +1417,5 @@
> +
> +  /**
> +   * Process ICC READ_RECORD command
> +   */
> +  _ICCRspReadRecord: function _ICCRspReadRecord(options) {

Same critique as above regarding the comment and method name. I suggest _processICCIOReadRecord().

@@ +1419,5 @@
> +   * Process ICC READ_RECORD command
> +   */
> +  _ICCRspReadRecord: function _ICCRspReadRecord(options) {
> +    if (options.cb) {
> +      options.cb(this);

Why are you passing `this`? The `RIL` object is available from the global scope, so the callback can just access it under that name. And if you wanted the callback to have a proper `this`, you should do

  options.callback.call(this);

which is what I would prefer.

@@ +1426,5 @@
> +
> +  /**
> +   * Process ICC READ_BINARY command
> +   */
> +  _ICCRspReadBinary: function _ICCRspReadBinary(options) {

Same critique as above regarding the comment and method name. I suggest _processICCIOReadBinary().

@@ +1428,5 @@
> +   * Process ICC READ_BINARY command
> +   */
> +  _ICCRspReadBinary: function _ICCRspReadBinary(options) {
> +    if (options.cb) {
> +      options.cb(this);

options.callback.call(this) (see above)

@@ +1435,5 @@
> +
> +  /**
> +   * Process ICC I/O response.
> +   */
> +  _processICCResponse: function _processICCResponse(options) {

In consistency with my other naming suggestions, I suggest calling this _processICCIO().
Comment 48 Philipp von Weitershausen [:philikon] 2012-04-17 20:11:42 PDT
Comment on attachment 614309 [details] [diff] [review]
Part 5: Get MSISDN

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

r=me with the points below addressed.

::: dom/system/gonk/ril_worker.js
@@ +885,5 @@
> +    this.sendDOMMessage({
> +        type:   "siminfo",
> +        msisdn: this.MSISDN,
> +    });
> +  },

Please use the term "ICC" instead of "SIM" whereever possible. To be consistent with existing method and message naming, I suggest naming the method _handleICCInfoChange and the message "iccinfochange".

Also, since we're adding IMSI, AD, MCC, MNC, etc., why don't we have a sub-object on `RIL` that keeps those attributes:

  let RIL = {
    ...

    iccInfo: {},

    ...
 }

and then we can simply do:

  _handleICCInfoChange: function _handleICCInfoChange() {
    this.iccInfo.type = "iccinfochange";
    this.sendDOMMessage(this.iccInfo);
  }

@@ +891,5 @@
> +  /**
> +   * Read the MSISDN from the ICC.
> +   */
> +  getMSISDN: function getMSISDN() {
> +    function callback(ril) {

Please refer to my review of part 4. You don't need to pass in the RIL object explicitly. You can either access it as a global object or, if you invoke the callback correctly as outlined in the review, continue to use `this`.

@@ +895,5 @@
> +    function callback(ril) {
> +      // Each octet is encoded into two chars.
> +      let recordLength = Buf.readUint32() / 2;
> +      // Skip prefixed alpha identifier
> +      Buf.seekIncoming(((recordLength - FOOTER_SIZE_BYTES) << 2), SEEK_CUR);

Magic numbers (see review of part 4).
Comment 49 Philipp von Weitershausen [:philikon] 2012-04-17 20:15:02 PDT
Comment on attachment 614311 [details] [diff] [review]
Part 7: Get AD and retrieve MCC, MNC from IMSI

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

r=me with points addressed

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +294,5 @@
>        case "siminfo":
> +        this.radioState.icc.msisdn = message.msisdn;
> +        this.radioState.icc.imsi = message.imsi;
> +        this.radioState.icc.mcc = message.mcc;
> +        this.radioState.icc.mnc = message.mnc;

Just take the whole object?

::: dom/system/gonk/ril_worker.js
@@ +662,5 @@
> +  /**
> +   * ICC records.
> +   * AD = Administrative Data
> +   */
> +  AD: null,

See my comment on part 5 about collapsing all these into a `RIL.iccInfo` object.

@@ +900,5 @@
> +          imsi:   this.IMSI,
> +          mcc:    this.MCC,
> +          mnc:    this.MNC,
> +      });
> +    }

Ditto.

@@ +947,5 @@
>    /**
> +   * Read the AD from the ICC.
> +   */
> +  getAD: function getAD() {
> +    function callback(ril) {

Please refer to my review of part 4. You don't need to pass in the RIL object explicitly.
Comment 50 Vicamo Yang [:vicamo][:vyang] 2012-04-17 20:18:00 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #46)
> Comment on attachment 614306 [details] [diff] [review]
> Part 2: Add Buf.seekIncoming/seekOutgoing
> 
> Review of attachment 614306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for creating dead code.
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +77,5 @@
> >  
> > +const SEEK_SET = 0;
> > +const SEEK_CUR = 1;
> > +const SEEK_END = 2;
> > +
> 
> You only use SEEK_CUR so far, so let's only implement that for now.
> Simplifies the code a lot.

I don't think so. Removing SEEK_SET/END saves only four lines in Buf.seek(). The really complexity comes from the ring-buffer design. You'll still have to work around the non-straight-forward index at the end. Not a single line in Buf.seekIncoming() can be saved.

> @@ +354,5 @@
> > +   *
> > +   * @param offset
> > +   * @param whence SEEK_SET, SEEK_CUR or SEEK_END
> > +   */
> > +  seekOutgoing: function seekOutgoing(offset, whence) {
> 
> Where do we need this? I don't see it used in your patches, not here and not
> in bug 738132 either. I don't like adding dead code, especially if it adds
> more complexity to existing code (the checked/unchecked business). Please
> remove.

The code was supposed to be referenced in bug 736706 USIM download. I can remove these functions and add them back whenever necessary.
Comment 51 Philipp von Weitershausen [:philikon] 2012-04-17 20:18:29 PDT
Comment on attachment 614313 [details] [diff] [review]
Part 8: Get USST

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

Please apply the same points that I have about part 7 to this part as well. r=me with that.

For the final review (and landing), I think it would be best to collapse all patches into one patch since none of the individual patches represent a working state.
Comment 52 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-17 20:26:44 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #48)
Hi Philikon, 
Thanks for your review.
I got some questions below.
> @@ +891,5 @@
> > +  /**
> > +   * Read the MSISDN from the ICC.
> > +   */
> > +  getMSISDN: function getMSISDN() {
> > +    function callback(ril) {
> 
> Please refer to my review of part 4. You don't need to pass in the RIL
> object explicitly. You can either access it as a global object or, if you
> invoke the callback correctly as outlined in the review, continue to use
> `this`.

Since the 'callback' is inner function, and in ECMA5 strict mode, inside inner function, 'this' would be undefined(unless we call that callback with Function.call/apply). So to prevent from confusing, I pass the 'ril' explicitly, instead of using callback.call in the first case.

What's your suggestion for this? Should I still move to use 'call' and use 'this'?

Thanks
Comment 53 Philipp von Weitershausen [:philikon] 2012-04-17 20:41:53 PDT
(In reply to Yoshi Huang[:yoshi] from comment #52)

> > Please refer to my review of part 4. You don't need to pass in the RIL
> > object explicitly. You can either access it as a global object or, if you
> > invoke the callback correctly as outlined in the review, continue to use
> > `this`.
> 
> Since the 'callback' is inner function, and in ECMA5 strict mode, inside
> inner function, 'this' would be undefined

Not only in ES5 strict mode. This is an inherent "quality" of JavaScript.

> (unless we call that callback with Function.call/apply).

Or you pre-bind the function with Function.prototype.bind().

> So to prevent from confusing, I pass the 'ril'
> explicitly, instead of using callback.call in the first case.

I don't think it's that much more confusing, but ok. If you prefer to use an explicit value, then let's use the global `RIL` identifier.
Comment 54 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-17 20:53:05 PDT
(In reply to Philipp von Weitershausen [:philikon] from comment #53)
> I don't think it's that much more confusing, but ok. If you prefer to use an
> explicit value, then let's use the global `RIL` identifier.
Hi, philikon.

Thanks for you review. Since you don't think using 'this' in inner function isn't confused so I will revise it by your review comments to use 'call'.

Other review comments will also be addressed by your review as well. And I'll make it in one single patch and send you another review quest.

But for the Part 2 Add Buf.seekIncoming/seekOutgoing patch, should I file another bug and move the Part 2 patch in that bug?

thanks
Comment 55 Philipp von Weitershausen [:philikon] 2012-04-17 20:58:57 PDT
(In reply to Yoshi Huang[:yoshi] from comment #54)
> But for the Part 2 Add Buf.seekIncoming/seekOutgoing patch, should I file
> another bug and move the Part 2 patch in that bug?

Well, since we only need Buf.seekIncoming, and only a third of that even, I think it's fine to have it in the same big patch.
Comment 56 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-18 03:11:59 PDT
Created attachment 616066 [details] [diff] [review]
[WIP] Fetch SIM records. v2

Philikon's comments addressed.
And merge those patches into one.
I put it in [WIP] because per discussion from Bug 738712 Comment #22
GsmPDUHelper.readHexOctetArray() should be revised.

thanks
Comment 57 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-18 18:59:41 PDT
(In reply to Yoshi Huang[:yoshi] from comment #56)
> Created attachment 616066 [details] [diff] [review]
> [WIP] Fetch SIM records. v2
> 
> Philikon's comments addressed.
> And merge those patches into one.
> I put it in [WIP] because per discussion from Bug 738712 Comment #22
> GsmPDUHelper.readHexOctetArray() should be revised.
> 
> thanks

Sorry should be Bug 738132 Comment #22
Comment 58 Vicamo Yang [:vicamo][:vyang] 2012-04-18 22:20:42 PDT
Comment on attachment 616066 [details] [diff] [review]
[WIP] Fetch SIM records. v2

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

Thanks :)

::: dom/system/gonk/ril_worker.js
@@ +340,5 @@
> +  ensureOutgoingAvailable: function ensureOutgoingAvailable(index) {
> +    if (index >= this.OUTGOING_BUFFER_LENGTH) {
> +      this.growOutgoingBuffer(index);
> +    } else if (index < PARCEL_SIZE_SIZE) {
> +      throw new Error("Trying to write data before the parcel begin!");

these changes are not required if no seekOutgoing API for now

@@ +345,4 @@
>      }
> +  },
> +
> +  writeUint8Unchecked: function writeUint8Unchecked(value) {

Ditto

@@ +353,5 @@
> +  writeUint8: function writeUint8(value) {
> +    this.ensureOutgoingAvailable(this.outgoingIndex);
> +    this.writeUint8Unchecked(value);
> +  },
> +

Ditto

@@ +402,5 @@
>      this.outgoingIndex = 0;
> +    this.writeUint8Unchecked((value >> 24) & 0xff);
> +    this.writeUint8Unchecked((value >> 16) & 0xff);
> +    this.writeUint8Unchecked((value >> 8) & 0xff);
> +    this.writeUint8Unchecked(value & 0xff);

Ditto
Comment 59 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-18 23:28:35 PDT
Created attachment 616471 [details] [diff] [review]
Fetch Sim Records. v2

Thanks for philikon's review, 
now philikon's comments and suggestions addressed.

thanks
Comment 60 Philipp von Weitershausen [:philikon] 2012-04-22 17:44:32 PDT
(In reply to Vicamo Yang [:vicamo] from comment #50) 
> > > +const SEEK_SET = 0;
> > > +const SEEK_CUR = 1;
> > > +const SEEK_END = 2;
> > > +
> > 
> > You only use SEEK_CUR so far, so let's only implement that for now.
> > Simplifies the code a lot.
> 
> I don't think so. Removing SEEK_SET/END saves only four lines in Buf.seek().
> The really complexity comes from the ring-buffer design. You'll still have
> to work around the non-straight-forward index at the end. Not a single line
> in Buf.seekIncoming() can be saved.

You are correct. I never said anything about saving lines in Buf.seekIncoming(). But all the rest is clutter and it diverts attention from the actual problem (code readability, etc.), so it should be removed IMHO.

> > Where do we need this? I don't see it used in your patches, not here and not
> > in bug 738132 either. I don't like adding dead code, especially if it adds
> > more complexity to existing code (the checked/unchecked business). Please
> > remove.
> 
> The code was supposed to be referenced in bug 736706 USIM download. I can
> remove these functions and add them back whenever necessary.

Ok. That would be better for understanding the context of this work. Thanks!
Comment 61 Philipp von Weitershausen [:philikon] 2012-04-22 17:46:09 PDT
Comment on attachment 616471 [details] [diff] [review]
Fetch Sim Records. v2

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

Nice work! r=me with the last remaining comments addressed (see below).

::: dom/system/gonk/ril_worker.js
@@ +74,5 @@
>  const UINT16_SIZE = 2;
>  const UINT32_SIZE = 4;
>  const PARCEL_SIZE_SIZE = UINT32_SIZE;
>  
> +const SEEK_CUR = 1;

I should have been clearer in comment 46: I think this constant and the `whence` parameter are superfluous since we have no other use case right now (and I'm not sure we ever will.) With just one possible parameter it's even more confusing than before, IMHO. Please remove it altogether. Thanks!

@@ +202,5 @@
>  
> +  seek: function seek(cur, end, offset, whence) {
> +    switch (whence) {
> +      case SEEK_CUR:
> +        return cur + offset;

This is a very complicated way of doing what's basically a simple addition. Please just inline this in `seekIncoming`.

@@ +217,5 @@
>  
> +  /**
> +   * Ensure position specified is readable.
> +   */
> +  ensureIncomingAvailable: function ensureIncomingAvailable(index) {

Extra credit if you document the `index` parameter in the comment. :)

@@ +235,5 @@
> +  seekIncoming: function seekIncoming(offset, whence) {
> +    // Translate to 0..currentParcelSize
> +    let cur = this.currentParcelSize - this.readAvailable;
> +
> +    let newIndex = this.seek(cur, this.currentParcelSize, offset, whence);

Get rid of `whence` and inline `seek`, please.
Comment 62 Yoshi Huang[:allstars.chh] (OOO ~ 9.30) 2012-04-22 20:30:16 PDT
Created attachment 617372 [details] [diff] [review]
Fetch Sim Records. v3

Address to philikon's comments, and rebase to latest m-c.

Thanks
Comment 63 Philipp von Weitershausen [:philikon] 2012-04-23 17:26:35 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a37f78bb19b3
Comment 64 Ed Morley [:emorley] 2012-04-24 04:34:46 PDT
https://hg.mozilla.org/mozilla-central/rev/a37f78bb19b3
Comment 65 Jason Smith [:jsmith] 2014-09-17 11:38:54 PDT
Mass-modify - removal of no longer relevant blocking flags.

Note You need to log in before you can comment on or make changes to this bug.