Closed Bug 860585 Opened 7 years ago Closed 6 years ago

B2G RIL: Move cardLock related API from mozMobileConnection to mozIccManager

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: edgar, Assigned: edgar)

References

Details

(Keywords: dev-doc-needed, Whiteboard: RN6/7, fixed-in-birch)

Attachments

(5 files, 11 obsolete files)

19.52 KB, patch
edgar
: review+
Details | Diff | Splinter Review
7.57 KB, patch
edgar
: review+
Details | Diff | Splinter Review
7.30 KB, patch
edgar
: review+
Details | Diff | Splinter Review
3.72 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1.55 KB, patch
edgar
: review+
Details | Diff | Splinter Review
In bug 857414, we plan to move all icc/sim related stuff into mozIccManger. This bug is filed for cardLock related API.
- getCardLock()
- unlockCardLock()
- setCardLock()
- onicccardlockerror
Sorry, I don't get it. Why cann't we address cardLock related stuff in bug 857414? And, seems your WIP of bug 857414 already dealt with cardLock part. Why would we still need this issue?
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #2)
> See my comment in https://bugzilla.mozilla.org/show_bug.cgi?id=857414#c8

I see. Then, please give more explicit bug description to each separate issue. For example, description of bug 857414 is quite confusing because it says "Move *all* icc/sim..." Or make it meta if it sould be.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #2)
> > See my comment in https://bugzilla.mozilla.org/show_bug.cgi?id=857414#c8
> 
> I see. Then, please give more explicit bug description to each separate
> issue. For example, description of bug 857414 is quite confusing because it
> says "Move *all* icc/sim..." Or make it meta if it sould be.

I have marked bug 857414 as a meta bug, thanks.
Depends on: 860682
Blocks: 861081
Depends on: 861486
Rebase
Attachment #736673 - Attachment is obsolete: true
Comment on attachment 750286 [details] [diff] [review]
Part 1: Move cardLock API from mozMobileConnection to mozIccManager (IDL), v2

Hi Jonas/Yoshi:

I would like to request review for interface changes first though bug 859220 is not landed yet. Please see comment #0 for the list of changed API. And in this patch, I also change a event's naming from 'ICC' to 'Icc'. The latter is what we are using in other places now.

Thanks
Attachment #750286 - Flags: superreview?(jonas)
Attachment #750286 - Flags: review?(allstars.chh)
Comment on attachment 750286 [details] [diff] [review]
Part 1: Move cardLock API from mozMobileConnection to mozIccManager (IDL), v2

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

Mostly looks good,
but can you look the comments from Bug 772357 again to decide if we should address that ?

Thanks

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +273,5 @@
>     * 'stksessionend' event is notified whenever STK Session is terminated by
>     * ICC.
>     */
>    [implicit_jscontext] attribute jsval onstksessionend;
>  

// UICC Card Lock interfaces.

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +47,5 @@
>  
>    /**
> +   * Card lock interfaces.
> +   */
> +  nsIDOMDOMRequest getCardLock(in nsIDOMWindow window, in DOMString lockType);

Can you also check https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c2 
~ https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c4
Attachment #750286 - Flags: review?(allstars.chh) → review+
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #11)
> Comment on attachment 750286 [details] [diff] [review]
> Part 1: Move cardLock API from mozMobileConnection to mozIccManager (IDL), v2
> 
> Review of attachment 750286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly looks good,
> but can you look the comments from Bug 772357 again to decide if we should
> address that ?
> 
> Thanks
> 
> ::: dom/icc/interfaces/nsIDOMIccManager.idl
> @@ +273,5 @@
> >     * 'stksessionend' event is notified whenever STK Session is terminated by
> >     * ICC.
> >     */
> >    [implicit_jscontext] attribute jsval onstksessionend;
> >  
> 
> // UICC Card Lock interfaces.
> 
> ::: dom/icc/interfaces/nsIIccProvider.idl
> @@ +47,5 @@
> >  
> >    /**
> > +   * Card lock interfaces.
> > +   */
> > +  nsIDOMDOMRequest getCardLock(in nsIDOMWindow window, in DOMString lockType);
> 
> Can you also check https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c2 
> ~ https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c4

