Closed Bug 1250387 Opened 4 years ago Closed 4 years ago

Kill GeckoConnectivityReceiver.java and improve GeckoNetworkManager.java

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: mfinkle, Assigned: shatur, Mentored)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(1 file, 4 obsolete files)

GeckoNetworkManager.java and GeckoConnectivityReceiver.java are both ConnectivityManager.CONNECTIVITY_ACTION broadcast receivers. We don't need both.

GeckoNetworkManager is a bit more sophisticated and has extra functionality for sending data back to Gecko.

GeckoConnectivityReceiver is very simple and the code in it's onReceive can be moved to GeckoNetworkManager. We can remove GeckoConnectivityReceiver.

Lastly, using the pattern in GeckoConnectivityReceiver for sending a "network link change" notification, we can simply add support to GeckoNetworkManager for sending a "network type changed" notification. GeckoNetworkManager already sends a different style of "type change" event to Gecko, but it's narrowly focused to DOM API that might only work in FxOS.
Mentor: margaret.leibovic, mark.finkle
Whiteboard: [lang=java]
Whiteboard: [lang=java] → [lang=java][good next bug]
(In reply to Mark Finkle (:mfinkle) from comment #0)

> Lastly, using the pattern in GeckoConnectivityReceiver for sending a
> "network link change" notification, we can simply add support to
> GeckoNetworkManager for sending a "network type changed" notification.
> GeckoNetworkManager already sends a different style of "type change" event
> to Gecko, but it's narrowly focused to DOM API that might only work in FxOS.

More explanation about this part.

GeckoConnectivityReceiver sends a notification when the network goes up or down [1]. This is tied to this notification in Gecko [2].

We want to also implement the notification for when the type of network changes (wifi to cellular or ethernet, for example). GeckoNetworkManager already watches for these changes [3]. We just need to send a similar Gecko notification which maps to this one [4].

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoConnectivityReceiver.java#85
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsINetworkLinkService.idl#57
[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoNetworkManager.java#255
[4] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsINetworkLinkService.idl#83
Hi Mark, can you assign this to me?
(In reply to Maurya Talisetti from comment #2)
> Hi Mark, can you assign this to me?

Maurya - Someone else (tushar ?) has been trying to get started on this bug and chatting in IRC, so I will give that person the first chance. If that person decides to drop the bug, we can assign it to you.
Assignee: nobody → tushar.saini1285
Sure, thanks for letting me know Mark!
Attached patch Kill_Gecko.patch (obsolete) — Splinter Review
Hi Mark,
Please Review. And let me know if something need changes.
Regards.
Attachment #8729496 - Flags: review?(mark.finkle)
Comment on attachment 8729496 [details] [diff] [review]
Kill_Gecko.patch

I'm on vacation, but I wanted to provide some feedback. I can take a closer look in a few days.

>diff --git a/mobile/android/base/java/org/mozilla/gecko/GeckoNetworkManager.java b/mobile/android/base/java/org/mozilla/gecko/GeckoNetworkManager.java

>-public class GeckoNetworkManager extends BroadcastReceiver implements NativeEventListener {
>+public class GeckoNetworkManager extends BroadcastReceiver implements NativeEventListener {   

You added some whitesoace to the end of this line

>+    public void updateLinkStatus(Context context, Intent intent) {
>+        ConnectivityManager cm = (ConnectivityManager)context.getSystemService(Context.CONNECTIVITY_SERVICE);
>+        NetworkInfo info = cm.getActiveNetworkInfo();
>+
>+        final String status;
>+        if (info == null) {
>+            status = LINK_DATA_UNKNOWN;
>+        } else if (!info.isConnected()) {
>+            status = LINK_DATA_DOWN;
>+        } else {
>+            status = LINK_DATA_UP;
>+        }
>+
>+        if (GeckoThread.isRunning()) {
>+            GeckoAppShell.sendEventToGecko(GeckoEvent.createNetworkLinkChangeEvent(status));
>+            GeckoAppShell.sendEventToGecko(GeckoEvent.createNetworkLinkChangeEvent(LINK_DATA_CHANGED));
>+        }
>     }

This part seems OK.

>         switch (ni.getType()) {
>         case ConnectivityManager.TYPE_BLUETOOTH:
>             return ConnectionType.BLUETOOTH;
>         case ConnectivityManager.TYPE_ETHERNET:
>+            network_type_status=LINK_TYPE_ETHERNET;
>             return ConnectionType.ETHERNET;
>         case ConnectivityManager.TYPE_MOBILE:
>+            switch (ni.getSubtype()) {
>+                case TelephonyManager.NETWORK_TYPE_GPRS:
>+                case TelephonyManager.NETWORK_TYPE_EDGE:
>+                case TelephonyManager.NETWORK_TYPE_CDMA:
>+                case TelephonyManager.NETWORK_TYPE_1xRTT:
>+                case TelephonyManager.NETWORK_TYPE_IDEN:
>+                    network_type_status=LINK_TYPE_2G;
>+                    break;
>+                case TelephonyManager.NETWORK_TYPE_UMTS:
>+                case TelephonyManager.NETWORK_TYPE_EVDO_0:
>+                case TelephonyManager.NETWORK_TYPE_EVDO_A:
>+                case TelephonyManager.NETWORK_TYPE_HSDPA:
>+                case TelephonyManager.NETWORK_TYPE_HSUPA:
>+                case TelephonyManager.NETWORK_TYPE_HSPA:
>+                case TelephonyManager.NETWORK_TYPE_EVDO_B:
>+                case TelephonyManager.NETWORK_TYPE_EHRPD:
>+                case TelephonyManager.NETWORK_TYPE_HSPAP:
>+                    network_type_status=LINK_TYPE_3G;
>+                    break;
>+                case TelephonyManager.NETWORK_TYPE_LTE:
>+                    network_type_status=LINK_TYPE_4G;
>+                    break;
>+                default:
>+                    network_type_status=LINK_TYPE_UNKONWN;
>+                    break;
>+            }
>         case ConnectivityManager.TYPE_WIMAX:
>+            network_type_status=LINK_TYPE_WIMAX;
>             return ConnectionType.CELLULAR;
>         case ConnectivityManager.TYPE_WIFI:
>+            network_type_status=LINK_TYPE_WIFI;
>             return ConnectionType.WIFI;
>         default:
>             Log.w(LOGTAG, "Ignoring the current network type.");
>+            network_type_status=LINK_TYPE_UNKONWN;
>             return ConnectionType.OTHER;


I'm not so happy with this function. I don't like how the function does two things: set a data member and return a value. 

Also, the TYPE_MOBILE block sets a data member, but falls through to the TYPE_WIMAX case, overwriting the data member. You'll need to stop falling through and explicitly return from the TYPE_MOBILE block.

Again, I'll look closer in a few days.
Comment on attachment 8729496 [details] [diff] [review]
Kill_Gecko.patch

OK. I still think we need to re-think the code a little. It might take some refactoring, or splitting the single function into two functions.
Attachment #8729496 - Flags: review?(mark.finkle) → review-
I want to hand this mentoring over to another Android developer too. I am too otherwise distracted to be responsive.
Grisha, would you be interested in helping mentor this bug?
Mentor: mark.finkle
Flags: needinfo?(gkruglov)
Margaret, certainly, I can take over. I was just studying this code yesterday :)
Flags: needinfo?(gkruglov)
Mentor: margaret.leibovic → gkruglov
Mentor: margaret.leibovic
Attached patch Kill_Gecko_v2.patch (obsolete) — Splinter Review
Hi.
  Please review and let me know if something needs modification.

Thanks
Attachment #8729496 - Attachment is obsolete: true
Attachment #8738043 - Flags: review?(gkruglov)
Comment on attachment 8738043 [details] [diff] [review]
Kill_Gecko_v2.patch

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

Structurally this seems fine, but I've left a bunch of comments regarding code style and a few other things. Please re-read through your patch as well, in case I've missed some styling issues.

::: mobile/android/base/java/org/mozilla/gecko/GeckoNetworkManager.java
@@ +45,5 @@
> +    private static final String LINK_DATA_DOWN = "down";
> +    private static final String LINK_DATA_CHANGED = "changed";
> +    private static final String LINK_DATA_UNKNOWN = "unknown";
> +
> +    // Constants to notify NS_NETWORK_LINK_TYPE_TOPIC

Use fully formed sentences in comments

@@ +46,5 @@
> +    private static final String LINK_DATA_CHANGED = "changed";
> +    private static final String LINK_DATA_UNKNOWN = "unknown";
> +
> +    // Constants to notify NS_NETWORK_LINK_TYPE_TOPIC
> +    private static final String LINK_TYPE_UNKONWN= "unknown";

wrong spacing around =. do "name = value".

@@ +277,5 @@
> +                                       wifiDhcpGatewayAddress(),
> +                                       getConnectionSubType()));
> +    }
> +
> +    public void updateLinkStatus(Context context, Intent intent) {

You don't need intent parameter here

@@ +342,5 @@
>              ni = cm.getActiveNetworkInfo();
>          } catch (SecurityException se) {} // if we don't have the permission, fall through to null check
>  
>          if (ni == null) {
> +            return null;

This is redundant

@@ +348,5 @@
> +        return ni;
> +    }
> +
> +    private ConnectionType getConnectionType() {
> +        NetworkInfo ni=getConnectivityManager();

Use proper spacing, e.g. "ni = getConnec..."

@@ +350,5 @@
> +
> +    private ConnectionType getConnectionType() {
> +        NetworkInfo ni=getConnectivityManager();
> +
> +        if(ni == null)

Proper spacing "if (", and always wrap blocks in curly braces

@@ +372,5 @@
>  
> +    private String getConnectionSubType() {
> +        NetworkInfo ni=getConnectivityManager();
> +
> +        if(ni == null)

again, spacing, and missing {}

@@ +376,5 @@
> +        if(ni == null)
> +            return LINK_TYPE_UNKONWN;
> +
> +        switch (ni.getType()) {
> +        case ConnectivityManager.TYPE_ETHERNET:

wrong indentation

@@ +397,5 @@
> +                case TelephonyManager.NETWORK_TYPE_EHRPD:
> +                case TelephonyManager.NETWORK_TYPE_HSPAP:
> +                    return LINK_TYPE_3G;
> +                case TelephonyManager.NETWORK_TYPE_LTE:
> +                    return LINK_TYPE_4G;

Usually a good idea to have a default clause for sanity; as for the type, perhaps return 2g? either you pick one, or return an "unknown" - but that would be treated as "no connection". so let's do 2g.
Attachment #8738043 - Flags: review?(gkruglov) → review-
Attached patch Kill_Geckov3.patch (obsolete) — Splinter Review
Hi
 I have done the changes described by you in comment 12.

Regarding this :
::: mobile/android/base/java/org/mozilla/gecko/GeckoNetworkManager.java
@@ +45,5 @@
> +    private static final String LINK_DATA_DOWN = "down";
> +    private static final String LINK_DATA_CHANGED = "changed";
> +    private static final String LINK_DATA_UNKNOWN = "unknown";
> +
> +    // Constants to notify NS_NETWORK_LINK_TYPE_TOPIC

Use fully formed sentences in comments


I think the comment is unnecessary as the comment above those constants is sufficient. 

Regards
Attachment #8738043 - Attachment is obsolete: true
Attachment #8740950 - Flags: review?(gkruglov)
Attached patch Kill_Geckov3.patch (obsolete) — Splinter Review
Sorry, above patch contains an error, resolved in here. For comment refer above.

Thanks
Attachment #8740950 - Attachment is obsolete: true
Attachment #8740950 - Flags: review?(gkruglov)
Attachment #8740957 - Flags: review?(gkruglov)
Comment on attachment 8740957 [details] [diff] [review]
Kill_Geckov3.patch

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

Looks good! Please see my comment regarding network types - that needs to be addressed. It'd be good to add some junit4 tests for the GeckoNetworkManager while we're at it, but we don't need to do that in this ticket.
Thanks!

::: mobile/android/base/java/org/mozilla/gecko/GeckoNetworkManager.java
@@ +391,5 @@
> +                        return LINK_TYPE_3G;
> +                    case TelephonyManager.NETWORK_TYPE_LTE:
> +                        return LINK_TYPE_4G;
> +                    default:
> +                        return LINK_TYPE_2G;

So looking over different options in TelephonyManager, I think the right thing to do here is explicitly catch 2G network types:
- NETWORK_TYPE_1xRTT
- NETWORK_TYPE_CDMA
- NETWORK_TYPE_EDGE
- NETWORK_TYPE_1DEN
- NETWORK_TYPE_GPRS

Also add case for NETWORK_TYPE_UNKNOWN, and return LINK_TYPE_2G - elsewhere in the code we treat "unknown" as "no connection", which I don't think is the case here - so let's not break that code.
Specifically, add a comment describing why you're not returning "unknown" state, and refer to Bug 1236130.

Additionally, ideally default (catch-all bucket for future network types) should return LINK_TYPE_UNKNOWN, but for the same reason as stated above, let's keep it to LINK_TYPE_2G for now.
Attachment #8740957 - Flags: review?(gkruglov) → review-
Err, I didn't mean to do r-. I'll r+ the patch once the networks type selection is adjusted. Thanks!
Attached patch Kill_Gecko.patchSplinter Review
Hi
  Done all changes you asked for. Regarding junit4 test, I will love to take that task if you think its suits me.

Thanks.
Attachment #8740957 - Attachment is obsolete: true
Attachment #8742592 - Flags: review?(gkruglov)
Comment on attachment 8742592 [details] [diff] [review]
Kill_Gecko.patch

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

LGTM
Attachment #8742592 - Flags: review?(gkruglov) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f2187390d83f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.