696038 - Battery API: Android backend

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Part A - Add a hal backend for Android

This patch is separate because I know some other patches are doing that and depending on the order of landing, this patch will or will not land.

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Part B - Boilerplate code to make hal communicate with the Android Java code

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Part C - Android backend

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 568370 [details] [diff] [review]
Part A - Add a hal backend for Android

>+#include "mozilla/dom/battery/BatteryConstants.h"
>+

As noted in the other bug, I think this is a "layering violation."

The boilerplate here looks fine but I'm not sure
RegisterBatteryObserver/UnregisterBatteryObserver is the API we want.
But if the API changes, I don't care about seeing updates to the
boilerplate.

Word of warning: you're going to race jlebar in getting AndroidHal.cpp
created, so godspeed :).

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 568371 [details] [diff] [review]
Part B - Boilerplate code to make hal communicate with the Android Java code

>diff --git a/hal/android/AndroidHal.cpp b/hal/android/AndroidHal.cpp
>--- a/hal/android/AndroidHal.cpp
>+++ b/hal/android/AndroidHal.cpp
>@@ -31,31 +31,44 @@
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "Hal.h"
>-#include "mozilla/dom/battery/BatteryConstants.h"
>+#include "AndroidBridge.h"
>+
>+/**
>+ * This macro intents to reduce a bit the boilerplate code mass.
>+ * Note that the Android Bridge can be used with |bridge| after using the macro.
>+ */
>+#define GET_AND_CHECK_ANDROID_BRIDGE \
>+  AndroidBridge* bridge = AndroidBridge::Bridge(); \
>+  if (!bridge) return;
> 

Macros that affect control flow are a non-starter.  Instead, use this
C++ nugget

  if (AndroidBridge* bridge = AndroidBridge::Bridge()) {
    // bridge->blah()
  }

> void
> RegisterBatteryObserver(dom::battery::BatteryInformation* aBatteryInfo)

I suspect this API will change, but like I said in the other patch I
don't care about seeing an updated patch.

