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 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 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 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)

(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? :)

(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.

(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 ;).

(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.

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

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

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.

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