736941 - B2G RIL: Fetch SIM record

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

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]]

bug 736706 depends on SST(SIM Service Table) that will be fetched in this issue.

Comment 2

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

Attachment: [WIP]fetch SIM records. v1

Get AD, SST and IMSI from simcard.

Comment 3

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

Attachment: [WIP]fetch SIM records. v2

Revised by Vicamo's suggestions.

Comment 4

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

Attachment: [WIP]fetch SIM records. v3

rebase to latest m-c.
note that need to apply patch of Bug 733990 first

Comment 5

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

Attachment: [WIP]fetch SIM records. v3

Comment 6

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

Attachment: [WIP]fetch SIM records. v4

Add send 'siminfo' event to RadioInterfaceLayer

Comment 7

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

Attachment: [WIP]fetch SIM records. v5

Revised based on latest patch of Bug 733990.

Comment 8

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

Attachment: [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]]

Depends on bug 736697 for GsmPDUHelper.readAddress(). The MSISDN number shares the same format with SMS sender address.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 1: add/sort constants

1) tear down to smaller patches
2) s/SST/UST/

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: Add Buf.skip()

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 3: Refactor _processMSISDNResponse

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: Fetch IMSI

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5: Add read/writeHexOctetArray()

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 6: Fetch Administrative Data

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 7: Fetch UST(USIM Service Table)

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 8: Fetch ICC Identification

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

(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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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]]

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]]

Attachment: Part 5: Add read/writeHexOctetArray() : V2

comment #19, should be writeHexOctetArray.

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 6: Fetch Administrative Data : V2

comment #20, illegal backslash character

Comment 23

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

Attachment: Part 1: Add/Sort constants

Comment 24

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

Attachment: Part 2: Add Buf.skip

Comment 25

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

Attachment: Part 3: Add GsmPDUHelper.read/writeHexOctetArray

Comment 26

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

Attachment: Part 4: processICCResponse

Comment 27

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

Attachment: 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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 6: Get IMSI

Comment 29

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

Attachment: Part 7: Get AD and retrieve MCC, MNC from IMSI

Comment 30

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

Attachment: Part 8: Get USST

Comment 31

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

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Depends on Bug 740300, 
Since getICCStatus won't be called.

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 2: Add Buf seekIncoming/seekOutgoing

refactor to support complete seek in both incoming & outgoing

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 4: processICCResponse

s/skip/seekIncoming/

Comment 35

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Part 5: Get MSISDN

s/skip/seekIncoming/

Comment 36

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

Attachment: Part 1: Add/Sort constants

Rebase to current m-c

Comment 37

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

Attachment: Part 2: Add Buf.seekIncoming/seekOutgoing

Rebase to current m-c
Thanks for Vicamo for providing this util function.

Comment 38

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

Attachment: Part 3: Add GsmPDUHelper.read/writeHexOctetArray

Thanks for Vicamo providing this.

Comment 39

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

Attachment: Part 4: processICCResponse

Rebase to latest m-c

Comment 40

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

Attachment: Part 5: Get MSISDN

Rebase to latest m-c

Comment 41

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

Attachment: Part 6: Get IMSI

Rebase to latest m-c

Comment 42

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

Attachment: 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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 8: Get USST

Rebase to latest m-c

Comment 44

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

Patches uploaded finish!
Thanks for Vicamo for rebasing my patches.

Comment 45

[Philipp von Weitershausen [:philikon]]

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]]

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]]

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]]

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]]

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]]

(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]]

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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]]

(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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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]]

(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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: [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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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]]

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Fetch Sim Records. v2

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

thanks

Comment 60

[Philipp von Weitershausen [:philikon]]

(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]]

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 Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Fetch Sim Records. v3

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

Thanks

Comment 63

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a37f78bb19b3

Comment 64

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a37f78bb19b3

Comment 65

[Jason Smith [:jsmith]]

Mass-modify - removal of no longer relevant blocking flags.