Clean up MOZ_B2G_RIL in NetworkManager

RESOLVED FIXED in 2.2 S3 (9jan)

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: edgar, Assigned: jessica)

Tracking

unspecified
2.2 S3 (9jan)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
I really don't like having a lot of MOZ_B2G_RIL in NetworkManager.

For the platform without RIL function, there will be no NetworkInterface with MOBILE type registered. It seems most of MOZ_B2G_RIL can be replaced by checking network type.

Let's try to clean up this.
(Reporter)

Updated

4 years ago
Blocks: 904514
(Reporter)

Comment 1

3 years ago
Created attachment 8534864 [details] [diff] [review]
WIP, patch

Just a WIP patch, didn't verify it yet.
(Assignee)

Comment 2

3 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #1)
> Created attachment 8534864 [details] [diff] [review]
> WIP, patch
> 
> Just a WIP patch, didn't verify it yet.

Thanks Edgar! Should I take it from here? :)
(Reporter)

Comment 3

3 years ago
Yes, please help me to finish it if you don't mind. :)
(Assignee)

Updated

3 years ago
Assignee: nobody → jjong
(Assignee)

Comment 4

3 years ago
Created attachment 8538297 [details] [diff] [review]
patch, v1.
Attachment #8534864 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Comment on attachment 8538297 [details] [diff] [review]
patch, v1.

Edgar, may I have your review on this? Thanks.
Attachment #8538297 - Flags: review?(echen)
(Reporter)

Comment 6

3 years ago
Comment on attachment 8538297 [details] [diff] [review]
patch, v1.

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

Thanks for taking this bug, please see my comments below.

::: dom/system/gonk/NetworkManager.js
@@ +340,2 @@
>          // Update data connection when Wifi connected/disconnected
> +        try {

The try-catch is for the exception thrown by Cc["@mozilla.org/foo;1"].getService(Ci.nsIFoo);
Per offline discussion, let's use `defineLazyGetter` and we could handle the exception there. Something like,

XPCOMUtils.defineLazyGetter(NetworkManager.prototype, "mRil", function() {
  try {
    return Cc["@mozilla.org/ril;1"].getService(Ci.nsIRadioInterfaceLayer);
  } catch (e) {}

  return null;
});

@@ +340,3 @@
>          // Update data connection when Wifi connected/disconnected
> +        try {
> +          if (network.type == Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {

Then checking |this.mRIL| here, if (network.type == Ci.nsINetworkInterface.NETWORK_TYPE_WIFI && this.mRil) { ...

@@ +376,3 @@
>          // Update data connection when Wifi connected/disconnected
> +        try {
> +          if (network.type == Ci.nsINetworkInterface.NETWORK_TYPE_WIFI) {

Ditto, check |this.mRIL|.

@@ +1357,1 @@
>  XPCOMUtils.defineLazyServiceGetter(NetworkManager.prototype, "mRil",

Rewrite with `defineLazyGetter`.
Attachment #8538297 - Flags: review?(echen)
(Assignee)

Comment 7

3 years ago
Created attachment 8540573 [details] [diff] [review]
patch, v2.

Address review comment 6.

Edgar, may I have your review again? Thanks.
Attachment #8540573 - Flags: review?(echen)
(Reporter)

Updated

3 years ago
Attachment #8538297 - Attachment is obsolete: true
(Reporter)

Comment 8

3 years ago
Comment on attachment 8540573 [details] [diff] [review]
patch, v2.

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

Looks good to me, thank you.
Attachment #8540573 - Flags: review?(echen) → review+
https://hg.mozilla.org/mozilla-central/rev/ed0fe59d8d69
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1139805
You need to log in before you can comment on or make changes to this bug.