Closed
Bug 952025
Opened 10 years ago
Closed 9 years ago
B2G RIL: rename EF_TYPE_TRANSPARENT|LINEAR_FIXED|CYCLIC
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(tracking-b2g:backlog)
RESOLVED
FIXED
tracking-b2g | backlog |
People
(Reporter: allstars.chh, Assigned: jdai)
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 13 obsolete files)
see https://bugzilla.mozilla.org/show_bug.cgi?id=947110#c27 LinearFixed, Cyclic, Transparent or BerTLV should be 'Structure of File', not 'Type of File'. 'Type of File' could be MF, DF, and EF.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → allstars.chh
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #8366560 -
Flags: review?(vyang)
Comment 2•10 years ago
|
||
Comment on attachment 8366560 [details] [diff] [review] Patch Review of attachment 8366560 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/ril_worker.js @@ +11230,5 @@ > options.callback = cb || options.callback; > RIL.iccIO(options); > } > > + options.type = EF_STRUCT_LINEAR_FIXED; Please also rename 'options.type' to 'options.structure'
Attachment #8366560 -
Flags: review?(vyang)
Updated•10 years ago
|
blocking-b2g: --- → backlog
Reporter | ||
Updated•9 years ago
|
Assignee: allstars.chh → nobody
Whiteboard: [good first bug]
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jdai
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Patch v1: rename option type to structure in ril_worker.js. Patch v2: rename option type to structure in test_ril_worker_icc_ICCIOHelper.js. update try server result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1685b750bc7
Attachment #8366560 -
Attachment is obsolete: true
Attachment #8539964 -
Attachment is obsolete: true
Attachment #8540654 -
Flags: review?(echen)
Comment 5•9 years ago
|
||
Comment on attachment 8540654 [details] [diff] [review] rename option type to structure, v2 Review of attachment 8540654 [details] [diff] [review]: ----------------------------------------------------------------- Please see my comments below. And you have to set the author information correct and add Bug Id in commit message for formal patch, see https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions. ::: dom/system/gonk/ril_worker.js @@ +1332,5 @@ > * @param options An object contains a valid value of > * RIL_PREFERRED_NETWORK_TYPE_TO_GECKO as its `type` attribute. > */ > setPreferredNetworkType: function(options) { > + let networkType = options.structure; Why we need to rename this to |structure|? @@ +6193,5 @@ > } > let Buf = this.context.Buf; > options.cid = Buf.readInt32().toString(); > options.active = Buf.readInt32(); // DATACALL_ACTIVE_* > + options.structure = Buf.readString(); Ditto @@ +6212,5 @@ > options.status = Buf.readInt32(); // DATACALL_FAIL_* > options.suggestedRetryTime = Buf.readInt32(); > options.cid = Buf.readInt32().toString(); > options.active = Buf.readInt32(); // DATACALL_ACTIVE_* > + options.structure = Buf.readString(); Ditto @@ +6294,5 @@ > this.sendChromeMessage(options); > return; > } > > + options.structure = this.context.Buf.readInt32List()[0]; Ditto
Attachment #8540654 -
Flags: review?(echen)
Assignee | ||
Comment 6•9 years ago
|
||
Part 1 v3: Cleanup unneeded rename code.
Attachment #8540654 -
Attachment is obsolete: true
Attachment #8541362 -
Flags: review?(echen)
Assignee | ||
Comment 7•9 years ago
|
||
Part 2 v3: Update the related testcases.
Attachment #8541364 -
Flags: review?(echen)
Assignee | ||
Comment 8•9 years ago
|
||
1. Cleanup unneeded code. 2. Add author information and Bug Id in commit message.
Attachment #8541362 -
Attachment is obsolete: true
Attachment #8541364 -
Attachment is obsolete: true
Attachment #8541362 -
Flags: review?(echen)
Attachment #8541364 -
Flags: review?(echen)
Attachment #8541370 -
Flags: review?(echen)
Assignee | ||
Comment 9•9 years ago
|
||
1.Update the related testcases. 2.Add author information and Bug Id in commit message.
Attachment #8541371 -
Flags: review?(echen)
Assignee | ||
Updated•9 years ago
|
Attachment #8541370 -
Attachment description: Refactor EF_TYPE_TRANSPARENT, LINEAR_FIXED, CYCLIC type of file to structure of filerefactor_to_correct_name.patch → Part1: Refactor EF_TYPE_TRANSPARENT, LINEAR_FIXED, CYCLIC type of file to structure of file(V4)
Assignee | ||
Updated•9 years ago
|
Attachment #8541370 -
Attachment description: Part1: Refactor EF_TYPE_TRANSPARENT, LINEAR_FIXED, CYCLIC type of file to structure of file(V4) → Part1: Refactor EF_TYPE_TRANSPARENT, LINEAR_FIXED, CYCLIC type of file to structure of file.(V4)
Assignee | ||
Updated•9 years ago
|
Attachment #8541371 -
Attachment description: Part 2 v4: Test case → Part 2: Test case.(V4)
Assignee | ||
Comment 10•9 years ago
|
||
Modify patch commit message.
Attachment #8541370 -
Attachment is obsolete: true
Attachment #8541371 -
Attachment is obsolete: true
Attachment #8541370 -
Flags: review?(echen)
Attachment #8541371 -
Flags: review?(echen)
Attachment #8541373 -
Flags: review?(echen)
Assignee | ||
Comment 11•9 years ago
|
||
Modify patch commit message.
Assignee | ||
Updated•9 years ago
|
Attachment #8541374 -
Attachment description: Part 2: Test case.(V5) → Part 2: Test case.(V2)
Attachment #8541374 -
Flags: review?(echen)
Comment 12•9 years ago
|
||
Comment on attachment 8541373 [details] [diff] [review] Part1: Refactor EF_TYPE_TRANSPARENT, LINEAR_FIXED, CYCLIC type of file to structure of file.(V5) Review of attachment 8541373 [details] [diff] [review]: ----------------------------------------------------------------- Please revise commit message to "... : Rename EF_TYPE_TRANSPARENT|LINEAR_FIXED|CYCLIC ...". Thank you. ::: dom/system/gonk/ril_worker.js @@ +12392,5 @@ > options.callback = cb || options.callback; > this.context.RIL.iccIO(options); > }).bind(this); > > + options.structure = EF_TYPE_LINEAR_FIXED; Please also help to rename EF_TYPE_* to EF_STRUCTURE_*.
Attachment #8541373 -
Flags: review?(echen)
Comment 13•9 years ago
|
||
Comment on attachment 8541374 [details] [diff] [review] Part 2: Test case.(V2) Review of attachment 8541374 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/tests/test_ril_worker_icc_ICCIOHelper.js @@ +122,5 @@ > } > buf.writeStringDelimiter(strLen); > > let options = {fileId: ICC_EF_ICCID, > + structure: EF_TYPE_TRANSPARENT}; Rename EF_TYPE_* to EF_STRUCTURE_*, thank you.
Attachment #8541374 -
Flags: review?(echen)
Assignee | ||
Comment 14•9 years ago
|
||
Rename EF_TYPE_* to EF_STRUCTURE_* to address comment 12.
Attachment #8541373 -
Attachment is obsolete: true
Attachment #8541374 -
Attachment is obsolete: true
Attachment #8541447 -
Flags: review?(echen)
Assignee | ||
Comment 15•9 years ago
|
||
Rename EF_TYPE_* to EF_STRUCTURE_* to address comment 13.
Attachment #8541449 -
Flags: review?(echen)
Comment 16•9 years ago
|
||
Comment on attachment 8541447 [details] [diff] [review] Part1: Refactor EF_TYPE_TRANSPARENT|LINEAR_FIXED|CYCLIC type of file to structure of file.(V6) Review of attachment 8541447 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment addressed. Thank you. We just rename the `type` to `structure` in this patch, so please help to revise the commit message a bit, s/Refactor/Rename/. ::: dom/system/gonk/ril_worker.js @@ +12544,5 @@ > // See TS 102 221 Table 11.4 for the content order of getResponse. > let iter = Iterator(berTlv.value); > let tlv = BerTlvHelper.searchForNextTag(BER_FCP_FILE_DESCRIPTOR_TAG, > iter); > + if (!tlv || (tlv.value.fileStructure !== UICC_EF_STRUCTURE[options.structure])) { nit: wrap long line into 80-char ruler and align. if (!tlv || (tlv.value.fileStructure !== UICC_EF_STRUCTURE[options.structure])) { @@ +12550,1 @@ > " but read " + tlv.value.fileStructure); Ditto throw new Error("Expected EF structure " + UICC_EF_STRUCTURE[options.structure] + " but read " + tlv.value.fileStructure); @@ +12608,5 @@ > > // Read Structure of EF, data[13] > + let efStructure = GsmPDUHelper.readHexOctet(); > + if (efStructure != options.structure) { > + throw new Error("Expected EF structure " + options.structure + " but read " + efStructure); Ditto throw new Error("Expected EF structure " + options.structure + " but read " + efStructure); @@ +12615,3 @@ > // Length of a record, data[14]. > // Only available for LINEAR_FIXED and CYCLIC. > + if (efStructure == EF_STRUCTURE_LINEAR_FIXED || efStructure == EF_STRUCTURE_CYCLIC) { Ditto if (efStructure == EF_STRUCTURE_LINEAR_FIXED || efStructure == EF_STRUCTURE_CYCLIC) {
Attachment #8541447 -
Flags: review?(echen) → review+
Comment 17•9 years ago
|
||
Comment on attachment 8541449 [details] [diff] [review] Part 2: Test case.(V3) Review of attachment 8541449 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thank you.
Attachment #8541449 -
Flags: review?(echen) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Update final patch of Part1 to address comment 16.
Attachment #8541447 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
Update final patch of Part1 to address comment 16.
Attachment #8541527 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8541528 -
Attachment description: Part1: Rename EF_TYPE_TRANSPARENT|LINEAR_FIXED|CYCLIC type of file to structure of file.(V8) → Part1: Rename EF_TYPE_TRANSPARENT|LINEAR_FIXED|CYCLIC type of file to structure of file.(V8), r=echen
Assignee | ||
Comment 20•9 years ago
|
||
Merge Part 1 and Part 2 into one patch.
Attachment #8541449 -
Attachment is obsolete: true
Attachment #8541528 -
Attachment is obsolete: true
Assignee | ||
Comment 21•9 years ago
|
||
TryServer: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3690917698a7
Comment 22•9 years ago
|
||
Thank you, John. https://hg.mozilla.org/integration/b2g-inbound/rev/0fa01129897c
Comment 23•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0fa01129897c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•