935843 - B2G RIL: Parse EF_IMG from SIM

Status: VERIFIED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

EF_IMG (TS 131.102, clause 4.6.1.1) is used to store images/icons on the SIM card.
We need to check if this EF contains any image/icon and parse it (i.e. for STK)

Comment 1

[Georgia Wang]

Attachment: Part1: add readIMG for parsing EF_IMG

Comment 2

[Georgia Wang]

Attachment: Part2: xpcshell for readIMG

Comment 3

[Georgia Wang]

Attachment: Part3: Add readIIDF for parsing EF_IIDF image data

Comment 4

[Georgia Wang]

Attachment: Part4: xpcshell for readIIDF

Comment 5

[Georgia Wang]

Attachment: Part5: Add readImageList for load an image list

Comment 6

[Georgia Wang]

Attachment: Part6: xpcshell for readImageList

Comment 7

[Georgia Wang]

TRY: https://tbpl.mozilla.org/?tree=Try&rev=b547c3a94c0a

Comment 8

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

Comment on attachment 8339201 [details] [diff] [review]
Part1: add readIMG for parsing EF_IMG

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

::: dom/system/gonk/ril_worker.js
@@ +11662,5 @@
> +   * @param onerror      Callback to be called when error.
> +   */
> +  readIMG: function readIMG(recordNumber, onsuccess, onerror) {
> +    function callback(options) {
> +      let strLen = Buf.readInt32();

let octetLen = strLen / 2;

@@ +11663,5 @@
> +   */
> +  readIMG: function readIMG(recordNumber, onsuccess, onerror) {
> +    function callback(options) {
> +      let strLen = Buf.readInt32();
> +      if (strLen / 2 != ICC_IMG_DESCRIPTION_SIZE_BYTES){

I don't know what's this.

@@ +11668,5 @@
> +        let error = onerror || debug;
> +        error("Record Size not match, strLen =", strLen);
> +        return;
> +      }
> +

The first octet should be number of actual image instances.

@@ +11669,5 @@
> +        error("Record Size not match, strLen =", strLen);
> +        return;
> +      }
> +
> +      let imageDescription = {width:  GsmPDUHelper.readHexOctet(),

There should be a for-loop to read this.

also try to use 
let foo = {
  ...
};

Comment 9

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

Comment on attachment 8339202 [details] [diff] [review]
Part2: xpcshell for readIMG

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

Cancelling r? as I think the raw data here doesn't quite match the specification.

Comment 10

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

Comment on attachment 8339203 [details] [diff] [review]
Part3: Add readIIDF for parsing EF_IIDF image data

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

::: dom/system/gonk/ril_worker.js
@@ +11700,5 @@
> +   *
> +   * @param fileId       The file of the image instance data.
> +   * @param offset       The offset of the image from EF begining.
> +   * @param dataLen      The data length of needed image.
> +   * @param dsc          Data coding scheme of the image.

What's dsc?
Or it should be dcs by your comment?

Try codingScheme

@@ +11708,5 @@
> +    function callback(options) {
> +      let strLen = Buf.readInt32();
> +      let endLen = offset + dataLen;
> +      if ((strLen / 2) < endLen) {
> +        error("Record Size not enough.");

Where does error come from?

Please add a test case with invalid data to check your error handling is correct.

@@ +11717,5 @@
> +      Buf.seekIncoming(offset * Buf.PDU_HEX_OCTET_SIZE);
> +
> +      let image = {};
> +      image.width = GsmPDUHelper.readHexOctet();
> +      image.height = GsmPDUHelper.readHexOctet();

let image = {
 width: ...
 height: ...
};

@@ +11725,5 @@
> +          image.body = GsmPDUHelper.readHexOctetArray(dataLen -
> +                                                      ICC_IMG_BASIC_HEADER_SIZE);
> +          break;
> +        case ICC_IMG_SCHEME_COLOUR:
> +        case ICC_IMG_SCHEME_TRANSPARENCY:

nit,

..: // Fall through

@@ +11726,5 @@
> +                                                      ICC_IMG_BASIC_HEADER_SIZE);
> +          break;
> +        case ICC_IMG_SCHEME_COLOUR:
> +        case ICC_IMG_SCHEME_TRANSPARENCY:
> +          let clut = {};

ditto
let clut = {
...
};

@@ +11728,5 @@
> +        case ICC_IMG_SCHEME_COLOUR:
> +        case ICC_IMG_SCHEME_TRANSPARENCY:
> +          let clut = {};
> +          clut.bits = GsmPDUHelper.readHexOctet();
> +          clut.entryNum = GsmPDUHelper.readHexOctet();

numOfEntries, or entryCount

@@ +11758,5 @@
> +    }
> +
> +    ICCIOHelper.loadTransparentEF({fileId: fileId,
> +                                   pathId: EF_PATH_MF_SIM + EF_PATH_DF_TELECOM +
> +                                           EF_PATH_DF_GRAPHICS,

If you update ICCFileHelper.getEFPatch, we don;t have to provide pathId here.

Comment 11

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

Comment on attachment 8339203 [details] [diff] [review]
Part3: Add readIIDF for parsing EF_IIDF image data

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

::: dom/system/gonk/ril_worker.js
@@ +11728,5 @@
> +        case ICC_IMG_SCHEME_COLOUR:
> +        case ICC_IMG_SCHEME_TRANSPARENCY:
> +          let clut = {};
> +          clut.bits = GsmPDUHelper.readHexOctet();
> +          clut.entryNum = GsmPDUHelper.readHexOctet();

From the specification
"The value 0 shall be interpreted as 256."

where did you handle this?

Comment 12

[Georgia Wang]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #10)
> Comment on attachment 8339203 [details] [diff] [review]
> Part3: Add readIIDF for parsing EF_IIDF image data
> 
> @@ +11708,5 @@
> > +    function callback(options) {
> > +      let strLen = Buf.readInt32();
> > +      let endLen = offset + dataLen;
> > +      if ((strLen / 2) < endLen) {
> > +        error("Record Size not enough.");
> 
> Where does error come from?
The icon data has length of "dataLen", start with "offset". 
So record size should be larger than offset + dataLen~  I'll add test for this. 

> Please add a test case with invalid data to check your error handling is
> correct.
> 

> @@ +11758,5 @@
> > +    }
> > +
> > +    ICCIOHelper.loadTransparentEF({fileId: fileId,
> > +                                   pathId: EF_PATH_MF_SIM + EF_PATH_DF_TELECOM +
> > +                                           EF_PATH_DF_GRAPHICS,
> 
> If you update ICCFileHelper.getEFPatch, we don;t have to provide pathId here.

The EF_iidf id is 4fXX, not fixed value. But there's other EF value start with '4F'.
So I just indicate pathId here.

Comment 13

[Georgia Wang]

Attachment: Part1: add readIMG for parsing EF_IMG.v2

Comment 14

[Georgia Wang]

Attachment: Part2: xpcshell for readIMG. v2

Comment 15

[Georgia Wang]

Attachment: Part3: Add readIIDF for parsing EF_IIDF image data. v2

Comment 16

[Georgia Wang]

Attachment: Part4: xpcshell for readIIDF. v2

Comment 17

[Georgia Wang]

Attachment: Part5: Add readImageList for load an image list. v2

Comment 18

[Georgia Wang]

Attachment: Part6: xpcshell for readImageList. v2

Comment 19

[Georgia Wang]

Attachment: Part1: add readIMG for parsing EF_IMG.v2

Comment 20

[Georgia Wang]

TRY： https://tbpl.mozilla.org/?tree=Try&rev=dd8e4ec7e721

Comment 21

[Georgia Wang]

Comment on attachment 8342882 [details] [diff] [review]
Part1: add readIMG for parsing EF_IMG.v2

Took Android as reference, add GET_RESPONSE_EF_IMG_SIZE_BYTES = 10.

http://androidxref.com/4.4_r1/xref/frameworks/opt/telephony/src/java/com/android/internal/telephony/uicc/IccFileHandler.java#52

Comment 22

[Georgia Wang]

Comment on attachment 8342808 [details] [diff] [review]
Part5: Add readImageList for load an image list. v2

readImageList can be used to read and parsing icons.
imageId are reported by STK commands such as setup menu.

Comment 23

[Ken Chang[:kenkai]]

Please this into backlog.

Comment 24

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

This is requirement from CAF for 2.1.
Set to 2.1? so that we can prioritize in 2.1

Comment 25

[Ken Chang[:kenkai]]

Hsinyi, you want to take this bug, right?

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 1 - readIMG and readIIDF

Keep working on decoding color table

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 2 - xpcshell - img_coding_scheme_basic

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 3 - xpcshell - data length not enough

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 4 - coding scheme color and color_transparency

Comment 30

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: WIP - part 5 - xpcshell - color and color_transparency

Comment 31

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF

This part does:
1) check if "IMG" is within ICC_Service
2) readIMG() parses EF_IMG, image descriptors
3) readIIDF() parses EF_IIDF, image instance data files

In this patch, I aim at decoding the sim files. I plan to transform the data into a compact format that gaia wants in bug 824145.

Comment 32

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF (v2)

This part does:
1) check if "IMG" is within ICC_Service
2) correct the EF_path
3) readIMG() parses EF_IMG, image descriptors
4) readIIDF() parses EF_IIDF, image instance data files

In this patch, I aim at decoding the sim files. I plan to transform the data into a compact format that gaia wants in bug 824145.

Comment 33

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2 - xpcshell test

Test case coverage:
1) test of parsing image with ICC_IMG_CODING_SCHEME_BASIC
2) test of parsing image without enough data
3) test of parsing image with ICC_IMG_CODING_SCHEME_COLOR
4) test of parsing image with ICC_IMG_CODING_SCHEME_COLOR_TRANSPARENCY

