874744 - B2G RIL: Move card state related attribute/event from mozMobileConnection to mozIccManager

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Edgar Chen [:edgar]]

To have a more clear design for mozMobileConnection and mozIccManager, we plan to move all icc/sim-related stuff from mozMobileConnection to mozIccManager as RIL developers have discussed in [1].

This bug is filed for moving card state related attribute/event:
- readonly attribute DOMString cardState; [2]
- readonly attribute long retryCount; [3]
- [implicit_jscontext] attribute jsval oncardstatechange; [4] 

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=782603#c42
[2] https://mxr.mozilla.org/mozilla-central/source/dom/network/interfaces/nsIDOMMobileConnection.idl#50
[3] https://mxr.mozilla.org/mozilla-central/source/dom/network/interfaces/nsIDOMMobileConnection.idl#58
[4] https://mxr.mozilla.org/mozilla-central/source/dom/network/interfaces/nsIDOMMobileConnection.idl#372

Comment 1

[Edgar Chen [:edgar]]

Attachment: Part 1: Move card state related attribute/event from mozMobileConnection to mozIccManager (IDL), v1

IDL Changes for card state related attribute/event

Comment 2

[Edgar Chen [:edgar]]

Comment on attachment 752666 [details] [diff] [review]
Part 1: Move card state related attribute/event from mozMobileConnection to mozIccManager (IDL), v1

Hi Jonas/Yoshi:

I would like to request review for interface changes first.

Thanks :)

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 752666 [details] [diff] [review]
Part 1: Move card state related attribute/event from mozMobileConnection to mozIccManager (IDL), v1

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

I prefer we fix retryCount first, so we don't waste time on fixing wrong things.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #3)
> I prefer we fix retryCount first, so we don't waste time on fixing wrong
                                                               ^^^^^^^^^^^^^^  Sorry I didn't mean it's totally wrong, I mean it's incomplete.
> things.

Comment 5

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #4)
> (In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #3)
> > I prefer we fix retryCount first, so we don't waste time on fixing wrong
>                                                               
> ^^^^^^^^^^^^^^  Sorry I didn't mean it's totally wrong, I mean it's
> incomplete.
> > things.

Agree with you, let's focus on moving cardState and oncardstatechange in this bug first.
For retryCount, I will file a new bug and we can discuss there.
Thanks ,Yoshi.

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 1: Move card state related attribute/event from mozMobileConnection to mozIccManager (IDL), v2

Address comment #5

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 754322 [details] [diff] [review]
Part 1: Move card state related attribute/event from mozMobileConnection to mozIccManager (IDL), v2

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

::: dom/icc/interfaces/nsIDOMIccManager.idl
@@ +279,5 @@
> +
> +  /**
> +   * Indicates the state of the device's ICC card.
> +   *
> +   * Possible values: null, 'unknown', 'absent', 'pinRequired', 'pukRequired',

There should be another bug discussing about the null and 'absent' state in IccManager, right?

Comment 8

[Edgar Chen [:edgar]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #7)
> Comment on attachment 754322 [details] [diff] [review]
> Part 1: Move card state related attribute/event from mozMobileConnection to
> mozIccManager (IDL), v2
> 
> Review of attachment 754322 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/interfaces/nsIDOMIccManager.idl
> @@ +279,5 @@
> > +
> > +  /**
> > +   * Indicates the state of the device's ICC card.
> > +   *
> > +   * Possible values: null, 'unknown', 'absent', 'pinRequired', 'pukRequired',
> 
> There should be another bug discussing about the null and 'absent' state in
> IccManager, right?

Yes, the possible values will have some changes for Multi-SIM implementation. (Will be discussing in another bug).
Thanks for your reminding.

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 2: Move card state related attribute/event from mozMobileConnection to mozIccManager (DOM), v1

Patch part2: DOM changes

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 3: Send cardstatechange event to IccManager, v1

Patch part3: RIL implementation changes.

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 4: Marionette tests for cardState, v1

This patch is for marionette tests of cardState.
The idea is switching airplane mode and checking the cardState and cardstatechange event.

Current emulator doesn't support airplane mode well. For example, even we turn off radio, the voice/data status is still connected. We need to correct the behavior of emulator first. I already have patches for that, will file a new bug.

Comment 12

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

Comment on attachment 754322 [details] [diff] [review]
Part 1: Move card state related attribute/event from mozMobileConnection to mozIccManager (IDL), v2

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

As long as the RIL owners are happy with this, I am :)

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 3: Send cardstatechange event to IccManager, v2

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 4: Marionette tests for cardState, v2

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 762564 [details] [diff] [review]
Part 4: Marionette tests for cardState, v2

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

::: dom/icc/tests/marionette/test_icc_card_state.js
@@ +8,5 @@
> +
> +let icc = navigator.mozIccManager;
> +ok(icc instanceof MozIccManager, "icc is instanceof " + icc.constructor);
> +
> +function setAirplaneMode(enabled) {

Common pattern is 'setXXXEnabled'

@@ +16,5 @@
> +  let obj = {
> +    "ril.radio.disabled": enabled
> +  };
> +
> +  ok(true, "set airplane mode to " + enabled);

I think there should be some log function for this.

@@ +48,5 @@
> +        icc.removeEventListener("cardstatechange", oncardstatechange);
> +        cleanUp();
> +        break;
> +    }
> +  });

I wouldn't suggest this kind of tests,
adding any 'case' would possibly break this test.

Either
- A simpler one, test on some particular cardState.
- wring a simple state machine for it

Comment 16

[Edgar Chen [:edgar]]

Attachment: Part 4: Marionette tests for cardState, v3

Add waitCardStateChangedEvent() to wait a specific cardState. After got the expected value, trigger a callback.

Comment 17

[Edgar Chen [:edgar]]

Attachment: Part 1: Move card state related attribute/event from mozMobileConnection to mozIccManager (IDL), v2, r=allstars.chh, sr=sicking

Add r=allstars.chh and sr=sicking after r+ and sr+.

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 2: Move card state related attribute/event from mozMobileConnection to mozIccManager (DOM), v1, r=bugs

Add r=bugs after r+.

Comment 19

[Edgar Chen [:edgar]]

Try server:
https://tbpl.mozilla.org/?tree=Try&rev=a869d25d772a
https://tbpl.mozilla.org/?tree=Try&rev=6f2ee8e785d0

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 3: Send cardstatechange event to IccManager, v3, r=allstars.chh

Rebase

Comment 21

[Edgar Chen [:edgar]]

Bug 874769 is already r+, we can land patches of gecko and gaia together.

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

http://hg.mozilla.org/projects/birch/rev/1fe53dbab301
http://hg.mozilla.org/projects/birch/rev/6e7ddf90435c
http://hg.mozilla.org/projects/birch/rev/f71aed96ca80
http://hg.mozilla.org/projects/birch/rev/e6a6096fa5f6

Comment 23

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/1fe53dbab301
https://hg.mozilla.org/mozilla-central/rev/6e7ddf90435c
https://hg.mozilla.org/mozilla-central/rev/f71aed96ca80
https://hg.mozilla.org/mozilla-central/rev/e6a6096fa5f6