By Following 3GPP 51.104 clause 6.4.7, device should have capability to support SIM refresh for operator to update SIM related information via OTA.

Correct typo: 3GPP.51.014.

Update RIL patch first. Will update test case part later once xpcshell is ready to be used.

Comment on attachment 800061 [details] [diff] [review] Bug 912365 - Part 1: RIL implementation on support SIM refresh Review of attachment 800061 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +3918,5 @@ > > /** > + * Process ICC refresh. > + */ > + _processIccRefresh: function _processIccRefresh() { For some history reason, we use 'ICC' in worker, whereas in other file, we use Icc. @@ +3941,5 @@ > + if (!this._isCdma) { > + this._processSimRefreshFileUpdate(efId); > + } else { > + this._processRuimRefreshFileUpdate(efId); > + } We could add a method in ICCRecordHelper. @@ +3955,5 @@ > + // Simply fire setRadioPower({on: false}) here, RadioInterfaceLayer will > + // trigger setRadioEnabled(true) after receiving GECKO_RADIOSTATE_OFF > + // state. > + this.setRadioPower({on: false}); > + break; Forgot what I said last week, we need a marionette test for this.

Hi Yoshi: Thanks for your review/comment. I will add marionette test for SIM_RESET case. and xpcshell test cases after 912909/912442 landed. Thanks again!! sku

Comment on attachment 802884 [details] [diff] [review] Bug 912365 - Part 1: RIL implementation on support SIM refresh. v1 Review of attachment 802884 [details] [diff] [review]: ----------------------------------------------------------------- Finish Bug 915977 first.

Update reference spec. 3GPP TS 51.014 6.4.7 REFRESH ... - SIM Reset. This mode causes the ME to run the GSM session termination procedure and to deactivate the SIM in accordance with TS 51.011 [20]. Subsequently, the ME activates the SIM again and starts a new card session. In case of a 3 Volt technology ME, the ME shall restart the SIM with the same supply voltage as in the previous session, if the ME can ensure that the SIM has not been changed in between. Otherwise, the ME shall perform the supply voltage switching in accordance with TS 11.12 [21]. The ME shall not send the TERMINAL RESPONSE; this is an exception from the normal procedure, where TERMINAL RESPONSE is sent after completion of the command. The SIM Application shall interpret a new activation of the contacts of the SIM as an implicit TERMINAL RESPONSE. The SIM Reset mode is used when a SIM application requires ATR or complete SIM initialization procedures to be performed. SIM Applications should take into account that early implementations of SIM Application Toolkit in some MEs may send a TERMINAL RESPONSE after performing the REFRESH command involving resetting the SIM electrically.

Comment on attachment 806496 [details] [diff] [review] Bug 912365 Part 1: RIL implementation on support SIM refresh. Review of attachment 806496 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9428,5 @@ > + // Simply fire setRadioPower(false) here, RadioInterfaceLayer will > + // trigger setRadioEnabled(true) after receiving state - > + // GECKO_RADIOSTATE_OFF. > + RIL.setRadioPower({on: false}); > + break; I thought you tell me you will fix other cases as well, but you only fix RESET?

Hi Yoshi: For current design (w/o my patch), FILE_UPDTE and SIM_INIT cases are covered already. All we need is to make up SIM_RESET case. Definitely, we can make logic more precisely to simply re-read spcific EF with FILE_UPDTE case. Should I also cover this part in this bug? Thanks!! sku

Hi Yoshi, You are right, I miss RUIM case. Will make this part up, and request review then. Thanks!! sku.

Comment on attachment 806493 [details] [diff] [review] Bug 912365 Part 2: Add test cases for support SIM refresh. Review of attachment 806493 [details] [diff] [review]: ----------------------------------------------------------------- WIP, wait for whole patch ready, then request review.

Comment on attachment 810392 [details] [diff] [review] Bug 912365 Part 1: RIL implementation on enhance SIM refresh. Review of attachment 810392 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9462,5 @@ > processRefresh: function processRefresh(cmdDetails, ctlvs) { > let refreshType = cmdDetails.commandQualifier; > + if (DEBUG) { > + debug("ICC Refresh type = " + refreshType); > + } What about AID? @@ +9473,5 @@ > if (DEBUG) { > debug("Refresh, list = " + list); > } > + if (ctlv.value.numOfFile === 1) { > + this._handleFetchingICCRecord(list.slice(-4)); You need another function to parse entire fileList. @@ +9502,5 @@ > + * @param efId > + * the string value of ICC EF id for single file fetching, or undefined > + * for multiple files fetching. > + */ > + _handleFetchingICCRecord: function _handleFetchingICCRecord(efId) { This function should simply dispatch to gsm or cdma. @@ +11057,5 @@ > + default: > + this.fetchICCRecords(); > + } > + }, > + It seems to me we can merge fetchICCRecord and fetchICCRecords into one function. @@ +13030,5 @@ > let RuimRecordHelper = { > + /** > + * Fetch Ruim record with specified ICC file id. > + */ > + fetchRuimRecord: function fetchRuimRecord(efId) { also for fetchRuimRecord and fetchRuimRecords.

Hi Yoshi: Thank you so much for your time. I need your suggestion/comment, please check my inline reply. (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #19) > Comment on attachment 810392 [details] [diff] [review] > Bug 912365 Part 1: RIL implementation on enhance SIM refresh. > > Review of attachment 810392 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ +9462,5 @@ > > processRefresh: function processRefresh(cmdDetails, ctlvs) { > > let refreshType = cmdDetails.commandQualifier; > > + if (DEBUG) { > > + debug("ICC Refresh type = " + refreshType); > > + } > > What about AID? Will make this part up in next version. Thanks for pointing it out. > > @@ +9473,5 @@ > > if (DEBUG) { > > debug("Refresh, list = " + list); > > } > > + if (ctlv.value.numOfFile === 1) { > > + this._handleFetchingICCRecord(list.slice(-4)); > > You need another function to parse entire fileList. The original idea about this is: Yes, there might be multiple files in fileList. but I intent to only handle single file update. Reason: We will fetch single EF by efID, and we also need a fallback to make sure if we did not handle that EF reading properly. (Ex: we do not handle reading EF_MSISDN (0x6f40) in fetchRecord, but add it in fetchICCRecords in the future). W/o this fallback, we might lost precise information update. For multiple files case, there will be un-necessary fetchICCRecords triggered if any EF file is missed handling in fetchRecrod due to fallback. therefore, I intent to let multiple files update to the same path as SIM_INIT case. How about your idea? > > @@ +9502,5 @@ > > + * @param efId > > + * the string value of ICC EF id for single file fetching, or undefined > > + * for multiple files fetching. > > + */ > > + _handleFetchingICCRecord: function _handleFetchingICCRecord(efId) { > > This function should simply dispatch to gsm or cdma. I am not sure if I got your point about this comment. Could you please explain it with a little more detail? > > @@ +11057,5 @@ > > + default: > > + this.fetchICCRecords(); > > + } > > + }, > > + > > It seems to me we can merge fetchICCRecord and fetchICCRecords into one > function. Sure, will do. > > @@ +13030,5 @@ > > let RuimRecordHelper = { > > + /** > > + * Fetch Ruim record with specified ICC file id. > > + */ > > + fetchRuimRecord: function fetchRuimRecord(efId) { > > also for fetchRuimRecord and fetchRuimRecords. Sure, Will do. Thanks again!! sku

(In reply to shawn ku [:sku] from comment #20) > > > > > @@ +9473,5 @@ > > > if (DEBUG) { > > > debug("Refresh, list = " + list); > > > } > > > + if (ctlv.value.numOfFile === 1) { > > > + this._handleFetchingICCRecord(list.slice(-4)); > > > > You need another function to parse entire fileList. > > The original idea about this is: > Yes, there might be multiple files in fileList. but I intent to only handle > single file update. > Reason: > We will fetch single EF by efID, and we also need a fallback to make sure > if we did not handle that EF reading properly. (Ex: we do not handle reading > EF_MSISDN (0x6f40) in fetchRecord, but add it in fetchICCRecords in the > future). W/o this fallback, we might lost precise information update. > Why do we need a fallback at all? We only care about information we got from fetchICCRecords, (and possibly ADN, FDN). If the EFID is not included in fetchICCRecords, why do we need to read it? > > > > > @@ +9502,5 @@ > > > + * @param efId > > > + * the string value of ICC EF id for single file fetching, or undefined > > > + * for multiple files fetching. > > > + */ > > > + _handleFetchingICCRecord: function _handleFetchingICCRecord(efId) { > > > > This function should simply dispatch to gsm or cdma. > > I am not sure if I got your point about this comment. Could you please > explain it with a little more detail? > _handleFetchIccRecord: function (efId) { if (this._CDMA) { //cdma fetchRecords } else { // gsm fetchRecords } }

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #21) > (In reply to shawn ku [:sku] from comment #20) > > > > > > > > @@ +9473,5 @@ > > > > if (DEBUG) { > > > > debug("Refresh, list = " + list); > > > > } > > > > + if (ctlv.value.numOfFile === 1) { > > > > + this._handleFetchingICCRecord(list.slice(-4)); > > > > > > You need another function to parse entire fileList. > > > > The original idea about this is: > > Yes, there might be multiple files in fileList. but I intent to only handle > > single file update. > > Reason: > > We will fetch single EF by efID, and we also need a fallback to make sure > > if we did not handle that EF reading properly. (Ex: we do not handle reading > > EF_MSISDN (0x6f40) in fetchRecord, but add it in fetchICCRecords in the > > future). W/o this fallback, we might lost precise information update. > > > Why do we need a fallback at all? > > We only care about information we got from fetchICCRecords, (and possibly > ADN, FDN). > If the EFID is not included in fetchICCRecords, > why do we need to read it? > That's the interesting thing. Bascially, TS 31.102 Annex A defined a set of EFs can be updated via OTA then notify via refresh. We might add more and more EFs reading in fetchICCRecords (and fetchRuimRecords) in the future as long as gecko or gaia need it. Meanwhile, we can not guarantee that new added EF will be included in refresh handling. That's why fallback is needed. If you still confuse about my explaination, I give my apology, and will expalin it to you f2f. Thanks!! > > > > > > > > @@ +9502,5 @@ > > > > + * @param efId > > > > + * the string value of ICC EF id for single file fetching, or undefined > > > > + * for multiple files fetching. > > > > + */ > > > > + _handleFetchingICCRecord: function _handleFetchingICCRecord(efId) { > > > > > > This function should simply dispatch to gsm or cdma. > > > > I am not sure if I got your point about this comment. Could you please > > explain it with a little more detail? > > > _handleFetchIccRecord: function (efId) { > if (this._CDMA) { > //cdma fetchRecords > } else { > // gsm fetchRecords > } > } understood.

(In reply to shawn ku [:sku] from comment #22) > That's the interesting thing. > Bascially, TS 31.102 Annex A defined a set of EFs can be updated via OTA > then notify via refresh. > > We might add more and more EFs reading in fetchICCRecords (and > fetchRuimRecords) in the future as long as gecko or gaia need it. Meanwhile, > we can not guarantee that new added EF will be included in refresh handling. > > That's why fallback is needed. > Currently fetchICCRecords will fetch all the information it needs, so you need: 1. parse fileList 2. revise fetchICCRecords to accept an array of efId, so it either fetch all needed info, or fetch efIds in the array, and if the efId in the array are not useful to us, just ingore it.

Comment on attachment 810395 [details] [diff] [review] Bug 912365 Part 2: Add test cases for enhance SIM refresh. Review of attachment 810395 [details] [diff] [review]: ----------------------------------------------------------------- Cancelling r? for Part 1 will be revised first.

Comment on attachment 812930 [details] [diff] [review] Bug 912365 Part 1: RIL implementation on support SIM refresh. v3. Review of attachment 812930 [details] [diff] [review]: ----------------------------------------------------------------- I think you need to split it into different parts to make it simpler. Part 1: SIM_RESET Part 2: retrieveAID Part 3: util for parsing fileList Part 4: util for fetch ICC records Part 5: revise MSISDN and MBDN Part 6: SIM_FILE_CHANGE then tests. ::: dom/system/gonk/ril_consts.js @@ +508,5 @@ > this.ICC_EF_CFIS = 0x6fcb; > this.ICC_EF_SPDI = 0x6fcd; > > +// USIM/SIM mandatory EFs. > +this.ICC_3GPP_MANDATORY_EF_LIST = [ remove 3GPP @@ +511,5 @@ > +// USIM/SIM mandatory EFs. > +this.ICC_3GPP_MANDATORY_EF_LIST = [ > + ICC_EF_ICCID, > + ICC_EF_IMSI, > + ICC_EF_MSISDN, MSISDN is not mandatory @@ +514,5 @@ > + ICC_EF_IMSI, > + ICC_EF_MSISDN, > + ICC_EF_AD, > + ICC_EF_SST, > + ICC_EF_MBDN MBDN isn't either @@ +515,5 @@ > + ICC_EF_MSISDN, > + ICC_EF_AD, > + ICC_EF_SST, > + ICC_EF_MBDN > +]; So there should be another part of patch to move readMSISDN and readMBDN to readSST. ::: dom/system/gonk/ril_worker.js @@ +9465,5 @@ > processRefresh: function processRefresh(cmdDetails, ctlvs) { > let refreshType = cmdDetails.commandQualifier; > + if (DEBUG) { > + debug("ICC Refresh type = " + refreshType); > + } Remove this debug @@ +9470,5 @@ > + let ctlv = StkProactiveCmdHelper.searchForTag( > + COMPREHENSIONTLV_TAG_AID, ctlvs); > + if (ctlv) { > + let aid = ctlv.value.aid; > + if (aid && aid !== RIL.aid) { if (aid !== RIL.aid) @@ +9472,5 @@ > + if (ctlv) { > + let aid = ctlv.value.aid; > + if (aid && aid !== RIL.aid) { > + // This refresh is for different app. Ignore it. > + return; return null; @@ +9491,5 @@ > + for (let i = 0; i < tmpEfIds.length; i++) { > + if (tmpEfIds[i].length >= 4) { > + efIds.push(RIL.parseInt(tmpEfIds[i].slice(-4), -1, 16)); > + } > + } Move these to a helper function @@ +10282,5 @@ > + s += String.fromCharCode(Buf.readUint16()); > + } > + if (DEBUG) { > + debug("ICC Refresh aid : " + s); > + } remove debug mesg @@ +11082,5 @@ > + * others mean generic request. > + */ > + fetchICCRecords: function fetchICCRecords(efIds) { > + if (efIds) { > + for (let i = 0; i < efIds.length ; i++) { let len = efIds ? efIds.length : 0; for (let i = 0; i < len; i++) { .. @@ +11086,5 @@ > + for (let i = 0; i < efIds.length ; i++) { > + if (ICC_3GPP_SUPPORTED_EF_LIST.indexOf(efIds[i]) === -1) { > + if (DEBUG) { > + debug("efId : " + efIds[i] + " is not handled yet."); > + } remove debug @@ +11097,5 @@ > + } > + return; > + } > + > + for (let key in ICC_3GPP_MANDATORY_EF_LIST) { for (let i of @@ +11249,5 @@ > } > > // Fetch SPN and PLMN list, if some of them are available. > if (ICCUtilsHelper.isICCServiceAvailable("SPN")) { > + if ((ICC_3GPP_OPTIONAL_EF_LIST.indexOf(ICC_EF_SPN) !== -1) && Why this check is needed? @@ +12170,5 @@ > + this.readCBMID(); > +}; > +ICCRecordHelper[ICC_EF_CBMIR] = function ICC_EF_CBMIR() { > + this.readCBMIR(); > +}; I don't think it's a good idea to write like this, what about write?

(In reply to Yoshi Huang(OOO ~ 10/14)[:allstars.chh][:yoshi] from comment #33) > Comment on attachment 812930 [details] [diff] [review] > Bug 912365 Part 1: RIL implementation on support SIM refresh. v3. > > Review of attachment 812930 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think you need to split it into different parts to make it simpler. > > Part 1: SIM_RESET > Part 2: retrieveAID > Part 3: util for parsing fileList > Part 4: util for fetch ICC records > Part 5: revise MSISDN and MBDN > Part 6: SIM_FILE_CHANGE > then tests. Thanks, will check how to supply it in next patch. > > ::: dom/system/gonk/ril_consts.js > @@ +508,5 @@ > > this.ICC_EF_CFIS = 0x6fcb; > > this.ICC_EF_SPDI = 0x6fcd; > > > > +// USIM/SIM mandatory EFs. > > +this.ICC_3GPP_MANDATORY_EF_LIST = [ > > remove 3GPP The original purpose is to seperate USIM/SIM and RUIM. USIM/SIM follow 3GPP standard. RUIM follow 3GPP2 standard. > > @@ +511,5 @@ > > +// USIM/SIM mandatory EFs. > > +this.ICC_3GPP_MANDATORY_EF_LIST = [ > > + ICC_EF_ICCID, > > + ICC_EF_IMSI, > > + ICC_EF_MSISDN, > > MSISDN is not mandatory Yes, MSISDN is optional EF. > > @@ +514,5 @@ > > + ICC_EF_IMSI, > > + ICC_EF_MSISDN, > > + ICC_EF_AD, > > + ICC_EF_SST, > > + ICC_EF_MBDN > > MBDN isn't either Yes, MBDN is optional EF. > > @@ +515,5 @@ > > + ICC_EF_MSISDN, > > + ICC_EF_AD, > > + ICC_EF_SST, > > + ICC_EF_MBDN > > +]; > > So there should be another part of patch to move readMSISDN and readMBDN to > readSST. Yes, that was my next bug to move EFs to readSST if optional. But this bug should focus on SIM refresh frist, I guess. > > ::: dom/system/gonk/ril_worker.js > @@ +9465,5 @@ > > processRefresh: function processRefresh(cmdDetails, ctlvs) { > > let refreshType = cmdDetails.commandQualifier; > > + if (DEBUG) { > > + debug("ICC Refresh type = " + refreshType); > > + } > > Remove this debug Will do. > > @@ +9470,5 @@ > > + let ctlv = StkProactiveCmdHelper.searchForTag( > > + COMPREHENSIONTLV_TAG_AID, ctlvs); > > + if (ctlv) { > > + let aid = ctlv.value.aid; > > + if (aid && aid !== RIL.aid) { > > if (aid !== RIL.aid) Will do. > > @@ +9472,5 @@ > > + if (ctlv) { > > + let aid = ctlv.value.aid; > > + if (aid && aid !== RIL.aid) { > > + // This refresh is for different app. Ignore it. > > + return; > > return null; Will do. > > @@ +9491,5 @@ > > + for (let i = 0; i < tmpEfIds.length; i++) { > > + if (tmpEfIds[i].length >= 4) { > > + efIds.push(RIL.parseInt(tmpEfIds[i].slice(-4), -1, 16)); > > + } > > + } > > Move these to a helper function Will do. > > @@ +10282,5 @@ > > + s += String.fromCharCode(Buf.readUint16()); > > + } > > + if (DEBUG) { > > + debug("ICC Refresh aid : " + s); > > + } > > remove debug mesg Roger that. > > @@ +11082,5 @@ > > + * others mean generic request. > > + */ > > + fetchICCRecords: function fetchICCRecords(efIds) { > > + if (efIds) { > > + for (let i = 0; i < efIds.length ; i++) { > > let len = efIds ? efIds.length : 0; > for (let i = 0; i < len; i++) { > .. Will do. > > @@ +11086,5 @@ > > + for (let i = 0; i < efIds.length ; i++) { > > + if (ICC_3GPP_SUPPORTED_EF_LIST.indexOf(efIds[i]) === -1) { > > + if (DEBUG) { > > + debug("efId : " + efIds[i] + " is not handled yet."); > > + } > > remove debug Will do. > > @@ +11097,5 @@ > > + } > > + return; > > + } > > + > > + for (let key in ICC_3GPP_MANDATORY_EF_LIST) { > > for (let i of Will do. > > @@ +11249,5 @@ > > } > > > > // Fetch SPN and PLMN list, if some of them are available. > > if (ICCUtilsHelper.isICCServiceAvailable("SPN")) { > > + if ((ICC_3GPP_OPTIONAL_EF_LIST.indexOf(ICC_EF_SPN) !== -1) && > > Why this check is needed? This was for the purpose that we had a talk before. The intent is to keep sync. between new adding EF and ICC refresh. We need a mechanism to let ppl know that new added EF need to be synced to somewhere in the future. > > @@ +12170,5 @@ > > + this.readCBMID(); > > +}; > > +ICCRecordHelper[ICC_EF_CBMIR] = function ICC_EF_CBMIR() { > > + this.readCBMIR(); > > +}; > > I don't think it's a good idea to write like this, > what about write? Is there futher comment you try to tell, but missed here? Thanks!! sku

Comment on attachment 812933 [details] [diff] [review] Bug 912365 Part 2: Add test cases for enhance SIM refresh. v3. Review of attachment 812933 [details] [diff] [review]: ----------------------------------------------------------------- need to revise RIL patch first, cancel request first.

(In reply to shawn ku [:sku] from comment #34) > > > > ::: dom/system/gonk/ril_consts.js > > @@ +508,5 @@ > > > this.ICC_EF_CFIS = 0x6fcb; > > > this.ICC_EF_SPDI = 0x6fcd; > > > > > > +// USIM/SIM mandatory EFs. > > > +this.ICC_3GPP_MANDATORY_EF_LIST = [ > > > > remove 3GPP > > The original purpose is to seperate USIM/SIM and RUIM. > USIM/SIM follow 3GPP standard. > RUIM follow 3GPP2 standard. > The code in the RIL are implemented according to 3GPP spec, not none of them has the '3GPP' in the naming. As you see the ICC_EF_CFIS above doesn't have '3GPP' in it either. > > > > @@ +11249,5 @@ > > > } > > > > > > // Fetch SPN and PLMN list, if some of them are available. > > > if (ICCUtilsHelper.isICCServiceAvailable("SPN")) { > > > + if ((ICC_3GPP_OPTIONAL_EF_LIST.indexOf(ICC_EF_SPN) !== -1) && > > > > Why this check is needed? > > This was for the purpose that we had a talk before. > The intent is to keep sync. between new adding EF and ICC refresh. > > We need a mechanism to let ppl know that new added EF need to be synced to > somewhere in the future. > I don't think adding a if-block here could help people understand what you're trying to do, neither did I. There should be a better way to address this. > > > > @@ +12170,5 @@ > > > + this.readCBMID(); > > > +}; > > > +ICCRecordHelper[ICC_EF_CBMIR] = function ICC_EF_CBMIR() { > > > + this.readCBMIR(); > > > +}; > > > > I don't think it's a good idea to write like this, > > what about write? > > Is there futher comment you try to tell, but missed here? > No, I mean the code here needs to be rewritten again if we want to supprot writng(update) EF.

Comment on attachment 818208 [details] [diff] [review] Bug 912365 - B2g RIL: Enhance SIM Refresh. Part1: SIM_RESET. Review of attachment 818208 [details] [diff] [review]: ----------------------------------------------------------------- Add r=me Please update the title for this patch. Bug 912365 - Part 1: ... ::: dom/system/gonk/ril_worker.js @@ +9462,5 @@ > } > break; > + case STK_REFRESH_UICC_RESET: > + // See TS 51.014 section 6.4.7 REFRESH, SIM Reset. > + // Simply fire setRadioPower(false) here, RadioInterfaceLayer will Simple call

Comment on attachment 818210 [details] [diff] [review] Bug 912365 - B2g RIL: Enhance SIM Refresh. Part 2: retrieveAID. Review of attachment 818210 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9452,5 @@ > + COMPREHENSIONTLV_TAG_AID, ctlvs); > + if (ctlv) { > + let aid = ctlv.value.aid; > + // As per TS.102.223 6.4.7, aid is optional for ICC refresh. > + if (aid !== undefined && aid !== RIL.aid) { Can you explain why add aid !== undefined here? I've pointed this out before in Comment 33. @@ +9453,5 @@ > + if (ctlv) { > + let aid = ctlv.value.aid; > + // As per TS.102.223 6.4.7, aid is optional for ICC refresh. > + if (aid !== undefined && aid !== RIL.aid) { > + // This refresh is for different app. Ignore it. Actually we cannot ignore it. File a new bug and add a TODO here.

Comment on attachment 818214 [details] [diff] [review] Bug 912365 - B2g RIL: Enhance SIM Refresh. Part 4: util for fetch ICC records. Review of attachment 818214 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9492,5 @@ > + _handleFetchingICCRecords: function _handleFetchingICCRecords(efIds) { > + if (RIL._isCdma) { > + RuimRecordHelper.fetchRuimRecords(efIds); > + } else { > + ICCRecordHelper.fetchICCRecords(efIds); You modify the signature for two functions here, but put that change in another patch. Please put them back into this part.

Comment on attachment 818214 [details] [diff] [review] Bug 912365 - B2g RIL: Enhance SIM Refresh. Part 4: util for fetch ICC records. Review of attachment 818214 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9488,5 @@ > + * > + * @param efIds > + * The hex array of EF id for file fetching. > + */ > + _handleFetchingICCRecords: function _handleFetchingICCRecords(efIds) { remove ING.

Comment on attachment 818212 [details] [diff] [review] Bug 912365 - B2g RIL: Enhance SIM Refresh. Part 3: util for parsing fileList. Review of attachment 818212 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9483,5 @@ > return null; > }, > > /** > + * Helper function for parsing file list regardind ICC refresh. Helper function for parsing file list into an array of EF ids. @@ +9486,5 @@ > /** > + * Helper function for parsing file list regardind ICC refresh. > + * > + * @param list > + * The string array of EF id. An arry of full path for EFs. Add @return ... @@ +9488,5 @@ > + * > + * @param list > + * The string array of EF id. > + */ > + _parseFileListForICCRefresh: function _parseFileListForICCRefresh(list) { ForICCRefresh seems unneccesary to me @@ +9489,5 @@ > + * @param list > + * The string array of EF id. > + */ > + _parseFileListForICCRefresh: function _parseFileListForICCRefresh(list) { > + let efIds = []; convert to lower case here?

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #44) > Comment on attachment 818210 [details] [diff] [review] > Bug 912365 - B2g RIL: Enhance SIM Refresh. Part 2: retrieveAID. > > Review of attachment 818210 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ +9452,5 @@ > > + COMPREHENSIONTLV_TAG_AID, ctlvs); > > + if (ctlv) { > > + let aid = ctlv.value.aid; > > + // As per TS.102.223 6.4.7, aid is optional for ICC refresh. > > + if (aid !== undefined && aid !== RIL.aid) { > > Can you explain why add aid !== undefined here? > I've pointed this out before in Comment 33. For refresh case, aid is a optional tlv, If aid is present, that indicates the refresh with specific application. If aid is absent, that indicates the refresh with "current" application. That's why we can *not* ignore the check of "aid !== undefined" here. •TS 102 223 6.4.7 REFRESH if no AID is indicated, then the terminal shall assume the REFRESH applies to the NAA application currently selected on the basic logical channel (logical channel 0). If no NAA is currently selected on the basic logical channel, the terminal shall send a TERMINAL RESPONSE (command performed successfully). > > @@ +9453,5 @@ > > + if (ctlv) { > > + let aid = ctlv.value.aid; > > + // As per TS.102.223 6.4.7, aid is optional for ICC refresh. > > + if (aid !== undefined && aid !== RIL.aid) { > > + // This refresh is for different app. Ignore it. > > Actually we cannot ignore it. > File a new bug and add a TODO here. Since we have a RIL._isCdma to check which application is active, we might only interest in the specific application. That means if "aid from refresh" is not equal to RIL.aid, that means the refresh for that refresh is ignoreable. Please correct me if anything is wrong. Thanks!! sku

(In reply to shawn ku [:sku] from comment #48) > > @@ +9452,5 @@ > > > + COMPREHENSIONTLV_TAG_AID, ctlvs); > > > + if (ctlv) { You already check if there's AID here, isn't it? > > > + let aid = ctlv.value.aid; > > > + // As per TS.102.223 6.4.7, aid is optional for ICC refresh. > > > + if (aid !== undefined && aid !== RIL.aid) { Will it be possible we receive a AID Comprehensive TLV, but value is empty in the TLV? > > > > Can you explain why add aid !== undefined here? > > I've pointed this out before in Comment 33. > > For refresh case, aid is a optional tlv, > If aid is present, that indicates the refresh with specific application. > If aid is absent, that indicates the refresh with "current" application. > > That's why we can *not* ignore the check of "aid !== undefined" here. > > > > > @@ +9453,5 @@ > > > + if (ctlv) { > > > + let aid = ctlv.value.aid; > > > + // As per TS.102.223 6.4.7, aid is optional for ICC refresh. > > > + if (aid !== undefined && aid !== RIL.aid) { > > > + // This refresh is for different app. Ignore it. > > > > Actually we cannot ignore it. > > File a new bug and add a TODO here. > > Since we have a RIL._isCdma to check which application is active, we might > only interest in the specific application. > That means if "aid from refresh" is not equal to RIL.aid, that means the > refresh for that refresh is ignoreable. > A SIM could have two GSM Sim applications on it.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #49) > (In reply to shawn ku [:sku] from comment #48) > > > @@ +9452,5 @@ > > > > + COMPREHENSIONTLV_TAG_AID, ctlvs); > > > > + if (ctlv) { > You already check if there's AID here, isn't it? > yes, you are right, I should revise this part in next patch. > > > > + let aid = ctlv.value.aid; > > > > + // As per TS.102.223 6.4.7, aid is optional for ICC refresh. > > > > + if (aid !== undefined && aid !== RIL.aid) { > Will it be possible we receive a AID Comprehensive TLV, but value is empty > in the TLV? This shouldn't happen. > > > > > > > Can you explain why add aid !== undefined here? > > > I've pointed this out before in Comment 33. > > > > For refresh case, aid is a optional tlv, > > If aid is present, that indicates the refresh with specific application. > > If aid is absent, that indicates the refresh with "current" application. > > > > That's why we can *not* ignore the check of "aid !== undefined" here. > > > > > > > > > @@ +9453,5 @@ > > > > + if (ctlv) { > > > > + let aid = ctlv.value.aid; > > > > + // As per TS.102.223 6.4.7, aid is optional for ICC refresh. > > > > + if (aid !== undefined && aid !== RIL.aid) { > > > > + // This refresh is for different app. Ignore it. > > > > > > Actually we cannot ignore it. > > > File a new bug and add a TODO here. > > > > Since we have a RIL._isCdma to check which application is active, we might > > only interest in the specific application. > > That means if "aid from refresh" is not equal to RIL.aid, that means the > > refresh for that refresh is ignoreable. > > > A SIM could have two GSM Sim applications on it. I will also check how to make this part complete in next patch. Thanks again!! sku

Comment on attachment 818215 [details] [diff] [review] Bug 912365 - B2g RIL: Enhance SIM Refresh. Part 5: SIM_FILE_CHANGE. Review of attachment 818215 [details] [diff] [review]: ----------------------------------------------------------------- clear r?, revise the patch first before requesting r? again.

Comment on attachment 818884 [details] [diff] [review] Bug 912365 - B2g RIL: Enhance SIM Refresh. Part 6: test cases. Review of attachment 818884 [details] [diff] [review]: ----------------------------------------------------------------- clear r?, revise the patch first before requesting r? again.

Comment on attachment 820143 [details] [diff] [review] Bug 912365 - Part 2: retrieveAID. B2g RIL: Enhance SIM Refresh. Review of attachment 820143 [details] [diff] [review]: ----------------------------------------------------------------- Add r=me, Update the patch title. s/B2g/B2G/

Comment on attachment 820144 [details] [diff] [review] Bug 912365 - Part 3: util for parsing fileList. B2g RIL: Enhance SIM Refresh. Review of attachment 820144 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9486,5 @@ > + * Helper function for parsing file list into an array of EF ids. > + * > + * @param list > + * A string of full path for EFs, ex: 3f007fff6fc73f007fff6f46. > + * @return A hex or empty array of EFs, ex: [0x6fc7, 0x6f46]. An array of EFs. If it's of integer, we don't really care it's 0x0f or 15.

Comment on attachment 820145 [details] [diff] [review] Bug 912365 - Part 4: util for fetch ICC records. B2g RIL: Enhance SIM Refresh. Review of attachment 820145 [details] [diff] [review]: ----------------------------------------------------------------- It seems you're mixing Part 4/Part 5. ::: dom/system/gonk/ril_worker.js @@ +9487,5 @@ > + * > + * @param efIds > + * The hex array of EF id for file fetching. > + */ > + _handleFetchICCRecords: function _handleFetchICCRecords(efIds) { You didn't use efIds at all?

Comment on attachment 820145 [details] [diff] [review] Bug 912365 - Part 4: util for fetch ICC records. B2g RIL: Enhance SIM Refresh. Review of attachment 820145 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9487,5 @@ > + * > + * @param efIds > + * The hex array of EF id for file fetching. > + */ > + _handleFetchICCRecords: function _handleFetchICCRecords(efIds) { We don't have to prefix it with '_'

Comment on attachment 820146 [details] [diff] [review] Bug 912365 - Part 5: SIM_FILE_CHANGE. B2g RIL: Enhance SIM Refresh. Review of attachment 820146 [details] [diff] [review]: ----------------------------------------------------------------- Try to re-integrate Part 4 and Part 5, or try to merge them if you feel it's more trouble to split them ::: dom/system/gonk/ril_worker.js @@ +11071,5 @@ > * @param efIds [optional] > * Valid efIds mean the request from refresh with FILE_CHANGE type, > * others mean the rest of requests. > */ > fetchICCRecords: function fetchICCRecords(efIds) { Seems adding a boolean for reading mandatory fields is better. You forgot updating RuimRecordHelper?

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #63) > Comment on attachment 820146 [details] [diff] [review] > Bug 912365 - Part 5: SIM_FILE_CHANGE. B2g RIL: Enhance SIM Refresh. > > Review of attachment 820146 [details] [diff] [review]: > ----------------------------------------------------------------- > > Try to re-integrate Part 4 and Part 5, > or try to merge them if you feel it's more trouble to split them > > ::: dom/system/gonk/ril_worker.js > @@ +11071,5 @@ > > * @param efIds [optional] > > * Valid efIds mean the request from refresh with FILE_CHANGE type, > > * others mean the rest of requests. > > */ > > fetchICCRecords: function fetchICCRecords(efIds) { > > Seems adding a boolean for reading mandatory fields is better. Hi Yoshi: there are entries to request fetchICCRecords. 1. via _processICCStatus(), reading mandatroy EFs. 2. via handleFetchICCRecords with FILE_UPDATE case, reading dedicated EFs. 3. via handleFetchICCRecords with SIM_INIT case, reading mandatory EFs. If we add a boolean check for mandatory reading, yes it's doable, however, we still need to check all legacy code to make sure we all make this parts up since we change the signature of fetchICCRecords. Besides, for api caller, set parameter to true in most cases is not reasonble design to me. If we intend to put efIds as parameter, undefined is a convenient way to check if manadatory EFs are requested. What do your think? > You forgot updating RuimRecordHelper? It seems I missed merging RuimRecordHelper part although I left //:TODO on it. Thanks for addressing that.

(In reply to shawn ku [:sku] from comment #64) > there are entries to request fetchICCRecords. > 1. via _processICCStatus(), reading mandatroy EFs. > 2. via handleFetchICCRecords with FILE_UPDATE case, reading dedicated EFs. > 3. via handleFetchICCRecords with SIM_INIT case, reading mandatory EFs. > > If we add a boolean check for mandatory reading, yes it's doable, however, > we still need to check all legacy code to make sure we all make this parts > up since we change the signature of fetchICCRecords. > Besides, for api caller, set parameter to true in most cases is not > reasonble design to me. > > If we intend to put efIds as parameter, undefined is a convenient way to > check if manadatory EFs are requested. > Using undefined to retrieve mandatory files isn't clear. Try to seperate your functions to make them do one thing and do it well.

Comment on attachment 823209 [details] [diff] [review] Bug 912365 - Part 4: SIM_FILE_CHANGE. B2G RIL: Enhance SIM Refresh. Review of attachment 823209 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9495,5 @@ > + break; > + case STK_REFRESH_NAA_INIT_AND_FULL_FILE_CHANGE: > + case STK_REFRESH_NAA_INIT_AND_FILE_CHANGE: > + case STK_REFRESH_NAA_INIT: > + this.handleFetchICCRecords(); this.handleFetchICCRecords(ICC_MANDATORY_EF_LIST); @@ +11100,5 @@ > + this._handleEfReading(efIds); > + return; > + } > + > + this._handleEfReading(ICC_MANDATORY_EF_LIST); fetchICCRecords: function (efIds) { this._handleEfReading(efIds); }

Comment on attachment 823850 [details] [diff] [review] Bug 912365 - Part 4: SIM_FILE_CHANGE. B2G RIL: Enhance SIM Refresh. v4. Review of attachment 823850 [details] [diff] [review]: ----------------------------------------------------------------- Thank you

Comment on attachment 823851 [details] [diff] [review] Bug 912365 - Part 5: Test Cases. B2G RIL: Enhance SIM Refresh. v4. Review of attachment 823851 [details] [diff] [review]: ----------------------------------------------------------------- xpcshell-tests looks good to me. ::: dom/icc/tests/marionette/test_stk_refresh.js @@ +58,5 @@ > + connection.removeEventListener("voicechange", onvoicechange); > + > + is(connection.voice.state, "registered"); > + is(connection.voice.emergencyCallsOnly, false); > + is(connection.voice.roaming, false); Can you check Bug 856553 for radioState? or file another bug for this? Also I think we might need to reset to radio state back.

Comment on attachment 823850 [details] [diff] [review] Bug 912365 - Part 4: SIM_FILE_CHANGE. B2G RIL: Enhance SIM Refresh. v4. Review of attachment 823850 [details] [diff] [review]: ----------------------------------------------------------------- Thank you ::: dom/system/gonk/ril_worker.js @@ +3024,5 @@ > } else if (this.appType == CARD_APPTYPE_RUIM) { > if (RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD) { > this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE); > } > RuimRecordHelper.fetchRuimRecords(); Should be RuimRecordHelper.fetchRuimRecords(ICC_MANDATORY_EF_LIST) here

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #76) > Comment on attachment 823850 [details] [diff] [review] > Bug 912365 - Part 4: SIM_FILE_CHANGE. B2G RIL: Enhance SIM Refresh. v4. > > Review of attachment 823850 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thank you > > ::: dom/system/gonk/ril_worker.js > @@ +3024,5 @@ > > } else if (this.appType == CARD_APPTYPE_RUIM) { > > if (RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD) { > > this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE); > > } > > RuimRecordHelper.fetchRuimRecords(); > > Should be RuimRecordHelper.fetchRuimRecords(ICC_MANDATORY_EF_LIST) here Hi Yoshi: ICC_MANDATORY_EF_LIST is for SIM/USIM mandatory EFs. For RUIM part, I prefer to add a new list (create a new bug). What do you think? Thanks!! sku

Comment on attachment 823850 [details] [diff] [review] Bug 912365 - Part 4: SIM_FILE_CHANGE. B2G RIL: Enhance SIM Refresh. v4. Review of attachment 823850 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +9516,5 @@ > + * The hex array of EF id for file fetching. > + */ > + handleFetchICCRecords: function handleFetchICCRecords(efIds) { > + if (RIL._isCdma) { > + RuimRecordHelper.fetchRuimRecords(efIds); Here. @@ +13107,5 @@ > }, > }; > > let RuimRecordHelper = { > + fetchRuimRecords: function fetchRuimRecords(efIds) { But you already add efIds here, if you'd like to move it to other bug, remove efIds here.

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #78) > Comment on attachment 823850 [details] [diff] [review] > Bug 912365 - Part 4: SIM_FILE_CHANGE. B2G RIL: Enhance SIM Refresh. v4. > > Review of attachment 823850 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/ril_worker.js > @@ +9516,5 @@ > > + * The hex array of EF id for file fetching. > > + */ > > + handleFetchICCRecords: function handleFetchICCRecords(efIds) { > > + if (RIL._isCdma) { > > + RuimRecordHelper.fetchRuimRecords(efIds); > > Here. > > @@ +13107,5 @@ > > }, > > }; > > > > let RuimRecordHelper = { > > + fetchRuimRecords: function fetchRuimRecords(efIds) { > > But you already add efIds here, > if you'd like to move it to other bug, remove efIds here. Yes, I add below code. And this is for ICC refresh part. The efID list should be different to what suppose to do after RUIM ready. > + handleFetchICCRecords: function handleFetchICCRecords(efIds) { > + if (RIL._isCdma) { > + RuimRecordHelper.fetchRuimRecords(efIds); For better code understanding here, I will remove efIds in above code, and add TODO to remind it in new bug. Thanks!! sku

Hi Yoshi: for "Bug 912365 - Part 4: SIM_FILE_CHANGE. B2G RIL: Enhance SIM Refresh. v5. r=yoshi.", please help me double check if address parts are all updated properly. Thanks for your time. sku

Can you check Bug 814637 with Edgar? I think your code will conflict with his.

Comment on attachment 832072 [details] [diff] [review] Bug 912365 - Part 5: Test Cases. B2G RIL: Enhance SIM Refresh. v5. cancel r?, check Bug 814637 first.

Since there are so many changes (ex: Bug 814637) in m-c that might need this patch to be re-considered, and this is not a blocking issue, I will marked it as WONTFIX.

I think the work here is almost 80%~90% done, and can help existing RIL codebase. Reopen this bug, If Shawn doesn't have time to finish it, I can take over this bug.

Please this into backlog.

Firefox OS is not being worked on