Closed Bug 736941 Opened 8 years ago Closed 8 years ago

B2G RIL: Fetch SIM record

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14
feature-b2g -

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(1 file, 38 obsolete files)

23.69 KB, patch
philikon
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → yhuang
bug 736706 depends on SST(SIM Service Table) that will be fetched in this issue.
Attached patch [WIP]fetch SIM records. v1 (obsolete) — Splinter Review
Get AD, SST and IMSI from simcard.
Attached patch [WIP]fetch SIM records. v2 (obsolete) — Splinter Review
Revised by Vicamo's suggestions.
Attachment #608245 - Attachment is obsolete: true
Attached patch [WIP]fetch SIM records. v3 (obsolete) — Splinter Review
rebase to latest m-c.
note that need to apply patch of Bug 733990 first
Attachment #608251 - Attachment is obsolete: true
Attached patch [WIP]fetch SIM records. v3 (obsolete) — Splinter Review
Attachment #608261 - Attachment is obsolete: true
Attached patch [WIP]fetch SIM records. v4 (obsolete) — Splinter Review
Add send 'siminfo' event to RadioInterfaceLayer
Attachment #608265 - Attachment is obsolete: true
Attached patch [WIP]fetch SIM records. v5 (obsolete) — Splinter Review
Revised based on latest patch of Bug 733990.
Attachment #608291 - Attachment is obsolete: true
Attached patch [WIP]fetch SIM records. v6 (obsolete) — Splinter Review
Update constants.
I'll make a formal review request when Bug 733990 is reviewed.
thanks.
Attachment #609263 - Attachment is obsolete: true
Depends on: 736697
Depends on bug 736697 for GsmPDUHelper.readAddress(). The MSISDN number shares the same format with SMS sender address.
Attached patch Part 1: add/sort constants (obsolete) — Splinter Review
1) tear down to smaller patches
2) s/SST/UST/
Attachment #609604 - Attachment is obsolete: true
Attached patch Part 4: Fetch IMSI (obsolete) — Splinter Review
(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 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 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 #19, should be writeHexOctetArray.
Attachment #610093 - Attachment is obsolete: true
comment #20, illegal backslash character
Attachment #610094 - Attachment is obsolete: true
Attached patch Part 1: Add/Sort constants (obsolete) — Splinter Review
Attachment #610089 - Attachment is obsolete: true
Attached patch Part 2: Add Buf.skip (obsolete) — Splinter Review
Attachment #610090 - Attachment is obsolete: true
Attached patch Part 4: processICCResponse (obsolete) — Splinter Review
Attachment #610091 - Attachment is obsolete: true
Attached patch Part 5: Get MSISDN (obsolete) — Splinter Review
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
Attached patch Part 6: Get IMSI (obsolete) — Splinter Review
Attachment #610092 - Attachment is obsolete: true
Attached patch Part 8: Get USST (obsolete) — Splinter Review
Attachment #610095 - Attachment is obsolete: true
Attachment #610096 - Attachment is obsolete: true
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)
Depends on Bug 740300, 
Since getICCStatus won't be called.
refactor to support complete seek in both incoming & outgoing
Attachment #610818 - Attachment is obsolete: true
Attached patch Part 4: processICCResponse (obsolete) — Splinter Review
s/skip/seekIncoming/
Attachment #610820 - Attachment is obsolete: true
Attached patch Part 5: Get MSISDN (obsolete) — Splinter Review
s/skip/seekIncoming/
Attachment #610821 - Attachment is obsolete: true
Attached patch Part 1: Add/Sort constants (obsolete) — Splinter Review
Rebase to current m-c
Attachment #610817 - Attachment is obsolete: true
Attachment #614305 - Flags: review?(philipp)
Rebase to current m-c
Thanks for Vicamo for providing this util function.
Attachment #613869 - Attachment is obsolete: true
Attachment #614306 - Flags: review?(philipp)
Thanks for Vicamo providing this.
Attachment #610819 - Attachment is obsolete: true
Attachment #614307 - Flags: review?(philipp)
Attached patch Part 4: processICCResponse (obsolete) — Splinter Review
Rebase to latest m-c
Attachment #613870 - Attachment is obsolete: true
Attachment #614308 - Flags: review?(philipp)
Attached patch Part 5: Get MSISDN (obsolete) — Splinter Review
Rebase to latest m-c
Attachment #613871 - Attachment is obsolete: true
Attachment #614309 - Flags: review?(philipp)
Attached patch Part 6: Get IMSI (obsolete) — Splinter Review
Rebase to latest m-c
Attachment #610822 - Attachment is obsolete: true
Attachment #614310 - Flags: review?(philipp)
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.
Attachment #610823 - Attachment is obsolete: true
Attachment #614311 - Flags: review?(philipp)
Attached patch Part 8: Get USST (obsolete) — Splinter Review
Rebase to latest m-c
Attachment #610825 - Attachment is obsolete: true
Attachment #614313 - Flags: review?(philipp)
Patches uploaded finish!
Thanks for Vicamo for rebasing my patches.
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.
Attachment #614305 - Flags: review?(philipp) → review+
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.
Attachment #614306 - Flags: review?(philipp) → review-
Attachment #614307 - Flags: review?(philipp) → review+
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().
Attachment #614308 - Flags: review?(philipp) → feedback+
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).
Attachment #614309 - Flags: review?(philipp) → review+
Attachment #614310 - Flags: review?(philipp) → review+
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.
Attachment #614311 - Flags: review?(philipp) → review+
(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 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.
Attachment #614313 - Flags: review?(philipp) → review+
(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
(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.
(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
(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.
Attached patch [WIP] Fetch SIM records. v2 (obsolete) — Splinter Review
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
Attachment #614305 - Attachment is obsolete: true
Attachment #614306 - Attachment is obsolete: true
Attachment #614307 - Attachment is obsolete: true
Attachment #614308 - Attachment is obsolete: true
Attachment #614309 - Attachment is obsolete: true
Attachment #614310 - Attachment is obsolete: true
Attachment #614311 - Attachment is obsolete: true
Attachment #614313 - Attachment is obsolete: true
(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 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
Attached patch Fetch Sim Records. v2 (obsolete) — Splinter Review
Thanks for philikon's review, 
now philikon's comments and suggestions addressed.

thanks
Attachment #616066 - Attachment is obsolete: true
Attachment #616471 - Flags: review?(philipp)
(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 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.
Attachment #616471 - Flags: review?(philipp) → review+
Address to philikon's comments, and rebase to latest m-c.

Thanks
Attachment #616471 - Attachment is obsolete: true
Attachment #617372 - Flags: review?(philipp)
Attachment #617372 - Flags: review?(philipp) → review+
https://hg.mozilla.org/mozilla-central/rev/a37f78bb19b3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
blocking-b2g: --- → koi?
feature-b2g: --- → -
Mass-modify - removal of no longer relevant blocking flags.
blocking-b2g: koi? → ---
You need to log in before you can comment on or make changes to this bug.