713687 - Network API: Android backend

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Part 1 WIP - Get current information

We need a backend for the Network API in Android. Mobile devices are the main target of that API. The idea is to expose whether a connection can be restricted (then, the DOM say if the connection *is* restricted with the help of a preference) and what is the current download bandwidth of the connection. For the bandwidth we try to guess depending on the connection type. We do not need to know the exact value for the moment. We could improve that later.

Part 1 will implement the getting part.
Part 2 will implement the update (and notification part). Basically, this is going to work like Battery API.

This is WIP for part 1 in case of you have time to give it a look before I finish the cleanup (writing a decent comment at the top of the file and copy-pasting implementation for Android XUL).

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Get current information

Asking review to Doug as he wrote something similar which has been backed out IIRC.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 584511 [details] [diff] [review]
Part 1 - Get current information

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

just to be clear, it was backed out because there wasn't a product "need" and asking for this type of data may have required additional permissions.  :D


Do we need more permissions?

::: mobile/android/base/GeckoNetworkManager.java
@@ +95,5 @@
> +{
> +  static private final double  kDefaultBandwidth    = -1.0;
> +  static private final boolean kDefaultCanBeMetered = false;
> +
> +  static private final double  kMaxBandwidth = 20.0;

OOC, why is the max bandwidth 20?

@@ +192,5 @@
> +        bandwidth = kMaxBandwidth;
> +        canBeMetered = false;
> +        break;
> +      case ConnectivityManager.TYPE_MOBILE:
> +        bandwidth = Math.min(getMobileNetworkSpeed(getMobileNetworkType()), kMaxBandwidth);

So, say I am on NETWORK_TYPE_LTE.  That means:

Math.min ( 50.0, 20.0 );

Why are we capping?

::: widget/src/android/AndroidBridge.cpp
@@ +1445,5 @@
> +    if (!arr || mJNIEnv->GetArrayLength(arr) != 2) {
> +        return;
> +    }
> +
> +    jdouble* info = mJNIEnv->GetDoubleArrayElements(arr, 0);

you also could:

 GetDoubleArrayElements(arr, &copy)

and then ReleaseDoubleArrayElements only if copy is true.

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Doug Turner (:dougt) from comment #2)
> Comment on attachment 584511 [details] [diff] [review]
> Part 1 - Get current information
> 
> Review of attachment 584511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just to be clear, it was backed out because there wasn't a product "need"
> and asking for this type of data may have required additional permissions. 
> :D

Yes, I followed that.

> Do we need more permissions?

No. For what I've seen, knowing the phone state (in a call or not) requires a permission but what I'm doing doesn't require anything. I didn't look deeply enough at your backed out patch to know what was requiring permissions but I don't think we will require anything for what we want to do for the Network API.
At least, the current patch doesn't require permissions and part 2 doesn't seem to require permissions as far as I've been.

> ::: mobile/android/base/GeckoNetworkManager.java
> @@ +95,5 @@
> > +{
> > +  static private final double  kDefaultBandwidth    = -1.0;
> > +  static private final boolean kDefaultCanBeMetered = false;
> > +
> > +  static private final double  kMaxBandwidth = 20.0;
> 
> OOC, why is the max bandwidth 20?

As said in the first big comment, we want an estimation so this 20.0 is totally arbitrary. For real, we have no idea if the connection in 0.0 or 100.0 but for the moment, we will assume that WiFi and Ethernet means fast connection so 20.0 is high enough (and around the most common ADSL bandwidth).
We could try to improve that someday but that's not the priority of this API.

> @@ +192,5 @@
> > +        bandwidth = kMaxBandwidth;
> > +        canBeMetered = false;
> > +        break;
> > +      case ConnectivityManager.TYPE_MOBILE:
> > +        bandwidth = Math.min(getMobileNetworkSpeed(getMobileNetworkType()), kMaxBandwidth);
> 
> So, say I am on NETWORK_TYPE_LTE.  That means:
> 
> Math.min ( 50.0, 20.0 );
> 
> Why are we capping?

Mostly because we don't care about values higher than 20.0: it is fast. Also, to anonymize the data. Though, I was planning to do a bit of that in the DOM side so I could keep 50.0 if you want and just move that capping in the DOM code. If we do that, we might want to change the WiiMax bandwidth value too.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Listen to network change and notify the DOM

Some changes mentioned in part 1 have been applied in this part. Please take this into account when reviewing part 1.

Also, I don't think the new boilerplate to communicate from Java to C++ is better: there are more files to changes and the changes seem more error prone (especially in GeckoEvent.java...).

Comment 5

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Listen to network changes and notify the DOM

With changes in Android XUL.

Comment 6

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Make GeckoNetworkManager a singleton

So we can move all the logic to GeckoNetworkManager.java.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: Part 4 - Use enable/disable notifications to prevent listening when not needed

Comment 8

[Mounir Lamouri (:mounir)]

I think we should merge GeckoConnectivityManager into GeckoNetworkManager in a follow-up.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 584511 [details] [diff] [review]
Part 1 - Get current information

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

just to be clear, it was backed out because there wasn't a product "need" and asking for this type of data may have required additional permissions.  :D


Do we need more permissions?

::: mobile/android/base/GeckoNetworkManager.java
@@ +95,5 @@
> +{
> +  static private final double  kDefaultBandwidth    = -1.0;
> +  static private final boolean kDefaultCanBeMetered = false;
> +
> +  static private final double  kMaxBandwidth = 20.0;

OOC, why is the max bandwidth 20?

@@ +192,5 @@
> +        bandwidth = kMaxBandwidth;
> +        canBeMetered = false;
> +        break;
> +      case ConnectivityManager.TYPE_MOBILE:
> +        bandwidth = Math.min(getMobileNetworkSpeed(getMobileNetworkType()), kMaxBandwidth);

So, say I am on NETWORK_TYPE_LTE.  That means:

Math.min ( 50.0, 20.0 );

Why are we capping?

::: widget/src/android/AndroidBridge.cpp
@@ +1445,5 @@
> +    if (!arr || mJNIEnv->GetArrayLength(arr) != 2) {
> +        return;
> +    }
> +
> +    jdouble* info = mJNIEnv->GetDoubleArrayElements(arr, 0);

you also could:

 GetDoubleArrayElements(arr, &copy)

and then ReleaseDoubleArrayElements only if copy is true.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 584574 [details] [diff] [review]
Part 2 - Listen to network changes and notify the DOM

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

r+.  let me know about the perf on context switch.

::: embedding/android/GeckoNetworkManager.java
@@ +137,5 @@
> +  public static void initialize() {
> +    sNetworkType = getNetworkType();
> +  }
> +
> +  public static void resume() {

this happens when gecko is put into the foreground.  Does this take any time?  We really shouldn't be doing anything that isn't required during this context switch.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 584575 [details] [diff] [review]
Part 3 - Make GeckoNetworkManager a singleton

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

::: mobile/android/base/GeckoNetworkManager.java
@@ +141,5 @@
>    public void onReceive(Context aContext, Intent aIntent) {
>      updateNetworkType();
>    }
>  
> +  public void initialize() {

Other public methods that do this sort of startup are called |init|.  Less typing too.  Up to you to change it or not.

@@ +155,3 @@
>    }
>  
> +  public void stop() {

calling stop if start failed, or if sInstance is null will throw.  should you protect this call?

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 584584 [details] [diff] [review]
Part 4 - Use enable/disable notifications to prevent listening when not needed

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

r+ w/ a comment about the two new booleans.

::: embedding/android/GeckoNetworkManager.java
@@ +132,5 @@
>  
>    private NetworkType  mNetworkType = NetworkType.NETWORK_NONE;
>    private IntentFilter mNetworkFilter = new IntentFilter();
> +  private boolean mShouldBeListening = false;
> +  private boolean mShouldNotify      = false;

The difference between these is subtle.  Mind adding a comment to explain what each do?

::: widget/src/android/AndroidBridge.cpp
@@ +1460,5 @@
> +AndroidBridge::EnableNetworkNotifications()
> +{
> +    ALOG_BRIDGE("AndroidBridge::EnableNetworkNotifications");
> +
> +    mJNIEnv->CallStaticVoidMethod(mGeckoAppShellClass, jEnableNetworkNotifications);

nit.  no space needed between the log and the static method call.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 584575 [details] [diff] [review]
Part 3 - Make GeckoNetworkManager a singleton

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

r+ if you protect stop() or tell me why you don't need to.

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to Doug Turner (:dougt) from comment #10)
> this happens when gecko is put into the foreground.  Does this take any
> time?  We really shouldn't be doing anything that isn't required during this
> context switch.

That should be fast. It's just about getting a value. At least, I hope Android API would have warned if that was something that would cost because naively we can imagine the value has been cached at least.

(In reply to Doug Turner (:dougt) from comment #13)
> r+ if you protect stop() or tell me why you don't need to.

Part 4 is actually protecting unregisterReceiver calls.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e652d673382f
https://hg.mozilla.org/mozilla-central/rev/b92399819ec4
https://hg.mozilla.org/mozilla-central/rev/9ee757bbbcd0
https://hg.mozilla.org/mozilla-central/rev/9f68bafdd685

Comment 16

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

Doug, do we want this in 11? If not, please mark the whiteboard not-fennec-11