Closed
Bug 1052825
Opened 10 years ago
Closed 10 years ago
Use enums for lockType in MozIcc.webidl and nsIIccProvider.idl
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(feature-b2g:2.2+, tracking-b2g:backlog)
RESOLVED
FIXED
2.2 S2 (19dec)
People
(Reporter: anshulj, Assigned: edgar)
References
Details
(Whiteboard: [priority1])
Attachments
(5 files, 10 obsolete files)
(deleted),
patch
|
edgar
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
edgar
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
edgar
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
edgar
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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
Comment 1•10 years ago
|
||
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•10 years ago
|
Assignee: nobody → echen
Assignee | ||
Updated•10 years ago
|
Blocks: 864489, b2g-ril-interface
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Correct comment and indentation.
Attachment #8521319 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 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•10 years ago
|
||
Attachment #8521323 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
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+
Updated•10 years ago
|
blocking-b2g: --- → backlog
feature-b2g: --- → 2.2+
Updated•10 years ago
|
Whiteboard: [priority1]
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → 2.2 S1 (5dec)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8521948 -
Attachment is obsolete: true
Attachment #8530117 -
Flags: review+
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8521320 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8521961 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 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•10 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•10 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•10 years ago
|
Attachment #8530122 -
Flags: review?(htsai)
Assignee | ||
Comment 17•10 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 18•10 years ago
|
||
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•10 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•10 years ago
|
||
Attachment #8530120 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 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)
Comment 22•10 years ago
|
||
shifting target milestone to S2 as it still in the middle of review.
Target Milestone: 2.2 S1 (5dec) → 2.2 S2 (19dec)
Comment 23•10 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.)
Thanks, Olli! I will look at this when I do the review for other parts :)
Comment 24•10 years ago
|
||
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•10 years ago
|
||
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 26•10 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]:
-----------------------------------------------------------------
::: 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 27•10 years ago
|
||
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 28•10 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]:
-----------------------------------------------------------------
::: 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•10 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•10 years ago
|
||
Address review comment #18.
Attachment #8530118 -
Attachment is obsolete: true
Attachment #8536507 -
Flags: review+
Assignee | ||
Comment 31•10 years ago
|
||
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•10 years ago
|
||
Address review comment #26.
Attachment #8530121 -
Attachment is obsolete: true
Assignee | ||
Comment 33•10 years ago
|
||
Rebase and address review comment #27
Attachment #8530122 -
Attachment is obsolete: true
Attachment #8536518 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8536516 -
Flags: review+
Assignee | ||
Comment 34•10 years ago
|
||
Assignee | ||
Comment 35•10 years ago
|
||
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•10 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•