Use enums for lockType in MozIcc.webidl and nsIIccProvider.idl

RESOLVED FIXED in 2.2 S2 (19dec)

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Anshul, Assigned: edgar)

Tracking

unspecified
2.2 S2 (19dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+, tracking-b2g:backlog)

Details

(Whiteboard: [priority1])

Attachments

(5 attachments, 10 obsolete attachments)

15.53 KB, patch
edgar
: review+
Details | Diff | Splinter Review
11.32 KB, patch
edgar
: review+
Details | Diff | Splinter Review
39.06 KB, patch
edgar
: review+
Details | Diff | Splinter Review
11.76 KB, patch
edgar
: review+
Details | Diff | Splinter Review
6.29 KB, patch
edgar
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Just like it's done in bug 937485 I would like to request to use the enums for lock types for the following APIs in both the WebIDL and IPDL interface.

- getCardLock
- unlockCardLock
- setCardLock
- getCardLockRetryCount

Updated

4 years ago
Blocks: 1047203
Included in my https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/864489/icc-ipdl WIP branch for bug 864489. Maybe resolved in that bug, maybe move the related parts here to be more clear & simple. Let's see.
(Assignee)

Updated

4 years ago
Assignee: nobody → echen
(Assignee)

Updated

4 years ago
Blocks: 864489, 959978
(Assignee)

Updated

4 years ago
Depends on: 1083745
(Assignee)

Updated

4 years ago
Depends on: 1085307
(Assignee)

Comment 2

4 years ago
Created attachment 8521319 [details] [diff] [review]
Part 1: Interface changes, v1
(Assignee)

Comment 3

4 years ago
Created attachment 8521320 [details] [diff] [review]
Part 2: DOM changes, v1
(Assignee)

Comment 4

4 years ago
Created attachment 8521323 [details] [diff] [review]
Part 3: RIL changes, v1
(Assignee)

Comment 5

4 years ago
Created attachment 8521948 [details] [diff] [review]
Part 1: Interface changes, v2

Correct comment and indentation.
Attachment #8521319 - Attachment is obsolete: true
(Assignee)

Comment 6

4 years ago
Comment on attachment 8521948 [details] [diff] [review]
Part 1: Interface changes, v2

Review of attachment 8521948 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Hsinyi, may I have your review for the interface changes? Thank you.
Attachment #8521948 - Flags: review?(htsai)
(Assignee)

Comment 7

4 years ago
Created attachment 8521961 [details] [diff] [review]
Part 3: RIL changes, v2
Attachment #8521323 - Attachment is obsolete: true
Comment on attachment 8521948 [details] [diff] [review]
Part 1: Interface changes, v2

Review of attachment 8521948 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you :)
Attachment #8521948 - Flags: review?(htsai) → review+
(Assignee)

Updated

4 years ago
Blocks: 1101486
blocking-b2g: --- → backlog
feature-b2g: --- → 2.2+
Whiteboard: [priority1]
(Assignee)

Updated

4 years ago
Target Milestone: --- → 2.2 S1 (5dec)
(Assignee)

Comment 9

4 years ago
Created attachment 8530117 [details] [diff] [review]
Part 1: Interface changes, v3
Attachment #8521948 - Attachment is obsolete: true
Attachment #8530117 - Flags: review+
(Assignee)

Comment 10

4 years ago
Created attachment 8530118 [details] [diff] [review]
Part 2: DOM changes, v2
Attachment #8521320 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
Created attachment 8530120 [details] [diff] [review]
Part 3: RIL and test changes, v3
Attachment #8521961 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Created attachment 8530121 [details] [diff] [review]
Part 4: Add test in mobileconnection for unlocking puk via MMI, v1
(Assignee)

Comment 13

4 years ago
Created attachment 8530122 [details] [diff] [review]
Part 5: Add test in telephony for unlocking puk via MMI, v1
(Assignee)

Comment 14

4 years ago
Comment on attachment 8530118 [details] [diff] [review]
Part 2: DOM changes, v2