Comment 34

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8463865 [details] [diff] [review]
part 1 - readIMG and readIIDF (v2)

Please see comment 32 for details.

Edgar, may I have your review? Thank you!

Comment 35

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8463869 [details] [diff] [review]
part 2 - xpcshell test

Please see comment 33 for details!

Comment 36

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8463865 [details] [diff] [review]
part 1 - readIMG and readIIDF (v2)

I seem miss some in spec. Cancelling review.

Comment 37

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8463869 [details] [diff] [review]
part 2 - xpcshell test

I seem miss some in spec. Cancelling review.

Comment 38

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF (v3)

This revision does:
1) Get icon id from stk proactive commands. 
2) Read image record according to the icon id
3) Since reading icon image is an async process, we don't send "stkcommand" messages until the icon is loaded.

Hi Edgar,

The patch shows the picture of what the whole picture looks like, which isn't fully verified yet.  As I am new to STK, could you give me some feedback to see if I am towards the right direction? Thank you!

Comment 39

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8464509 [details] [diff] [review]
part 1 - readIMG and readIIDF (v3)

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

::: dom/system/gonk/ril_worker.js
@@ +5393,5 @@
>  
>      cmdDetails.rilMessageType = "stkcommand";
>      cmdDetails.options =
>        this.context.StkCommandParamsFactory.createParam(cmdDetails, ctlvs);
> +    if (!cmdDetails.pending) {

This line should be corrected as |if (!cmdDetails.options || !cmdDetails.options.pending) | :p

Comment 40

[Edgar Chen [:edgar]]

Comment on attachment 8464509 [details] [diff] [review]
part 1 - readIMG and readIIDF (v3)

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

Overall structure looks good (I did not check the detailed with spec yet).
Please see my below comments, thank you. :)

