1054148 - Clean up MOZ_B2G_RIL in NetworkManager

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

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.

Comment 1

[Edgar Chen [:edgar]]

Attachment: WIP, patch

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

Comment 2

[Jessica Jong [:jessica] (inactive)]

(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? :)

Comment 3

[Edgar Chen [:edgar]]

Yes, please help me to finish it if you don't mind. :)

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 5

[Jessica Jong [:jessica] (inactive)]

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

Edgar, may I have your review on this? Thanks.

Comment 6

[Edgar Chen [:edgar]]

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`.

Comment 7

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Address review comment 6.

Edgar, may I have your review again? Thanks.

Comment 8

[Edgar Chen [:edgar]]

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

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

Looks good to me, thank you.

Comment 9

[Jessica Jong [:jessica] (inactive)]

try looks good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17b112f0239f

Comment 10

[Jessica Jong [:jessica] (inactive)]

https://hg.mozilla.org/integration/b2g-inbound/rev/ed0fe59d8d69

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ed0fe59d8d69