Implement java-side browser sensor polling for device motion and device orientation DOM events.

content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java

Index: content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/DeviceOrientation.java b/content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java
similarity index 44%
rename from content/public/android/java/src/org/chromium/content/browser/DeviceOrientation.java
rename to content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java
index a77cd28b27ab6901f6a12f7a2e4e012b3e7c8f71..31eaadb40305bb4f7fdb99fde9c565dff3286cb1 100644
--- a/content/public/android/java/src/org/chromium/content/browser/DeviceOrientation.java
+++ b/content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -12,17 +12,23 @@ import android.hardware.SensorManager;
 import android.os.Handler;
 import android.os.Looper;
 
+import com.google.common.collect.ImmutableMap;

[Peter Beverloo, 2013/03/13 14:02:47]

This is now unused.

[timvolodine, 2013/03/13 15:29:49]

Done.

+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Sets;
+
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
 import org.chromium.base.WeakContext;
 
 import java.util.List;
+import java.util.Map;

[Peter Beverloo, 2013/03/13 14:02:47]

This is now unused.

[timvolodine, 2013/03/13 15:29:49]

Done.

+import java.util.Set;
 
 /**
- * Android implementation of the DeviceOrientation API.
+ * Android implementation of the device motion and orientation APIs.
  */
 @JNINamespace("content")
