Closed Bug 1052825 Opened 6 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
2.2 S2 (19dec)
feature-b2g 2.2+
tracking-b2g backlog

People

(Reporter: anshulj, Assigned: edgar)

References

Details

(Whiteboard: [priority1])

Attachments

(5 files, 10 obsolete files)

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
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
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: nobody → echen
Depends on: 1083745
Depends on: 1085307
Attached patch Part 1: Interface changes, v1 (obsolete) — Splinter Review
Attached patch Part 2: DOM changes, v1 (obsolete) — Splinter Review
Attached patch Part 3: RIL changes, v1 (obsolete) — Splinter Review
Attached patch Part 1: Interface changes, v2 (obsolete) — Splinter Review
Correct comment and indentation.
Attachment #8521319 - Attachment is obsolete: true
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)
Attached patch Part 3: RIL changes, v2 (obsolete) — Splinter Review
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+
Blocks: 1101486
blocking-b2g: --- → backlog
feature-b2g: --- → 2.2+
Whiteboard: [priority1]
Target Milestone: --- → 2.2 S1 (5dec)
Attachment #8521948 - Attachment is obsolete: true
Attachment #8530117 - Flags: review+
Attached patch Part 2: DOM changes, v2 (obsolete) — Splinter Review
Attachment #8521320 - Attachment is obsolete: true
Attached patch Part 3: RIL and test changes, v3 (obsolete) — Splinter Review
Attachment #8521961 - Attachment is obsolete: true
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)
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)
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)
Attachment #8530122 - Flags: review?(htsai)
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+
(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.
Attached patch Part 3: RIL and test changes, v4 (obsolete) — Splinter Review
Attachment #8530120 - Attachment is obsolete: true
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
(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. :)
Address review comment #18.
Attachment #8530118 - Attachment is obsolete: true
Attachment #8536507 - Flags: review+
Address review comment #25.
- Revert the changes for test_ril_worker_icc_ICCFileHelper.js.
Attachment #8534842 - Attachment is obsolete: true
Attachment #8536513 - Flags: review+
Rebase and address review comment #27
Attachment #8530122 - Attachment is obsolete: true
Attachment #8536518 - Flags: review+
Attachment #8536516 - Flags: review+
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.