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)

ARM
Gonk (Firefox OS)
defect
Not set
normal

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)

10.88 KB, patch
Details | Diff | Splinter Review
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.
Assignee: nobody → allstars.chh
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)
blocking-b2g: --- → backlog
Assignee: allstars.chh → nobody
Whiteboard: [good first bug]
Assignee: nobody → jdai
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 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)
Part 1 v3: Cleanup unneeded rename code.
Attachment #8540654 - Attachment is obsolete: true
Attachment #8541362 - Flags: review?(echen)
Attached patch Part 2 v3: Test case (obsolete) — Splinter Review
Part 2 v3: Update the related testcases.
Attachment #8541364 - Flags: review?(echen)
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)
Attached patch Part 2: Test case.(V4) (obsolete) — Splinter Review
1.Update the related testcases.
2.Add author information and Bug Id in commit message.
Attachment #8541371 - Flags: review?(echen)
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)
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)
Attachment #8541371 - Attachment description: Part 2 v4: Test case → Part 2: Test case.(V4)
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)
Attached patch Part 2: Test case.(V2) (obsolete) — Splinter Review
Modify patch commit message.
Attachment #8541374 - Attachment description: Part 2: Test case.(V5) → Part 2: Test case.(V2)
Attachment #8541374 - Flags: review?(echen)
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 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)
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)
Attached patch Part 2: Test case.(V3) (obsolete) — Splinter Review
Rename EF_TYPE_* to EF_STRUCTURE_* to address comment 13.
Attachment #8541449 - Flags: review?(echen)
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 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+
Update final patch of Part1 to address comment 16.
Attachment #8541447 - Attachment is obsolete: true
Update final patch of Part1 to address comment 16.
Attachment #8541527 - Attachment is obsolete: true
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
Merge Part 1 and Part 2 into one patch.
Attachment #8541449 - Attachment is obsolete: true
Attachment #8541528 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0fa01129897c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: