The ME should reads the content of EF_PBR to determine the configuration phonebook. (File type, File identifiers, ....) Please check session 4.4.2.1 and 5.3.1 in TS 131.102.

I did some change in getPBR(). 1). store all entries of EF_PBR into iccInfoPrivate.PBR

Comment on attachment 692223 [details] [diff] [review] WIP, Read all entries of EF_PBR, v1 This is WIP patch without clean up. I did some change in getPBR(): 1). Store all entries of EF_PBR into iccInfoPrivate.PBR as below format iccInfoPrivate.PBR.'EF_ADN'[0] ---> {type, fid, sfi} [1] ---> {type, fid, sfi} iccInfoPrivate.PBR.'EF_EMAIL'[0] ---> {type, fid, sfi} [1] ---> {type, fid, sfi} 2). Add onSuccess callback which will be executed after got PBR result. This change makes getPBR() re-usable. (Writing contacts may need this method?) (Or maybe we can just use PBR result in iccInfoPrivate so that we don't need to do SIM access to get PBR every time.) Any suggest or comment is welcome :)

Comment on attachment 692223 [details] [diff] [review] WIP, Read all entries of EF_PBR, v1 Review of attachment 692223 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +2381,5 @@ > this.getADN(options); > break; > case CARD_APPTYPE_USIM: > + this.getPBR(options, > + function onSuccess(options) { try to move onSuccess into options @@ +2386,5 @@ > + // ** [start] for Debug > + debug("Edgar: getPBR: getADN"); > + // ** [end] > + let adnEfid = this.iccInfoPrivate.PBR[ICC_USIM_EFADN_TAG][0].fid; > + this.getADN({fileId: adnEfid, reuse options object @@ +2411,4 @@ > let bufLen = Buf.readUint32(); > > + // read all entries of EF_PBR and save them into this.iccInfoPrivate.PBR > + while (bufLen > 0) { I am not sure PBR will contain multi tagType. @@ +2424,5 @@ > + // or not: > + // - Length=2, Value:'FID (2 bytes)' > + // - Length=3, Value:'FID (2 bytes)','SFI (1 byte)' > + let item = {type: tagType, > + fid: (value[0] << 8) | value[1]}; For ICC IO we already use fileId, we should be consistent in that. @@ +2435,5 @@ > + // Please reference example in Annex G of TS 131.102 > + if (!this.iccInfoPrivate.PBR[tag]) { > + this.iccInfoPrivate.PBR[tag] = []; > + } > + this.iccInfoPrivate.PBR[tag].push(item); I don't understand here, seems this.iccInfoPrivate.PBR[tag] = item (or fileId); is enough @@ +2437,5 @@ > + this.iccInfoPrivate.PBR[tag] = []; > + } > + this.iccInfoPrivate.PBR[tag].push(item); > + } > + bufLen -= (4 + (length * 2)); (length + 2) * 2; // +2 for tagType and length

Comment on attachment 692223 [details] [diff] [review] WIP, Read all entries of EF_PBR, v1 Review of attachment 692223 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +2411,4 @@ > let bufLen = Buf.readUint32(); > > + // read all entries of EF_PBR and save them into this.iccInfoPrivate.PBR > + while (bufLen > 0) { please ignore this. @@ +2443,2 @@ > > Buf.readStringDelimiter(bufLen); bufLen should be 0 now, are you sure you want to do this?

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #4) > Comment on attachment 692223 [details] [diff] [review] > WIP, Read all entries of EF_PBR, v1 > > Review of attachment 692223 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ +2411,4 @@ > > let bufLen = Buf.readUint32(); > > > > + // read all entries of EF_PBR and save them into this.iccInfoPrivate.PBR > > + while (bufLen > 0) { > > please ignore this. I mean please ignore my previous comment, it's correct to use while here.

Comment on attachment 692223 [details] [diff] [review] WIP, Read all entries of EF_PBR, v1 I found it is failed to read EF_PBR with AT&T sim card. (It works well with Chunghwa Telecom sim card). I will update new patch later after I fix it and address review comment. Thanks.

(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #3) > Comment on attachment 692223 [details] [diff] [review] > WIP, Read all entries of EF_PBR, v1 > > Review of attachment 692223 [details] [diff] [review]: > ----------------------------------------------------------------- > @@ +2435,5 @@ > > + // Please reference example in Annex G of TS 131.102 > > + if (!this.iccInfoPrivate.PBR[tag]) { > > + this.iccInfoPrivate.PBR[tag] = []; > > + } > > + this.iccInfoPrivate.PBR[tag].push(item); > > I don't understand here, > seems this.iccInfoPrivate.PBR[tag] = item (or fileId); is enough EF_PBR may contain multiple fields with same tag value, but different file identifier. For example, AT&T SIM card seems support two ANR in phone book. The EF_PBR in AT&T SIM card has two EF_ANR tags with different file identifier. So use array to handle this. I will correct comment to make it more clear. Thanks.

1). Fix unable to read EF_PBR with AT&T sim card. (In AT&T SIM card, EF_PBR contains 'FF' at the end which are the unused bytes) 2). Move onsuccess into options 3). Add more comment. 4). Address review comment #3

