Closed
Bug 816893
Opened 12 years ago
Closed 11 years ago
B2G RIL: Add ICC IO Helpers
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: allstars.chh, Assigned: allstars.chh)
References
Details
Attachments
(5 files, 29 obsolete files)
68 bytes,
text/plain
|
vicamo
:
review+
|
Details |
25.04 KB,
patch
|
Details | Diff | Splinter Review | |
2.94 KB,
patch
|
Details | Diff | Splinter Review | |
79.23 KB,
patch
|
Details | Diff | Splinter Review | |
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 | ||
Updated•12 years ago
|
Assignee: nobody → allstars.chh
Status: NEW → ASSIGNED
Assignee | ||
Updated•12 years ago
|
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
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
Assignee | ||
Comment 7•12 years ago
|
||
update fileId when calling getADN
Attachment #687004 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #687019 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #687005 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #689654 -
Attachment description: Part 2: ICC IO error handling. → Part 2: ICC IO error handling. v2
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #687006 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #687007 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #689653 -
Flags: review?(htsai)
Assignee | ||
Updated•12 years ago
|
Attachment #689654 -
Flags: review?(htsai)
Assignee | ||
Updated•12 years ago
|
Attachment #689655 -
Flags: review?(htsai)
Assignee | ||
Updated•12 years ago
|
Attachment #689657 -
Flags: review?(htsai)
Assignee | ||
Updated•12 years ago
|
Attachment #689660 -
Flags: review?(htsai)
Comment 14•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #689654 -
Flags: review?(htsai) → review+
Comment 15•12 years ago
|
||
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 16•12 years ago
|
||
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 17•12 years ago
|
||
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 18•12 years ago
|
||
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 | ||
Updated•12 years ago
|
Assignee: allstars.chh → nobody
Assignee | ||
Comment 19•12 years ago
|
||
Addressed to Hsinyi's and Vicamo's comments.
Attachment #689653 -
Attachment is obsolete: true
Attachment #690773 -
Flags: review?(htsai)
Assignee | ||
Comment 20•12 years ago
|
||
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
Assignee | ||
Comment 21•12 years ago
|
||
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 | ||
Comment 22•12 years ago
|
||
Assignee: nobody → allstars.chh
Assignee | ||
Comment 23•12 years ago
|
||
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
Assignee | ||
Comment 24•12 years ago
|
||
Attachment #689654 -
Attachment is obsolete: true
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #689657 -
Attachment is obsolete: true
Assignee | ||
Comment 26•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #694706 -
Flags: review?(htsai)
Assignee | ||
Updated•12 years ago
|
Attachment #694710 -
Flags: review?(htsai)
Assignee | ||
Comment 31•12 years ago
|
||
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
Assignee | ||
Comment 32•12 years ago
|
||
remove ws.
Attachment #694706 -
Attachment is obsolete: true
Attachment #694706 -
Flags: review?(htsai)
Attachment #694723 -
Flags: review?(htsai)
Assignee | ||
Comment 33•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Attachment #694709 -
Flags: review?(htsai)
Comment 34•12 years ago
|
||
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
Assignee | ||
Comment 35•12 years ago
|
||
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 36•12 years ago
|
||
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 37•12 years ago
|
||
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 38•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #692145 -
Flags: review+
Assignee | ||
Comment 39•12 years ago
|
||
Addressed to comments.
Attachment #694723 -
Attachment is obsolete: true
Assignee | ||
Comment 40•12 years ago
|
||
rebase
Assignee | ||
Updated•12 years ago
|
Attachment #695610 -
Attachment is obsolete: true
Assignee | ||
Comment 42•12 years ago
|
||
Addressed to comments.
Attachment #694709 -
Attachment is obsolete: true
Assignee | ||
Comment 43•12 years ago
|
||
Addressed to comments.
Attachment #694710 -
Attachment is obsolete: true
Assignee | ||
Comment 44•12 years ago
|
||
(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);
Assignee | ||
Comment 45•12 years ago
|
||
Attachment #695613 -
Attachment is obsolete: true
Assignee | ||
Comment 46•11 years ago
|
||
rebase, update with Bug 824605
Attachment #695609 -
Attachment is obsolete: true
Comment 50•11 years ago
|
||
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
Assignee | ||
Comment 51•11 years ago
|
||
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
Assignee | ||
Comment 52•11 years ago
|
||
fix marionette test fail when merging patch of Bug 814605.
Attachment #696918 -
Attachment is obsolete: true
Assignee | ||
Comment 53•11 years ago
|
||
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
Assignee | ||
Comment 54•11 years ago
|
||
Don't use 'this' in error.
Attachment #696919 -
Attachment is obsolete: true
Comment 55•11 years ago
|
||
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
Comment 56•11 years ago
|
||
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
Assignee | ||
Comment 57•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd8b59d84902 https://hg.mozilla.org/integration/mozilla-inbound/rev/58615c1dbc7f https://hg.mozilla.org/integration/mozilla-inbound/rev/4804e200cc5a https://hg.mozilla.org/integration/mozilla-inbound/rev/8e186f3b8f25
Comment 58•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fd8b59d84902 https://hg.mozilla.org/mozilla-central/rev/58615c1dbc7f https://hg.mozilla.org/mozilla-central/rev/4804e200cc5a https://hg.mozilla.org/mozilla-central/rev/8e186f3b8f25
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 59•11 years ago
|
||
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
Comment 60•11 years ago
|
||
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
Comment 61•11 years ago
|
||
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
Comment 62•11 years ago
|
||
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.
Description
•