Closed Bug 875721 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(6 files, 14 obsolete files)

4.84 KB, patch
edgar
: review+
Details | Diff | Splinter Review
2.06 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1.71 KB, patch
edgar
: review+
Details | Diff | Splinter Review
3.66 KB, patch
edgar
: review+
Details | Diff | Splinter Review
14.50 KB, patch
edgar
: review+
Details | Diff | Splinter Review
4.85 KB, patch
edgar
: review+
Details | Diff | Splinter Review
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
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 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
Attachment #756394 - Flags: superreview?(jonas)
Attachment #756394 - Flags: review?(allstars.chh)
Blocks: 877978
Attachment #756394 - Flags: review?(allstars.chh) → review+
Assignee: nobody → echen
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
Attachment #756394 - Flags: superreview?(jonas) → superreview+
Add r=allstars.chh and sr=sicking after r+ and sr+.
Attachment #756394 - Attachment is obsolete: true
Attachment #763993 - Flags: superreview+
Attachment #763993 - Flags: review+
Rebase
Attachment #757344 - Attachment is obsolete: true
Attachment #763998 - Attachment description: Part 5: R → Part 5: RIL implementation changes for iccInfo, v2
Attachment #757346 - Attachment is obsolete: true
Attachment #757339 - Flags: review?(bugs)
Rebase
Attachment #757341 - Attachment is obsolete: true
Attachment #771914 - Flags: review?(kchen)
Rebase
Attachment #763996 - Attachment is obsolete: true
Attachment #771915 - Flags: review?(echou)
Rebase
Attachment #763998 - Attachment is obsolete: true
Attachment #771916 - Flags: review?(allstars.chh)
Attachment #757347 - Flags: review?(allstars.chh)
Attachment #771914 - Flags: review?(kchen) → review+
Attachment #771915 - Flags: review?(echou) → review+
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.
Attachment #757347 - Flags: review?(allstars.chh) → review+
Attachment #771916 - Flags: review?(allstars.chh) → review+
Attachment #757339 - Flags: review?(bugs) → review+
Add r=kchen after r+.
Attachment #771914 - Attachment is obsolete: true
Attachment #772489 - Flags: review+
Add r=echou after r+.
Attachment #771915 - Attachment is obsolete: true
Attachment #772492 - Flags: review+
Add r=allstars.chh after r+.
Attachment #771916 - Attachment is obsolete: true
Attachment #772493 - Flags: review+
1). Address review comment #16.
2). Add r=allstars.chh after r+.
Attachment #757347 - Attachment is obsolete: true
Attachment #772498 - Flags: review+
Rebase
Attachment #772493 - Attachment is obsolete: true
Attachment #774493 - Flags: review+
Attachment #774492 - Flags: superreview+
Attachment #774492 - Flags: review+
Bug 877978 is all r+, we can land patches of gecko and gaia together.
Keywords: checkin-needed
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?
Flags: needinfo?(echou)
Flags: needinfo?(echen)
followup: bug 893546.
Flags: needinfo?(echou)
Follow-up bug, bug 893546, is already landed.
Flags: needinfo?(echen)
Blocks: 921402
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?
Flags: needinfo?(echen)
(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.
Flags: needinfo?(echen)
Blocks: 946557
Blocks: 979808
You need to log in before you can comment on or make changes to this bug.