1). use seekIncoming() to skip unused bytes. 2). move codes of parsing fid and sfi into decodeSimTlvs()

Comment on attachment 694323 [details] [diff] [review] WIP, Read all entries of EF_PBR, v3 Review of attachment 694323 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +2275,5 @@ > let simTlv = { > tag : GsmPDUHelper.readHexOctet(), > length : GsmPDUHelper.readHexOctet(), > }; > + simTlv.value = GsmPDUHelper.readHexOctetArray(simTlv.length); Try not to create an array if we don't have to. @@ +2281,5 @@ > + // The length value indicates if an SFI value is available for the EF > + // or not: > + // - Length=2, Value:'FID (2 bytes)' > + // - Length=3, Value:'FID (2 bytes)','SFI (1 byte)' > + simTlv.fileId = (simTlv.value[0] << 8) | simTlv.value[1]; do GsmPDUHelper.readHexOctet here @@ +2396,5 @@ > + debug("Edgar: getPBR: getADN"); > + // ** [end] > + let adnEfid = this.iccInfoPrivate.PBR[ICC_USIM_EFADN_TAG][0].fileId; > + this.getADN({fileId: adnEfid, > + requestId: options.requestId}); reuse options if possible. @@ +2425,5 @@ > + let bufLen = totalLen; > + while (bufLen > 0) { > + let tagType = GsmPDUHelper.readHexOctet(); > + > + if (tagType == 255) { 0xff @@ +2455,5 @@ > + // file identifier. > + if (!this.iccInfoPrivate.PBR[tag]) { > + this.iccInfoPrivate.PBR[tag] = []; > + } > + this.iccInfoPrivate.PBR[tag].push(item); reuse tlvs[i] so you don't have to create 'item' @@ +2468,5 @@ > + if (options.onsuccess) { > + // ** [start] for Debug > + debug("Edgar: getPBR: options.onsuccess"); > + // ** [end] > + options.onsuccess.call(this, options); keep in mind 'this' will change. We'll use more helpers in Bug 816893 @@ +2485,5 @@ > + if (options.onsuccess) { > + // ** [start] for Debug > + debug("Edgar: getPBR: options.onsuccess"); > + // ** [end] > + options.onsuccess.call(this, options); ditto @@ +2486,5 @@ > + // ** [start] for Debug > + debug("Edgar: getPBR: options.onsuccess"); > + // ** [end] > + options.onsuccess.call(this, options); > + } early return here. so you don't have to create an 'else' clause below

1). Address review comment #10 2). Read all phonebook set. The structure of iccInfoPrivate.PBR becomes an array as below: iccInfoPrivate.PBR[0] ---> phonebook set 1 iccInfoPrivate.PBR[1] ---> phoneboot set 2

rebase (based on bug 816893)

Comment on attachment 695618 [details] [diff] [review] Read all entries of EF_PBR, v5 Review of attachment 695618 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +8768,5 @@ > } > > function error(options) { > + // Delete iccInfoPrivate.PBR if any error occurred. > + delete this.iccInfoPrivate.PBR; Should be RIL.iccInfoPrivate.PBR

1). rename variable to make it easier to understand 2). use bind for onsuccess callback 3). add tagType in decodeSimTlvs 4). address comment #13

Oops! uploaded wrong file. Sorry for the mistake.

1). Remove function 'searchForIccUsimTag' which will not be used any more. 2). Add typeIndex which is used for EF_IAP. 3). Correct some comment.

I think it's better to rewrite those contact-related functions, so I'll write a helper to ICC contacts.

This patch reads the whole parcels of EF_PBR, and also I refactor ICC-Contact related code by adding ICCContactHelper.

Comment on attachment 702732 [details] [diff] [review] Part 1: readPBR and refactor ICCContact Review of attachment 702732 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9502,5 @@ > + let gotPbrCb = function gotPbrCb(pbr) { > + if (pbr.adn) { > + let fileId = pbr.adn.fileId; > + ICCRecordHelper.readADN(fileId, onsuccess, onerror); > + } I'll add else { onerror... } here.

Comment on attachment 702732 [details] [diff] [review] Part 1: readPBR and refactor ICCContact Review of attachment 702732 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +8855,5 @@ > + > + readLen += tlvLen + 2; // +2 for tag and tlvLen > + } > + Buf.readStringDelimiter(strLen); > + check pbrTlvs.length == 0 here

Comment on attachment 702732 [details] [diff] [review] Part 1: readPBR and refactor ICCContact Review of attachment 702732 [details] [diff] [review]: ----------------------------------------------------------------- Please take care of error/zero-length handling as your comment 20 and comment 21. Thank you :)

Addressed comments.

Comment on attachment 703748 [details] [diff] [review] Part 1: readPBR and refactor ICCContact v2 Review of attachment 703748 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +8860,5 @@ > + readLen += tlvLen + 2; // +2 for tag and tlvLen > + } > + Buf.readStringDelimiter(strLen); > + > + if (pbrTlvs.length == 0){ nit: put space between ) and {

nit addressed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/be757f2c314e https://hg.mozilla.org/integration/mozilla-inbound/rev/ca185542d941

https://hg.mozilla.org/mozilla-central/rev/be757f2c314e https://hg.mozilla.org/mozilla-central/rev/ca185542d941