::: dom/system/gonk/ril_worker.js
@@ +10604,5 @@
> +      menu.iconSelfExplanatory = iconId.qualifier == 0 ? true : false;
> +
> +      let onsuccess = (function(result) {
> +        menu.iconRawData = result;
> +        this.context.RIL.sendChromeMessage(menu);

sendChromMessage(cmdDetails), here and elsewhere.

And maybe we could have a helper function for these stk icon things? (line 10603-10612)

@@ +13203,5 @@
>        callback: callback.bind(this)
>      });
>    },
>  
> +  readIMG: function(recordNumber, onsuccess) {

Do we need an onerror callback for the error handling, ex: fail to loadLinearFixedEF()?

@@ +13206,5 @@
>  
> +  readIMG: function(recordNumber, onsuccess) {
> +     if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {
> +       if (DEBUG) this.context.debug("IMG: IMG service is not available");
> +       return false;

readIMG() returns a boolean looks a little bit weird to me. Could we move this check to caller?

@@ +13249,5 @@
> +    });
> +    return true;
> +  },
> +
> +  readIIDF: function(fileId, offset, dataLen, codingScheme, onsuccess) {

Ditto

Comment 41

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Edgar Chen [:edgar][:echen] from comment #40)
> Comment on attachment 8464509 [details] [diff] [review]
> part 1 - readIMG and readIIDF (v3)
> 
> Review of attachment 8464509 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall structure looks good (I did not check the detailed with spec yet).
> Please see my below comments, thank you. :)
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +10604,5 @@
> > +      menu.iconSelfExplanatory = iconId.qualifier == 0 ? true : false;
> > +
> > +      let onsuccess = (function(result) {
> > +        menu.iconRawData = result;
> > +        this.context.RIL.sendChromeMessage(menu);
> 
> sendChromMessage(cmdDetails), here and elsewhere.
> 
> And maybe we could have a helper function for these stk icon things? (line
> 10603-10612)
> 

Totally agree :P

> @@ +13203,5 @@
> >        callback: callback.bind(this)
> >      });
> >    },
> >  
> > +  readIMG: function(recordNumber, onsuccess) {
> 
> Do we need an onerror callback for the error handling, ex: fail to
> loadLinearFixedEF()?
> 

We need, yes.

> @@ +13206,5 @@
> >  
> > +  readIMG: function(recordNumber, onsuccess) {
> > +     if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {
> > +       if (DEBUG) this.context.debug("IMG: IMG service is not available");
> > +       return false;
> 
> readIMG() returns a boolean looks a little bit weird to me. Could we move
> this check to caller?
> 

Makes sense to me, too. Will do.

> @@ +13249,5 @@
> > +    });
> > +    return true;
> > +  },
> > +
> > +  readIIDF: function(fileId, offset, dataLen, codingScheme, onsuccess) {
> 
> Ditto

Thanks for the feedback, Edgar.

Comment 42

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF (v4)

This revision does:
1) Address review comment 40
2) Create a helper IconLoader to read and decode icons.

Comment 43

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js

Read EF_IMG and EF_IIDF

Comment 44

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 3 - xpcshell test - test_ril_worker_icc_IconLoader.js

Add a new test for IconLoader

Comment 45

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8467723 [details] [diff] [review]
part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js

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

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +987,5 @@
> +      codingScheme: ICC_IMG_CODING_SCHEME_COLOR_TRANSPARENCY,
> +      bitsPerImgPoint: 0x03,
> +      numOfClutEntries: 0x08,
> +      body: [0x10, 0x20, 0x30, 0x40, 0x50, 0x60, 0x70, 0x80, 0x90, 0xa0, 0xb0,
> +             0xc0, 0xd0,

"]" is missing... will add it in the next revision.

Comment 46

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF (v5)

This revision does:
1) Address review comment 40
2) Create a helper IconLoader to read and decode icons.

Hi Edgar,
Please help review this, thank you :)

Comment 47

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js (v2)

Test readIMG() and readIIDF()

Comment 48

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 3 - xpcshell test - test_ril_worker_icc_IconLoader.js (v2)

Test iconLoader.loadIcon and .loadIcons

Comment 49

[Hsin-Yi Tsai (she/her) [:hsinyi]]

test_ril_worker_stk.js needs modification, too. Patch ongoing!

Comment 50

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF (v6)

This revision does:
1) Address a careless undefined error :(
2) Address review comment 40
3) Create a helper IconLoader to read and decode icons.

The whole flow is:
1) Read IconId or IconIdList from STK proactive commands and put the notification into pending
2) Call IconLoader.loadIcon or .loadIcons which reads EF_IMG and EF_IIDF
3) Once success, IconLoader will parse the raw data
4) Then, the pending proactive command will be sent with the icon image

Hi Edgar,
Please help review this. Let me know if you'd like me to split the patch. Thank you :)

Comment 51

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 4 - xpcshell test - test_ril_worker_stk.js

Read COMPREHENSIONTLV_TAG_ICON_ID and COMPREHENSIONTLV_TAG_ICON_ID_LIST

