Closed
Bug 1250387
Opened 8 years ago
Closed 8 years ago
Kill GeckoConnectivityReceiver.java and improve GeckoNetworkManager.java
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox48 fixed)
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)
16.72 KB,
patch
|
Grisha
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Mentor: margaret.leibovic, mark.finkle
Whiteboard: [lang=java]
Updated•8 years ago
|
Whiteboard: [lang=java] → [lang=java][good next bug]
Reporter | ||
Comment 1•8 years ago
|
||
(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
Comment 2•8 years ago
|
||
Hi Mark, can you assign this to me?
Reporter | ||
Comment 3•8 years ago
|
||
(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
Comment 4•8 years ago
|
||
Sure, thanks for letting me know Mark!
Assignee | ||
Comment 5•8 years ago
|
||
Hi Mark, Please Review. And let me know if something need changes. Regards.
Attachment #8729496 -
Flags: review?(mark.finkle)
Reporter | ||
Comment 6•8 years ago
|
||
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.
Reporter | ||
Comment 7•8 years ago
|
||
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-
Reporter | ||
Comment 8•8 years ago
|
||
I want to hand this mentoring over to another Android developer too. I am too otherwise distracted to be responsive.
Comment 9•8 years ago
|
||
Grisha, would you be interested in helping mentor this bug?
Mentor: mark.finkle
Flags: needinfo?(gkruglov)
Comment 10•8 years ago
|
||
Margaret, certainly, I can take over. I was just studying this code yesterday :)
Flags: needinfo?(gkruglov)
Updated•8 years ago
|
Mentor: margaret.leibovic → gkruglov
Updated•8 years ago
|
Mentor: margaret.leibovic
Assignee | ||
Comment 11•8 years ago
|
||
Hi. Please review and let me know if something needs modification. Thanks
Attachment #8729496 -
Attachment is obsolete: true
Attachment #8738043 -
Flags: review?(gkruglov)
Comment 12•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8738043 -
Flags: review?(gkruglov) → review-
Assignee | ||
Comment 13•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
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 15•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8740957 -
Flags: review?(gkruglov) → review-
Comment 16•8 years ago
|
||
Err, I didn't mean to do r-. I'll r+ the patch once the networks type selection is adjusted. Thanks!
Assignee | ||
Comment 17•8 years ago
|
||
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 18•8 years ago
|
||
Comment on attachment 8742592 [details] [diff] [review] Kill_Gecko.patch Review of attachment 8742592 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8742592 -
Flags: review?(gkruglov) → review+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f2187390d83f
Keywords: checkin-needed
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f2187390d83f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•