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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Anshul]

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

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 2

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 1: Interface changes, v1

Comment 3

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 2: DOM changes, v1

Comment 4

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 3: RIL changes, v1

Comment 5

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 1: Interface changes, v2

Correct comment and indentation.

Comment 6

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

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.

Comment 7

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 3: RIL changes, v2

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

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

Thank you :)

Comment 9

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 1: Interface changes, v3

Comment 10

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 2: DOM changes, v2

Comment 11

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 3: RIL and test changes, v3

Comment 12

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 4: Add test in mobileconnection for unlocking puk via MMI, v1

Comment 13

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 5: Add test in telephony for unlocking puk via MMI, v1

Comment 14

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

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.

Comment 15

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

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?

Comment 16

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

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.

Comment 17

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

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.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 19

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

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

Comment 20

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 3: RIL and test changes, v4

Comment 21

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

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.

Comment 22

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

shifting target milestone to S2 as it still in the middle of review.

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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 25

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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 :)

Comment 26

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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/

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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/

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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

Comment 29

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

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

Comment 30

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 2: DOM changes, v3, r=smaug

Address review comment #18.

Comment 31

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 3: RIL and test changes, v5, r=hsinyi

Address review comment #25.
- Revert the changes for test_ril_worker_icc_ICCFileHelper.js.

Comment 32

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 4: Add test in mobileconnection for unlocking puk via MMI, v2, r=hsinyi

Address review comment #26.

Comment 33

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Part 5: Add test in telephony for unlocking puk via MMI, v2, r=hsinyi

Rebase and address review comment #27

Comment 34

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Try looks good: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dd033b04b286

Comment 35

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

https://hg.mozilla.org/integration/b2g-inbound/rev/4d28acaa9aac
https://hg.mozilla.org/integration/b2g-inbound/rev/a7c483766f60
https://hg.mozilla.org/integration/b2g-inbound/rev/100e9676dae8
https://hg.mozilla.org/integration/b2g-inbound/rev/e55b62072a39
https://hg.mozilla.org/integration/b2g-inbound/rev/3e9eae669e16

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d28acaa9aac
https://hg.mozilla.org/mozilla-central/rev/a7c483766f60
https://hg.mozilla.org/mozilla-central/rev/100e9676dae8
https://hg.mozilla.org/mozilla-central/rev/e55b62072a39
https://hg.mozilla.org/mozilla-central/rev/3e9eae669e16