Closed Bug 935843 Opened 6 years ago Closed 5 years ago

B2G RIL: Parse EF_IMG from SIM

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.1, tracking-b2g:backlog)

VERIFIED FIXED
2.1 S3 (29aug)
feature-b2g 2.1
tracking-b2g backlog

People

(Reporter: allstars.chh, Assigned: hsinyi)

References

Details

Attachments

(6 files, 41 obsolete files)

12.26 KB, patch
Details | Diff | Splinter Review
29.37 KB, patch
vicamo
: review+
edgar
: review+
Details | Diff | Splinter Review
18.04 KB, patch
edgar
: review+
Details | Diff | Splinter Review
48.35 KB, patch
edgar
: review+
Details | Diff | Splinter Review
3.91 KB, patch
edgar
: review+
Details | Diff | Splinter Review
2.18 KB, patch
edgar
: review+
Details | Diff | Splinter Review
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)
Attached patch Part2: xpcshell for readIMG (obsolete) — Splinter Review
Attached patch Part4: xpcshell for readIIDF (obsolete) — Splinter Review
Attachment #8339201 - Flags: review?(allstars.chh)
Attachment #8339202 - Flags: review?(allstars.chh)
Attachment #8339203 - Flags: review?(allstars.chh)
Attachment #8339204 - Flags: review?(allstars.chh)
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 = {
  ...
};
Attachment #8339201 - Flags: review?(allstars.chh) → review-
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.
Attachment #8339202 - Flags: review?(allstars.chh)
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 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?
Attachment #8339203 - Flags: review?(allstars.chh) → review-
Attachment #8339204 - Flags: review?(allstars.chh)
(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.
Attachment #8339201 - Attachment is obsolete: true
Attached patch Part2: xpcshell for readIMG. v2 (obsolete) — Splinter Review
Attachment #8339202 - Attachment is obsolete: true
Attachment #8339203 - Attachment is obsolete: true
Attached patch Part4: xpcshell for readIIDF. v2 (obsolete) — Splinter Review
Attachment #8339204 - Attachment is obsolete: true
Attachment #8339205 - Attachment is obsolete: true
Attachment #8339206 - Attachment is obsolete: true
Attachment #8342794 - Attachment is obsolete: true
Attachment #8342882 - Flags: review?(allstars.chh)
Attachment #8342797 - Flags: review?(allstars.chh)
Attachment #8342798 - Flags: review?(allstars.chh)
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
Attachment #8342806 - Flags: review?(allstars.chh)
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.
Attachment #8342808 - Flags: review?(allstars.chh)
Attachment #8342809 - Flags: review?(allstars.chh)
Assignee: gwang → nobody
Attachment #8342797 - Flags: review?(allstars.chh)
Attachment #8342798 - Flags: review?(allstars.chh)
Attachment #8342806 - Flags: review?(allstars.chh)
Attachment #8342808 - Flags: review?(allstars.chh)
Attachment #8342809 - Flags: review?(allstars.chh)
Attachment #8342882 - Flags: review?(allstars.chh)
Please this into backlog.
blocking-b2g: --- → backlog
This is requirement from CAF for 2.1.
Set to 2.1? so that we can prioritize in 2.1
blocking-b2g: backlog → 2.1?
Hsinyi, you want to take this bug, right?
Flags: needinfo?(htsai)
Assignee: nobody → htsai
Flags: needinfo?(htsai)
blocking-b2g: 2.1? → backlog
feature-b2g: --- → 2.1
Keep working on decoding color table
Attachment #8342797 - Attachment is obsolete: true
Attachment #8342798 - Attachment is obsolete: true
Attachment #8342806 - Attachment is obsolete: true
Attachment #8342808 - Attachment is obsolete: true
Attachment #8342809 - Attachment is obsolete: true
Attachment #8342882 - Attachment is obsolete: true
Attachment #8458607 - Attachment description: WIP - part 2 - xpcshell - data length not enough → WIP - part 3 - xpcshell - data length not enough
Target Milestone: --- → 2.1 S2 (15aug)
Attached patch part 1 - readIMG and readIIDF (obsolete) — Splinter Review
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.
Attachment #8458603 - Attachment is obsolete: true
Attachment #8462502 - Attachment is obsolete: true
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.
Attached patch part 2 - xpcshell test (obsolete) — Splinter Review
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
Attachment #8458605 - Attachment is obsolete: true
Attachment #8458607 - Attachment is obsolete: true
Attachment #8462503 - Attachment is obsolete: true
Attachment #8463854 - Attachment is obsolete: true
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!
Attachment #8463865 - Flags: review?(echen)
Comment on attachment 8463869 [details] [diff] [review]
part 2 - xpcshell test

Please see comment 33 for details!
Attachment #8463869 - Flags: review?(echen)
Comment on attachment 8463865 [details] [diff] [review]
part 1 - readIMG and readIIDF (v2)

I seem miss some in spec. Cancelling review.
Attachment #8463865 - Flags: review?(echen)
Comment on attachment 8463869 [details] [diff] [review]
part 2 - xpcshell test

I seem miss some in spec. Cancelling review.
Attachment #8463869 - Flags: review?(echen)
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!
Attachment #8463865 - Attachment is obsolete: true
Attachment #8464509 - Flags: review?(echen)
Attachment #8464509 - Flags: review?(echen) → feedback?(echen)
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 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
Attachment #8464509 - Flags: feedback?(echen)
(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.
This revision does:
1) Address review comment 40
2) Create a helper IconLoader to read and decode icons.
Attachment #8463869 - Attachment is obsolete: true
Attachment #8464509 - Attachment is obsolete: true
Read EF_IMG and EF_IIDF
Add a new test for IconLoader
QA Whiteboard: [COM=RIL]
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.
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 :)
Attachment #8467722 - Attachment is obsolete: true
Attachment #8469173 - Flags: review?(echen)
Test readIMG() and readIIDF()
Attachment #8469175 - Flags: review?(echen)
Test iconLoader.loadIcon and .loadIcons
Attachment #8467723 - Attachment is obsolete: true
Attachment #8467724 - Attachment is obsolete: true
Attachment #8469178 - Flags: review?(echen)
test_ril_worker_stk.js needs modification, too. Patch ongoing!
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 :)
Attachment #8469173 - Attachment is obsolete: true
Attachment #8469173 - Flags: review?(echen)
Attachment #8469203 - Flags: review?(echen)
Read COMPREHENSIONTLV_TAG_ICON_ID and COMPREHENSIONTLV_TAG_ICON_ID_LIST
Attachment #8469204 - Flags: review?(echen)
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);
Attachment #8469203 - Flags: review?(echen)
(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 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.
Attachment #8469178 - Flags: review?(echen)
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.
Attachment #8469175 - Flags: review?(echen)
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.
Attachment #8469204 - Flags: review?(echen) → review+
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 :)
Attachment #8469203 - Attachment is obsolete: true
Attachment #8470700 - Flags: review?(echen)
Revision:
In test_reading_img_length_error, call "onerror."
Attachment #8469175 - Attachment is obsolete: true
Attachment #8470702 - Flags: review?(echen)
Revision:

replace iconLoader.loadIcon with iconLoader.loadIcons due to comment 52
Attachment #8469178 - Attachment is obsolete: true
Attachment #8470703 - Flags: review?(echen)
Revision:
replace iconIdList.identifier with iconIdList.identifiers due to comment 52
Attachment #8469204 - Attachment is obsolete: true
Revision:
1) comment 57
2) per discussion with Jessica, we don't assign iconSelfExplanatory to each item. We introduce itemIconSelfExplanatory instead.
Attachment #8470700 - Attachment is obsolete: true
Attachment #8470700 - Flags: review?(echen)
Attachment #8470737 - Flags: review?(echen)
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 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!
Attachment #8470737 - Flags: review?(echen)
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.
(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 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
Attachment #8470703 - Flags: review?(echen)
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
Attachment #8470702 - Flags: review?(echen) → review+
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
Attachment #8470737 - Attachment is obsolete: true
Attachment #8471502 - Flags: review?(echen)
Comment 67 addressed. Thank you for the review, Edgar!
Attachment #8470702 - Attachment is obsolete: true
Revision based on comment 64.
Attachment #8470703 - Attachment is obsolete: true
Attachment #8471504 - Flags: review?(echen)
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
Attachment #8471502 - Flags: review?(echen)
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.
Attachment #8471504 - Flags: review?(echen) → review+
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).
Attachment #8471502 - Flags: review-
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!
Attachment #8471502 - Attachment is obsolete: true
Attachment #8472793 - Flags: review?(echen)
I modified one test data to cover comment 71.
Attachment #8471503 - Attachment is obsolete: true
Attachment #8472794 - Flags: review?(echen)
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.
Attachment #8472793 - Flags: review-
Comment on attachment 8472793 [details] [diff] [review]
part 1 - readIMG and readIIDF (v10)

Let me address Vicamo's comments first!
Attachment #8472793 - Flags: review?(echen)
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.
Attachment #8472794 - Flags: review?(echen)
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.
Attachment #8472793 - Attachment is obsolete: true
Attachment #8473614 - Flags: review?(vyang)
Attachment #8473614 - Flags: review?(echen)
Modification for comment 76 to cover reading all images of one IMG record.

Hi Edgar,
Could you please take a look again, thank you?
Attachment #8472794 - Attachment is obsolete: true
Attachment #8473616 - Flags: review?(echen)
Oops, here is the right file.
Attachment #8473616 - Attachment is obsolete: true
Attachment #8473616 - Flags: review?(echen)
Attachment #8473617 - Flags: review?(echen)
Modification for comment 76 to cover reading all images of one IMG record.

Hi Edgar,
Could you please take a look again, thank you?
Attachment #8471504 - Attachment is obsolete: true
Attachment #8473619 - Flags: review?(echen)
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.
Attachment #8473614 - Flags: review?(echen)
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.
Attachment #8473617 - Flags: review?(echen) → review+
This is under review and revision should reply on DOM peer's opinion on bug 824145. Move target to Sprint 3.
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
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.
(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 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.
Attachment #8473619 - Flags: review?(echen)
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();
Attachment #8473614 - Flags: review?(vyang) → review+
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. :)
Attachment #8473614 - Flags: review+
Attachment #8473619 - Flags: review+
Attached patch part 1b - readIMG and readIIDF (obsolete) — Splinter Review
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
Attachment #8474389 - Flags: review?(echen)
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!
Attachment #8474409 - Flags: review?(echen)
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?
Attachment #8474389 - Flags: review?(echen)
Attachment #8474409 - Flags: review?(echen) → review+
You are right... Orz
Attachment #8474389 - Attachment is obsolete: true
Attachment #8474418 - Flags: review?(echen)
Comment on attachment 8474418 [details] [diff] [review]
part 1b - readIMG and readIIDF (v2)

Thank you. :)
Attachment #8474418 - Flags: review?(echen) → review+
https://tbpl.mozilla.org/?tree=Try&rev=32f2ba4cf401 try green. Going to land the patches. Thanks for the review efforts, Edgar and Vicamo!
Blocks: 1061535
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
Status: RESOLVED → VERIFIED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.