Review of attachment 8530118 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Olli,
In this bug, we would like to use WebIDL enum for lockType in MozIcc and also define some WebIDL dictionaries for the function argument.
For internal interface, in order to remove all jsval usage, so we simply spread the attributes. 
You can see part 1 (attachment #8530117 [details] [diff] [review]) for more detailed regarding to interface changes.

May I have your review for DOM changes? Thank you.
Attachment #8530118 - Flags: review?(bugs)
(Assignee)

Comment 15

4 years ago
Comment on attachment 8530120 [details] [diff] [review]
Part 3: RIL and test changes, v3

Review of attachment 8530120 [details] [diff] [review]:
-----------------------------------------------------------------

This part is for the ril changes and the test changes.
Regarding to the test changes, since we move lockType to WebIDL enum, the invalid type will trigger an exception in dom binding, test case need a revise.

Hi Hsinyi, may I have your review?
Attachment #8530120 - Flags: review?(htsai)
(Assignee)

Comment 16

4 years ago
Comment on attachment 8530121 [details] [diff] [review]
Part 4: Add test in mobileconnection for unlocking puk via MMI, v1

Review of attachment 8530121 [details] [diff] [review]:
-----------------------------------------------------------------

Add test for unlocking puk via MMI.
Attachment #8530121 - Flags: review?(htsai)
(Assignee)

Updated

4 years ago
Attachment #8530122 - Flags: review?(htsai)
(Assignee)

Comment 17

4 years ago
Comment on attachment 8530120 [details] [diff] [review]
Part 3: RIL and test changes, v3

Review of attachment 8530120 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I found test_ril_worker_icc_CardLock.js needs a revising as well, cancel review first.
Attachment #8530120 - Flags: review?(htsai)
Comment on attachment 8530118 [details] [diff] [review]
Part 2: DOM changes, v2

>+  if (aOptions.mEnabled.WasPassed()) {
>+    // Enable card lock.
>+    const nsString& password = (aOptions.mLockType == IccLockType::Fdn)
>+                             ? aOptions.mPin2 : aOptions.mPin;
Maybe put ? to the end of the previous line


Whoever reviews the other patches should look at this too, especially Icc::SetCardLock part.
We don't want to accidentally break the lock by passing some wrong params.
(and all this needs to be manually tested.)
Attachment #8530118 - Flags: review?(bugs) → review+
(Assignee)

Comment 19

4 years ago
(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 8530118 [details] [diff] [review]
> Part 2: DOM changes, v2
> 
> >+  if (aOptions.mEnabled.WasPassed()) {
> >+    // Enable card lock.
> >+    const nsString& password = (aOptions.mLockType == IccLockType::Fdn)
> >+                             ? aOptions.mPin2 : aOptions.mPin;
> Maybe put ? to the end of the previous line
> 
> 
> Whoever reviews the other patches should look at this too, especially
> Icc::SetCardLock part.
> We don't want to accidentally break the lock by passing some wrong params.
> (and all this needs to be manually tested.)

Thank for pointing this out.
Bug 1101486 is filed for unifying the argument set of card lock api, I guess it can help this situation.
(Assignee)

Comment 20

4 years ago
Created attachment 8534842 [details] [diff] [review]
Part 3: RIL and test changes, v4
Attachment #8530120 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Comment on attachment 8534842 [details] [diff] [review]
Part 3: RIL and test changes, v4

Review of attachment 8534842 [details] [diff] [review]:
-----------------------------------------------------------------

Address comment #17. I refactor test_ril_worker_icc_CardLock.js a bit and also add more test in it.
Attachment #8534842 - Flags: review?(htsai)
shifting target milestone to S2 as it still in the middle of review.
Target Milestone: 2.2 S1 (5dec) → 2.2 S2 (19dec)
(In reply to Olli Pettay [:smaug] from comment #18)
> Comment on attachment 8530118 [details] [diff] [review]
> Part 2: DOM changes, v2
> 
> >+  if (aOptions.mEnabled.WasPassed()) {
> >+    // Enable card lock.
> >+    const nsString& password = (aOptions.mLockType == IccLockType::Fdn)
> >+                             ? aOptions.mPin2 : aOptions.mPin;
> Maybe put ? to the end of the previous line
> 
> 
> Whoever reviews the other patches should look at this too, especially
> Icc::SetCardLock part.
> We don't want to accidentally break the lock by passing some wrong params.
> (and all this needs to be manually tested.)

Thanks, Olli! I will look at this when I do the review for other parts :)
Comment on attachment 8534842 [details] [diff] [review]
Part 3: RIL and test changes, v4

Review of attachment 8534842 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/tests/test_ril_worker_icc_ICCFileHelper.js
@@ +9,5 @@
> +
> +/**
> + * Verify ICCFileHelper.getEFPath
> + */
> +add_test(function test_path_id_for_spid_and_spn() {

hi Edgar,
Is this new file meant to be in this bug @@?
Comment on attachment 8534842 [details] [diff] [review]
Part 3: RIL and test changes, v4

Review of attachment 8534842 [details] [diff] [review]:
-----------------------------------------------------------------

Changes to xpcshell.ini and test_ril_worker_icc_ICCFileHelper.js seem be accidentally misplaced in this bug. r=me with those two removed. Please ask for review again if I misunderstood anything, thank you :)
Attachment #8534842 - Flags: review?(htsai) → review+
Comment on attachment 8530121 [details] [diff] [review]
Part 4: Add test in mobileconnection for unlocking puk via MMI, v1

Review of attachment 8530121 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobileconnection/tests/marionette/test_mobile_mmi_unlock_puk.js
@@ +64,5 @@
> +      is(aError.name, aErrorName, "Check name");
> +      is(aError.message, "", "Check message");
> +      is(aError.serviceCode, "scPuk", "Check service code");
> +      is(aError.additionalInformation, aRetryCount,
> +         "Chech additional information");

nit: s/Chech/Check/
Attachment #8530121 - Flags: review?(htsai) → review+
Comment on attachment 8530122 [details] [diff] [review]
Part 5: Add test in telephony for unlocking puk via MMI, v1

Review of attachment 8530122 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/telephony/test/marionette/test_mmi_unlock_puk.js
@@ +97,5 @@
> +      ok(!aResult.success, "check success");
> +      is(aResult.serviceCode, "scPuk", "Check service code");
> +      is(aResult.statusMessage, aErrorName, "Check statusMessage");
> +      is(aResult.additionalInformation, aRetryCount,
> +         "Chech additional information");

nit: s/Chech/Check/

@@ +142,5 @@
> +                                      "emMmiErrorMismatchPin"))
> +    // Test passing invalid puk (> 8 digit).
> +    .then(() => testUnlockPukMmiError("123456789", DEFAULT_PIN, DEFAULT_PIN,
> +                                      "emMmiErrorInvalidPin"))
> +    // Test passing incorrect pin.

Should s/pin/puk/
Attachment #8530122 - Flags: review?(htsai) → review+
Comment on attachment 8530121 [details] [diff] [review]
Part 4: Add test in mobileconnection for unlocking puk via MMI, v1

Review of attachment 8530121 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/mobileconnection/tests/marionette/test_mobile_mmi_unlock_puk.js
@@ +109,5 @@
> +                                      "emMmiErrorMismatchPin"))
> +    // Test passing invalid puk (> 8 digit).
> +    .then(() => testUnlockPukMmiError("123456789", DEFAULT_PIN, DEFAULT_PIN,
> +                                      "emMmiErrorInvalidPin"))
> +    // Test passing incorrect pin.

should s/pin/puk
(Assignee)

Comment 29

4 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #25)
> Comment on attachment 8534842 [details] [diff] [review]
> Part 3: RIL and test changes, v4
> 
> Review of attachment 8534842 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changes to xpcshell.ini and test_ril_worker_icc_ICCFileHelper.js seem be
> accidentally misplaced in this bug. r=me with those two removed. Please ask
> for review again if I misunderstood anything, thank you :)

Okay, I will revert the changes for test_ril_worker_icc_ICCFileHelper.js. Thanks for your review. :)
(Assignee)

Comment 30

4 years ago
Created attachment 8536507 [details] [diff] [review]
Part 2: DOM changes, v3, r=smaug

Address review comment #18.
Attachment #8530118 - Attachment is obsolete: true
Attachment #8536507 - Flags: review+
(Assignee)

Comment 31

4 years ago
Created attachment 8536513 [details] [diff] [review]
Part 3: RIL and test changes, v5, r=hsinyi

Address review comment #25.
- Revert the changes for test_ril_worker_icc_ICCFileHelper.js.
Attachment #8534842 - Attachment is obsolete: true
Attachment #8536513 - Flags: review+
(Assignee)

Comment 32

4 years ago
Created attachment 8536516 [details] [diff] [review]
Part 4: Add test in mobileconnection for unlocking puk via MMI, v2, r=hsinyi

Address review comment #26.
Attachment #8530121 - Attachment is obsolete: true
(Assignee)

Comment 33

4 years ago
Created attachment 8536518 [details] [diff] [review]
Part 5: Add test in telephony for unlocking puk via MMI, v2, r=hsinyi

Rebase and address review comment #27
Attachment #8530122 - Attachment is obsolete: true
Attachment #8536518 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8536516 - Flags: review+
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.