If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla25

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

(Blocks: 1 bug)

Trunk
mozilla25
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: RN6/7, fixed-in-birch)

Attachments

(4 attachments, 7 obsolete attachments)

2.98 KB, patch
allstars
: review+
Details | Diff | Splinter Review
8.69 KB, patch
edgar
: review+
Details | Diff | Splinter Review
4.49 KB, patch
edgar
: review+
Details | Diff | Splinter Review
2.24 KB, patch
edgar
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 874769
(Assignee)

Comment 1

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

IDL Changes for card state related attribute/event
(Assignee)

Comment 2

5 years ago
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 :)
Attachment #752666 - Flags: superreview?(jonas)
Attachment #752666 - Flags: review?(allstars.chh)
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.
Attachment #752666 - Flags: superreview?(jonas)
Attachment #752666 - Flags: review?(allstars.chh)
(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.
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

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

Address comment #5
Attachment #752666 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #754322 - Flags: review?(allstars.chh)
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?
Attachment #754322 - Flags: review?(allstars.chh) → review+
(Assignee)

Comment 8

4 years ago
(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.
(Assignee)

Updated

4 years ago
Attachment #754322 - Flags: superreview?(jonas)

Updated

4 years ago
Whiteboard: RN5/29
(Assignee)

Comment 9

4 years ago
Created attachment 755736 [details] [diff] [review]
Part 2: Move card state related attribute/event from mozMobileConnection to mozIccManager (DOM), v1

Patch part2: DOM changes
(Assignee)

Comment 10

4 years ago
Created attachment 755738 [details] [diff] [review]
Part 3: Send cardstatechange event to IccManager, v1

Patch part3: RIL implementation changes.
(Assignee)

Comment 11

4 years ago
Created attachment 755748 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Depends on: 877530

Updated

4 years ago
Whiteboard: RN5/29 → RN6/7
(Assignee)

Updated

4 years ago
Depends on: 879666
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 :)
Attachment #754322 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 13

4 years ago
Created attachment 762553 [details] [diff] [review]
Part 3: Send cardstatechange event to IccManager, v2
Attachment #755738 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Created attachment 762564 [details] [diff] [review]
Part 4: Marionette tests for cardState, v2
Attachment #755748 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #755736 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Attachment #762553 - Flags: review?(allstars.chh)
(Assignee)

Updated

4 years ago
Attachment #762564 - Flags: review?(allstars.chh)
Attachment #762553 - Flags: review?(allstars.chh) → review+
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
Attachment #762564 - Flags: review?(allstars.chh) → review-
Attachment #755736 - Flags: review?(bugs) → review+
(Assignee)

Comment 16

4 years ago
Created attachment 763425 [details] [diff] [review]
Part 4: Marionette tests for cardState, v3

Add waitCardStateChangedEvent() to wait a specific cardState. After got the expected value, trigger a callback.
Attachment #762564 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #763425 - Flags: review?(allstars.chh)
Attachment #763425 - Flags: review?(allstars.chh) → review+
(Assignee)

Comment 17

4 years ago
Created attachment 764007 [details] [diff] [review]
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+.
Attachment #754322 - Attachment is obsolete: true
Attachment #764007 - Flags: superreview+
Attachment #764007 - Flags: review+
(Assignee)

Comment 18

4 years ago
Created attachment 764009 [details] [diff] [review]
Part 2: Move card state related attribute/event from mozMobileConnection to mozIccManager (DOM), v1, r=bugs

Add r=bugs after r+.
Attachment #755736 - Attachment is obsolete: true
Attachment #764009 - Flags: review+
(Assignee)

Comment 19

4 years ago
Try server:
https://tbpl.mozilla.org/?tree=Try&rev=a869d25d772a
https://tbpl.mozilla.org/?tree=Try&rev=6f2ee8e785d0
(Assignee)

Comment 20

4 years ago
Created attachment 771644 [details] [diff] [review]
Part 3: Send cardstatechange event to IccManager, v3, r=allstars.chh

Rebase
Attachment #762553 - Attachment is obsolete: true
Attachment #771644 - Flags: review+
(Assignee)

Comment 21

4 years ago
Bug 874769 is already r+, we can land patches of gecko and gaia together.
Keywords: checkin-needed
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
Keywords: checkin-needed
Whiteboard: RN6/7 → RN6/7, fixed-in-birch
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 979808
You need to log in before you can comment on or make changes to this bug.