Closed Bug 697641 Opened 13 years ago Closed 12 years ago

Implement proximity sensor backend for android

Categories

(Core :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13
Tracking Status
firefox11 --- wontfix

People

(Reporter: cjones, Assigned: sinker)

References

Details

(Whiteboard: [not-fennec-11])

Attachments

(5 files, 53 obsolete files)

10.17 KB, patch
Details | Diff | Splinter Review
6.96 KB, patch
Details | Diff | Splinter Review
14.52 KB, patch
Details | Diff | Splinter Review
5.79 KB, patch
Details | Diff | Splinter Review
5.28 KB, patch
cjones
: review+
Details | Diff | Splinter Review
While we wait on the discussion of the DOM API, we can go ahead and implement the gecko hal backends for the various sensors, can start here with proximity.

I think a reasonable hal API might be (roughly derived from w3c/android APIs)

  enum SensorType {
    ProximitySensor,
    //...
  };

  #define MAX_SENSOR_VALUES 3

  struct SensorData {
    SensorType sensor;
    TimeStamp timestamp;
    float values[MAX_SENSOR_VALUES];
    size_t numValues;
  };

  struct SensorCalibration {
    SensorType sensor;
    size_t numValues;
    const char* units[MAX_SENSOR_VALUES];  // XXX or maybe a Unit enum?
    const char* description;
  };

  typedef IObserver<SensorData> ISensorObserver;

  void RegisterSensorObserver(SensorType sensor, ISensorObserver* observer);
  void UnregisterSensorObserver(SensorType sensor, ISensorObserver* observer);
  SensorCalibration GetCalibration(SensorType sensor);

Thinker, I would start here by adding an android hal backend (hal/android/AndroidHal.cpp) that registers to listen for updates to the android TYPE_PROXIMITY sensor.  The upper-level hal work here blocks on some new code we're adding in bug 678694.  A good example to start from is the battery backend in bug 696038.

After this, we can look at a Gonk implementation and start connecting the dots to the DOM API.
So, do you want to use this hal to replace existing code about sensors?
No, let's not worry about the existing code right now.  These two new sensors are probably going to have web APIs that are different than how the current ones (gps, accelerometer) are exposed, so let's keep going on the hal backends.

Eventually, it would make sense to move all the implementations to hal/, but that's not worth the time right now.
0014-Testing-page-for-proximity-sensor.patch will modify URL of default home for testing.  It should not be committed.
Samsung Galaxy S2 has a proximity sensor near its earphone.  It detects closing of your finger or any other part of body.  Once you close to earphone, testing page will show a distance value.  For my S2, it is 0 for closing, 5 for leaving.
Wow, this is fantastic!

Can you give a brief overview of what these patches do, so we can figure out what parts might go in bug 697361 and who should review what?
Comment on attachment 571261 [details] [diff] [review]
0001-Add-HAL-for-sensor-into-build-system.patch

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

This patch makes hal/android/ as a part of build system, to join the building process.
Comment on attachment 571262 [details] [diff] [review]
0002-Pass-events-from-Dalvik-Java-code-to-Gecko.patch

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

This patch make pass sensor events from Dalvik side to Gecko.  It goes through GeckoAppShell.java, AndroidBridge.cpp, nsAppShell.cpp, and end at Sensor.cpp.  Sensor.cpp dispatches events to observers (ISensorObserver) from Gecko and from other native modules.
Comment on attachment 571263 [details] [diff] [review]
0003-Dig-it-through-from-Gecko-to-Android-for-Sensor-feat.patch

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

This patch enable proximity sensor while first observer is registered, and disable it while all observers are removed.
Comment on attachment 571264 [details] [diff] [review]
0004-Add-nsSensorManager-component.patch

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

This patch defines and implements nsISensorManager and relative interfaces.  It adapts class Sensor to XPCOM.
Comment on attachment 571266 [details] [diff] [review]
0005-Register-nsSensorManager-component-to-Gecko.patch

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

This patch makes nsISensorManager and relative code as a part of layout module, and registers nsSensorManager component with Gecko to make it available for JS code.
Comment on attachment 571267 [details] [diff] [review]
0006-Implement-QueryInterface-for-nsSensorManager-to-supp.patch

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

Fix QueryInterface of nsSensorManager component to support casting from nsISupports to nsISensorManager.
Comment on attachment 571268 [details] [diff] [review]
0007-Map-sensor-type-correctly-for-sensor-backend-HAL.patch

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

Fix mapping table of sensor types from native to dalvik.  This table is hard coded.  It should keep synchronized with what defined in GeckoAppShell.java.  We need a way to keep this table updated to date automatically.  (By the way, I don't like to map IDs from native space to dalvik space with a simple math rule.  A mapping table is more flexible.  Although, it is not always necessary, or over engineering.  But, it is a kind of taste.)
Other patches (0008~00013) are bug fixing and refactoring.
Hi Thinker, these patches will need a few cleanups to match the conventions we use for Gecko code.  (Sorry, conventions are annoying at first, but you'll get used to them :/.)

The first convention is to have a series of patches that are "clean".  That is, if you find a bug in one of your earlier patches X after you've written later patches Y and Z, then the convention is to merge the fix back into the earlier patch X.  There are two reasons for that
 (1) Accumulating "bugfix" patches doesn't scale; you might end up with many more of those than "real" patches.  It's a better use of both your time and the reviewers' time for there to only be "real" patches.
 (2) Reviewers will be looking for bugs in your earlier "real" patches, and will likely spot some of the bugs you fix in the later "bugfix" patches.  If reviewers have to check the end of your patch queue every time they spot a potential bug in an earlier patch, to see if you've already fixed it, it's not a good use of their time.
 (3) Some people prefer to have a "clean" commit history, which leads to better blame and logs in version control systems.

Unfortunately, this convention doesn't fit very well the usual git workflow of committing often.  For mozilla-central work, most people use the Mercurial mq extension[1] to manage their patch queues.  If you prefer to work in git, there's a similar tool called StGit[2], but I haven't tried it.

The second convention is dividing patches up by "interface" and "implementation".  Where to draw the line is often a matter of taste, but one common case is when you add a new IDL interface like nsISensorManager.  Newly-added IDL interfaces should usually go in a separate patch, but note that people will often add several new interfaces in the same patch.  The reason for this separation is that newly-added interfaces usually require "super-review"[3].  The person from whom you request super-review (sr) is different from the person from whom you request review (r), and having separate patches helps super-reviewers (who are usually very busy) review your code more quickly.  It's also easier to think more carefully about new interfaces when they're separate from the implementation details.

You've done a great job here of dividing your patches into logical, "bite-sized" chunks of work.  That really helps reviewers review code more quickly.  That's the third convention I was going to mention, but you've already got it down! :)

Like I said, conventions are somewhat annoying at first, but I've found these work pretty well for writing Gecko code.

[1] http://mercurial.selenic.com/wiki/MqExtension
[2] http://www.procode.org/stgit/
[3] http://www.mozilla.org/hacking/reviewers.html
Thinker, I think it would be best for the nsISensorManager patches to go in bug 697361.  Those will be more closely related to the DOM API.  (Also, I can't review those :).)  Everything at the hal/ level and below can go here, and we can land that work without waiting on the DOM side.
Oh, there's actually one more convention I forgot to mention:

When you have a series of patches, make sure they all build on their own.  That is, if you have a series X, Y, and Z, make sure that mozilla-central still builds and runs cleanly after applying X, X+Y, and X+Y+Z.  This is usually easy to do; if you're added new IDL interfaces, for example, you can have a patch that just |hg add|/|git add|s the files, but doesn't compile them.  Then no build or runtime bustage is possible.

The reason this is important is for bisecting over the commit history.  When we find a new bug, often the first thing we do is find the first commit that resulted in the bug.  Then you know exactly what code to look in, to find the cause of the bug.  When the commit history has a lot of broken internal states, bisecting becomes much harder to do.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #29)
> When you have a series of patches, make sure they all build on their own. 
> That is, if you have a series X, Y, and Z, make sure that mozilla-central
> still builds and runs cleanly after applying X, X+Y, and X+Y+Z.  This is
> usually easy to do; if you're added new IDL interfaces, for example, you can
> have a patch that just |hg add|/|git add|s the files, but doesn't compile
> them.  Then no build or runtime bustage is possible.

Having them build is very important otherwise it would make bisecting much harder as said Chris. Having them pass tests would be nice too. It probably doesn't apply here but that's something to keep in mind I think.
Attached patch 01-define-nsISensorManager.patch (obsolete) — Splinter Review
Defines nsISensorManger and relative interfaces
Attachment #571261 - Attachment is obsolete: true
Attachment #571262 - Attachment is obsolete: true
Attachment #571263 - Attachment is obsolete: true
Attachment #571264 - Attachment is obsolete: true
Attachment #571266 - Attachment is obsolete: true
Attachment #571267 - Attachment is obsolete: true
Attachment #571268 - Attachment is obsolete: true
Attachment #571269 - Attachment is obsolete: true
Attachment #571271 - Attachment is obsolete: true
Attachment #571272 - Attachment is obsolete: true
Attachment #571273 - Attachment is obsolete: true
Attachment #571274 - Attachment is obsolete: true
Attachment #571275 - Attachment is obsolete: true
Attachment #571276 - Attachment is obsolete: true
Make nsISensorManger.idl to be built
Attached patch 03-HAL-android-sensor.patch (obsolete) — Splinter Review
A bridge of Android Sensor manager API for Gecko

With this interface, Gecko code can leverage capabilities of accessing
sensors provied by Android.
Attachment #571685 - Attachment is patch: true
Add nsSensorManager component to implement nsISesnorManager interface
Pass events from Dalvik Java code to Gecko
Export Sensor Manager API of Android to Gecko
Attached patch 07-add-to-build-system.patch (obsolete) — Splinter Review
Add hal/android subdirectory to build system
Register nsSensorManager component to Gecko in nsLayoutModule.cpp
(In reply to Chris Jones [:cjones] [:warhammer] from comment #28)
> Thinker, I think it would be best for the nsISensorManager patches to go in
> bug 697361.  Those will be more closely related to the DOM API.  (Also, I
> can't review those :).)  Everything at the hal/ level and below can go here,
> and we can land that work without waiting on the DOM side.

But, I can not build reset patches without nsISensorManager.  It would break build process if nsISesnorManager being removed from here.
Hmm ... I don't understand why that is.  Can you explain?  In general we're trying to avoid having hal/ code depend on XPCOM components.

The advantage to splitting the work is that we can land the hal/ code as soon as possible, and you won't need to keep maintaining those patches.  (They won't "bitrot" ;).  But if we need nsISensorManager here, we can bring in the right people.
nsSensorManager is a XPCOM component that implements nsISensorManager.  Without this idl, how do you compile nsSensorManager?
But the way, we may open another bug for nsSensorManager and nsISensorManager.  The reset patches can be built without them.
How about to move both nsISensorManager and nsSensorManager to bug 697361?
(In reply to Thinker Li from comment #43)
> How about to move both nsISensorManager and nsSensorManager to bug 697361?

Yes, that's what I meant --- sorry about the miscommunication.

Let's get the backend landed! :)
Attachment #571685 - Attachment is obsolete: true
Comment on attachment 571686 [details] [diff] [review]
02-add-nsISensorManager-to-build.patch

move to bug 697361
Attachment #571686 - Attachment is obsolete: true
Comment on attachment 571688 [details] [diff] [review]
04-nsSensorManager-component.patch

Move to bug 697361
Attachment #571688 - Attachment is obsolete: true
Comment on attachment 571692 [details] [diff] [review]
08-register-nsSensorManager-in-nsLayoutModule.patch

Move to bug 697361
Attachment #571692 - Attachment is obsolete: true
Comment on attachment 571687 [details] [diff] [review]
03-HAL-android-sensor.patch

Hi thinker,

Most of this patch looks OK, but there are a few more things that need to be fixed up:

 - Make sure you generate the patches with the --git options and 8 lines of context.  With mercurial, you can set the following options in your .hgrc

[diff]
git = 1
showfunc = 1
unified = 8

It's usually better to work off of hg.mozilla.org instead of the git clone, but if you work in git, make sure you set similar options.

 - Please add a commit message to your patches that looks like

Bug 697641, part 1: Added a hal:: API for sensor access. r=cjones

We have tools that know how to parse the "Bug XXXXXX" and "r=[reviewer]" parts, so it's good to stick to convention there.  People often put "part [i]" for multi-part patches, but our tools don't care about that.

 - When you submit your patches for review, in the "Description:" field in bugzilla, please enter the last part of your commit message.  For example

  Description: [part 1: Added a hal:: API for sensor access]

This is often enough information to let reviewers know what the patch is supposed to do.  It also helps to write a note in the "Comment:" field on the attachments page.

Another tradition: if you need to submit multiple versions of the same patch, for example because reviewers request changes, it's customary to add a "v[i]" note to the description, too.  For example,

 (First version)   part 1: Added a hal:: API for sensor access
 (Second version)  part 1: Added a hal:: API for sensor access, v2
 (Third version)   part 1: Added a hal:: API for sensor access, v3

and so on.

OK, now on to the contents of the patch.

 1. This patch needs to add an interface to hal/ for sensors.  Basically, the code you added to hal/android/Sensor.h here, needs to be in hal/Hal.h.  Hal.h does some magic to generate interfaces for platform-specific backends (hal_impl), the proxying backend we use in content process (hal_sandbox), and an interface in hal:: that knows which of hal_impl and hal_sandbox to call.  We need this "layer of indirection" because we'll have multiple implementations of any particular API.  For the sensors here, we'll have an implementation for android that's completely different from the gonk implementation.  And we also always need "sandbox" and "fallback" implementations.

 2. You need a "fallback" implementation of the hal:: sensor API.  See hal/fallback/FallbackHal.cpp.  This is a "no-op" implementation for platforms that don't support this sensor API.

Sorry again for the administrative overhead here, but you only need to learn it once :).  It'll be easier after this.

When you post updated patches, please request me for review.  In the "review" dropdown box, please mark "Requestee: :cjones".
Sensor API interface and an implementation for Android.
Attachment #571687 - Attachment is obsolete: true
With this API, Gecko code can access sensors provied by system.  This
patch include an implementation for Android.
Attachment #573789 - Attachment is obsolete: true
Attachment #574250 - Flags: review?(jones.chris.g)
Attachment #571690 - Attachment is obsolete: true
Attachment #574251 - Flags: review?(jones.chris.g)
Modify Makefiles to include impl. of sensor API in building procedure.
Attachment #571691 - Attachment is obsolete: true
Attachment #574252 - Flags: review?(jones.chris.g)
Add sensor APIs to PHal protocol to make them available for sandbox.
Attachment #574254 - Flags: review?(jones.chris.g)
Attachment #571689 - Attachment is obsolete: true
Attachment #574256 - Flags: review?(jones.chris.g)
Comment on attachment 574250 [details] [diff] [review]
part 1: Add a hal API for sensor access, v2

>diff --git a/hal/Hal.h b/hal/Hal.h
>--- a/hal/Hal.h
>+++ b/hal/Hal.h
>@@ -1,10 +1,10 @@
>-/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>-/* vim: sw=2 ts=8 et ft=cpp : */
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* vim: sw=4 ts=8 et ft=cpp : */

Nit: Please don't change the modeline; Gecko coding style prefers
2-space indent.  (I hate it, personally, but that's been agreed on
... :/)

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Indentation

> #include "base/basictypes.h"
> #include "mozilla/Types.h"
> #include "nsTArray.h"
>+#include "hal_sensor.h"
> 

Nit: The naming style for headers is "LikeThis.h".  (I don't like
*that*, either, but again it's the style ... ;).

But in this case, I think the sensor API is small enough that you can
simply include it here, inline.  That is, just below the definition of
|class WindowIdentifier|, add the contents of your hal_sensor.h file.
Please keep the definition of the sensor datatypes in the mozilla::hal
namespace, like WindowIdentifier is.

> #ifndef MOZ_HAL_NAMESPACE
> // This goop plays some cpp tricks to ensure a uniform API across the
> // API entry point, "sandbox" implementations (for content processes),
> // and "impl" backends where the real work happens.  After this runs
> // through cpp, there will be three sets of identical APIs
> //   hal_impl:: --- the platform-specific implementation of an API.
> //   hal_sandbox:: --- forwards calls up to the parent process
>@@ -70,16 +71,30 @@ namespace MOZ_HAL_NAMESPACE /*hal*/ {
>  * by |pattern|.  Each element in the pattern is the number of
>  * milliseconds to turn the vibrator on or off.  The first element in
>  * |pattern| is an "on" element, the next is "off", and so on.
>  *
>  * If |pattern| is empty, any in-progress vibration is canceled.
>  */
> void Vibrate(const nsTArray<uint32>& pattern);
> 
>+/** Register an observer for the sensor of given type.
>+ *
>+ * The observer will receive data whenever the data generated by the
>+ * sensor is avaiable.
>+ */
>+void RegisterSensorObserver(mozilla::hal::sensor::SensorType sensor,
>+                            mozilla::hal::sensor::ISensorObserver *observer);

Nit: We try to limit namespace nesting to two deep; for example, at
most "mozilla::foo".  I don't think defining a "mozilla::hal::sensor"
adds any benefit compared to the extra characters needed to type it
out.  Let's stick to mozilla::hal.

In these declarations, since you're already within the "mozilla"
namespace, you can shorten the typenames to

  void RegisterSensorObserver(hal::SensorType aSensor,
                              hal::ISensorObserver* aObserver);

for example.

You're naming the arguments as |sensor|, |observer|, but (*very*
sadly) Gecko convention uses an "a" prefix for them.  For example,
|aSensor|, |aObserver|.  I *really* dislike that style, but again it's
part of Gecko convention, so let's stay consistent.

>+/** Post a value generated by proximity sensor. */
>+void PostProximityEvent(double distance);

Let's call this |NotifyProximitySensorChange(double distance)| for consistency with
the battery API.

Please add a comment here noting that this API shouldn't be called
directly by hal clients.  For example,

/**
 * Post a value generated by proximity sensor.
 *
 * This API is internal to hal; clients shouldn't call it directly.
 */

The |NotifyProximitySensorChange()| function should only be defined in
the hal:: namespace, not the MOZ_HAL_NAMESPACE (because we don't need
backend-specific implementations of it), but I see that Mounir defined
NotifyBatteryChange() in the MOZ_HAL_NAMESPACE so no worries for now.
If you feel like fixing NotifyBatteryChange(), please do :).

(In the future, we probably want to make a hal_internal header and/or
namespace for these, but that's not needed yet, I don't think.  Let's
not worry about it for this bug.)

>diff --git a/hal/android/Sensor.cpp b/hal/android/Sensor.cpp

>+#include "mozilla/Hal.h"
>+#include "nsTArray.h"
>+#include "AndroidBridge.h"
>+
>+using namespace mozilla::hal::sensor;
>+
>+namespace mozilla {
>+namespace hal_impl {
>+
>+static nsTArray<ISensorObserver *> *hal_observers;
>+

Nit: please call this |gSensorObservers|.  The "g" prefix is the the
naming style for static global variables (which is another of the
things that I hate....).

Non-nit: this array needs to be freed before Firefox shuts down, or
else it will show up as a leak and cause some tests to fail.  The
easiest way to do this is just free |gSensorObservers| whenever the
last listener is removed.  Then the next time a listener is added,
allocate |gSensorObservers| again.

Instead of using a raw nsTArray, you can instead use the
ObserverList<T> helper that Mounir added.  See the example in Hal.cpp,
for BatteryInformation.  So the declaration here would be

  typedef ObserverList<ISensorObserver> SensorObserverList;
  static SensorObserverList* gSensorObservers;

>+static nsTArray<ISensorObserver *> &
>+_get_observers(SensorType sensor_type) {

Nit: naming style is |GetObservers(SensorType aSensorType)|.

>+static int sensor_type_2_android_map[][2] = {
>+    // Must be consistent with definitions in
>+    // embedding/android/GeckoAppShell.java
>+    { SENSOR_ORIENTATION, 1 },
>+    { SENSOR_ACCELERATION, 2 },
>+    { SENSOR_PROXIMITY, 3 },
>+    { -1, -1 }
>+};
>+

Why not re-use |enum SensorType| as indexes into gHalObservers?
You're already using NUM_SENSOR_TYPE to allocate the array, which is
great!  That would mean that the android .java file needs to stay in
sync with the values in Hal.h, instead of the other way around.

You can make sure the first element in the enumeration has the value
0, and then use the enumeration constants to index the sensor-listener
array.  For example,

 enum SensorType {
     SENSOR_UNKNOWN = -1,
     SENSOR_ORIENTATION = 0,
     SENSOR_ACCELERATION,
     SENSOR_PROXIMITY,
     NUM_SENSOR_TYPE
 };

 //...
 gSensorObservers[SENSOR_ORIENTATION]/*...*/

Does that make sense?  If you make that change, then you can get rid
of sensor_type_2_android_map here.

>+void PostProximityEvent(double distance) {

>+    sdata = new SensorData();

There's no need to allocate a new SensorData on the heap here.  You
can allocate it on the stack instead.

>+    for(i = 0; i < observers.Length(); i++) {
>+        observer = observers[i];
>+        observer->observe(sdata);
>+    }

The ObserverList helper will do this for you.

>diff --git a/hal/hal_sensor.h b/hal/hal_sensor.h

(As noted above, it probably makes sense to move this into Hal.h.)

>+struct SensorData {

It would be a lot easier to define this in hal/sandbox/PHal.ipdl so
that it gets automatically serialized/deserialized for you when sent
across processes.  See |struct BatteryInformation {| in PHal.ipdl for
an example.  (Sorry, I didn't mention this in comment 0.)

>+    SensorType sensor;
>+    PRTime timestamp;
>+    float values[MAX_SENSOR_VALUES];
>+    size_t numValues;
>+};

In IPDL, this would be declared as follows

 struct SensorData {
   SensorType type;
   TimeStamp timestamp;
   float[] values;
 };

>+
>+template<typename T>
>+class IObserver {

We already have a helper for this, defined in xpcom/glue/Observer.h.
Let's use that here.

OK, there are a few things to clean up here, but it's looking mostly
OK :).  We're almost done with all the initiation into Gecko code
... ;)

Please be sure to ping me with questions if you have them :).
Attachment #574250 - Flags: review?(jones.chris.g)
Comment on attachment 574251 [details] [diff] [review]
part 2: Export Android Sensor Manager API to Gecko, v2

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

>+    private static final int SENSOR_ORIENTATION = 1;
>+    private static final int SENSOR_ACCELERATION = 2;
>+    private static final int SENSOR_PROXIMITY = 3;
>+

Please add a comment here about keeping these values synchronized with
the |enum| in Hal.h.

>diff --git a/widget/src/android/AndroidBridge.h b/widget/src/android/AndroidBridge.h

>+    void EnableSensor(int aSensorType);
>+
>+    void DisableSensor(int aSensorType);
>+

Please use your |SensorType| enum type from Hal.h here.

Otherwise looks good.  Would like to see the second version of this
patch.
Attachment #574251 - Flags: review?(jones.chris.g)
Comment on attachment 574252 [details] [diff] [review]
part 3: Add impl. of Sensor HAL to build system, v2

You don't need this patch anymore.
Attachment #574252 - Flags: review?(jones.chris.g)
Comment on attachment 574254 [details] [diff] [review]
part 4: Make Sensor API available for Sandbox

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp
>--- a/hal/Hal.cpp
>+++ b/hal/Hal.cpp
>@@ -68,10 +68,33 @@ InSandbox()
> 
> void
> Vibrate(const nsTArray<uint32>& pattern)
> {
>   AssertMainThread();
>   PROXY_IF_SANDBOXED(Vibrate(pattern));
> }
> 
>+void
>+RegisterSensorObserver(mozilla::hal::sensor::SensorType sensor,
>+                       mozilla::hal::sensor::ISensorObserver *observer)

Nit: Since we're already in |namespace hal| here, you can simply declare

 void
 RegisterSensorObserver(SensorType sensor,
                        ISensorObserver *observer)
>+{
>+  AssertMainThread();
>+  PROXY_IF_SANDBOXED(RegisterSensorObserver(sensor, observer));
>+}
>+

Since the sensor-data broadcasting code is the same across all
backends, you don't need to proxy it --- you can keep the list of
listeners here in Hal.cpp.  Take a look at what the BatteryInfo code
does.


> } // namespace hal
> } // namespace mozilla
>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl

>+    
>+    RegisterSensorObserver(int32 sensor, intptr_t observer);
>+    UnregisterSensorObserver(int32 sensor, intptr_t observer);

Please use |SensorType| here instead of |int32|.  You don't want to
proxy the |observer| parameter over IPC.  The pointer to an observer
in a content process doesn't have any meaning in another process.

You'll want to copy what the BatteryInfo backend does here and use a
"semi-private" |EnableSensorNotifications(SensorType)| message here.

>+    PostProximityEvent(double distance);
>+

You shouldn't need to send this value up from content processes.  I
think this message is unnecessary.

>+child:
>+    SensorObserve(intptr_t observer, int32 sensor,
>+                  PRTime timestamp, float[] values);
> 

When you define |struct SensorData| here in PHal.ipdl in the earlier
patch, please use that struct here.  (Using TimeStamp instead of
PRTime.)

>diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp

I think most of this code will change when Hal.cpp is managing the
sensor listeners.

Again, you'll want to copy what the BatteryInfo API does for proxying
enable/disable sensor notifications for a particular type.
Attachment #574254 - Flags: review?(jones.chris.g)
Attachment #574256 - Flags: review?(jones.chris.g) → review+
Thinker, I've requested a fair number of changes here, but most of them are either minor style fixes, or some code reorganization that should result in a bit less code.  I think it would help for you to read over the implementation of the BatteryInfo API and see how it works.  That's pretty much how we want the sensor backend to work, too, I think.

And of course, please ping me if you have questions :).
(In reply to Chris Jones [:cjones] [:warhammer] from comment #58)
> 
> >+    
> >+    RegisterSensorObserver(int32 sensor, intptr_t observer);
> >+    UnregisterSensorObserver(int32 sensor, intptr_t observer);
> 
> Please use |SensorType| here instead of |int32|.  You don't want to
> proxy the |observer| parameter over IPC.  The pointer to an observer
> in a content process doesn't have any meaning in another process.
|observer| is only used as a cookie to distinguish observers.
By the way, I will move to enable & disable style API, and maintain a list of observers for each process.
Target Milestone: --- → mozilla10
Version: Trunk → Other Branch
Target Milestone: mozilla10 → ---
Version: Other Branch → Trunk
(In reply to Chris Jones [:cjones] [:warhammer] from comment #55)
> >+static int sensor_type_2_android_map[][2] = {
> >+    // Must be consistent with definitions in
> >+    // embedding/android/GeckoAppShell.java
> >+    { SENSOR_ORIENTATION, 1 },
> >+    { SENSOR_ACCELERATION, 2 },
> >+    { SENSOR_PROXIMITY, 3 },
> >+    { -1, -1 }
> >+};
> >+
> 
> Why not re-use |enum SensorType| as indexes into gHalObservers?
> You're already using NUM_SENSOR_TYPE to allocate the array, which is
> great!  That would mean that the android .java file needs to stay in
> sync with the values in Hal.h, instead of the other way around.
> 
> You can make sure the first element in the enumeration has the value
> 0, and then use the enumeration constants to index the sensor-listener
> array.  For example,
> 
>  enum SensorType {
>      SENSOR_UNKNOWN = -1,
>      SENSOR_ORIENTATION = 0,
>      SENSOR_ACCELERATION,
>      SENSOR_PROXIMITY,
>      NUM_SENSOR_TYPE
>  };
> 
>  //...
>  gSensorObservers[SENSOR_ORIENTATION]/*...*/
> 
> Does that make sense?  If you make that change, then you can get rid
> of sensor_type_2_android_map here.
> 

It will introduce implicit relationship between two type coding systems.
I am not sure it is a good idea to introduce this implicit relationship and potential of silent fault by breaking rule later.
TBH, I expect us to only add to the enum values, never change them after the fact.  So I'm not so worried.  We should assert that the values are within range when using them, and if something does change a value in the .java unexpectedly, then we'll catch that with tests.
Attachment #574251 - Attachment is obsolete: true
Attachment #575392 - Flags: review?(jones.chris.g)
Attachment #574250 - Attachment is obsolete: true
Attachment #574252 - Attachment is obsolete: true
Attachment #575393 - Flags: review?(jones.chris.g)
Attachment #575392 - Attachment is obsolete: true
Attachment #575392 - Flags: review?(jones.chris.g)
Attachment #575395 - Flags: review?(jones.chris.g)
Attachment #575393 - Attachment is obsolete: true
Attachment #575393 - Flags: review?(jones.chris.g)
Attachment #575396 - Flags: review?(jones.chris.g)
Attachment #574254 - Attachment is obsolete: true
Attachment #575397 - Flags: review?(jones.chris.g)
Attachment #575397 - Attachment is patch: true
Attachment #574256 - Attachment is obsolete: true
Attachment #575398 - Flags: review?(jones.chris.g)
Attachment #575395 - Flags: review?(jones.chris.g) → review+
Comment on attachment 575396 [details] [diff] [review]
part 2: Add a hal API for sensor access, v3

>diff --git a/hal/Hal.h b/hal/Hal.h

>+
>+/**
>+ * Enable sensor notifications from the backend
>+ *
>+ * This method is only visible from implementation of sensor manager.
>+ * Rest of the system should not try this.
>+ */
>+void EnableSensorNotifications(hal::SensorType aSensor);
>+
>+/**
>+ * Disable sensor notifications from the backend
>+ *
>+ * This method is only visible from implementation of sensor manager.
>+ * Rest of the system should not try this.
>+ */
>+void DisableSensorNotifications(hal::SensorType aSensor);
>+
>+/**
>+ * Register an observer for the sensor of given type.
>+ *
>+ * The observer will receive data whenever the data generated by the
>+ * sensor is avaiable.
>+ */
>+void RegisterSensorObserver(hal::SensorType aSensor,
>+                            hal::ISensorObserver *aObserver);
>+
>+/**
>+ * Unregister an observer for the sensor of given type.
>+ */
>+void UnregisterSensorObserver(hal::SensorType aSensor,
>+                              hal::ISensorObserver *aObserver);
>+

This is a minor nit, but please move the Register*()/Unregister*()
above the Enable*()/Disable*() functions.  Register*()/Unregister*()
is the "public" API, the ones that clients should use, so it would be
good for clients to see those functions first.

>+/**
>+ * Post a value generated by a sensor.
>+ *
>+ * This API is internal to hal; clients shouldn't call it directly.
>+ */
>+void NotifySensorChange(const hal::SensorData &aSensorData);
>+
>+/**
>+ * Post a value generated by proximity sensor.
>+ *
>+ * This API is internal to hal; clients shouldn't call it directly.
>+ */
>+void NotifyProximityChange(double aDistance);
>+

Since this function is a subset of the NotifySensorChange() function
above, how about we get rid of it in favor of the more general
NotifySensorChange()?  That will keep the API surface area smaller.
It's not clear which of these two functions clients should use.

>diff --git a/hal/HalSensor.h b/hal/HalSensor.h

>+struct SensorData {
>+  SensorType sensor;
>+  PRTime timestamp;
>+  float values[MAX_SENSOR_VALUES];
>+  size_t numValues;
>+};
>+

As I requested in comment 55, let's define this |SensorData| struct in
PHal.ipdl.  That way it will be automatically serialized/deserialized,
and we can remove the |numValues| field.

We need to have units for the sensor values given to clients.  If we
don't do that, then each client (e.g. the DOM API) will have to have
platform-specific code to interpret the sensor value, which is what
hal is designed to avoid.  What unit does the value returned by the
android backend have?  (Meters, centimeters, ...?)  We need to
document that.

>+#endif /* __HAL_SENSOR_H_ */
>diff --git a/hal/Makefile.in b/hal/Makefile.in

>+ifeq (Android,$(OS_TARGET))
>+CPPSRCS += \
>+  AndroidSensor.cpp \
>+  $(NULL)

Put this under the existing |ifeq (Android,$(OS_TARGET))| block above.

>+else
>+CPPSRCS += \
>+  FallbackSensor.cpp \

And similarly here, under the existing |else| above.

>diff --git a/hal/android/AndroidSensor.cpp b/hal/android/AndroidSensor.cpp

>+typedef ObserverList<SensorData> SensorObserverList;
>+static SensorObserverList *gSensorObservers = NULL;
>+

This observer code is generic; there's nothing android-specific about
it.  To avoid duplicating it for all the different backends, let's
move it into a common source file.  Hal.cpp is fine.  See how the
battery code was written, for an example.

>+static SensorObserverList &
>+_GetObservers(SensorType sensor_type) {

Nit: As I noted in the first review, naming style is |GetObservers()|,
no leading underscore.

>+/**
>+ * Translate ID of sensor type from Sensor service to Android.
>+ *
>+ * Must be consistent with the definition of sensor types in
>+ * embedding/android/GeckoAppShell.java
>+ */
>+static int
>+_MapSensorType(SensorType sensor_type) {

MapSensorType().

>+  if(sensor_type == SENSOR_UNKNOWN)
>+    return -1;
>+  return sensor_type + 1;

Please assert that the returned value is less than NUM_SENSOR_TYPE.

Looking good! :) I'd like to see one more version with the above
fixed, please.  If you have any questions, please feel free to ask me.
Attachment #575396 - Flags: review?(jones.chris.g)
Comment on attachment 575397 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v3

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp

>+void
>+RegisterSensorObserver(SensorType aSensor, ISensorObserver *aObserver)
>+{
>+  AssertMainThread();
>+  hal_impl::RegisterSensorObserver(aSensor, aObserver);
>+}
>+
>+void
>+UnregisterSensorObserver(SensorType aSensor, ISensorObserver *aObserver)
>+{
>+  AssertMainThread();
>+  hal_impl::UnregisterSensorObserver(aSensor, aObserver);
>+}
>+
>+void
>+NotifySensorChange(const hal::SensorData &aSensorData) {
>+  AssertMainThread();
>+  hal_impl::NotifySensorChange(aSensorData);
>+}
>+
>+void
>+NotifyProximityChange(double aDistance)
>+{
>+  AssertMainThread();
>+  hal_impl::NotifyProximityChange(aDistance);
>+}
>+

(Note that with the changes I requested for the previous patch, all
these functions will change.)

>diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl

>+parent:    
>+    EnableSensorNotifications(int32 aSensor);
>+    DisableSensorNotifications(int32 aSensor);
>+

Instead of int32, use |SensorType| here.  Let's add a serializer for
SensorType.  See the serializer for |mozilla::PixelFormat| in
ipc/glue/IPCParamTraits.h to see how serializers for enums should be
defined.  (Basically, check that the value is legal, then write it as
int32, and similarly when reading.)

>diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp

>+  void Notify(const SensorData& aSensorData) {
>+    bool r;
>+    
>+    r = SendNotifySensorChange(aSensorData);
>+    NS_ASSERTION(r, "HalParent::SendNotifySensorChange() is fail");

Don't need to assert here; it's OK if this call fails.  See how the
battery code deals with this return value.

Again, looking good!  Would like to review the next version.
Attachment #575397 - Flags: review?(jones.chris.g)
Comment on attachment 575398 [details] [diff] [review]
part 4: Pass sensor events from Android to Gecko, v3

Looks good.  We can normalize the android sensor data to a known unit
in a later patch, if you wish.
Attachment #575398 - Flags: review?(jones.chris.g) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #69)
> Comment on attachment 575396 [details] [diff] [review] [diff] [details] [review]
> part 2: Add a hal API for sensor access, v3
> 
> >diff --git a/hal/HalSensor.h b/hal/HalSensor.h
> 
> >+struct SensorData {
> >+  SensorType sensor;
> >+  PRTime timestamp;
> >+  float values[MAX_SENSOR_VALUES];
> >+  size_t numValues;
> >+};
> >+
> 
> As I requested in comment 55, let's define this |SensorData| struct in
> PHal.ipdl.  That way it will be automatically serialized/deserialized,
> and we can remove the |numValues| field.
> 
> We need to have units for the sensor values given to clients.  If we
> don't do that, then each client (e.g. the DOM API) will have to have
> platform-specific code to interpret the sensor value, which is what
> hal is designed to avoid.  What unit does the value returned by the
> android backend have?  (Meters, centimeters, ...?)  We need to
> document that.
I had tried to move SensorData to PHal.ipdl.  But, parser of IPDL is not supporting |enum| type now, if I am right.  It means we should stop using enum type at monent, or cast to and back every time accessing SensorData::sensor.  That is why I keep it untouched in last patch.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #70)
> Comment on attachment 575397 [details] [diff] [review] [diff] [details] [review]
> part 3: Make Sensor API available for Sandbox, v3
> 

> >diff --git a/hal/sandbox/PHal.ipdl b/hal/sandbox/PHal.ipdl
> 
> >+parent:    
> >+    EnableSensorNotifications(int32 aSensor);
> >+    DisableSensorNotifications(int32 aSensor);
> >+
> 
> Instead of int32, use |SensorType| here.  Let's add a serializer for
> SensorType.  See the serializer for |mozilla::PixelFormat| in
> ipc/glue/IPCParamTraits.h to see how serializers for enums should be
> defined.  (Basically, check that the value is legal, then write it as
> int32, and similarly when reading.)
As I have mentioned, I can not use |enum| type with IPDL, now.
If I am wrong, please correct me.
enums are just C++ types like any other.  For an |enum Foo|, you can do

  using Foo;

in the .ipdl file to "import" the enum type.  Then you can use |enum Foo| as |Foo| in message declarations, e.g. |async Message(Foo f);|.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #74)
> enums are just C++ types like any other.  For an |enum Foo|, you can do
> 
>   using Foo;
> 
> in the .ipdl file to "import" the enum type.  Then you can use |enum Foo| as
> |Foo| in message declarations, e.g. |async Message(Foo f);|.
I had tried it.  But, it yells

  /root/mozilla-central/ipc/chromium/src/chrome/common/ipc_message_utils.h:80: error: 'Read' is not a member of 'IPC::ParamTraits<mozilla::hal::SensorType>'

I must define |IPC::ParamTraits<mozilla::hal::SensorType>|.  I think it is more reasonable that code generator do it for us.  That is why I stop it here.  I can workaround it by defining |IPC::ParamTraits<...>|.  But, should I do it now?  Or, Should I keep it untouched, now, open another bug for supporting |enum| in IPDL, and go back later?
The IPDL compiler doesn't know anything about "imported" C++ types (by design), so it can't generate the serialization code automatically.  I've thought about adding enums to the IPDL language, but the motivation isn't strong.

Let's go ahead and define the ParamTraits for SensorType.  One thing that might help is a helper template for serializing enums.  Something like

 template<typename E, E lowGuard, E highGuard>
 struct EnumSerializer {
   //...
   static bool IsLegal(E aValue) { return lowGuard < aValue < highGuard; }
   //...
 };

(For the "//..." parts, you can copy from the example in comment 70.)

Then for a particular enum,

 template<> 
 struct ParamTraits<mozilla::hal::SensorType> :
   public EnumSerializer<mozilla::hal::SensorType,
                         mozilla::hal::SENSOR_UNKNOWN, mozilla::hal::NUM_SENSOR_TYPE>
 {};

Does that make sense?  If you don't want to add the helper for this bug, that's fine, we can do it in a followup.
That is exactly what I think.  Another issue is mutually inclusion.  If we define SensorData in PHal.ipdl and it includes Hal.h, we also need to include PHal.h in Hal.h for SensorData, or clients should include PHal.h instead of Hal.h even they don't know IPC.  It is not so reasonable.  These is two solutions,
 1. provide a factory function for SensorData to hide detial of SensorData from rest of the world, and
 2. Introduce another ipdl file.
What do you think?
(In reply to Thinker Li from comment #77)
> That is exactly what I think.  Another issue is mutually inclusion.  If we
> define SensorData in PHal.ipdl and it includes Hal.h, we also need to
> include PHal.h in Hal.h for SensorData, or clients should include PHal.h
> instead of Hal.h even they don't know IPC.  It is not so reasonable.  These
> is two solutions,
>  1. provide a factory function for SensorData to hide detial of SensorData
> from rest of the world, and
>  2. Introduce another ipdl file.
> What do you think?
Ok! Forget it! I can remove including Hal.h totally.
Attachment #575396 - Attachment is obsolete: true
Attachment #576425 - Flags: review?(jones.chris.g)
Attachment #576425 - Attachment description: part 1: Export Android Sensor Manager API to Gecko, v4 → part 2: Add a hal API for sensor access, v4
Attachment #575397 - Attachment is obsolete: true
Attachment #576426 - Flags: review?(jones.chris.g)
Skips NotifyProximityChange(), calls NotifySensorChange() directly.
Attachment #575398 - Attachment is obsolete: true
Attachment #576427 - Flags: review?(jones.chris.g)
Comment on attachment 576425 [details] [diff] [review]
part 2: Add a hal API for sensor access, v4

>diff --git a/hal/HalSensor.h b/hal/HalSensor.h

>+#define MAX_SENSOR_VALUES 3
>+

This doesn't seem to be used anymore, so maybe it can be removed.
Never mind if you use it in a later patch :).

>+#include "mozilla/ipc/RPCChannel.h"
>+

I don't think you want to include that header here ... maybe you're
looking for

  #include "IPC/IPCMessageUtils.h"

?

>+namespace IPC {
>+  /**
>+   * Serialize any enum type for IPDL.
>+   *

The parameters of this template need to be documented.

>+    static bool IsLegal(const paramType &aParam) {
>+      return lowGuard <= aParam < highGuard;

As proposed, "lowGuard" is 1 less than the first legal value, but here
you're using it as the first legal value.  So I guess call it
"smallestLegalValue" :).

>+    inline static void

|inline| is redundant here.

>+    Write(Message* aMsg, const paramType& aParam) {
>+      WriteParam(aMsg, (int32)aParam);

As in the example I pointed to, this should be

    if(!IsLegal(aParam))
      NS_RUNTIMEABORT("Illegal enum value");
    WriteParam(aMsg, int32(aParam));

>+    inline static bool

(Redundant here too.)

>+    Read(const Message* aMsg, void** aIter, paramType* aResult) {
>+      if(!ReadParam(aMsg, aIter, aResult))
>+        return false;
>+      return true;

As in the example I pointed to, this should be

    int32 format;
    if (!ReadParam(msg, iter, &format) ||
        !IsLegal(paramType(format))) {
      return false;
    }
    *result = paramType(format);
    return true;

>diff --git a/hal/android/AndroidSensor.cpp b/hal/android/AndroidSensor.cpp

>+#include "Hal.h"
>+#include "AndroidBridge.h"
>+

|using namespace mozilla::hal;| and then drop the "hal::"
 qualification of type names below.

>+MapSensorType(hal::SensorType sensor_type) {

Nit: |aSensorType|.

>+  if(sensor_type == hal::SENSOR_UNKNOWN ||
>+     sensor_type >= hal::NUM_SENSOR_TYPE)
>+    return -1;
>+  return sensor_type + 1;

You need to guard against aSensorType < 0.  E.g. the code below does
this and is a little clearer to my eyes

  return (SENSOR_UNKNOWN < aSensorType && aSensorType < hal::NUM_SENSOR_TYPE) ?
    aSensorType + 1 : -1;

>+void
>+EnableSensorNotifications(hal::SensorType aSensor) {
>+  int android_sensor = MapSensorType(aSensor);
>+  
>+  NS_ASSERTION(android_sensor != -1, "invalid SensorType");

Use MOZ_ASSERT() here.

>+  AndroidBridge::Bridge()->EnableSensor(android_sensor);
>+}

>+void
>+DisableSensorNotifications(hal::SensorType aSensor) {
>+  int android_sensor = MapSensorType(aSensor);
>+  
>+  NS_ASSERTION(android_sensor != -1, "invalid SensorType");

MOZ_ASSERT here too.

I'm marking r+ for this patch, but please fix what I requested above.
Because I'm marking "r+", I don't need to see the next version with
the comments above addressed.  But if you want to re-request review,
that's fine too :).
Attachment #576425 - Flags: review?(jones.chris.g) → review+
Comment on attachment 576426 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v4

>+  void Notify(const SensorData& aSensorData) {
>+    bool r;
>+    
>+    r = SendNotifySensorChange(aSensorData);
>+    NS_ASSERTION(r, "HalParent::SendNotifySensorChange() is fail");

You didn't address my comment from the previous review.  Sending this
message might fail, and if it does it's not a big problem.  You don't
want to assert here.  Instead, do

  unused << SendNotifySensorChange(aSensorData);

Again, I'm marking r+, but please fix the issue above.
Attachment #576426 - Flags: review?(jones.chris.g) → review+
Attachment #576427 - Flags: review?(jones.chris.g) → review+
We're not normalizing the units of the proximity sensor (or even documenting them ...), but since this bug is turning into something of a death march, let's do that when we add the DOM sensor API.
Attachment #576425 - Attachment is obsolete: true
Attachment #576426 - Attachment is obsolete: true
Attachment #576720 - Flags: review?(jones.chris.g)
Comment on attachment 576722 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v5

Sorry! I should edit old patches, but I upload new ones.
Attachment #576722 - Flags: review?(jones.chris.g)
Comment on attachment 576720 [details] [diff] [review]
part 2: Add a hal API for sensor access, v5

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

::: hal/HalSensor.h
@@ +92,5 @@
> +   *
> +   * \sa https://developer.mozilla.org/en/IPDL/Type_Serialization
> +   */
> +  template <typename E, E smallestLegalValue, E highGuard>
> +  struct EnumSerializer {

I did push something similar in mozilla-central: you can now use IPC::EnumSerializer which is defined in IPCMessageUtils.h.
Comment on attachment 576720 [details] [diff] [review]
part 2: Add a hal API for sensor access, v5

Looks good, thanks!
Attachment #576720 - Flags: review?(jones.chris.g) → review+
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #89)
> Comment on attachment 576720 [details] [diff] [review] [diff] [details] [review]
> part 2: Add a hal API for sensor access, v5
> 
> Review of attachment 576720 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: hal/HalSensor.h
> @@ +92,5 @@
> > +   *
> > +   * \sa https://developer.mozilla.org/en/IPDL/Type_Serialization
> > +   */
> > +  template <typename E, E smallestLegalValue, E highGuard>
> > +  struct EnumSerializer {
> 
> I did push something similar in mozilla-central: you can now use
> IPC::EnumSerializer which is defined in IPCMessageUtils.h.

Thinker, you'll probably want to remove EnumSerializer from your patches here; I don't think they'll compile with both versions.  Your comment has more information than Mounir's, so please feel free to merge yours with his :).  I don't need to review that merge.
Attachment #576722 - Flags: review?(jones.chris.g) → review+
Attachment #577206 - Flags: review?
Attachment #576720 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #577206 - Flags: review?
(Marking whiteboard just to make it easier to keep track of which bugs in the checkin-needed search cannot be landed due to bug 709193)
Whiteboard: [waiting on bug 709193]
mozilla-central is now open again (subject to sheriff metering until the backlog clears), adjusting whiteboard accordingly.
Whiteboard: [waiting on bug 709193]
Part 1 doesn't apply on tip.  Please update the patches?
Keywords: checkin-needed
--
Attachment #575395 [details] [diff] - Attachment is obsolete: true
--
Attachment #576427 [details] [diff] - Attachment is obsolete: true
--
Attachment #576722 [details] [diff] - Attachment is obsolete: true
--
Attachment #577206 [details] [diff] - Attachment is obsolete: true
Attachment #575395 - Attachment is obsolete: true
Attachment #576427 - Attachment is obsolete: true
Attachment #576722 - Attachment is obsolete: true
Attachment #577206 - Attachment is obsolete: true
Attachment #582274 - Attachment description: part 1: Export Android Sensor Manager API to Gecko, v3 → part 1: Export Android Sensor Manager API to Gecko, v4
Attachment #582277 - Attachment description: part 2: Add a hal API for sensor access, v6 → part 2: Add a hal API for sensor access, v7
Attachment #582276 - Attachment description: part 3: Make Sensor API available for Sandbox, v5 → part 3: Make Sensor API available for Sandbox, v6
Attachment #582275 - Attachment description: part 4: Pass sensor events from Android to Gecko, v4 → part 4: Pass sensor events from Android to Gecko, v5
Should I close this bug?  Since we are going to stop supporting Android.
(In reply to Thinker Li from comment #100)
> Should I close this bug?  Since we are going to stop supporting Android.

We are not going to stop supporting Android. We are going to stop supporting Android widget/backend in B2G. That's quite different. We are still shipping a browser on Android.
Now, we need to know if we want to support this feature in our Android browser.
We absolutely want this in fennec-android! :)

Thinker, do you have level 1 commit access yet?  The next steps to landing this code are
 (0) Rebase on latest hg.mozilla.org/mozilla-central
 (1) Push patches to tryserver
 (2) If all goes well, add a "checkin-needed" Keyword to this bug.

Someone will notice the "checkin-needed" flag and then land this code on mozilla-central.
--
Attachment #582275 [details] [diff] - Attachment is obsolete: true
--
Attachment #582277 [details] [diff] - Attachment is obsolete: true
Attachment #582275 - Attachment is obsolete: true
Attachment #582277 - Attachment is obsolete: true
Keywords: checkin-needed
Let's push this to the tryserver before requesting checkin.
Keywords: checkin-needed
I had pushed it last night.

https://tbpl.mozilla.org/?tree=Try&rev=0e7c165da3ab
Great!  Sorry for unsetting the flag.  In the future, please link to the push-to-try results :).
Keywords: checkin-needed
I tried applying the patches on inbound tip, but they fail to apply (even after I fixed them up for the file moves in widget, part 4 fails to apply).  Please update the patches?
Keywords: checkin-needed
Try run for ab6cb5b4f761 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ab6cb5b4f761
Results (out of 161 total builds):
    exception: 1
    success: 125
    warnings: 31
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-ab6cb5b4f761
Keywords: checkin-needed
applying https://bug697641.bugzilla.mozilla.org/attachment.cgi?id=582274
unable to find 'widget/src/android/AndroidBridge.cpp' for patching
2 out of 2 hunks FAILED -- saving rejects to file widget/src/android/AndroidBridge.cpp.rej
unable to find 'widget/src/android/AndroidBridge.h' for patching
2 out of 2 hunks FAILED -- saving rejects to file widget/src/android/AndroidBridge.h.rej
abort: patch failed to apply

see also comment 108...
Keywords: checkin-needed
--
Attachment #582274 [details] [diff] - Attachment is obsolete: true
--
Attachment #584505 [details] [diff] - Attachment is obsolete: true
Attachment #582274 - Attachment is obsolete: true
Attachment #584505 - Attachment is obsolete: true
Keywords: checkin-needed
sorry! I forgot to update attached patches.  I have updated them, now.
backed out, cause it doesn't build on Windows
Target Milestone: mozilla12 → ---
--
Attachment #582276 [details] [diff] - Attachment is obsolete: true
Attachment #582276 - Attachment is obsolete: true
Even though it's difficult to tell through all the red from hg, that landing also completely broke native Fennec.
Try run for ad8a1e3e7eca is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ad8a1e3e7eca
Results (out of 162 total builds):
    exception: 2
    success: 110
    warnings: 18
    failure: 32
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-ad8a1e3e7eca
 Timed out after 06 hours without completing.
Depends on: 718617
There is a conflict between mozilla::ObserverList and ObserverList defined by ipc/chromium.  The conflict appears only for windows platforms.  I guess it is about implementation of C++ compiler.  There is no more conflict if move ObserverList defined by ipc/chromium to namespace "base".  So, I file bug 718617.
--
Attachment #584506 [details] [diff] - Attachment is obsolete: true
--
Attachment #588881 [details] [diff] - Attachment is obsolete: true
Attachment #584506 - Attachment is obsolete: true
Attachment #588622 - Attachment is obsolete: true
Attachment #588881 - Attachment is obsolete: true
Try run for d60fb34b19a5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d60fb34b19a5
Results (out of 14 total builds):
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-d60fb34b19a5
Try run for 0cf032b4c148 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0cf032b4c148
Results (out of 14 total builds):
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-0cf032b4c148
Try run for 4e57c6f6d70c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=4e57c6f6d70c
Results (out of 162 total builds):
    success: 125
    warnings: 35
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-4e57c6f6d70c
Blocks: 714413
--
Attachment #592627 [details] [diff] - Attachment is obsolete: true
Attachment #588621 - Attachment is obsolete: true
Attachment #592627 - Attachment is obsolete: true
Keywords: checkin-needed
--
Attachment #592625 [details] [diff] - Attachment is obsolete: true
--
Attachment #592625 [details] [diff] - Attachment is obsolete: true
Attachment #592625 - Attachment is obsolete: true
Attachment #593057 - Attachment is obsolete: true
Attachment #594153 - Flags: review?(jones.chris.g)
Try run for 14bbfe0d2866 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=14bbfe0d2866
Results (out of 14 total builds):
    exception: 7
    failure: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-14bbfe0d2866
Try run for 88b5e2aca480 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=88b5e2aca480
Results (out of 206 total builds):
    success: 180
    warnings: 25
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-88b5e2aca480
 Timed out after 06 hours without completing.
Comment on attachment 594153 [details] [diff] [review]
part 5: Export Android Sensor Manager for mobile/

rubber-stamp r+, assuming this is mostly a copy-and-paste from the other android code.
Attachment #594153 - Flags: review?(jones.chris.g) → review+
Keywords: checkin-needed
Attachment #594146 - Attachment is obsolete: true
Congratulations, Thinker!
Whiteboard: [not-fennec-11]
You need to log in before you can comment on or make changes to this bug.