-class DeviceOrientation implements SensorEventListener {
+class DeviceMotionAndOrientation implements SensorEventListener {
 
     // These fields are lazily initialized by getHandler().
     private Thread mThread;
@@ -44,14 +50,40 @@ class DeviceOrientation implements SensorEventListener {
     // The geomagnetic vector expressed in the body frame.
     private float[] mMagneticFieldVector;
 
+    // The linear acceleration vector expressed in the body frame.
+    private float[] mAccelerationVector;

[Peter Beverloo, 2013/03/13 14:02:47]

Should we perhaps use this member instead of mGravityVector for the
accelerometer results?  I think it has a better name.  In either case, we don't
need both of them.

[timvolodine, 2013/03/13 15:29:49]

Done.

+
+    // The angular speed at which the device is rotating around the body frame.
+    private float[] mGyroscopeVector;

[Peter Beverloo, 2013/03/13 14:02:47]

We don't need this member.

[timvolodine, 2013/03/13 15:29:49]

Done.

+
     // Lazily initialized when registering for notifications.
     private SensorManager mSensorManager;
 
     // The only instance of that class and its associated lock.
-    private static DeviceOrientation sSingleton;
+    private static DeviceMotionAndOrientation sSingleton;
     private static Object sSingletonLock = new Object();
 
-    private DeviceOrientation() {
+    /**
+     * constants for using in JNI calls, also see
+     * content/browser/device_orientation/data_fetcher_impl_android.cc
+     */
+    private static final int DEVICE_ORIENTATION = 0;
+    private static final int DEVICE_MOTION = 1;
+
+    private static ImmutableSet<Integer> DEVICE_ORIENTATION_SENSORS = ImmutableSet.of(
+            Sensor.TYPE_ACCELEROMETER,
+            Sensor.TYPE_MAGNETIC_FIELD);
+
+    private static ImmutableSet<Integer> DEVICE_MOTION_SENSORS = ImmutableSet.of(
+            Sensor.TYPE_ACCELEROMETER,
+            Sensor.TYPE_LINEAR_ACCELERATION,
+            Sensor.TYPE_GYROSCOPE);
+
+    private Set<Integer> mActiveSensors = Sets.newHashSet();
+    private boolean mDeviceMotionIsActive = false;
+    private boolean mDeviceOrientationIsActive = false;
+
+    private DeviceMotionAndOrientation() {
     }
 
     /**
@@ -61,33 +93,69 @@ class DeviceOrientation implements SensorEventListener {
      * @param nativePtr Value to pass to nativeGotOrientation() for each event.
      * @param rateInMilliseconds Requested callback rate in milliseconds. The
      *            actual rate may be higher. Unwanted events should be ignored.
+     * @param eventType type of event to listen to, can be either 0 for device

[Miguel Garcia, 2013/03/13 13:37:09]

can you refer to the constants you created instead of 0/1. That would avoid
having to explain what 0 and 1 mean.

[Peter Beverloo, 2013/03/13 14:02:47]

s/type/Type/.  Sentences begin with a capital.

[timvolodine, 2013/03/13 15:29:49]

Done.

[timvolodine, 2013/03/13 15:29:49]

Done.

+     *            orientation, or 1 for device motion.
      * @return True on success.
      */
     @CalledByNative
-    public boolean start(int nativePtr, int rateInMilliseconds) {
+    public boolean start(int nativePtr, int eventType, int rateInMilliseconds) {
+        boolean success = false;
+        Set<Integer> sensorsToActivate;
         synchronized (mNativePtrLock) {
-            stop();
-            if (registerForSensors(rateInMilliseconds)) {
+            switch (eventType) {
+                case DEVICE_ORIENTATION:
+                    sensorsToActivate = Sets.newHashSet(DEVICE_ORIENTATION_SENSORS);
+                    sensorsToActivate.removeAll(mActiveSensors);

[Miguel Garcia, 2013/03/13 13:37:09]

Since registerForSensors already filters out active sensors why not just call
registerForSensors(DEVICE_ORIENTATION_SENSORS, rateInMilliseconds, true) ?

[Peter Beverloo, 2013/03/13 14:02:47]

+1. Dito on line 113.

[timvolodine, 2013/03/13 15:29:49]

I've changed the registerForSensors method a bit, so this is now possible.
Otherwise in the case with DEVICE_MOTION we might unregister too many sensors
upon failure.

On 2013/03/13 13:37:09, Miguel Garcia wrote:

[timvolodine, 2013/03/13 15:29:49]

Done.

+                    success = registerForSensors(sensorsToActivate, rateInMilliseconds, true);
+                    break;
+                case DEVICE_MOTION:
+                    Set<Integer> sensorsToActivate = Sets.newHashSet(DEVICE_MOTION_SENSORS);
+                    sensorsToActivate.removeAll(mActiveSensors);
+                    success = registerForSensors(sensorsToActivate, rateInMilliseconds, false);

[Miguel Garcia, 2013/03/13 13:37:09]

can you explain why we allow missing sensors on motion but not on orientation?

[Peter Beverloo, 2013/03/13 14:02:47]

The specification doesn't mandate that either sensor has to exist.  We pass NULL
in the IDL if the implementation can't provide the data.

[timvolodine, 2013/03/13 15:29:49]

as Peter said, it's the spec
On 2013/03/13 13:37:09, Miguel Garcia wrote:

[timvolodine, 2013/03/13 15:29:49]

Done.

[Miguel Garcia, 2013/03/13 16:04:29]

Yeas I know I meant adding a comment explaining exactly that :) so a random
reader understands why

On 2013/03/13 15:29:49, timvolodine wrote:

[timvolodine, 2013/03/13 17:53:01]

Done.

+                    break;
+                default:
+                    return false;

[Peter Beverloo, 2013/03/13 14:02:47]

Can we assert here as this shouldn't happen?

[timvolodine, 2013/03/13 15:29:49]

Done.

+            }
+            if (success) {
                 mNativePtr = nativePtr;
-                return true;
+                setActiveEventType(eventType, true);
             }
-            return false;
+            return success;
         }
     }
 
     /**
-     * Stop listening for sensor events. Always succeeds.
+     * Stop listening for particular sensor events. Always succeeds.

[Miguel Garcia, 2013/03/13 16:04:29]

I think you need to upgrade this java doc. Something like

"Stop listening events for a given event type, it ensures that sensors for
overlapping event types are not disabled if they are still in use by a different
type."

[timvolodine, 2013/03/13 17:53:01]

Done.

      *
+     * @param eventType type of event to listen to, can be either 0 for device

[Peter Beverloo, 2013/03/13 14:02:47]

s/type/Type/.

[timvolodine, 2013/03/13 15:29:49]

Done.

+     *            orientation, or 1 for device motion.
      * We strictly guarantee that nativeGotOrientation() will not be called

[Miguel Garcia, 2013/03/13 13:37:09]

how about the other nativeGot* methods?

[timvolodine, 2013/03/13 15:29:49]

Done.

      * after this method returns.
      */
     @CalledByNative
-    public void stop() {
+    public void stop(int eventType) {
+        Set<Integer> sensorsToRemainActive = Sets.newHashSet();
         synchronized (mNativePtrLock) {
-            if (mNativePtr != 0) {
-                mNativePtr = 0;
-                unregisterForSensors();
+            switch (eventType) {
+                case DEVICE_ORIENTATION:
+                    if (mDeviceMotionIsActive) {
+                        sensorsToRemainActive.addAll(DEVICE_MOTION_SENSORS);
+                    }
+                    break;
+                case DEVICE_MOTION:
+                    if (mDeviceOrientationIsActive) {
+                        sensorsToRemainActive.addAll(DEVICE_ORIENTATION_SENSORS);
+                    }
+                    break;
+                default:
+                    return;

[Peter Beverloo, 2013/03/13 14:02:47]

Dito: Can we assert here as well?

[timvolodine, 2013/03/13 15:29:49]

Done.

             }
+
+            Set<Integer> sensorsToDeactivate = Sets.newHashSet(mActiveSensors);
+            sensorsToDeactivate.removeAll(sensorsToRemainActive);
+            unregisterForSensors(sensorsToDeactivate);

[Miguel Garcia, 2013/03/13 13:37:09]

I don't understand all these set operations
can't you simply do ?

case orientation:
  unregisterForSensors(mDeviceMotionIsActive ? DEVICE_MOTION_SENSORS :
mActiveSensors);
case motion:
  unregisterForSensors(mDeviceOrientationIsActive > DEVICE_ORIENTATION_SENSORS :
mActiveSensors);

[timvolodine, 2013/03/13 15:29:49]

the set of sensors for both events overlaps, so we need to be careful not to
unregister too much, hence all these set operations
On 2013/03/13 13:37:09, Miguel Garcia wrote:

[Miguel Garcia, 2013/03/13 16:04:29]

Would it feel more natural to have 3 sets?
common_sensors
orientation_only_sensors
motion_only_sensors.

Then all the register/unregister code will become way simpler

On 2013/03/13 15:29:49, timvolodine wrote:

[timvolodine, 2013/03/13 17:53:01]

that'll probably work too, but not sure if it would simplify the code much.
We'll still need active sensors, sensorsToDeactivate=orientation_only -
common_sensors or something like that...

On 2013/03/13 16:04:29, Miguel Garcia wrote:

+            mNativePtr = 0;
+            setActiveEventType(eventType, false);
         }
     }
 
@@ -104,8 +172,24 @@ class DeviceOrientation implements SensorEventListener {
                     mGravityVector = new float[3];
                 }
                 System.arraycopy(event.values, 0, mGravityVector, 0, mGravityVector.length);
+                if (mDeviceMotionIsActive) {

[Miguel Garcia, 2013/03/13 13:37:09]

why don't you use the same pattern than before, you collect the results (and
what you need to do) in the switch statement then later on you do call whatever
native methods need to be called. This way half of the case: statements call a
native method and the other half don't making it pretty confusing.

[Peter Beverloo, 2013/03/13 14:02:47]

I think this is the right approach.  We don't need to do further operations on
the accelerometer, linear acceleration or gyroscope data for Device Motion. 
Device Orientation requires us to know both the accelerometer and magnetic field
data, which is why we call it later on.

[timvolodine, 2013/03/13 15:29:49]

Done.

[timvolodine, 2013/03/13 15:29:49]

agree with Peter's comment, we don't need to compute anything for the motion
case..
On 2013/03/13 13:37:09, Miguel Garcia wrote:

[Miguel Garcia, 2013/03/13 16:04:29]

ok
On 2013/03/13 15:29:49, timvolodine wrote:

[timvolodine, 2013/03/13 17:53:01]

Done.

+                    gotAccelerationIncludingGravity(mGravityVector[0], mGravityVector[1],
+                                                    mGravityVector[2]);
+                }
+                break;
+            case Sensor.TYPE_LINEAR_ACCELERATION:
+                if (mAccelerationVector == null) {
+                    mAccelerationVector = new float[3];
+                }
+                gotAcceleration(mAccelerationVector[0], mAccelerationVector[1],
+                                mAccelerationVector[2]);

[Peter Beverloo, 2013/03/13 14:02:47]

What fills mAccelerationVector? :-).  This should call getAcceleration with the
event.values[{0, 1, 2}] data.

[timvolodine, 2013/03/13 15:29:49]

Done.

+                break;
+            case Sensor.TYPE_GYROSCOPE:
+                if (mGyroscopeVector == null) {
+                    mGyroscopeVector = new float[3];
+                }
+                gotRotationRate(mGyroscopeVector[0], mGyroscopeVector[1], mGyroscopeVector[2]);

[Peter Beverloo, 2013/03/13 14:02:47]

Dito to line 184.

[timvolodine, 2013/03/13 15:29:49]

Done.

                 break;
-
             case Sensor.TYPE_MAGNETIC_FIELD:
                 if (mMagneticFieldVector == null) {
                     mMagneticFieldVector = new float[3];
@@ -113,16 +197,17 @@ class DeviceOrientation implements SensorEventListener {
                 System.arraycopy(event.values, 0, mMagneticFieldVector, 0,
                                  mMagneticFieldVector.length);
                 break;
-
             default:
                 // Unexpected
                 return;
         }
 
-        getOrientationUsingGetRotationMatrix();
+        if (mDeviceOrientationIsActive) {
+            getOrientationUsingGetRotationMatrix();
+        }
     }
 
-    void getOrientationUsingGetRotationMatrix() {
+    private void getOrientationUsingGetRotationMatrix() {
         if (mGravityVector == null || mMagneticFieldVector == null) {
             return;
         }
@@ -163,15 +248,6 @@ class DeviceOrientation implements SensorEventListener {
         gotOrientation(alpha, beta, gamma);
     }
 
-    boolean registerForSensors(int rateInMilliseconds) {
-        if (registerForSensorType(Sensor.TYPE_ACCELEROMETER, rateInMilliseconds)
-                && registerForSensorType(Sensor.TYPE_MAGNETIC_FIELD, rateInMilliseconds)) {
-            return true;
-        }
-        unregisterForSensors();
-        return false;
-    }
-
     private SensorManager getSensorManager() {
         if (mSensorManager != null) {
             return mSensorManager;
@@ -180,12 +256,50 @@ class DeviceOrientation implements SensorEventListener {
         return mSensorManager;
     }
 
-    void unregisterForSensors() {
-        SensorManager sensorManager = getSensorManager();
-        if (sensorManager == null) {
-            return;
+    private void setActiveEventType(int eventType, boolean value) {
+        switch (eventType) {
+            case DEVICE_ORIENTATION:
+                mDeviceOrientationIsActive = value;
+                return;
+            case DEVICE_MOTION:
+                mDeviceMotionIsActive = value;
+                return;
+        }
+    }
+
+    /**
+     * @param sensorTypes List of sensors to activate.
+     * @param rateInMilliseconds intented delay (in ms) between sensor readings.

[Peter Beverloo, 2013/03/13 14:02:47]

s/intented/Intended/.  Can you spell out ms?  Ideally we'd move away from these
approximations in favor of specifying SENSOR_DELAY_*.

[timvolodine, 2013/03/13 15:29:49]

Done.

+     * @param failOnMissingSensor If true the method returns true only if all sensors could be
+     *                            activated. When false the method return true if at least one
+     *                            sensor in sensorTypes could be activated.
+     */
+    private boolean registerForSensors(Iterable<Integer> sensorTypes, int rateInMilliseconds,

[Miguel Garcia, 2013/03/13 13:37:09]

can this be called registerSensors while you are at it?

[timvolodine, 2013/03/13 15:29:49]

Done.

+                                       boolean failOnMissingSensor) {
+        boolean success = false;
+        for (Integer sensorType : sensorTypes) {
+            if (!mActiveSensors.contains(sensorType)) {
+                boolean result = registerForSensorType(sensorType, rateInMilliseconds);
+                success |= result;
+                if (!result && failOnMissingSensor) {
+                    // restore the previous state upon failure
+                    unregisterForSensors(sensorTypes);
+                    return false;
+                }
+            }
+            mActiveSensors.add(sensorType);

[Miguel Garcia, 2013/03/13 13:37:09]

should'nt you be checkout if the sensor activation has worked before adding it
to the list?

[timvolodine, 2013/03/13 15:29:49]

right
Done.
On 2013/03/13 13:37:09, Miguel Garcia wrote:

+        }
+        return success;
+    }
+
+    private void unregisterForSensors(Iterable<Integer> sensorTypes) {
+        for (Integer sensorType : sensorTypes) {
+            if (mActiveSensors.contains(sensorType)) {
+                getSensorManager().unregisterListener(this,
+                    getSensorManager().getDefaultSensor(sensorType));
+                mActiveSensors.remove(sensorType);
+            }
         }
-        sensorManager.unregisterListener(this);
     }
 
     boolean registerForSensorType(int type, int rateInMilliseconds) {
@@ -193,20 +307,17 @@ class DeviceOrientation implements SensorEventListener {
         if (sensorManager == null) {
             return false;
         }
-        List<Sensor> sensors = sensorManager.getSensorList(type);
-        if (sensors.isEmpty()) {
+        Sensor defaultSensor = sensorManager.getDefaultSensor(type);

[Peter Beverloo, 2013/03/13 14:02:47]

Please see my replies in regards to this on the old patch set.  I left two
comments there.

[timvolodine, 2013/03/13 15:29:49]

Done.

+        if (defaultSensor == null) {
             return false;
         }
 
         final int rateInMicroseconds = 1000 * rateInMilliseconds;
-        // We want to err on the side of getting more events than we need.
-        final int requestedRate = rateInMicroseconds / 2;
-
-        // TODO(steveblock): Consider handling multiple sensors.
-        return sensorManager.registerListener(this, sensors.get(0), requestedRate, getHandler());
+        return sensorManager.registerListener(this, defaultSensor, rateInMicroseconds,
+                                              getHandler());
     }
 
-    void gotOrientation(double alpha, double beta, double gamma) {
+    private void gotOrientation(double alpha, double beta, double gamma) {
         synchronized (mNativePtrLock) {
             if (mNativePtr != 0) {
               nativeGotOrientation(mNativePtr, alpha, beta, gamma);
@@ -214,6 +325,30 @@ class DeviceOrientation implements SensorEventListener {
         }
     }
 
+    private void gotAcceleration(double x, double y, double z) {
+        synchronized (mNativePtrLock) {
+            if (mNativePtr != 0) {
+              nativeGotAcceleration(mNativePtr, x, y, z);
+            }
+        }
+    }
+
+    private void gotAccelerationIncludingGravity(double x, double y, double z) {
+        synchronized (mNativePtrLock) {
+            if (mNativePtr != 0) {
+              nativeGotAccelerationIncludingGravity(mNativePtr, x, y, z);
+            }
+        }
+    }
+
+    private void gotRotationRate(double alpha, double beta, double gamma) {
+        synchronized (mNativePtrLock) {
+            if (mNativePtr != 0) {
+              nativeGotRotationRate(mNativePtr, alpha, beta, gamma);
+            }
+        }
+    }
+
     private Handler getHandler() {
         synchronized (mHandlerLock) {
             // If we don't have a background thread, start it now.
@@ -251,19 +386,46 @@ class DeviceOrientation implements SensorEventListener {
     }
 
     @CalledByNative
-    private static DeviceOrientation getInstance() {
+    private static DeviceMotionAndOrientation getInstance() {
         synchronized (sSingletonLock) {
             if (sSingleton == null) {
-                sSingleton = new DeviceOrientation();
+                sSingleton = new DeviceMotionAndOrientation();
             }
             return sSingleton;
         }
     }
 
     /**
-     * See chrome/browser/device_orientation/data_fetcher_impl_android.cc
+     * Native JNI calls,
+     * see content/browser/device_orientation/data_fetcher_impl_android.cc
+     */
+
+    /**
+     * Orientation of the device with respect to its reference frame.
      */
     private native void nativeGotOrientation(
             int nativeDataFetcherImplAndroid,
             double alpha, double beta, double gamma);
-}
+
+    /**
+     * Linear acceleration without gravity of the device with respect to its body frame.
+     */
+    private native void nativeGotAcceleration(
+            int nativeDataFetcherImplAndroid,
+            double x, double y, double z);
+
+    /**
+     * Acceleration including gravity of the device with respect to its body frame.
+     */
+    private native void nativeGotAccelerationIncludingGravity(
+            int nativeDataFetcherImplAndroid,
+            double x, double y, double z);
+
+    /**
+     * Rotation rate of the device with respect to its body frame.
+     */
+    private native void nativeGotRotationRate(
+            int nativeDataFetcherImplAndroid,
+            double alpha, double beta, double gamma);
+
+}