The default bug view has changed. See this FAQ.

B2G RIL: Fetch SIM record

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: allstars, Assigned: allstars)

Tracking

Trunk
mozilla14
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:-)

Details

Attachments

(1 attachment, 38 obsolete attachments)

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
Blocks: 729440
Blocks: 736706
bug 736706 depends on SST(SIM Service Table) that will be fetched in this issue.
Blocks: 715788
Created attachment 608245 [details] [diff] [review]
[WIP]fetch SIM records. v1

Get AD, SST and IMSI from simcard.
Created attachment 608251 [details] [diff] [review]
[WIP]fetch SIM records. v2

Revised by Vicamo's suggestions.
Attachment #608245 - Attachment is obsolete: true
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
Attachment #608251 - Attachment is obsolete: true
Created attachment 608265 [details] [diff] [review]
[WIP]fetch SIM records. v3
Attachment #608261 - Attachment is obsolete: true
Created attachment 608291 [details] [diff] [review]
[WIP]fetch SIM records. v4

Add send 'siminfo' event to RadioInterfaceLayer
Attachment #608265 - Attachment is obsolete: true
Created attachment 609263 [details] [diff] [review]
[WIP]fetch SIM records. v5

Revised based on latest patch of Bug 733990.
Attachment #608291 - Attachment is obsolete: true
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.
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.
Created attachment 610089 [details] [diff] [review]
Part 1: add/sort constants

1) tear down to smaller patches
2) s/SST/UST/
Attachment #609604 - Attachment is obsolete: true
Created attachment 610090 [details] [diff] [review]
Part 2: Add Buf.skip()
Created attachment 610091 [details] [diff] [review]
Part 3: Refactor _processMSISDNResponse
Created attachment 610092 [details] [diff] [review]
Part 4: Fetch IMSI
Created attachment 610093 [details] [diff] [review]
Part 5: Add read/writeHexOctetArray()
Created attachment 610094 [details] [diff] [review]
Part 6: Fetch Administrative Data
Created attachment 610095 [details] [diff] [review]
Part 7: Fetch UST(USIM Service Table)
Created attachment 610096 [details] [diff] [review]
Part 8: Fetch ICC Identification
(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
Created attachment 610424 [details] [diff] [review]
Part 5: Add read/writeHexOctetArray() : V2

comment #19, should be writeHexOctetArray.
Attachment #610093 - Attachment is obsolete: true
Created attachment 610425 [details] [diff] [review]
Part 6: Fetch Administrative Data : V2

comment #20, illegal backslash character
Attachment #610094 - Attachment is obsolete: true
Created attachment 610817 [details] [diff] [review]
Part 1: Add/Sort constants
Attachment #610089 - Attachment is obsolete: true
Created attachment 610818 [details] [diff] [review]
Part 2: Add Buf.skip
Attachment #610090 - Attachment is obsolete: true
Created attachment 610819 [details] [diff] [review]
Part 3: Add GsmPDUHelper.read/writeHexOctetArray
Attachment #610424 - Attachment is obsolete: true
Created attachment 610820 [details] [diff] [review]
Part 4: processICCResponse
Attachment #610091 - Attachment is obsolete: true
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
Created attachment 610822 [details] [diff] [review]
Part 6: Get IMSI
Attachment #610092 - Attachment is obsolete: true
Created attachment 610823 [details] [diff] [review]
Part 7: Get AD and retrieve MCC, MNC from IMSI
Attachment #610425 - Attachment is obsolete: true
Created attachment 610825 [details] [diff] [review]
Part 8: Get USST
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: 740300
Depends on Bug 740300, 
Since getICCStatus won't be called.
Created attachment 613869 [details] [diff] [review]
Part 2: Add Buf seekIncoming/seekOutgoing

refactor to support complete seek in both incoming & outgoing
Attachment #610818 - Attachment is obsolete: true
Created attachment 613870 [details] [diff] [review]
Part 4: processICCResponse

s/skip/seekIncoming/
Attachment #610820 - Attachment is obsolete: true
Created attachment 613871 [details] [diff] [review]
Part 5: Get MSISDN

s/skip/seekIncoming/
Attachment #610821 - Attachment is obsolete: true
Blocks: 738132
Created attachment 614305 [details] [diff] [review]
Part 1: Add/Sort constants

Rebase to current m-c
Attachment #610817 - Attachment is obsolete: true
Attachment #614305 - Flags: review?(philipp)
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.
Attachment #613869 - Attachment is obsolete: true
Attachment #614306 - Flags: review?(philipp)
Created attachment 614307 [details] [diff] [review]
Part 3: Add GsmPDUHelper.read/writeHexOctetArray

Thanks for Vicamo providing this.
Attachment #610819 - Attachment is obsolete: true
Attachment #614307 - Flags: review?(philipp)
Created attachment 614308 [details] [diff] [review]
Part 4: processICCResponse

Rebase to latest m-c
Attachment #613870 - Attachment is obsolete: true
Attachment #614308 - Flags: review?(philipp)
Created attachment 614309 [details] [diff] [review]
Part 5: Get MSISDN

Rebase to latest m-c
Attachment #613871 - Attachment is obsolete: true
Attachment #614309 - Flags: review?(philipp)
Created attachment 614310 [details] [diff] [review]
Part 6: Get IMSI

Rebase to latest m-c
Attachment #610822 - Attachment is obsolete: true
Attachment #614310 - Flags: review?(philipp)
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.
Attachment #610823 - Attachment is obsolete: true
Attachment #614311 - Flags: review?(philipp)
Created attachment 614313 [details] [diff] [review]
Part 8: Get USST

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.
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
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
Created attachment 616471 [details] [diff] [review]
Fetch Sim Records. v2

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+
Created attachment 617372 [details] [diff] [review]
Fetch Sim Records. v3

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/integration/mozilla-inbound/rev/a37f78bb19b3
https://hg.mozilla.org/mozilla-central/rev/a37f78bb19b3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14

Updated

3 years ago
blocking-b2g: --- → koi?
feature-b2g: --- → -
Mass-modify - removal of no longer relevant blocking flags.

Updated

3 years ago
blocking-b2g: koi? → ---
You need to log in before you can comment on or make changes to this bug.