>+void
>+AndroidBridge::RegisterBatteryObserver(dom::battery::BatteryInformation* aBatteryInfo)
>+{
>+    ALOG_BRIDGE("AndroidBridge::RegisterBatteryObserver");
>+
>+    AutoLocalJNIFrame jniFrame;
>+
>+    // To prevent calling too many methods trough JNI, the Java method returns

"through"

>+    // an array of float even if we actually want a float and a boolean.

Ew ... gross.  I don't have a better idea though.

>+void
>+AndroidBridge::UnregisterBatteryObserver()
>+{
>+    ALOG_BRIDGE("AndroidBridge::UnregisterBatteryObserver");
>+
>+    GetJNIForThread()->CallStaticVoidMethod(mGeckoAppShellClass, jRemoveBatteryObserver);

Shouldn't this be mJNIEnv?

>+NS_EXPORT void JNICALL
>+Java_org_mozilla_gecko_GeckoAppShell_notifyBatteryChange(JNIEnv* jenv, jclass,
>+                                                         jfloat aLevel,
>+                                                         jboolean aCharging)
>+{
>+    if (!nsAppShell::gAppShell) {
>+        return;
>+    }
>+
>+    nsCOMPtr<dom::battery::BatteryInformation> batteryInfo =

If this were the final API, I would say that dom::battery is too much
namespace, and would request a |using namespace|, but I think this
will change.

r=me with GET_AND_CHECK_ANDROID_BRIDGE removal, and pending HAL API
updates.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 568372 [details] [diff] [review]
Part C - Android backend

>diff --git a/embedding/android/GeckoBatteryManager.java b/embedding/android/GeckoBatteryManager.java

>+        // Likely, if plugged > 0, it's plugged and charging but the doc isn't
>+        // clear about that.
>+        sCharging = plugged != 0 && plugged != -1;

(plugged != 0)

Comment 7

[Mounir Lamouri (:mounir)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> Word of warning: you're going to race jlebar in getting AndroidHal.cpp
> created, so godspeed :).

I know. That's actually why I put this in a separate patch. It will be easier to rebase if needed.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> Comment on attachment 568372 [details] [diff] [review] [diff] [details] [review]
> Part C - Android backend
> 
> >diff --git a/embedding/android/GeckoBatteryManager.java b/embedding/android/GeckoBatteryManager.java
> 
> >+        // Likely, if plugged > 0, it's plugged and charging but the doc isn't
> >+        // clear about that.
> >+        sCharging = plugged != 0 && plugged != -1;
> 
> (plugged != 0)

I'm assuming you meant |sCharging = (plugged != 0 && plugged != -1);| ?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> >+    // an array of float even if we actually want a float and a boolean.
> 
> Ew ... gross.  I don't have a better idea though.

Gross but commented... That's half a sin, isnt't? :)

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> >+#define GET_AND_CHECK_ANDROID_BRIDGE \
> >+  AndroidBridge* bridge = AndroidBridge::Bridge(); \
> >+  if (!bridge) return;
> > 
> 
> Macros that affect control flow are a non-starter. 

I removed that macro but I think we might want to add a few FORWARD_TO_ANDROID_BRIDGE's macro that would prevent writing code like:
returnType myFunc(paramType1 myParam1) {
  AndroidBridge* bridge = AndroidBridge::Bridge();
  if (!bridge) {
    return;
  }

  bridge->myFunc(myParam1);
}

Though, for the moment, we only have two methods like that, we will see if they get more and more numerous.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #7)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> > Comment on attachment 568372 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > Part C - Android backend
> > 
> > >diff --git a/embedding/android/GeckoBatteryManager.java b/embedding/android/GeckoBatteryManager.java
> > 
> > >+        // Likely, if plugged > 0, it's plugged and charging but the doc isn't
> > >+        // clear about that.
> > >+        sCharging = plugged != 0 && plugged != -1;
> > 
> > (plugged != 0)
> 
> I'm assuming you meant |sCharging = (plugged != 0 && plugged != -1);| ?
> 

This is already the else branch for if (plugged == -1) so the plugged expr is unnecessary there.

> (In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> > >+    // an array of float even if we actually want a float and a boolean.
> > 
> > Ew ... gross.  I don't have a better idea though.
> 
> Gross but commented... That's half a sin, isnt't? :)

Heh.  No worries ;).

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #8)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> > >+#define GET_AND_CHECK_ANDROID_BRIDGE \
> > >+  AndroidBridge* bridge = AndroidBridge::Bridge(); \
> > >+  if (!bridge) return;
> > > 
> > 
> > Macros that affect control flow are a non-starter. 
> 
> I removed that macro but I think we might want to add a few
> FORWARD_TO_ANDROID_BRIDGE's macro that would prevent writing code like:
> returnType myFunc(paramType1 myParam1) {
>   AndroidBridge* bridge = AndroidBridge::Bridge();
>   if (!bridge) {
>     return;
>   }
> 
>   bridge->myFunc(myParam1);
> }
> 
> Though, for the moment, we only have two methods like that, we will see if
> they get more and more numerous.

A macro that had the same semantics as a single statement, but only performed an action if the bridge is available, is better (i.e. doesn't affect control flow).  Early returns in macros make for very hard-to-follow code.

Comment 11

[Mounir Lamouri (:mounir)]

I don't understand how the code would be hard to follow given that the macro would be living out of any method like:

namespace mozilla {
namespace hal {

FORWARD_TO_ANDROID_BRIDGE_VOID_1(RegisterBatteryObserver, BatteryInformation, aBatteryInfo);
FORWARD_TO_ANDROID_BRIDGE_VOID_0(UnregisterBatteryObserver);

void MyMethod() {
}

void MyOtherMethod() {
}

} // hal
} // namespace

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oh sure, that's fine if we need it.  You can use templates in that case instead of macros, but whatever is easier.

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Part A - Add a hal backend for Android

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Part B - Boilerplate code to make hal communicate with the Android Java code

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Part C - Android backend

Comment 16

[Mounir Lamouri (:mounir)]

Those patches are using the new hal API.
Chris, as requested, I will not ask for a new review but feel free to have a look and give some feedback if needed.

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a71befc6ee9
https://hg.mozilla.org/mozilla-central/rev/5ffe95aa3f68
https://hg.mozilla.org/mozilla-central/rev/044bfd439934