Comment 52

[Edgar Chen [:edgar]]

Comment on attachment 8469203 [details] [diff] [review]
part 1 - readIMG and readIIDF (v6)

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

Sorry for the late.
Please see my comments below. Thank you.

::: dom/system/gonk/ril_worker.js
@@ +10760,5 @@
>      if (ctlv) {
>        menu.nextActionList = ctlv.value;
>      }
>  
> +    if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {

We only need to check IMG service when TAG_ICON_ID or TAG_ICON_ID_LIST is present.

@@ +10768,5 @@
> +
> +    ctlv = StkProactiveCmdHelper.searchForTag(COMPREHENSIONTLV_TAG_ICON_ID, ctlvs);
> +    let iconId;
> +    let ids = [];
> +    if (ctlv) {

So, what about
if (ctlv && this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {
  ...
}

@@ +10775,5 @@
> +    }
> +
> +    ctlv = StkProactiveCmdHelper.searchForTag(COMPREHENSIONTLV_TAG_ICON_ID_LIST, ctlvs);
> +    let iconIdList;
> +    if (ctlv) {

Ditto

@@ +10819,5 @@
> +      menu.pending = true;
> +    } else if (ids.length === menu.items.length + 1) {
> +      this.context.IconLoader.loadIcons(ids, onsuccess, onerror);
> +      menu.pending = true;
> +    }

How about the case: ids.length === menu.items.length? (ICON_ID_LIST is present, but ICON_ID is not. Although I don't know is it really a case.)

And could all of them just share the same loader function (the array one, I guess)?

@@ +10857,5 @@
>      if (cmdDetails.commandQualifier & 0x80) {
>        textMsg.userClear = true;
>      }
>  
> +    if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {

Ditto

@@ +10896,5 @@
>        throw new Error("Stk Set Up Idle Text: Required value missing : Text String");
>      }
>      textMsg.text = ctlv.value.textString;
>  
> +    if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {

Ditto

@@ +10966,5 @@
>      if (cmdDetails.commandQualifier & 0x80) {
>        input.isHelpAvailable = true;
>      }
>  
> +    if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {

Ditto

@@ +11041,5 @@
>      if (cmdDetails.commandQualifier & 0x80) {
>        input.isHelpAvailable = true;
>      }
>  
> +    if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {

Ditto.

@@ +11076,5 @@
>      if (ctlv) {
>        textMsg.text = ctlv.value.identifier;
>      }
>  
> +    if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {

Ditto.

@@ +11132,5 @@
>      if (ctlv) {
>        call.duration = ctlv.value;
>      }
>  
> +    if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {

Ditto

@@ +11178,5 @@
>      }
>  
>      browser.mode = cmdDetails.commandQualifier & 0x03;
>  
> +    if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {

Ditto

@@ +11228,5 @@
>  
>      // vibrate is only defined in TS 102.223
>      playTone.isVibrate = (cmdDetails.commandQualifier & 0x01) !== 0x00;
>  
> +    if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {

Ditto

@@ +11307,5 @@
>      if (ctlv) {
>        bipMsg.text = ctlv.value.identifier;
>      }
>  
> +   if (!this.context.ICCUtilsHelper.isICCServiceAvailable("IMG")) {

Ditto

@@ +11712,5 @@
> +    }
> +
> +    let iconIdList = {
> +      qualifier: this.context.GsmPDUHelper.readHexOctet(),
> +      identifier: []

Nit: s/identifier/identifiers/

@@ +13530,5 @@
> +      let strLen = Buf.readInt32();
> +      // Each octet is encoded into two chars.
> +      let octetLen = strLen / 2;
> +
> +      // We read only the 1st instance for now.

Nit: // Number of Actual Image Instances.

@@ +13573,5 @@
> +      let RIL = this.context.RIL;
> +      let GsmPDUHelper = this.context.GsmPDUHelper;
> +      let strLen = Buf.readInt32();
> +      // Each octet is encoded into two chars.
> +      let octetLen = strLen / 2;

Nit: add one empty line here.

@@ +13580,5 @@
> +        // of Image Instance Data."
> +        if (onerror) {
> +          onerror();
> +        }
> +        throw new Error("readIIDF: Data length is not enough.");

We already report failure via onerror callback, do we still need to throw an exception?

@@ +15631,5 @@
> +  recordNumber: -1,
> +  icons: null,
> +  state: -1,
> +
> +  loadIcon: function(recordNumber, onsuccess, onerror) {

Could you try to merge |loadIcon| and |loadIcons|?
(|loadIcon| is the same with the case that |loadIcons| loads an icon array which has only one item)

@@ +15753,5 @@
> +  },
> +
> +  getMask: function(bitsPerImgPoint) {
> +    let mask = 0x00;
> +    switch (bitsPerImgPoint) {

Don't need switch-case.

mask = 0xff >> (8 - bitsPerImgPoint);

Comment 53

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Edgar Chen [:edgar][:echen] from comment #52)
> 
> @@ +13580,5 @@
> > +        // of Image Instance Data."
> > +        if (onerror) {
> > +          onerror();
> > +        }
> > +        throw new Error("readIIDF: Data length is not enough.");
> 
> We already report failure via onerror callback, do we still need to throw an
> exception?
> 

No. Should be okay to remove it.

> @@ +15631,5 @@
> > +  recordNumber: -1,
> > +  icons: null,
> > +  state: -1,
> > +
> > +  loadIcon: function(recordNumber, onsuccess, onerror) {
> 
> Could you try to merge |loadIcon| and |loadIcons|?
> (|loadIcon| is the same with the case that |loadIcons| loads an icon array
> which has only one item)
> 

Sure, let me do this.

> @@ +15753,5 @@
> > +  },
> > +
> > +  getMask: function(bitsPerImgPoint) {
> > +    let mask = 0x00;
> > +    switch (bitsPerImgPoint) {
> 
> Don't need switch-case.
> 
> mask = 0xff >> (8 - bitsPerImgPoint);

Thanks for this :)

Comment 54

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8469178 [details] [diff] [review]
part 3 - xpcshell test - test_ril_worker_icc_IconLoader.js (v2)

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

Need to address comment 52 first. Cancel the review.

Comment 55

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8469175 [details] [diff] [review]
part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js (v2)

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

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +733,5 @@
> +
> +/**
> + * Verify reading EF_IMG and EF_IIDF with the case data length is not enough
> + */
> +add_test(function test_reading_img_length_error() {

I will modify this due to the comment 52.

Comment 56

[Edgar Chen [:edgar]]

Comment on attachment 8469204 [details] [diff] [review]
part 4 - xpcshell test - test_ril_worker_stk.js

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

Looks good, thank you.

Comment 57

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF (v7)

Hi Edgar,

Comment 52 has been addressed:
1) check IMG service when TAG_ICON_ID or TAG_ICON_ID_LIST is present
2) drop iconLoader.loadIcon; keep iconLoader.loadIcons only
3) remove getMask()
4) don't throw exception but call onerror when data length isn't enough in readIIDF

Please help review again, thank you :)

Comment 58

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js (v3)

Revision:
In test_reading_img_length_error, call "onerror."

Comment 59

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 3 - xpcshell test - test_ril_worker_icc_IconLoader.js (v3)

Revision:

replace iconLoader.loadIcon with iconLoader.loadIcons due to comment 52

Comment 60

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 4 - xpcshell test - test_ril_worker_stk.js (v2) r=edgar

Revision:
replace iconIdList.identifier with iconIdList.identifiers due to comment 52

Comment 61

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF (v8)

Revision:
1) comment 57
2) per discussion with Jessica, we don't assign iconSelfExplanatory to each item. We introduce itemIconSelfExplanatory instead.

Comment 62

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8470737 [details] [diff] [review]
part 1 - readIMG and readIIDF (v8)

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

::: dom/system/gonk/ril_worker.js
@@ +11055,5 @@
>      if (ctlv) {
>        textMsg.text = ctlv.value.identifier;
>      }
>  
> +    ctlv = StkProactiveCmdHelper.searchForTag(COMPREHENSIONTLV_TAG_ICON_ID, ctlvs);

Should be "this.context.StkProactiveCmdHelper"

I will work on a new test case for this.

Comment 63

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8470737 [details] [diff] [review]
part 1 - readIMG and readIIDF (v8)

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

Let me complete the coverage of test_ril_worker_stk.js then ask for review!

Comment 64

[Edgar Chen [:edgar]]

Comment on attachment 8470737 [details] [diff] [review]
part 1 - readIMG and readIIDF (v8)

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

::: dom/system/gonk/ril_worker.js
@@ +10798,5 @@
> +        for (let i = 0; i < result.length; i++) {
> +          menu.items[i].icon = result[i];
> +        }
> +      } else {
> +        // Loading a single icon.

I would like to make this simpler.
Let onsuccess always carries result as array, and all cases can be covered by line 10793 - line 10800.

@@ +15603,5 @@
> +
> +    if (recordNumbers.length > 1) {
> +      this.state = STATE_MULTI_ICONS;
> +    } else {
> +      this.state = STATE_SINGLE_ICON;

I would like to make IconLoader simpler as well.
If onsuccess always carries result as array, I guess we don't need two different states, STATE_SINGLE_ICON and STATE_MULTI_ICONS.

Comment 65

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Edgar Chen [:edgar][:echen] from comment #64)
> Comment on attachment 8470737 [details] [diff] [review]
> part 1 - readIMG and readIIDF (v8)
> 
> Review of attachment 8470737 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +10798,5 @@
> > +        for (let i = 0; i < result.length; i++) {
> > +          menu.items[i].icon = result[i];
> > +        }
> > +      } else {
> > +        // Loading a single icon.
> 
> I would like to make this simpler.
> Let onsuccess always carries result as array, and all cases can be covered
> by line 10793 - line 10800.
> 
> @@ +15603,5 @@
> > +
> > +    if (recordNumbers.length > 1) {
> > +      this.state = STATE_MULTI_ICONS;
> > +    } else {
> > +      this.state = STATE_SINGLE_ICON;
> 
> I would like to make IconLoader simpler as well.
> If onsuccess always carries result as array, I guess we don't need two
> different states, STATE_SINGLE_ICON and STATE_MULTI_ICONS.

Sure, let me see how to simplify this. Thanks!

Comment 66

[Edgar Chen [:edgar]]

Comment on attachment 8470703 [details] [diff] [review]
part 3 - xpcshell test - test_ril_worker_icc_IconLoader.js (v3)

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

Looks good, but this test may need modifying due to comment #64, I would like to check it again. Thank you.

::: dom/system/gonk/tests/test_ril_worker_icc_IconLoader.js
@@ +783,5 @@
> +
> +  function do_test() {
> +    simRecordHelper.readIMG = function fakeReadIMG(recordNumber, onsuccess, onerror) {
> +      onsuccess(test_data.rawData[recordNumber]);
> +  };

nit: indention

Comment 67

[Edgar Chen [:edgar]]

Comment on attachment 8470702 [details] [diff] [review]
part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js (v3)

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

Nice, thank you.

::: dom/system/gonk/tests/test_ril_worker_icc_SimRecordHelper.js
@@ +830,5 @@
> +                codingScheme: ICC_IMG_CODING_SCHEME_COLOR,
> +                bitsPerImgPoint: 0x03,
> +                numOfClutEntries: 0x08,
> +                body: [0x10, 0x20, 0x30, 0x40, 0x50, 0x60, 0x70, 0x80, 0x90, 0xa0, 0xb0,
> +                0xc0, 0xd0],

nit: indention

@@ +832,5 @@
> +                numOfClutEntries: 0x08,
> +                body: [0x10, 0x20, 0x30, 0x40, 0x50, 0x60, 0x70, 0x80, 0x90, 0xa0, 0xb0,
> +                0xc0, 0xd0],
> +                clut: [0x00, 0x01, 0x02,
> +                0x10, 0x11, 0x12,

Ditto

Comment 68

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF (v9)

Revision:
1) comment 57 & comment 64
2) per discussion with Jessica, we don't assign iconSelfExplanatory to each item. We introduce itemIconSelfExplanatory instead.
3) address some careless undefined errors caught by marionette and xpcshell. Thank TESTS~

Hi Edgar,
Me again! Thank you :P

Comment 69

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js (v4). r=edgar

Comment 67 addressed. Thank you for the review, Edgar!

Comment 70

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 3 - xpcshell test - test_ril_worker_icc_IconLoader.js (v4)

Revision based on comment 64.

Comment 71

[Edgar Chen [:edgar]]

Comment on attachment 8471502 [details] [diff] [review]
part 1 - readIMG and readIIDF (v9)

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

Over all looks good. Please see my comments below, thank you.

::: dom/system/gonk/ril_worker.js
@@ +10763,5 @@
>  
> +    ctlv = StkProactiveCmdHelper.searchForTag(COMPREHENSIONTLV_TAG_ICON_ID, ctlvs);
> +    let iconId;
> +    let ids = [];
> +    if (ctlv) {

nit: put "ctlv = ..." in front of "if (ctlv)"

ctlv = StkProactiveCmdHelper.searchForTag(COMPREHENSIONTLV_TAG_ICON_ID, ctlvs);
if (ctlv) {
 ...

@@ +10771,5 @@
> +    }
> +
> +    ctlv = StkProactiveCmdHelper.searchForTag(COMPREHENSIONTLV_TAG_ICON_ID_LIST, ctlvs);
> +    let iconIdList;
> +    if (ctlv) {

ditto

@@ +11047,5 @@
>      if (ctlv) {
>        textMsg.text = ctlv.value.identifier;
>      }
>  
> +    ctlv = this.context.StkProactiveCmdHelper.searchForTag(

nit: ctlv = StkProactiveCmdHelper.searchForTag(...)

@@ +13553,5 @@
> +
> +      switch (codingScheme) {
> +        case ICC_IMG_CODING_SCHEME_BASIC:
> +          rawData.body = GsmPDUHelper.readHexOctetArray(
> +              dataLen - ICC_IMG_HEADER_SIZE_BASIC);

Should we seek the data to the end of buf because we won't read CLUT if the scheme is BASIC type?

@@ +15598,5 @@
> +    this.onerror = onerror;
> +    this.startLoadingIcon(recordNumbers[0]);
> +  },
> +
> +  startLoadingIcon: function(recordNumber) {

nit: add a '_' prefix given that this function should only be used inside IconLoader.

@@ +15604,5 @@
> +                                         this.processIcon.bind(this),
> +                                         this.stopLoadingIcon.bind(this));
> +  },
> +
> +  stopLoadingIcon: function() {

ditto

@@ +15607,5 @@
> +
> +  stopLoadingIcon: function() {
> +    if (this.onerror) {
> +      this.onerror();
> +      delete this.onerror;

also delete this.onsuccess here?

@@ +15615,5 @@
> +    this.recordNumbers = null;
> +    this.icons = null;
> +  },
> +
> +  processIcon: function(icon) {

ditto, add a '_' prefix

@@ +15627,5 @@
> +      this.startLoadingIcon(this.recordNumbers[this.currentRecordIndex]);
> +    } else {
> +      if (this.onsuccess) {
> +        this.onsuccess(this.icons);
> +        delete this.onsuccess;

also delete this.onerror here?

@@ +15636,5 @@
> +      this.icons = null;
> +    }
> +  },
> +
> +  parseRawData: function(rawData) {

ditto, add a '_' prefix

Comment 72

[Edgar Chen [:edgar]]

Comment on attachment 8471504 [details] [diff] [review]
part 3 - xpcshell test - test_ril_worker_icc_IconLoader.js (v4)

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

Nice, thank you.

Comment 73

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

Comment on attachment 8471502 [details] [diff] [review]
part 1 - readIMG and readIIDF (v9)

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

::: dom/system/gonk/ril_worker.js
@@ +15595,5 @@
> +    this.recordNumbers = recordNumbers;
> +    this.currentRecordIndex = 0;
> +    this.onsuccess = onsuccess;
> +    this.onerror = onerror;
> +    this.startLoadingIcon(recordNumbers[0]);

Hey! Please don't pollute helper object namespace. Create a execution context and bind it into callback arguments list.

  this.startLoadingIcon({
    recordNumbers: recordNumbers,
    onsuccess: onsuccess,
    onerror: onerror,
    ...
  });

So far the only exception is BitBufferHelper and it's meant to be used in this way (startRead/startWrite/readSomething/writeSomething).

Comment 74

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF (v10)

Revision:
1) read data to the end in the case of CODING_SCHEME_BASIC per comment 71
2) rewrite iconLoader to use a separate execution context and add _ prefix for private functions per comment 71 and comment 73.

Edgar,
May I have your review again? Thank you!

Comment 75

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js (v5)

I modified one test data to cover comment 71.

Comment 76

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

Comment on attachment 8472793 [details] [diff] [review]
part 1 - readIMG and readIIDF (v10)

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

::: dom/system/gonk/ril_worker.js
@@ +10789,5 @@
> +
> +    let onsuccess = (function(result) {
> +      if (iconId) {
> +        menu.icon = result[0];
> +        result = result.slice(1);

From bug 824145, we know there might be multiple icons defined per record. Considering we have ids = [ <id-0>, <id-1>, ...<id-N>], and the returned result may be [ <icon-0-0>, <icon-0-1>, <icon-1>, <icon-2-0>, <icon-2-1>, ...<icon-N-M>]. Obviously we will have wrongly assigned icons here.

@@ +10800,5 @@
> +      this.context.RIL.sendChromeMessage(cmdDetails);
> +    }).bind(this);
> +
> +    this.context.IconLoader.loadIcons(ids, onsuccess, onerror);
> +    menu.pending = true;

Please move this one line ahead before |loadIcons()|. |loadIcons()| takes two callbacks and they may be called __before__ |loadIcons()| returns.  It also follows that pending flag has to be cleared in onerror and onsuccess.

@@ +13490,5 @@
> +      // Each octet is encoded into two chars.
> +      let octetLen = strLen / 2;
> +
> +      // Number of actual image instances.
> +      GsmPDUHelper.readHexOctet();

Apparently this still supports one icon per EFimg only....

@@ +13556,5 @@
> +          rawData.body = GsmPDUHelper.readHexOctetArray(
> +            dataLen - ICC_IMG_HEADER_SIZE_BASIC);
> +          Buf.seekIncoming((octetLen - offset - dataLen) * Buf.PDU_HEX_OCTET_SIZE);
> +          break;
> +        case ICC_IMG_CODING_SCHEME_COLOR:

nit: a line break before this case is appreciated.

@@ +15587,5 @@
> +  context: null,
> +
> +  loadIcons: function(recordNumbers, onsuccess, onerror) {
> +    if(!recordNumbers || !recordNumbers.length) {
> +      return;

Whenever you take callback functions in the argument list, make sure they're either called or passed down to other functions before current execution context returns.

@@ +15618,5 @@
> +      }
> +    }).bind(this);
> +
> +    options.currentRecordIndex = 0;
> +    options.callback = callback;

You don't need to do this.  Just replace all |options.callback| with |callback| will do.

@@ +15632,5 @@
> +      case ICC_IMG_CODING_SCHEME_BASIC:
> +        return {pixels: rawData.body,
> +                width: rawData.width,
> +                height: rawData.height};
> +      case ICC_IMG_CODING_SCHEME_COLOR:

nit: a line break before this case is appreciated.

Comment 77

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8472793 [details] [diff] [review]
part 1 - readIMG and readIIDF (v10)

Let me address Vicamo's comments first!

Comment 78

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8472794 [details] [diff] [review]
part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js (v5)

I will need  to modify the test case due to Vicamo's opinion on reading multiple image instances per record. Cancelling the review request.

Comment 79

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1 - readIMG and readIIDF (v11)

Addressed comment 76:
1) Read all images of every specified EF_IMG record. The data format will be array of image array of records, i.e.
   [[Icon_R1_1, Icon_R1_2, ..., Icon_R1_x], [Icon_R2_1..., Icon_R2_y], ..., [Icon_RN_1,..., Icon_RN_z]]
2) Make sure callback is either passed down or called
3) Move |.pending = true| ahead

Vicamo and Edgar,
Your review is appreciated.

Comment 80

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js (v6)

Modification for comment 76 to cover reading all images of one IMG record.

Hi Edgar,
Could you please take a look again, thank you?

Comment 81

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js (v6)

Oops, here is the right file.

Comment 82

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 3 - xpcshell test - test_ril_worker_icc_IconLoader.js (v5)

Modification for comment 76 to cover reading all images of one IMG record.

Hi Edgar,
Could you please take a look again, thank you?

Comment 83

[Edgar Chen [:edgar]]

Comment on attachment 8473614 [details] [diff] [review]
part 1 - readIMG and readIIDF (v11)

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

Looks good, but it doesn't match with the latest version of webidl interface (please see bug 824145 comment #42), eg: in the latest version, it puts the iconSelfExplanatory in each stkItem entry.
I would like to review this again after the interface is locked down. And a new patch may be needed.

Thank you.

::: dom/system/gonk/ril_worker.js
@@ +13536,5 @@
> +      let instances;
> +      let currentInstance = 0;
> +      let readNextInstance = (function(img) {
> +        if (!instances) {
> +          instances = [];

nit: initialize |instances| in it's declaration then we don't need this if check.

Comment 84

[Edgar Chen [:edgar]]

Comment on attachment 8473617 [details] [diff] [review]
part 2 - xpcshell test - test_ril_worker_icc_SimRecordHelper.js (v6)

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

Nice, thank you.

Comment 85

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This is under review and revision should reply on DOM peer's opinion on bug 824145. Move target to Sprint 3.

Comment 86

[Edgar Chen [:edgar]]

Comment on attachment 8473614 [details] [diff] [review]
part 1 - readIMG and readIIDF (v11)

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

::: dom/system/gonk/ril_worker.js
@@ +15727,5 @@
> +          bitIndex -= bitsPerImgPoint;
> +        }
> +        return {pixels: pixels,
> +                width: rawData.width,
> +                height: rawData.height};

Similar with comment #83, the latest version of webidl interface exposes "IccImageCodingScheme" with enum type in MozStkIcon.

Comment 87

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Edgar Chen [:edgar][:echen] from comment #86)
> Comment on attachment 8473614 [details] [diff] [review]
> part 1 - readIMG and readIIDF (v11)
> 
> Review of attachment 8473614 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +15727,5 @@
> > +          bitIndex -= bitsPerImgPoint;
> > +        }
> > +        return {pixels: pixels,
> > +                width: rawData.width,
> > +                height: rawData.height};
> 
> Similar with comment #83, the latest version of webidl interface exposes
> "IccImageCodingScheme" with enum type in MozStkIcon.

Hi Edgar,

In my original plan, this bug focuses on parsing the data from EF but not transforming the data format that we are going to expose to web.

So, bug 824145 would take care of the data format to webidl. Does that sound reasonable to you?

Comment 88

[Edgar Chen [:edgar]]

Comment on attachment 8473619 [details] [diff] [review]
part 3 - xpcshell test - test_ril_worker_icc_IconLoader.js (v5)

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

Nice, but I would like to review again because some modifying might be needed due to comment #86.

Comment 89

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

Comment on attachment 8473614 [details] [diff] [review]
part 1 - readIMG and readIIDF (v11)

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

::: dom/system/gonk/ril_worker.js
@@ +10814,5 @@
> +
> +    let onsuccess = (function(result) {
> +      if (iconId) {
> +        menu.icons = result[0];
> +        result = result.slice(1);

nit: menu.icons = result.shift();

Comment 90

[Edgar Chen [:edgar]]

Comment on attachment 8473614 [details] [diff] [review]
part 1 - readIMG and readIIDF (v11)

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #87)
> (In reply to Edgar Chen [:edgar][:echen] from comment #86)
> > Comment on attachment 8473614 [details] [diff] [review]
> > part 1 - readIMG and readIIDF (v11)
> > 
> > Review of attachment 8473614 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/ril_worker.js
> > @@ +15727,5 @@
> > > +          bitIndex -= bitsPerImgPoint;
> > > +        }
> > > +        return {pixels: pixels,
> > > +                width: rawData.width,
> > > +                height: rawData.height};
> > 
> > Similar with comment #83, the latest version of webidl interface exposes
> > "IccImageCodingScheme" with enum type in MozStkIcon.
> 
> Hi Edgar,
> 
> In my original plan, this bug focuses on parsing the data from EF but not
> transforming the data format that we are going to expose to web.
> 
> So, bug 824145 would take care of the data format to webidl. Does that sound
> reasonable to you?

I see. Let's focus on parsing the data from EF here.
I am ok with the data format now, comment #83 and comment #86 will be addressed in bug 824145 if needed.
Thank you. :)

Comment 91

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1b - readIMG and readIIDF

Hi Edgar,
When integrated with Jessica's work bug 824145, I found that we should have more solid error handling in the following cases. And this patch addresses that. I will squash this into part 1 for final landing. 

1) check the EF_IMG data length
2) check the EF_IIDF fileId

Comment 92

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 2b - xpcshell test - test_ril_worker_icc_SimRecordHelper.js

This patch adds test cases for:
1) wrong EF_IMG data length
2) invalid EF_IIDF fileId

It will be squashed into part2 when landing, thank you!

Comment 93

[Edgar Chen [:edgar]]

Comment on attachment 8474389 [details] [diff] [review]
part 1b - readIMG and readIIDF

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

::: dom/system/gonk/ril_worker.js
@@ +13523,5 @@
> +          octetLen != (9 * numInstances + 2)) {
> +        if (onerror) {
> +          onerror();
> +          return;
> +        }

return here instead?

Comment 94

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Attachment: part 1b - readIMG and readIIDF (v2)

You are right... Orz

Comment 95

[Edgar Chen [:edgar]]

Comment on attachment 8474418 [details] [diff] [review]
part 1b - readIMG and readIIDF (v2)

Thank you. :)

Comment 96

[Hsin-Yi Tsai (she/her) [:hsinyi]]

https://tbpl.mozilla.org/?tree=Try&rev=32f2ba4cf401 try green. Going to land the patches. Thanks for the review efforts, Edgar and Vicamo!

Comment 97

[Hsin-Yi Tsai (she/her) [:hsinyi]]

   https://hg.mozilla.org/integration/b2g-inbound/rev/ea5f0bee481b
   https://hg.mozilla.org/integration/b2g-inbound/rev/86018b4be532
   https://hg.mozilla.org/integration/b2g-inbound/rev/f4c2a5238573
   https://hg.mozilla.org/integration/b2g-inbound/rev/45a87a403420

Comment 98

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ea5f0bee481b
https://hg.mozilla.org/mozilla-central/rev/86018b4be532
https://hg.mozilla.org/mozilla-central/rev/f4c2a5238573
https://hg.mozilla.org/mozilla-central/rev/45a87a403420

Comment 99

[Eric Chang [:ericcc] [:echang]]

Verified landed, emulator okay.
Gaia      fbb297c39aab5f17b179533d2a9a6c5166b2c197
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/fb5e796da813
BuildID   20140902160204
Version   34.0a2
ro.build.version.incremental=110
ro.build.date=Fri Jun 27 15:57:58 CST 2014
B1TC00011230