Closed Bug 939680 Opened 6 years ago Closed 6 years ago

Implement nsINetworkLinkService.linkType on Android

Categories

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

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla28

People

(Reporter: mfinkle, Unassigned)

References

Details

Attachments

(1 file)

Attached patch support linktypeSplinter Review
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.
Attachment #8333701 - Flags: review?(blassey.bugs)
Blocks: 938571
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
Attachment #8333701 - Flags: review?(blassey.bugs) → review+
(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.
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
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
https://hg.mozilla.org/mozilla-central/rev/dc4911c44a86
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.