939680 - Implement nsINetworkLinkService.linkType on Android

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: support linktype

Currently the method id NOT_IMPL'd but I'd like to use it from JavaScript JSM so I can scan for LAN devices, like second screen systems, only when on ethernet or wifi. 

Turns out the GONK-only service nsINetworkManager has something similar too, but I can't use that service currently and nsINetworkLinkService could use the implementation anyway.

This patch gets the method to work, but I have a few questions that should be double-checked:
1. The XPCOM interface returns an uint32_t, but I don't see any JNI returning an unsigned int so I just used an int. Is that OK?
2. The GONK nsINetworkManager code uses different enums than the nsINetworkLinkService, which simply uses 2G, 3G and 4G. I made guesses as to how to group the fine grained 2.5G, 2.75G, 3.5G and 3.9G values into the broader categories. Let me know if you want those changed and I can also collapse the case statements too.
3. I initially made the GeneratedJNIWrappers.h/.cpp changes by hand, then the build yelled at me to use a script so I did. The script does not alpha-order the methods. I assumed it would.
4. You could argue that I should use GeckoNetworkService.getNetworkType (the GONK only Java facing implementation) to re-use code. Since the enumerations are different, I would still need a switch statement to convert to the nsInetowrkLinkService enums and I didn't want to get tied to that code. If you want me to, I can try it though.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8333701 [details] [diff] [review]
support linktype

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

(In reply to Mark Finkle (:mfinkle) from comment #0)
> Created attachment 8333701 [details] [diff] [review]
> support linktype
> 
> Currently the method id NOT_IMPL'd but I'd like to use it from JavaScript
> JSM so I can scan for LAN devices, like second screen systems, only when on
> ethernet or wifi. 
> 
> Turns out the GONK-only service nsINetworkManager has something similar too,
> but I can't use that service currently and nsINetworkLinkService could use
> the implementation anyway.
> 
> This patch gets the method to work, but I have a few questions that should
> be double-checked:
> 1. The XPCOM interface returns an uint32_t, but I don't see any JNI
> returning an unsigned int so I just used an int. Is that OK?
That's OK as long as you don't pass number that will overflow, which you won't since your enum only goes to 7
> 2. The GONK nsINetworkManager code uses different enums than the
> nsINetworkLinkService, which simply uses 2G, 3G and 4G. I made guesses as to
> how to group the fine grained 2.5G, 2.75G, 3.5G and 3.9G values into the
> broader categories. Let me know if you want those changed and I can also
> collapse the case statements too.
comments inline
> 3. I initially made the GeneratedJNIWrappers.h/.cpp changes by hand, then
> the build yelled at me to use a script so I did. The script does not
> alpha-order the methods. I assumed it would.
That's really not something I care about :-)
> 4. You could argue that I should use GeckoNetworkService.getNetworkType (the
> GONK only Java facing implementation) to re-use code. Since the enumerations
> are different, I would still need a switch statement to convert to the
> nsInetowrkLinkService enums and I didn't want to get tied to that code. If
> you want me to, I can try it though.
I agree with you

::: mobile/android/base/GeckoAppShell.java
@@ +1579,5 @@
> +        switch (tm.getNetworkType()) {
> +            case TelephonyManager.NETWORK_TYPE_IDEN:
> +            case TelephonyManager.NETWORK_TYPE_CDMA:
> +                return LINK_TYPE_2G;
> +            case TelephonyManager.NETWORK_TYPE_GPRS:

GPRS is a 2G network, not 2.5G

@@ +1581,5 @@
> +            case TelephonyManager.NETWORK_TYPE_CDMA:
> +                return LINK_TYPE_2G;
> +            case TelephonyManager.NETWORK_TYPE_GPRS:
> +            case TelephonyManager.NETWORK_TYPE_1xRTT:
> +                return LINK_TYPE_2G; // 2.5G

Here's where we're splitting hairs, 1xRTT is 3G, but is actually slower than some 2G networks. Returning 2G her would probably be the more practical thing, while returning 3G would be the more technically correct thing.

@@ +1583,5 @@
> +            case TelephonyManager.NETWORK_TYPE_GPRS:
> +            case TelephonyManager.NETWORK_TYPE_1xRTT:
> +                return LINK_TYPE_2G; // 2.5G
> +            case TelephonyManager.NETWORK_TYPE_EDGE:
> +                return LINK_TYPE_2G; // 2.75G

EDGE is commonly called 2.5G (there really isn't any such thing as 2.5G)

@@ +1595,5 @@
> +            case TelephonyManager.NETWORK_TYPE_EVDO_B:
> +            case TelephonyManager.NETWORK_TYPE_EHRPD:
> +                return LINK_TYPE_3G; // 3.5G
> +            case TelephonyManager.NETWORK_TYPE_HSPAP:
> +                return LINK_TYPE_4G; // 3.75G

HSPA+ is a 3G network, no problem labeling it 3.#G (probably 3.9G, its almost as fast as LTE), but we should return LINK_TYPE_3G.

@@ +1597,5 @@
> +                return LINK_TYPE_3G; // 3.5G
> +            case TelephonyManager.NETWORK_TYPE_HSPAP:
> +                return LINK_TYPE_4G; // 3.75G
> +            case TelephonyManager.NETWORK_TYPE_LTE:
> +                return LINK_TYPE_4G; // 3.9G

LTE is actually a 4G network, no need to label it 3.9G

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1)
> > +            case TelephonyManager.NETWORK_TYPE_LTE:
> > +                return LINK_TYPE_4G; // 3.9G
> 
> LTE is actually a 4G network, no need to label it 3.9G

I stand corrected, LTE is *not* actually 4G, so labeling it 3.9G is correct. It is generally accepted as 4G, so returning LINK_TYPE_4G is the right thing to do here.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Landed with requested changes

https://hg.mozilla.org/integration/mozilla-inbound/rev/86253e0141c8

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I backed this out temporarily because a conflicting patch landed on fx-team. I'll wait until that patch makes it to inbound and will reland.

https://hg.mozilla.org/integration/mozilla-inbound/rev/45414e5a4324

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

The other patch was backed out too, but I am landing this on fx-team to make sure the other patch requires a rebase before landing.

https://hg.mozilla.org/integration/fx-team/rev/dc4911c44a86

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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