B2G RIL: rename EF_TYPE_TRANSPARENT|LINEAR_FIXED|CYCLIC

RESOLVED FIXED

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: allstars, Assigned: jdai)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 13 obsolete attachments)

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.
(Reporter)

Updated

4 years ago
Assignee: nobody → allstars.chh
(Reporter)

Updated

4 years ago
Attachment #8366560 - Flags: review?(vyang)
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

4 years ago
blocking-b2g: --- → backlog
(Reporter)

Updated

3 years ago
Assignee: allstars.chh → nobody
Whiteboard: [good first bug]
(Assignee)

Updated

3 years ago
Assignee: nobody → jdai
(Assignee)

Comment 3

3 years ago
Created attachment 8539964 [details] [diff] [review]
rename option type to structure, v1
(Assignee)

Comment 4

3 years ago
Created attachment 8540654 [details] [diff] [review]
rename option type to structure, v2

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

3 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

3 years ago
Created attachment 8541362 [details] [diff] [review]
Part 1 v3: Refactor EF_TYPE_TRANSPARENT, LINEAR_FIXED, CYCLIC type of file to structure of file

Part 1 v3: Cleanup unneeded rename code.
Attachment #8540654 - Attachment is obsolete: true
Attachment #8541362 - Flags: review?(echen)
(Assignee)

Comment 7

3 years ago
Created attachment 8541364 [details] [diff] [review]
Part 2 v3: Test case

Part 2 v3: Update the related testcases.
Attachment #8541364 - Flags: review?(echen)
(Assignee)

Comment 8

3 years ago
Created attachment 8541370 [details] [diff] [review]
Part1: Refactor EF_TYPE_TRANSPARENT, LINEAR_FIXED, CYCLIC type of file to structure of file.(V4)

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

3 years ago
Created attachment 8541371 [details] [diff] [review]
Part 2: Test case.(V4)

1.Update the related testcases.
2.Add author information and Bug Id in commit message.
Attachment #8541371 - Flags: review?(echen)
(Assignee)

Updated

3 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

3 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

3 years ago
Attachment #8541371 - Attachment description: Part 2 v4: Test case → Part 2: Test case.(V4)
(Assignee)

Comment 10

3 years ago
Created attachment 8541373 [details] [diff] [review]
Part1: Refactor EF_TYPE_TRANSPARENT, LINEAR_FIXED, CYCLIC type of file to structure of file.(V5)

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

3 years ago
Created attachment 8541374 [details] [diff] [review]
Part 2: Test case.(V2)

Modify patch commit message.
(Assignee)

Updated

3 years ago
Attachment #8541374 - Attachment description: Part 2: Test case.(V5) → Part 2: Test case.(V2)
Attachment #8541374 - Flags: review?(echen)

Comment 12

3 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

3 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

3 years ago
Created attachment 8541447 [details] [diff] [review]
Part1: Refactor EF_TYPE_TRANSPARENT|LINEAR_FIXED|CYCLIC type of file to structure of file.(V6)

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

3 years ago
Created attachment 8541449 [details] [diff] [review]
Part 2: Test case.(V3)

Rename EF_TYPE_* to EF_STRUCTURE_* to address comment 13.
Attachment #8541449 - Flags: review?(echen)

Comment 16

3 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

3 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

3 years ago
Created attachment 8541527 [details] [diff] [review]
Part1: Rename EF_TYPE_TRANSPARENT|LINEAR_FIXED|CYCLIC type of file to structure of file.(V7)

Update final patch of Part1 to address comment 16.
Attachment #8541447 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Created attachment 8541528 [details] [diff] [review]
Part1: Rename EF_TYPE_TRANSPARENT|LINEAR_FIXED|CYCLIC type of file to structure of file.(V8), r=echen

Update final patch of Part1 to address comment 16.
Attachment #8541527 - Attachment is obsolete: true
(Assignee)

Updated

3 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

3 years ago
Created attachment 8541534 [details] [diff] [review]
Rename EF_TYPE_TRANSPARENT|LINEAR_FIXED|CYCLIC type of file to structure of file. (final), r=echen

Merge Part 1 and Part 2 into one patch.
Attachment #8541528 - Attachment is obsolete: true
Attachment #8541449 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0fa01129897c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.