For error event part, I would like to deal with in a follow-up, bug 873380.
Thanks
(In reply to Edgar Chen [:edgar][:echen] from comment #12)
> > Can you also check https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c2 
> > ~ https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c4
> 
> For error event part, I would like to deal with in a follow-up, bug 873380.
> Thanks

Sorry for not being clear, I mean consider renamt it to 'getCardLockState'
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #13)
> (In reply to Edgar Chen [:edgar][:echen] from comment #12)
> > > Can you also check https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c2 
> > > ~ https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c4
> > 
> > For error event part, I would like to deal with in a follow-up, bug 873380.
> > Thanks
> 
> Sorry for not being clear, I mean consider renamt it to 'getCardLockState'

Oh, got it, I will address this here. Thanks.
Blocks: 873380
Address review comment #11
Attachment #750286 - Attachment is obsolete: true
Attachment #750286 - Flags: superreview?(jonas)
Attachment #752077 - Flags: superreview?(jonas)
Attachment #752077 - Flags: review+
1). Rebase
2). s/getCardLock/getCardLockState
Attachment #736674 - Attachment is obsolete: true
1). Rebase
2). Rename *GetCardLock to *GetCardLockState
Attachment #736675 - Attachment is obsolete: true
IccManager is using 'mobileconnection' permission
Attachment #736676 - Attachment is obsolete: true
We have renamed *GetCardLock to *GetCardLockState, xpcshell tests needs to change as well.
Comment on attachment 752103 [details] [diff] [review]
Part 2: Move cardLock API from mozMobileConnection to mozIccManager (DOM), v2

Hi :smaug,
This bug is to move card lock related API from mozMobileConnection to mozIccManager to have a clear API design. Please help to review the DOM changes when you are available.
Thanks in advance
Attachment #752103 - Flags: review?(bugs)
Attachment #752109 - Flags: review?(allstars.chh)
Attachment #752112 - Flags: review?(allstars.chh)
Attachment #752114 - Flags: review?(allstars.chh)
Comment on attachment 752103 [details] [diff] [review]
Part 2: Move cardLock API from mozMobileConnection to mozIccManager (DOM), v2

assuming the API change gets sr, r+
Attachment #752103 - Flags: review?(bugs) → review+
Comment on attachment 752112 [details] [diff] [review]
Part 4: Marionette tests for cardLock, v2

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

Please see Bug 756375 and duplicate it if you already done that in this bug.

::: dom/icc/tests/marionette/test_icc_card_lock.js
@@ +33,5 @@
> +  icc.addEventListener("icccardlockerror", function handler(result) {
> +    is(result.lockType, "pin");
> +    /* the default pin retries is 3, failed twice becomes to 1 */
> +    is(result.retryCount, 1);
> +

Try to make testPinChangeFailedNotification and testPinChangeFailed independant from each other,
so removing one of them or changing the execution order won't affect the tests.
Attachment #752112 - Flags: review?(allstars.chh)
Attachment #752109 - Flags: review?(allstars.chh) → review+
Attachment #752114 - Flags: review?(allstars.chh) → review+
Duplicate of this bug: 756375
Reset pin retries in testPinFailed* tests to make them indenpent from each other.
Attachment #752112 - Attachment is obsolete: true
Attachment #752602 - Flags: review?(allstars.chh)
Whiteboard: RN5/29
Whiteboard: RN5/29 → RN6/7
Attachment #752602 - Flags: review?(allstars.chh) → review+
Comment on attachment 752077 [details] [diff] [review]
Part 1: Move cardLock API from mozMobileConnection to mozIccManager (IDL), v3, r=allstars.chh

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

As long as the RIL owners are ok with this, I am.
Attachment #752077 - Flags: superreview?(jonas) → superreview+
1). Rebase
2). Add sr=sicking after sr+
Attachment #752077 - Attachment is obsolete: true
Attachment #761524 - Flags: superreview+
Attachment #761524 - Flags: review+
Add r=bugs after r+
Attachment #752103 - Attachment is obsolete: true
Attachment #761529 - Flags: review+
1). Rebase
2). Add r=allstars.chh after r+
Attachment #752109 - Attachment is obsolete: true
Attachment #761532 - Flags: review+
Add r=allstars.chh after r+
Attachment #752602 - Attachment is obsolete: true
Attachment #761534 - Flags: review+
Add r=allstars.chh after r+
Attachment #752114 - Attachment is obsolete: true
Attachment #761537 - Flags: review+
Gaia patches, bug 861081, is already r+. We could merge patches of Gaia and Gecko together.
Keywords: checkin-needed
Blocks: 979808
You need to log in before you can comment on or make changes to this bug.