Closed Bug 1054148 Opened 5 years ago Closed 5 years ago

Clean up MOZ_B2G_RIL in NetworkManager

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S3 (9jan)

People

(Reporter: edgar, Assigned: jessica)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 904514
Attached patch WIP, patch (obsolete) — Splinter Review
Just a WIP patch, didn't verify it yet.
(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? :)
Yes, please help me to finish it if you don't mind. :)
Assignee: nobody → jjong
Attached patch patch, v1. (obsolete) — Splinter Review
Attachment #8534864 - Attachment is obsolete: true
Comment on attachment 8538297 [details] [diff] [review]
patch, v1.

Edgar, may I have your review on this? Thanks.
Attachment #8538297 - Flags: review?(echen)
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)
Attached patch patch, v2.Splinter Review
Address review comment 6.

Edgar, may I have your review again? Thanks.
Attachment #8540573 - Flags: review?(echen)
Attachment #8538297 - Attachment is obsolete: true
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
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1139805
You need to log in before you can comment on or make changes to this bug.