Last Comment Bug 713687 - Network API: Android backend
: Network API: Android backend
Status: RESOLVED FIXED
[not-fennec-11]
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 716943
Blocks: 677166
  Show dependency treegraph
 
Reported: 2011-12-27 10:39 PST by Mounir Lamouri (:mounir)
Modified: 2012-01-29 21:16 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 WIP - Get current information (14.53 KB, patch)
2011-12-27 10:39 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 1 - Get current information (23.63 KB, patch)
2011-12-27 17:01 PST, Mounir Lamouri (:mounir)
doug.turner: review+
Details | Diff | Splinter Review
Part 2 - Listen to network change and notify the DOM (31.42 KB, patch)
2011-12-28 07:08 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 2 - Listen to network changes and notify the DOM (35.16 KB, patch)
2011-12-28 08:08 PST, Mounir Lamouri (:mounir)
doug.turner: review+
Details | Diff | Splinter Review
Part 3 - Make GeckoNetworkManager a singleton (16.54 KB, patch)
2011-12-28 08:13 PST, Mounir Lamouri (:mounir)
doug.turner: review+
Details | Diff | Splinter Review
Part 4 - Use enable/disable notifications to prevent listening when not needed (12.24 KB, patch)
2011-12-28 09:27 PST, Mounir Lamouri (:mounir)
doug.turner: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-12-27 10:39:42 PST
Created attachment 584456 [details] [diff] [review]
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) 2011-12-27 17:01:19 PST
Created attachment 584511 [details] [diff] [review]
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) 2011-12-27 20:54:56 PST
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) 2011-12-28 03:27:25 PST
(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) 2011-12-28 07:08:58 PST
Created attachment 584567 [details] [diff] [review]
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) 2011-12-28 08:08:44 PST
Created attachment 584574 [details] [diff] [review]
Part 2 - Listen to network changes and notify the DOM

With changes in Android XUL.
Comment 6 Mounir Lamouri (:mounir) 2011-12-28 08:13:57 PST
Created attachment 584575 [details] [diff] [review]
Part 3 - Make GeckoNetworkManager a singleton

So we can move all the logic to GeckoNetworkManager.java.
Comment 7 Mounir Lamouri (:mounir) 2011-12-28 09:27:12 PST
Created attachment 584584 [details] [diff] [review]
Part 4 - Use enable/disable notifications to prevent listening when not needed
Comment 8 Mounir Lamouri (:mounir) 2011-12-28 09:32:58 PST
I think we should merge GeckoConnectivityManager into GeckoNetworkManager in a follow-up.
Comment 9 Doug Turner (:dougt) 2011-12-28 17:11:08 PST
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) 2011-12-28 17:29:52 PST
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) 2011-12-28 17:33:18 PST
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) 2011-12-28 17:36:26 PST
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) 2011-12-28 17:37:18 PST
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) 2012-01-16 06:01:20 PST
(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 16 Brad Lassey [:blassey] (use needinfo?) 2012-01-27 23:41:58 PST
Doug, do we want this in 11? If not, please mark the whiteboard not-fennec-11

Note You need to log in before you can comment on or make changes to this bug.