Last Comment Bug 696038 - Battery API: Android backend
: Battery API: Android backend
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Mounir Lamouri (:mounir)
:
: Jim Chen [:jchen] [:darchons]
Mentors:
Depends on: 703689 678694 702858
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-20 05:41 PDT by Mounir Lamouri (:mounir)
Modified: 2011-11-18 11:59 PST (History)
4 users (show)
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part A - Add a hal backend for Android (3.31 KB, patch)
2011-10-20 05:49 PDT, Mounir Lamouri (:mounir)
cjones.bugs: review+
Details | Diff | Splinter Review
Part B - Boilerplate code to make hal communicate with the Android Java code (16.37 KB, patch)
2011-10-20 05:50 PDT, Mounir Lamouri (:mounir)
cjones.bugs: review+
Details | Diff | Splinter Review
Part C - Android backend (7.17 KB, patch)
2011-10-20 05:51 PDT, Mounir Lamouri (:mounir)
cjones.bugs: review+
Details | Diff | Splinter Review
Part A - Add a hal backend for Android (3.34 KB, patch)
2011-10-28 05:20 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part B - Boilerplate code to make hal communicate with the Android Java code (17.20 KB, patch)
2011-10-28 05:21 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part C - Android backend (7.31 KB, patch)
2011-10-28 05:21 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-10-20 05:41:31 PDT

    
Comment 1 Mounir Lamouri (:mounir) 2011-10-20 05:49:34 PDT
Created attachment 568370 [details] [diff] [review]
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) 2011-10-20 05:50:34 PDT
Created attachment 568371 [details] [diff] [review]
Part B - Boilerplate code to make hal communicate with the Android Java code
Comment 3 Mounir Lamouri (:mounir) 2011-10-20 05:51:07 PDT
Created attachment 568372 [details] [diff] [review]
Part C - Android backend
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-25 18:18:59 PDT
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 2011-10-25 18:30:19 PDT
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 2011-10-25 18:36:40 PDT
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) 2011-10-26 07:29:51 PDT
(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) 2011-10-26 07:33:56 PDT
(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 2011-10-26 13:13:38 PDT
(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 2011-10-26 13:15:38 PDT
(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) 2011-10-26 16:44:05 PDT
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 2011-10-26 18:07:46 PDT
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) 2011-10-28 05:20:26 PDT
Created attachment 570222 [details] [diff] [review]
Part A - Add a hal backend for Android
Comment 14 Mounir Lamouri (:mounir) 2011-10-28 05:21:06 PDT
Created attachment 570223 [details] [diff] [review]
Part B - Boilerplate code to make hal communicate with the Android Java code
Comment 15 Mounir Lamouri (:mounir) 2011-10-28 05:21:37 PDT
Created attachment 570224 [details] [diff] [review]
Part C - Android backend
Comment 16 Mounir Lamouri (:mounir) 2011-10-28 05:22:56 PDT
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.

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