860585 - B2G RIL: Move cardLock related API from mozMobileConnection to mozIccManager

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Edgar Chen [:edgar] (PTO until 02/09)]

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

Comment 1

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

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?

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

See my comment in https://bugzilla.mozilla.org/show_bug.cgi?id=857414#c8

Comment 3

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

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

Comment 4

[Edgar Chen [:edgar] (PTO until 02/09)]

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

Comment 5

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 1: Move cardLock API from mozMobileConnection to mozIccManager (IDL), v1

Comment 6

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 2: Move cardLock API from mozMobileConnection to mozIccManager (DOM), v1

Comment 7

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 3: RIL implementation changes for cardLock, v1

Comment 8

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 4: Marionette tests for cardLock

Comment 9

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 1: Move cardLock API from mozMobileConnection to mozIccManager (IDL), v2

Rebase

Comment 10

[Edgar Chen [:edgar] (PTO until 02/09)]

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

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

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

Comment 12

[Edgar Chen [:edgar] (PTO until 02/09)]

(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

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

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

Comment 14

[Edgar Chen [:edgar] (PTO until 02/09)]

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

Comment 15

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 1: Move cardLock API from mozMobileConnection to mozIccManager (IDL), v3, r=allstars.chh

Address review comment #11

Comment 16

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 2: Move cardLock API from mozMobileConnection to mozIccManager (DOM), v2

1). Rebase
2). s/getCardLock/getCardLockState

Comment 17

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 3: RIL implementation changes for cardLock, v2

1). Rebase
2). Rename *GetCardLock to *GetCardLockState

Comment 18

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 4: Marionette tests for cardLock, v2

IccManager is using 'mobileconnection' permission

Comment 19

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 5: Fix xpcshell tests for cardlock, v1

We have renamed *GetCardLock to *GetCardLockState, xpcshell tests needs to change as well.

Comment 20

[Edgar Chen [:edgar] (PTO until 02/09)]

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

Comment 21

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

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+

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

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.

Comment 24

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 4: Marionette tests for cardLock, v3

Reset pin retries in testPinFailed* tests to make them indenpent from each other.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 26

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 1: Move cardLock API from mozMobileConnection to mozIccManager (IDL), v4, r=allstars.chh, sr=sicking

1). Rebase
2). Add sr=sicking after sr+

Comment 27

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 2: Move cardLock API from mozMobileConnection to mozIccManager (DOM), v2, r=bugs

Add r=bugs after r+

Comment 28

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 3: RIL implementation changes for cardLock, v3, r=allstars.chh

1). Rebase
2). Add r=allstars.chh after r+

Comment 29

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 4: Marionette tests for cardLock, v3, r=allstars.chh

Add r=allstars.chh after r+

Comment 30

[Edgar Chen [:edgar] (PTO until 02/09)]

Attachment: Part 5: Fix xpcshell tests for cardlock, v1, r=allstars.chh

Add r=allstars.chh after r+

Comment 31

[Edgar Chen [:edgar] (PTO until 02/09)]

try server:
https://tbpl.mozilla.org/?tree=Try&rev=cda8a6c3d280
https://tbpl.mozilla.org/?tree=Try&rev=1995fdcbf696

Comment 32

[Edgar Chen [:edgar] (PTO until 02/09)]

Gaia patches, bug 861081, is already r+. We could merge patches of Gaia and Gecko together.

Comment 33

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Feb.7]

http://hg.mozilla.org/projects/birch/rev/93841dcb2fd1
http://hg.mozilla.org/projects/birch/rev/022e181285dd
http://hg.mozilla.org/projects/birch/rev/8c02b3704429
http://hg.mozilla.org/projects/birch/rev/147fb303f74c
http://hg.mozilla.org/projects/birch/rev/8d498a65c6fe

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/93841dcb2fd1
https://hg.mozilla.org/mozilla-central/rev/022e181285dd
https://hg.mozilla.org/mozilla-central/rev/8c02b3704429
https://hg.mozilla.org/mozilla-central/rev/147fb303f74c
https://hg.mozilla.org/mozilla-central/rev/8d498a65c6fe

Comment 35

[Kohei Yoshino]

Documents need to be updated.
https://developer.mozilla.org/en-US/docs/Web/API/MozMobileConnection