Closed Bug 816893 Opened 12 years ago Closed 12 years ago

B2G RIL: Add ICC IO Helpers

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(5 files, 29 obsolete files)

Patch to enable ADN in emulator
68 bytes, text/plain
vicamo
: review+
Details
Part 3: ICCUtilsHelper. v5 (former Part 5)
25.04 KB, patch
Details | Diff | Splinter Review
Part 4: Marionette tests for reading SIM contact. v4
2.94 KB, patch
Details | Diff | Splinter Review
Part 1: Refactor ICC IO. v10
79.23 KB, patch
Details | Diff | Splinter Review
Part 2: ICC IO error handling. v6
4.39 KB, patch
Details | Diff | Splinter Review
We could create Helpers to handling ICC IO related functionalities. And we should also refactor some helper functions scattered in ril_worker.
Assignee: nobody → allstars.chh
Status: NEW → ASSIGNED
Comment on attachment 687004 [details] [diff] [review] Part 1: Refactor ICC IO Review of attachment 687004 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1548,5 @@ > this.sendDOMMessage(options); > } > > this.iccInfo.adn = []; > + options.fileId = ICC_EF_ADN; I'll check this again
Attached patch Part 1: Refactor ICC IO. v2 (obsolete) — — Splinter Review
update fileId when calling getADN
Attachment #687004 - Attachment is obsolete: true
Attached patch Part 1: Refactor ICC IO. v3 (obsolete) — — Splinter Review
Attachment #687019 - Attachment is obsolete: true
Attachment #689654 - Attachment description: Part 2: ICC IO error handling. → Part 2: ICC IO error handling. v2
Attached patch Part 4: ICC Record Helper. v2 (obsolete) — — Splinter Review
Attachment #687007 - Attachment is obsolete: true
Attached patch Part 4: ICC Record Helper. v3 (obsolete) — — Splinter Review
fix typo
Attachment #689656 - Attachment is obsolete: true
Comment on attachment 689653 [details] [diff] [review] Part 1: Refactor ICC IO. v3 Review of attachment 689653 [details] [diff] [review]: ----------------------------------------------------------------- Great start! r=me but please address comments below, thanks! ::: dom/system/gonk/ril_worker.js @@ +1879,5 @@ > this.sendDOMMessage(options); > } > > this.iccInfo.adn = []; > + options.fileId = options.fileId; Let's just remove this redundancy. @@ +8460,5 @@ > return berTlv; > } > }; > > +let ICCFileHelper = { Please give an overall description of this helper. Thanks! @@ +8533,5 @@ > + * @return The pathId or null in case of an error or invalid input. > + */ > + getEFPath: function getEFPath(fileId) { > + // TODO: Bug 726098, change to use cdmaSubscriptionAppIndex when in CDMA. > + let index = RIL.iccStatus.gsmUmtsSubscriptionAppIndex; We probably want to add the following for completing error handling, right? if (index == -1) { return null; } @@ +8555,5 @@ > + } > + }, > +}; > + > +let ICCIOHelper = { Please give an overall description of this helper. Thanks!
Attachment #689653 - Flags: review?(htsai) → review+
Attachment #689654 - Flags: review?(htsai) → review+
Comment on attachment 689655 [details] [diff] [review] Part 3: remove loadAll and refactor parseDiallingNumber. v2 Review of attachment 689655 [details] [diff] [review]: ----------------------------------------------------------------- Please address comments below, thank you :) ::: dom/system/gonk/ril_worker.js @@ +1760,4 @@ > if (DEBUG) { > for (let i = 0; i < this.iccInfo.fdn.length; i++) { > debug("FDN[" + i + "] alphaId = " + this.iccInfo.fdn[i].alphaId + > + " number = " + this.iccInfo.fdn[i].number); nit: two spaces between " and + @@ +1765,5 @@ > } > + // To prevent DataCloneError when sending parcels, > + // We need to delete those properties which are not > + // 'Structured Clone Data', > + // in this case, those callback functions. Please wrap comments at 79 and be careful of grammar here. @@ +6442,5 @@ > + * succesfully. > + * @param finishCallback > + * The function should be invoked when the final ICC record is > + * processed. > + */ We don't have the latter two parameters for now. Please remove them from the comment.
Attachment #689655 - Flags: review?(htsai) → review+
Comment on attachment 689660 [details] [diff] [review] Part 4: ICC Record Helper. v3 Review of attachment 689660 [details] [diff] [review]: ----------------------------------------------------------------- Errors seem happen during refactoring. Please see my comments below! ::: dom/system/gonk/ril_worker.js @@ +8090,5 @@ > }; > ICCIOHelper[ICC_COMMAND_UPDATE_BINARY] = null; > ICCIOHelper[ICC_COMMAND_UPDATE_RECORD] = null; > > +let ICCRecordHelper = { Please give an overall description of this helper. Thank you! @@ +8127,5 @@ > + this: this}); > + }, > + > + /** > + * Read the ICCD from the ICC card. nit: s/ICC card/ICC @@ +8185,5 @@ > + debug("AD: " + str); > + } > + > + if (RIL.iccInfo.imsi) { > + // MCC is the first 3 digits of IMSI nit: please place a period at the end of a line. @@ +8187,5 @@ > + > + if (RIL.iccInfo.imsi) { > + // MCC is the first 3 digits of IMSI > + RIL.iccInfo.mcc = parseInt(RIL.iccInfo.imsi.substr(0,3)); > + // The 4th byte of the response is the length of MNC ditto. @@ +8206,5 @@ > + getSPN: function getSPN() { > + function callback() { > + let length = Buf.readUint32(); > + // Each octet is encoded into two chars. > + // Minus 1 because first is used to store display condition ditto. @@ +8235,5 @@ > + /** > + * Read the (U)SIM Service Table from the ICC. > + */ > + getSST: function getSST() { > + function callback() { This callback function seems wrong. Please check it again. Thank you. @@ +8626,5 @@ > + * @return An array of string corresponding to the PLMNs. > + */ > + readPLMNEntries: function readPLMNEntries(length) { > + let plmnList = []; > + // each PLMN entry has 3 byte nit: s/each/Each & s/byte/bytes.
Attachment #689660 - Flags: review?(htsai)
Comment on attachment 689657 [details] [diff] [review] Part 5: ICCUtilsHelper Review of attachment 689657 [details] [diff] [review]: ----------------------------------------------------------------- Good job, r=me but I still want to see my comment being addressed :) Also, we still need to revise related test cases such as test_ril_worker_icc.js before we can land your hard-working patches. ::: dom/system/gonk/ril_worker.js @@ +8535,5 @@ > }, > > +}; > + > +let ICCUtilsHelper = { Please give an overall description of this helper. Thanks!
Attachment #689657 - Flags: review?(htsai) → review+
Comment on attachment 689653 [details] [diff] [review] Part 1: Refactor ICC IO. v3 Review of attachment 689653 [details] [diff] [review]: ----------------------------------------------------------------- I don't just want a helper, I want everything specific, not only utility functions, but also data members, to ICC being moved to one new object. ::: dom/system/gonk/ril_worker.js @@ +1326,5 @@ > } > > + ICCIOHelper.loadTransparentEF({fileId: ICC_EF_PHASE, > + callback: callback, > + this: this}); 'this' is a keyword, you should never ever create an attribute named 'this'. You can, instead, |callback: callback.bind(this)|.
Attachment #689653 - Flags: review+ → review-
Assignee: allstars.chh → nobody
Attached patch Part 1: Refactor ICC IO. v4 (obsolete) — — Splinter Review
Addressed to Hsinyi's and Vicamo's comments.
Attachment #689653 - Attachment is obsolete: true
Attachment #690773 - Flags: review?(htsai)
Comment on attachment 689655 [details] [diff] [review] Part 3: remove loadAll and refactor parseDiallingNumber. v2 Review of attachment 689655 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +1760,4 @@ > if (DEBUG) { > for (let i = 0; i < this.iccInfo.fdn.length; i++) { > debug("FDN[" + i + "] alphaId = " + this.iccInfo.fdn[i].alphaId + > + " number = " + this.iccInfo.fdn[i].number); two spaces are intentional to make them align
Comment on attachment 690773 [details] [diff] [review] Part 1: Refactor ICC IO. v4 will update new version of this patch
Attachment #690773 - Flags: review?(htsai)
Assignee: nobody → allstars.chh
Attached patch Part 1: Refactor ICC IO. v5 (obsolete) — — Splinter Review
This patches merges former Part 3 and Part 4.
Attachment #689655 - Attachment is obsolete: true
Attachment #689660 - Attachment is obsolete: true
Attachment #690773 - Attachment is obsolete: true
Blocks: 821584
rebase
Attachment #692162 - Attachment is obsolete: true
rebase
Attachment #692163 - Attachment is obsolete: true
Comment on attachment 694706 [details] [diff] [review] Part 1: Refactor ICC IO. v6 (former Part 1, Part 3 and Part 4) Review of attachment 694706 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +8387,5 @@ > + }, > + > + /** > + * Load next record from current record Id. > + */ ws
remove ws.
Attachment #694706 - Attachment is obsolete: true
Attachment #694706 - Flags: review?(htsai)
Attachment #694723 - Flags: review?(htsai)
Comment on attachment 694723 [details] [diff] [review] Part 1: Refactor ICC IO. v7 (former Part 1, Part 3 and Part 4) Review of attachment 694723 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +6053,5 @@ > + let ffLen; // The length of trailing 0xff to be read. > + let length = Buf.readUint32(); > + > + let alphaLen = options.recordSize - MSISDN_FOOTER_SIZE_BYTES; > + let alphaId = GsmPDUHelper.readAlphaIdentifier(alphaLen); should be 'this' @@ +6055,5 @@ > + > + let alphaLen = options.recordSize - MSISDN_FOOTER_SIZE_BYTES; > + let alphaId = GsmPDUHelper.readAlphaIdentifier(alphaLen); > + > + let numLen = GsmPDUHelper.readHexOctet(); ditto @@ +6063,5 @@ > + return; > + } > + > + contact = {alphaId: alphaId, > + number: GsmPDUHelper.readDiallingNumber(numLen)}; ditto
Try run for 8fa354d9bc61 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=8fa354d9bc61 Results (out of 310 total builds): success: 285 warnings: 24 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-8fa354d9bc61
Comment on attachment 694709 [details] [diff] [review] Part 3: ICCUtilsHelper. v3 (former Part 5) Review of attachment 694709 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/test_ril_worker_icc.js @@ +190,5 @@ > run_next_test(); > }); > > /** > * Verify RIL.isICCServiceAvailable. Will update comments as well.
Comment on attachment 694710 [details] [diff] [review] Part 4: Marionette tests for reading SIM contact Review of attachment 694710 [details] [diff] [review]: ----------------------------------------------------------------- Great but please address my comments, thank you:) ::: dom/contacts/tests/marionette/test_sim_contacts.js @@ +4,5 @@ > +MARIONETTE_TIMEOUT = 30000; > + > +SpecialPowers.addPermission("contacts-read", true, document); > + > +let mozContacts = window.navigator.mozContacts; add 'ok(mozContacts instanceof MozContacts);' @@ +13,5 @@ > + let simContacts = request.result; > + > + is(simContacts.length, 4); > + > + is(simContacts[0].name, "Mozilla"); Could you please add a comment saying that these are the emulator's hard coded sim contacts. Thanks.
Attachment #694710 - Flags: review?(htsai) → review+
Comment on attachment 694723 [details] [diff] [review] Part 1: Refactor ICC IO. v7 (former Part 1, Part 3 and Part 4) Review of attachment 694723 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +8271,5 @@ > + return null; > + }, > + > + /** > + * This functions handles EFs for SIM. nit: s/functions/function @@ +8294,5 @@ > + } > + }, > + > + /** > + * This functions handles EFs for USIM. ditto. @@ +8648,5 @@ > + getSPN: function getSPN() { > + function callback() { > + let length = Buf.readUint32(); > + // Each octet is encoded into two chars. > + // Minus 1 because first is used to store display condition. s/because first is/because the first octet is/ @@ +8743,5 @@ > + * Get ICC FDN. > + * > + * @paran requestId > + * Request id from RadioInterfaceLayer. > + */ s/paran/param @@ +8762,5 @@ > + } > + } > + // To prevent DataCloneError when sending parcels, we need to delete > + // those properties which are not 'Structured Clone Data', in this case, > + // those callback functions. I don't think this comment is clear enough. Could you please make it more explicit? Thanks. @@ +8782,5 @@ > + * Get ICC ADN. > + * > + * @param fileId > + * EF id of the ADN. > + * @paran requestId s/paran/param @@ +8850,5 @@ > + > + /** > + * Get USIM Phonebook. > + * > + * @params requestId s/params/param @@ +9204,5 @@ > + // in format: > + // Byte 1: MCC[2] | MCC[1] > + // Byte 2: MNC[3] | MCC[3] > + // Byte 3: MNC[2] | MNC[1] > + // Therefore, we need to rearrage them. s/rearrage/rearrange
Attachment #694723 - Flags: review?(htsai) → review+
Comment on attachment 694709 [details] [diff] [review] Part 3: ICCUtilsHelper. v3 (former Part 5) Review of attachment 694709 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch and test case. Please make sure other ril xpcshell/marionette test cases, not only test_ril_worker_icc.js, aren't broken by this refactoring. ::: dom/system/gonk/ril_worker.js @@ +9081,5 @@ > }, > > +}; > + > +let ICCUtilsHelper = { Please give a general description of this helper function.
Attachment #694709 - Flags: review?(htsai) → review+
Addressed to comments.
Attachment #694723 - Attachment is obsolete: true
Attachment #695610 - Attachment is obsolete: true
rebase
Attachment #694707 - Attachment is obsolete: true
Addressed to comments.
Attachment #694709 - Attachment is obsolete: true
Addressed to comments.
Attachment #694710 - Attachment is obsolete: true
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #36) > Comment on attachment 694710 [details] [diff] [review] > Part 4: Marionette tests for reading SIM contact > > Review of attachment 694710 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great but please address my comments, thank you:) > > ::: dom/contacts/tests/marionette/test_sim_contacts.js > @@ +4,5 @@ > > +MARIONETTE_TIMEOUT = 30000; > > + > > +SpecialPowers.addPermission("contacts-read", true, document); > > + > > +let mozContacts = window.navigator.mozContacts; > > add 'ok(mozContacts instanceof MozContacts);' > should be just ok(mozContacts);
Blocks: 809725
rebase
Attachment #695611 - Attachment is obsolete: true
Try run for 53a02016c9df is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=53a02016c9df Results (out of 306 total builds): exception: 2 success: 287 warnings: 16 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-53a02016c9df
Comment on attachment 696918 [details] [diff] [review] Part 1: Refactor ICC IO. v9 (former Part 1, Part 3 and Part 4) Review of attachment 696918 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +8919,5 @@ > + function callback() { > + let length = Buf.readUint32(); > + let readLen = 0; > + let endLoop = false; > + this.iccInfoPrivate.SPDI = null; should be RIL. @@ +8930,5 @@ > + // The value part itself is a TLV. > + continue; > + case SPDI_TAG_PLMN_LIST: > + // This PLMN list is what we want. > + this.iccInfoPrivate.SPDI = this.readPLMNEntries(tlvLen / 3); ditto
fix marionette test fail when merging patch of Bug 814605.
Attachment #696918 - Attachment is obsolete: true
Comment on attachment 696919 [details] [diff] [review] Part 2: ICC IO error handling. v5 Review of attachment 696919 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +4408,1 @@ > options.onerror.call(this, options); should not use 'this'. @@ +4428,1 @@ > options.onerror.call(this, options); ditto
Don't use 'this' in error.
Attachment #696919 - Attachment is obsolete: true
Try run for 06dcc224dc43 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=06dcc224dc43 Results (out of 305 total builds): exception: 4 success: 284 warnings: 16 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-06dcc224dc43
Try run for 47c7089a6442 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=47c7089a6442 Results (out of 333 total builds): exception: 2 success: 315 warnings: 14 failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-47c7089a6442
Try run for 06dcc224dc43 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=06dcc224dc43 Results (out of 306 total builds): exception: 4 success: 284 warnings: 17 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-06dcc224dc43
Try run for 47c7089a6442 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=47c7089a6442 Results (out of 334 total builds): exception: 2 success: 315 warnings: 15 failure: 2 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-47c7089a6442
Try run for 53a02016c9df is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=53a02016c9df Results (out of 307 total builds): exception: 2 success: 287 warnings: 17 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-53a02016c9df
Try run for 53a02016c9df is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=53a02016c9df Results (out of 327 total builds): exception: 2 success: 296 warnings: 28 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/yhuang@mozilla.com-53a02016c9df
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: