Last Comment Bug 697641 - Implement proximity sensor backend for android
: Implement proximity sensor backend for android
Status: RESOLVED FIXED
[not-fennec-11]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla13
Assigned To: Thinker Li [:sinker]
:
Mentors:
Depends on: 718617
Blocks: 697361 714413
  Show dependency treegraph
 
Reported: 2011-10-26 19:17 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-02-06 10:31 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix


Attachments
0001-Add-HAL-for-sensor-into-build-system.patch (1.20 KB, patch)
2011-11-02 01:59 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0002-Pass-events-from-Dalvik-Java-code-to-Gecko.patch (24.84 KB, patch)
2011-11-02 02:01 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0003-Dig-it-through-from-Gecko-to-Android-for-Sensor-feat.patch (3.28 KB, patch)
2011-11-02 02:02 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0004-Add-nsSensorManager-component.patch (14.43 KB, patch)
2011-11-02 02:03 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0005-Register-nsSensorManager-component-to-Gecko.patch (5.77 KB, patch)
2011-11-02 02:04 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0006-Implement-QueryInterface-for-nsSensorManager-to-supp.patch (1.07 KB, patch)
2011-11-02 02:05 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0007-Map-sensor-type-correctly-for-sensor-backend-HAL.patch (3.35 KB, patch)
2011-11-02 02:06 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0008-Move-definition-of-sensor-types-from-nsISensorData-t.patch (1.47 KB, patch)
2011-11-02 02:07 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0009-Fix-bug-of-QueryInterface-for-nsSensorValue-nsSensor.patch (1.54 KB, patch)
2011-11-02 02:08 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0010-Remove-a-memory-leaking.patch (707 bytes, patch)
2011-11-02 02:08 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0011-Fix-buggy-precondition-checking-of-nsSensorData-GetV.patch (1.45 KB, patch)
2011-11-02 02:09 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0012-Replace-pointer-checking-with-NS_ENSURE_ARG_POINTER-.patch (1.13 KB, patch)
2011-11-02 02:10 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0013-Refactory-nsSensorManager.patch (5.11 KB, patch)
2011-11-02 02:11 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
0014-Testing-page-for-proximity-sensor.patch (8.21 KB, patch)
2011-11-02 02:13 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
01-define-nsISensorManager.patch (3.00 KB, patch)
2011-11-03 10:11 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
02-add-nsISensorManager-to-build.patch (468 bytes, patch)
2011-11-03 10:12 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
03-HAL-android-sensor.patch (7.21 KB, patch)
2011-11-03 10:14 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
04-nsSensorManager-component.patch (11.42 KB, patch)
2011-11-03 10:16 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
05-Pass-sensor-events-from-Android-to-Gecko.patch (4.92 KB, patch)
2011-11-03 10:17 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
06-export-android-sensor-manager.patch (4.13 KB, patch)
2011-11-03 10:18 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
07-add-to-build-system.patch (3.00 KB, patch)
2011-11-03 10:19 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
08-register-nsSensorManager-in-nsLayoutModule.patch (1.95 KB, patch)
2011-11-03 10:20 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 1: Add a hal API for sensor access (9.46 KB, patch)
2011-11-11 06:56 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 1: Add a hal API for sensor access, v2 (11.84 KB, patch)
2011-11-14 01:41 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 2: Export Android Sensor Manager API to Gecko, v2 (6.86 KB, patch)
2011-11-14 01:43 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 3: Add impl. of Sensor HAL to build system, v2 (3.80 KB, patch)
2011-11-14 01:46 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 4: Make Sensor API available for Sandbox (7.80 KB, patch)
2011-11-14 01:49 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 5: Pass sensor events from Android to Gecko, v2 (9.92 KB, patch)
2011-11-14 01:50 PST, Thinker Li [:sinker]
cjones.bugs: review+
Details | Diff | Splinter Review
part 1: Export Android Sensor Manager API to Gecko, v3 (11.84 KB, patch)
2011-11-17 21:58 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 2: Add a hal API for sensor access, v3 (6.86 KB, patch)
2011-11-17 22:01 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 1: Export Android Sensor Manager API to Gecko, v3 (6.94 KB, patch)
2011-11-17 22:04 PST, Thinker Li [:sinker]
cjones.bugs: review+
Details | Diff | Splinter Review
part 2: Add a hal API for sensor access, v3 (14.03 KB, patch)
2011-11-17 22:05 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 3: Make Sensor API available for Sandbox, v3 (6.95 KB, patch)
2011-11-17 22:07 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 4: Pass sensor events from Android to Gecko, v3 (10.01 KB, patch)
2011-11-17 22:09 PST, Thinker Li [:sinker]
cjones.bugs: review+
Details | Diff | Splinter Review
part 2: Add a hal API for sensor access, v4 (12.74 KB, patch)
2011-11-23 01:09 PST, Thinker Li [:sinker]
cjones.bugs: review+
Details | Diff | Splinter Review
part 3: Make Sensor API available for Sandbox, v4 (6.96 KB, patch)
2011-11-23 01:11 PST, Thinker Li [:sinker]
cjones.bugs: review+
Details | Diff | Splinter Review
part 4: Pass sensor events from Android to Gecko, v4 (10.16 KB, patch)
2011-11-23 01:14 PST, Thinker Li [:sinker]
cjones.bugs: review+
Details | Diff | Splinter Review
part 2: Add a hal API for sensor access, v5 (13.69 KB, patch)
2011-11-24 04:14 PST, Thinker Li [:sinker]
cjones.bugs: review+
Details | Diff | Splinter Review
part 3: Make Sensor API available for Sandbox, v5 (6.86 KB, patch)
2011-11-24 04:14 PST, Thinker Li [:sinker]
cjones.bugs: review+
Details | Diff | Splinter Review
part 2: Add a hal API for sensor access, v6 (13.77 KB, patch)
2011-11-28 01:38 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 1: Export Android Sensor Manager API to Gecko, v4 (7.10 KB, patch)
2011-12-16 07:52 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 4: Pass sensor events from Android to Gecko, v5 (10.12 KB, patch)
2011-12-16 07:53 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 3: Make Sensor API available for Sandbox, v6 (4.77 KB, patch)
2011-12-16 07:54 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 2: Add a hal API for sensor access, v7 (13.90 KB, patch)
2011-12-16 07:55 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 4: Pass sensor events from Android to Gecko, v6 (10.13 KB, patch)
2011-12-27 16:36 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 2: Add a hal API for sensor access, v8 (14.18 KB, patch)
2011-12-27 16:37 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 1: Export Android Sensor Manager API to Gecko, v5 (7.07 KB, patch)
2012-01-14 03:52 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 4: Pass sensor events from Android to Gecko, v7 (10.07 KB, patch)
2012-01-14 03:53 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 3: Make Sensor API available for Sandbox, v7 (5.75 KB, patch)
2012-01-16 07:42 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 2: Add a hal API for sensor access, v9 (14.48 KB, patch)
2012-01-30 00:27 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 4: Pass sensor events from Android to Gecko, v8 (10.17 KB, patch)
2012-01-30 00:28 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 3: Make Sensor API available for Sandbox, v8 (6.07 KB, patch)
2012-01-30 00:28 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 1: Export Android Sensor Manager API to Gecko, v6 (6.96 KB, patch)
2012-01-31 05:32 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 3: Make Sensor API available for Sandbox, v9 (5.98 KB, patch)
2012-01-31 05:32 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 2: Add a hal API for sensor access, v10 (14.52 KB, patch)
2012-02-03 05:45 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 2: Add a hal API for sensor access, v10 (14.52 KB, patch)
2012-02-03 05:49 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 3: Make Sensor API available for Sandbox, v10 (5.79 KB, patch)
2012-02-03 05:53 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
part 5: Export Android Sensor Manager for mobile/ (5.28 KB, patch)
2012-02-03 06:13 PST, Thinker Li [:sinker]
cjones.bugs: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-26 19:17:01 PDT
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.
Comment 1 Thinker Li [:sinker] 2011-10-27 20:35:01 PDT
So, do you want to use this hal to replace existing code about sensors?
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-28 11:50:18 PDT
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.
Comment 3 Thinker Li [:sinker] 2011-11-02 01:59:58 PDT
Created attachment 571261 [details] [diff] [review]
0001-Add-HAL-for-sensor-into-build-system.patch
Comment 4 Thinker Li [:sinker] 2011-11-02 02:01:01 PDT
Created attachment 571262 [details] [diff] [review]
0002-Pass-events-from-Dalvik-Java-code-to-Gecko.patch
Comment 5 Thinker Li [:sinker] 2011-11-02 02:02:33 PDT
Created attachment 571263 [details] [diff] [review]
0003-Dig-it-through-from-Gecko-to-Android-for-Sensor-feat.patch
Comment 6 Thinker Li [:sinker] 2011-11-02 02:03:29 PDT
Created attachment 571264 [details] [diff] [review]
0004-Add-nsSensorManager-component.patch
Comment 7 Thinker Li [:sinker] 2011-11-02 02:04:42 PDT
Created attachment 571266 [details] [diff] [review]
0005-Register-nsSensorManager-component-to-Gecko.patch
Comment 8 Thinker Li [:sinker] 2011-11-02 02:05:47 PDT
Created attachment 571267 [details] [diff] [review]
0006-Implement-QueryInterface-for-nsSensorManager-to-supp.patch
Comment 9 Thinker Li [:sinker] 2011-11-02 02:06:31 PDT
Created attachment 571268 [details] [diff] [review]
0007-Map-sensor-type-correctly-for-sensor-backend-HAL.patch
Comment 10 Thinker Li [:sinker] 2011-11-02 02:07:25 PDT
Created attachment 571269 [details] [diff] [review]
0008-Move-definition-of-sensor-types-from-nsISensorData-t.patch
Comment 11 Thinker Li [:sinker] 2011-11-02 02:08:08 PDT
Created attachment 571271 [details] [diff] [review]
0009-Fix-bug-of-QueryInterface-for-nsSensorValue-nsSensor.patch
Comment 12 Thinker Li [:sinker] 2011-11-02 02:08:58 PDT
Created attachment 571272 [details] [diff] [review]
0010-Remove-a-memory-leaking.patch
Comment 13 Thinker Li [:sinker] 2011-11-02 02:09:55 PDT
Created attachment 571273 [details] [diff] [review]
0011-Fix-buggy-precondition-checking-of-nsSensorData-GetV.patch
Comment 14 Thinker Li [:sinker] 2011-11-02 02:10:47 PDT
Created attachment 571274 [details] [diff] [review]
0012-Replace-pointer-checking-with-NS_ENSURE_ARG_POINTER-.patch
Comment 15 Thinker Li [:sinker] 2011-11-02 02:11:26 PDT
Created attachment 571275 [details] [diff] [review]
0013-Refactory-nsSensorManager.patch
Comment 16 Thinker Li [:sinker] 2011-11-02 02:13:24 PDT
Created attachment 571276 [details] [diff] [review]
0014-Testing-page-for-proximity-sensor.patch

0014-Testing-page-for-proximity-sensor.patch will modify URL of default home for testing.  It should not be committed.
Comment 17 Thinker Li [:sinker] 2011-11-02 02:17:46 PDT
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.
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-02 13:44:22 PDT
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 19 Thinker Li [:sinker] 2011-11-02 17:28:13 PDT
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 20 Thinker Li [:sinker] 2011-11-02 17:36:01 PDT
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 21 Thinker Li [:sinker] 2011-11-02 17:43:47 PDT
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 22 Thinker Li [:sinker] 2011-11-02 17:46:21 PDT
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 23 Thinker Li [:sinker] 2011-11-02 17:50:24 PDT
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 24 Thinker Li [:sinker] 2011-11-02 17:52:00 PDT
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 25 Thinker Li [:sinker] 2011-11-02 18:00:39 PDT
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.)
Comment 26 Thinker Li [:sinker] 2011-11-02 18:04:08 PDT
Other patches (0008~00013) are bug fixing and refactoring.
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-02 23:58:06 PDT
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
Comment 28 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-03 00:08:29 PDT
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.
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-03 00:13:05 PDT
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.
Comment 30 Mounir Lamouri (:mounir) 2011-11-03 03:29:50 PDT
(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.
Comment 31 Thinker Li [:sinker] 2011-11-03 10:11:25 PDT
Created attachment 571685 [details] [diff] [review]
01-define-nsISensorManager.patch

Defines nsISensorManger and relative interfaces
Comment 32 Thinker Li [:sinker] 2011-11-03 10:12:40 PDT
Created attachment 571686 [details] [diff] [review]
02-add-nsISensorManager-to-build.patch

Make nsISensorManger.idl to be built
Comment 33 Thinker Li [:sinker] 2011-11-03 10:14:08 PDT
Created attachment 571687 [details] [diff] [review]
03-HAL-android-sensor.patch

A bridge of Android Sensor manager API for Gecko

With this interface, Gecko code can leverage capabilities of accessing
sensors provied by Android.
Comment 34 Thinker Li [:sinker] 2011-11-03 10:16:11 PDT
Created attachment 571688 [details] [diff] [review]
04-nsSensorManager-component.patch

Add nsSensorManager component to implement nsISesnorManager interface
Comment 35 Thinker Li [:sinker] 2011-11-03 10:17:05 PDT
Created attachment 571689 [details] [diff] [review]
05-Pass-sensor-events-from-Android-to-Gecko.patch

Pass events from Dalvik Java code to Gecko
Comment 36 Thinker Li [:sinker] 2011-11-03 10:18:08 PDT
Created attachment 571690 [details] [diff] [review]
06-export-android-sensor-manager.patch

Export Sensor Manager API of Android to Gecko
Comment 37 Thinker Li [:sinker] 2011-11-03 10:19:15 PDT
Created attachment 571691 [details] [diff] [review]
07-add-to-build-system.patch

Add hal/android subdirectory to build system
Comment 38 Thinker Li [:sinker] 2011-11-03 10:20:01 PDT
Created attachment 571692 [details] [diff] [review]
08-register-nsSensorManager-in-nsLayoutModule.patch

Register nsSensorManager component to Gecko in nsLayoutModule.cpp
Comment 39 Thinker Li [:sinker] 2011-11-03 10:25:44 PDT
(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.
Comment 40 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-03 17:55:01 PDT
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.
Comment 41 Thinker Li [:sinker] 2011-11-03 19:26:34 PDT
nsSensorManager is a XPCOM component that implements nsISensorManager.  Without this idl, how do you compile nsSensorManager?
Comment 42 Thinker Li [:sinker] 2011-11-03 19:29:16 PDT
But the way, we may open another bug for nsSensorManager and nsISensorManager.  The reset patches can be built without them.
Comment 43 Thinker Li [:sinker] 2011-11-03 20:29:42 PDT
How about to move both nsISensorManager and nsSensorManager to bug 697361?
Comment 44 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-03 23:31:49 PDT
(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! :)
Comment 45 Thinker Li [:sinker] 2011-11-04 02:11:57 PDT
Comment on attachment 571686 [details] [diff] [review]
02-add-nsISensorManager-to-build.patch

move to bug 697361
Comment 46 Thinker Li [:sinker] 2011-11-04 02:12:33 PDT
Comment on attachment 571688 [details] [diff] [review]
04-nsSensorManager-component.patch

Move to bug 697361
Comment 47 Thinker Li [:sinker] 2011-11-04 02:13:01 PDT
Comment on attachment 571692 [details] [diff] [review]
08-register-nsSensorManager-in-nsLayoutModule.patch

Move to bug 697361
Comment 48 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-10 00:08:02 PST
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".
Comment 49 Thinker Li [:sinker] 2011-11-11 06:56:17 PST
Created attachment 573789 [details] [diff] [review]
part 1: Add a hal API for sensor access

Sensor API interface and an implementation for Android.
Comment 50 Thinker Li [:sinker] 2011-11-14 01:41:22 PST
Created attachment 574250 [details] [diff] [review]
part 1: Add a hal API for sensor access, v2

With this API, Gecko code can access sensors provied by system.  This
patch include an implementation for Android.
Comment 51 Thinker Li [:sinker] 2011-11-14 01:43:41 PST
Created attachment 574251 [details] [diff] [review]
part 2: Export Android Sensor Manager API to Gecko, v2
Comment 52 Thinker Li [:sinker] 2011-11-14 01:46:35 PST
Created attachment 574252 [details] [diff] [review]
part 3: Add impl. of Sensor HAL to build system, v2

Modify Makefiles to include impl. of sensor API in building procedure.
Comment 53 Thinker Li [:sinker] 2011-11-14 01:49:32 PST
Created attachment 574254 [details] [diff] [review]
part 4: Make Sensor API available for Sandbox

Add sensor APIs to PHal protocol to make them available for sandbox.
Comment 54 Thinker Li [:sinker] 2011-11-14 01:50:59 PST
Created attachment 574256 [details] [diff] [review]
part 5: Pass sensor events from Android to Gecko, v2
Comment 55 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-14 20:50:03 PST
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 :).
Comment 56 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-14 21:11:10 PST
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.
Comment 57 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-14 21:12:18 PST
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.
Comment 58 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-14 21:37:51 PST
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.
Comment 59 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-14 21:41:30 PST
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 :).
Comment 60 Thinker Li [:sinker] 2011-11-14 22:08:12 PST
(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.
Comment 61 Thinker Li [:sinker] 2011-11-16 05:06:08 PST
(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.
Comment 62 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-16 12:02:57 PST
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.
Comment 63 Thinker Li [:sinker] 2011-11-17 21:58:15 PST
Created attachment 575392 [details] [diff] [review]
part 1: Export Android Sensor Manager API to Gecko, v3
Comment 64 Thinker Li [:sinker] 2011-11-17 22:01:21 PST
Created attachment 575393 [details] [diff] [review]
part 2: Add a hal API for sensor access, v3
Comment 65 Thinker Li [:sinker] 2011-11-17 22:04:24 PST
Created attachment 575395 [details] [diff] [review]
part 1: Export Android Sensor Manager API to Gecko, v3
Comment 66 Thinker Li [:sinker] 2011-11-17 22:05:29 PST
Created attachment 575396 [details] [diff] [review]
part 2: Add a hal API for sensor access, v3
Comment 67 Thinker Li [:sinker] 2011-11-17 22:07:59 PST
Created attachment 575397 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v3
Comment 68 Thinker Li [:sinker] 2011-11-17 22:09:34 PST
Created attachment 575398 [details] [diff] [review]
part 4: Pass sensor events from Android to Gecko, v3
Comment 69 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-21 23:09:20 PST
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.
Comment 70 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-21 23:16:58 PST
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.
Comment 71 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-21 23:18:55 PST
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.
Comment 72 Thinker Li [:sinker] 2011-11-22 19:20:00 PST
(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.
Comment 73 Thinker Li [:sinker] 2011-11-22 20:12:02 PST
(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.
Comment 74 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-22 20:18:05 PST
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);|.
Comment 75 Thinker Li [:sinker] 2011-11-22 21:17:53 PST
(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?
Comment 76 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-22 21:59:12 PST
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.
Comment 77 Thinker Li [:sinker] 2011-11-22 22:21:43 PST
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?
Comment 78 Thinker Li [:sinker] 2011-11-22 22:24:55 PST
(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.
Comment 79 Thinker Li [:sinker] 2011-11-23 01:09:13 PST
Created attachment 576425 [details] [diff] [review]
part 2: Add a hal API for sensor access, v4
Comment 80 Thinker Li [:sinker] 2011-11-23 01:11:27 PST
Created attachment 576426 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v4
Comment 81 Thinker Li [:sinker] 2011-11-23 01:14:16 PST
Created attachment 576427 [details] [diff] [review]
part 4: Pass sensor events from Android to Gecko, v4

Skips NotifyProximityChange(), calls NotifySensorChange() directly.
Comment 82 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-23 18:33:32 PST
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 :).
Comment 83 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-23 18:38:07 PST
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.
Comment 84 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-23 18:40:36 PST
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.
Comment 85 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-23 18:40:49 PST
Nice work!
Comment 86 Thinker Li [:sinker] 2011-11-24 04:14:06 PST
Created attachment 576720 [details] [diff] [review]
part 2: Add a hal API for sensor access, v5
Comment 87 Thinker Li [:sinker] 2011-11-24 04:14:56 PST
Created attachment 576722 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v5
Comment 88 Thinker Li [:sinker] 2011-11-24 08:31:45 PST
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.
Comment 89 Mounir Lamouri (:mounir) 2011-11-25 03:21:34 PST
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 90 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-26 17:33:03 PST
Comment on attachment 576720 [details] [diff] [review]
part 2: Add a hal API for sensor access, v5

Looks good, thanks!
Comment 91 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-26 17:35:43 PST
(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.
Comment 92 Thinker Li [:sinker] 2011-11-28 01:38:34 PST
Created attachment 577206 [details] [diff] [review]
part 2: Add a hal API for sensor access, v6
Comment 93 Ed Morley [:emorley] 2011-12-12 17:34:22 PST
(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)
Comment 94 Ed Morley [:emorley] 2011-12-14 14:25:29 PST
mozilla-central is now open again (subject to sheriff metering until the backlog clears), adjusting whiteboard accordingly.
Comment 95 Boris Zbarsky [:bz] 2011-12-15 11:36:27 PST
Part 1 doesn't apply on tip.  Please update the patches?
Comment 96 Thinker Li [:sinker] 2011-12-16 07:52:57 PST
Created attachment 582274 [details] [diff] [review]
part 1: Export Android Sensor Manager API to Gecko, v4

--
Attachment #575395 [details] [diff] - Attachment is obsolete: true
Comment 97 Thinker Li [:sinker] 2011-12-16 07:53:43 PST
Created attachment 582275 [details] [diff] [review]
part 4: Pass sensor events from Android to Gecko, v5

--
Attachment #576427 [details] [diff] - Attachment is obsolete: true
Comment 98 Thinker Li [:sinker] 2011-12-16 07:54:26 PST
Created attachment 582276 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v6

--
Attachment #576722 [details] [diff] - Attachment is obsolete: true
Comment 99 Thinker Li [:sinker] 2011-12-16 07:55:10 PST
Created attachment 582277 [details] [diff] [review]
part 2: Add a hal API for sensor access, v7

--
Attachment #577206 [details] [diff] - Attachment is obsolete: true
Comment 100 Thinker Li [:sinker] 2011-12-27 02:19:06 PST
Should I close this bug?  Since we are going to stop supporting Android.
Comment 101 Mounir Lamouri (:mounir) 2011-12-27 02:33:44 PST
(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.
Comment 102 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-27 11:23:59 PST
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.
Comment 103 Thinker Li [:sinker] 2011-12-27 16:36:48 PST
Created attachment 584505 [details] [diff] [review]
part 4: Pass sensor events from Android to Gecko, v6

--
Attachment #582275 [details] [diff] - Attachment is obsolete: true
Comment 104 Thinker Li [:sinker] 2011-12-27 16:37:27 PST
Created attachment 584506 [details] [diff] [review]
part 2: Add a hal API for sensor access, v8

--
Attachment #582277 [details] [diff] - Attachment is obsolete: true
Comment 105 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-27 16:51:43 PST
Let's push this to the tryserver before requesting checkin.
Comment 106 Thinker Li [:sinker] 2011-12-27 17:02:15 PST
I had pushed it last night.

https://tbpl.mozilla.org/?tree=Try&rev=0e7c165da3ab
Comment 107 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-27 17:31:07 PST
Great!  Sorry for unsetting the flag.  In the future, please link to the push-to-try results :).
Comment 108 Boris Zbarsky [:bz] 2012-01-09 21:26:20 PST
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?
Comment 109 Mozilla RelEng Bot 2012-01-13 04:46:03 PST
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
Comment 110 Dão Gottwald [:dao] 2012-01-14 03:30:30 PST
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...
Comment 111 Thinker Li [:sinker] 2012-01-14 03:52:51 PST
Created attachment 588621 [details] [diff] [review]
part 1: Export Android Sensor Manager API to Gecko, v5

--
Attachment #582274 [details] [diff] - Attachment is obsolete: true
Comment 112 Thinker Li [:sinker] 2012-01-14 03:53:00 PST
Created attachment 588622 [details] [diff] [review]
part 4: Pass sensor events from Android to Gecko, v7

--
Attachment #584505 [details] [diff] - Attachment is obsolete: true
Comment 113 Thinker Li [:sinker] 2012-01-14 03:56:35 PST
sorry! I forgot to update attached patches.  I have updated them, now.
Comment 115 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-01-16 06:44:55 PST
backed out, cause it doesn't build on Windows
Comment 116 Thinker Li [:sinker] 2012-01-16 07:42:02 PST
Created attachment 588881 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v7

--
Attachment #582276 [details] [diff] - Attachment is obsolete: true
Comment 117 Phil Ringnalda (:philor, back in August) 2012-01-16 10:12:38 PST
Even though it's difficult to tell through all the red from hg, that landing also completely broke native Fennec.
Comment 118 Mozilla RelEng Bot 2012-01-16 14:05:11 PST
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.
Comment 119 Thinker Li [:sinker] 2012-01-17 05:47:06 PST
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.
Comment 120 Thinker Li [:sinker] 2012-01-30 00:27:59 PST
Created attachment 592625 [details] [diff] [review]
part 2: Add a hal API for sensor access, v9

--
Attachment #584506 [details] [diff] - Attachment is obsolete: true
Comment 121 Thinker Li [:sinker] 2012-01-30 00:28:11 PST
Created attachment 592626 [details] [diff] [review]
part 4: Pass sensor events from Android to Gecko, v8

--
Attachment #588622 [details] [diff] - Attachment is obsolete: true
Comment 122 Thinker Li [:sinker] 2012-01-30 00:28:25 PST
Created attachment 592627 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v8

--
Attachment #588881 [details] [diff] - Attachment is obsolete: true
Comment 123 Mozilla RelEng Bot 2012-01-30 01:45:19 PST
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
Comment 124 Mozilla RelEng Bot 2012-01-30 02:15:19 PST
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
Comment 125 Mozilla RelEng Bot 2012-01-30 06:00:33 PST
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
Comment 126 Thinker Li [:sinker] 2012-01-31 05:32:33 PST
Created attachment 593056 [details] [diff] [review]
part 1: Export Android Sensor Manager API to Gecko, v6

--
Attachment #588621 [details] [diff] - Attachment is obsolete: true
Comment 127 Thinker Li [:sinker] 2012-01-31 05:32:47 PST
Created attachment 593057 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v9

--
Attachment #592627 [details] [diff] - Attachment is obsolete: true
Comment 128 Thinker Li [:sinker] 2012-01-31 18:36:55 PST
Try run log
https://tbpl.mozilla.org/?tree=Try&rev=e69b13af3d1a&pusher=tlee@mozilla.com
Comment 131 Thinker Li [:sinker] 2012-02-03 05:45:24 PST
Created attachment 594146 [details] [diff] [review]
part 2: Add a hal API for sensor access, v10

--
Attachment #592625 [details] [diff] - Attachment is obsolete: true
Comment 132 Thinker Li [:sinker] 2012-02-03 05:49:04 PST
Created attachment 594148 [details] [diff] [review]
part 2: Add a hal API for sensor access, v10

--
Attachment #592625 [details] [diff] - Attachment is obsolete: true
Comment 133 Thinker Li [:sinker] 2012-02-03 05:53:14 PST
Created attachment 594150 [details] [diff] [review]
part 3: Make Sensor API available for Sandbox, v10

--
Attachment #593057 [details] [diff] - Attachment is obsolete: true
Comment 134 Thinker Li [:sinker] 2012-02-03 06:13:25 PST
Created attachment 594153 [details] [diff] [review]
part 5: Export Android Sensor Manager for mobile/
Comment 135 Mozilla RelEng Bot 2012-02-03 07:22:07 PST
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
Comment 136 Mozilla RelEng Bot 2012-02-03 13:45:22 PST
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 137 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-03 18:18:23 PST
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.
Comment 140 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-06 01:59:19 PST
Congratulations, Thinker!

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