875721 - B2G RIL: Move iccInfo 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 iccInfo related attribute/event:
- readonly attribute DOMString iccInfo; [2]
- [implicit_jscontext] attribute jsval oniccinfochange; [3] 

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

Comment 1

[Edgar Chen [:edgar]]

And in this bug, I would like to rename nsIDOMMozMobileICCInfo to nsIDOMMozIccInfo. Cause this interface only contains information related to icc, it seems more clear to me without 'Mobile'.

Comment 2

[Edgar Chen [:edgar]]

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

Comment 3

[Edgar Chen [:edgar]]

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

Hi Yoshi/Jonas:

I would like to request review for interface changes first.

Changes as below:
1. Move iccInfo from mozMobileConnection to mozIccManager.
2. Rename nsIDOMMozMobileICCInfo to nsIDOMMozIccInfo.

Thanks

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 2: Move iccInfo related attribute/event from mozMobileConnection to mozIccManager (DOM), v2

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 3: GPS changes for IccInfo, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 4: Bluetooth changes for iccInfo, v1

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL implementation changes for iccInfo, v1

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 6: Fix marionette tests for iccInfo, v1

Comment 9

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

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

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

I'll defer to the RIL owners here

Comment 10

[Edgar Chen [:edgar]]

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

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

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 4: Bluetooth changes for iccInfo, v2

Rebase

Comment 12

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL implementation changes for iccInfo, v2

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 3: GPS changes for IccInfo, v2

Rebase

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 4: Bluetooth changes for iccInfo, v3

Rebase

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL implementation changes for iccInfo, v3

Rebase

Comment 16

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

Comment on attachment 757347 [details] [diff] [review]
Part 6: Fix marionette tests for iccInfo, v1

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

::: dom/icc/tests/marionette/test_icc_info.js
@@ +11,1 @@
>  

I think we could do it as

let iccInfo = ...;
ok(iccInfo ....);

And remove all the 'icc.' prefixes below.

Comment 17

[Edgar Chen [:edgar]]

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

Rebase

Comment 18

[Edgar Chen [:edgar]]

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

Add r=bugs after r+.

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 3: GPS changes for IccInfo, v2, r=kchen

Add r=kchen after r+.

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 4: Bluetooth changes for iccInfo, v3, r=echou

Add r=echou after r+.

Comment 21

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL implementation changes for iccInfo, v3, r=allstars.chh

Add r=allstars.chh after r+.

Comment 22

[Edgar Chen [:edgar]]

Attachment: Part 6: Fix marionette tests for iccInfo, v2, r=allstars.chh

1). Address review comment #16.
2). Add r=allstars.chh after r+.

Comment 23

[Edgar Chen [:edgar]]

Try server:
https://tbpl.mozilla.org/?tree=Try&rev=c82aed1c103a
https://tbpl.mozilla.org/?tree=Try&rev=cf6f8852c2ee

Comment 24

[Edgar Chen [:edgar]]

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

Rebase

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 5: RIL implementation changes for iccInfo, v4, r=allstars.chh

Rebase

Comment 26

[Edgar Chen [:edgar]]

Bug 877978 is all r+, we can land patches of gecko and gaia together.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/309c74044e8d
https://hg.mozilla.org/projects/birch/rev/51e77c028e72
https://hg.mozilla.org/projects/birch/rev/b37a8382f554
https://hg.mozilla.org/projects/birch/rev/ca4652861e7d
https://hg.mozilla.org/projects/birch/rev/42e0eab9d677
https://hg.mozilla.org/projects/birch/rev/ccedb3285544

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/309c74044e8d
https://hg.mozilla.org/mozilla-central/rev/51e77c028e72
https://hg.mozilla.org/mozilla-central/rev/b37a8382f554
https://hg.mozilla.org/mozilla-central/rev/ca4652861e7d
https://hg.mozilla.org/mozilla-central/rev/42e0eab9d677
https://hg.mozilla.org/mozilla-central/rev/ccedb3285544

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 772492 [details] [diff] [review]
Part 4: Bluetooth changes for iccInfo, v3, r=echou

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

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +642,3 @@
>  
> +  nsIDOMMozIccInfo* iccInfo;
> +  icc->GetIccInfo(&iccInfo);

How is this not a leak?

Comment 30

[Eric Chou [:ericchou] [:echou]]

followup: bug 893546.

Comment 31

[Edgar Chen [:edgar]]

Follow-up bug, bug 893546, is already landed.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

When dom/network/tests/marionette/test_mobile_iccinfo.js was moved to dom/icc/tests/marionette/test_icc_info.js, it was never added to the manifest in dom/icc/tests/marionette. So this test hasn't been running in automation since. Was that on purpose?

Comment 33

[Edgar Chen [:edgar]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #32)
> When dom/network/tests/marionette/test_mobile_iccinfo.js was moved to
> dom/icc/tests/marionette/test_icc_info.js, it was never added to the
> manifest in dom/icc/tests/marionette. So this test hasn't been running in
> automation since. Was that on purpose?

Oh, it was a miss, I will file a bug for bringing the test back in automation.
Thank for catching this.