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

-// 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.
 
 package org.chromium.content.browser;
 
 import android.content.Context;
 import android.hardware.Sensor;
 import android.hardware.SensorEvent;
 import android.hardware.SensorEventListener;
 import android.hardware.SensorManager;
 import android.os.Handler;
 import android.os.Looper;
 
+import com.google.common.collect.ImmutableMap;
+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;
+import java.util.Set;
 
 /**
- * Android implementation of the DeviceOrientation API.
+ * Android implementation of the device motion and orientation API.

[Peter Beverloo, 2013/03/12 17:11:22]

nit: s/API/APIs/.

[timvolodine, 2013/03/13 12:32:24]

Done.

  */
 @JNINamespace("content")
-class DeviceOrientation implements SensorEventListener {
+class DeviceMotionAndOrientation implements SensorEventListener {
 
     // These fields are lazily initialized by getHandler().
     private Thread mThread;
     private Handler mHandler;
 
     // The lock to access the mHandler.
     private Object mHandlerLock = new Object();
 
     // Non-zero if and only if we're listening for events.
     // To avoid race conditions on the C++ side, access must be synchronized.
     private int mNativePtr;
 
     // The lock to access the mNativePtr.
     private Object mNativePtrLock = new Object();
 
     // The gravity vector expressed in the body frame.
     private float[] mGravityVector;
 
     // The geomagnetic vector expressed in the body frame.
     private float[] mMagneticFieldVector;
 
+    // The linear acceleration vector expressed in the body frame.
+    private float[] mAccelerationVector;
+
+    // Gyroscope

[Peter Beverloo, 2013/03/12 17:11:22]

"The angular speed at which the device is rotating around the body frame."

[timvolodine, 2013/03/13 12:32:24]

Done.

+    private float[] mGyroVector;

[Peter Beverloo, 2013/03/12 17:11:22]

Let's spell out the full name: mGyroscopeVector.

[timvolodine, 2013/03/13 12:32:24]

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
+     * @see #start
+     * @see #stop
+     */
+    private static enum EventType {

[Miguel Garcia, 2013/03/12 17:00:26]

what happened to my comment about not using an enum?

[Peter Beverloo, 2013/03/12 17:11:22]

Please use int constants instead of enums (enums imply a significant weight on
the dex files).

[timvolodine, 2013/03/13 12:32:24]

sorry seem to have missed that while uploading new diff

On 2013/03/12 17:00:26, Miguel Garcia wrote:

[timvolodine, 2013/03/13 12:32:24]

Done.

+        DEVICE_ORIENTATION(0),
+        DEVICE_MOTION(1);

[Miguel Garcia, 2013/03/12 17:00:26]

test?

[timvolodine, 2013/03/13 12:32:24]

hmm not sure, is this N/A anymore (?)

+
+        public int value;
+        private EventType(int value) {
+            this.value = value;
+        }
+    }
+
+    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 static Map<Integer, ImmutableSet<Integer>> SENSORS_FOR_EVENT = ImmutableMap.of(
+        EventType.DEVICE_ORIENTATION.value, DEVICE_ORIENTATION_SENSORS,
+        EventType.DEVICE_MOTION.value, DEVICE_MOTION_SENSORS);

[Peter Beverloo, 2013/03/12 17:11:22]

For line 82 to 95: I think this is much too verbose.  We know that the
accelerometer is necessary for both events, and then there are event-specific
sensors.  It would be really easy and much clearer to just implement this with
an if-statement or two in start() and stop, which would take away the need to
have all these sets.

[timvolodine, 2013/03/13 12:32:24]

Done.

+
+    // keep an array of active event types for speed because it is used in @see #

[Miguel Garcia, 2013/03/12 17:00:26]

@see # ?

where is it used?

[Peter Beverloo, 2013/03/12 17:11:22]

nit: Grammar. You seem to be missing a reference here.

[timvolodine, 2013/03/13 12:32:24]

Done.

[timvolodine, 2013/03/13 12:32:24]

Done.

+    private boolean[] mIsSpecEventActive = new boolean[EventType.values().length];

[Peter Beverloo, 2013/03/12 17:11:22]

As I mentioned in the native code review, please don't use the "Spec" prefix. 
Since there are only two event types with no new ones planned, I think it'd be
nicer to just have two booleans.

[timvolodine, 2013/03/13 12:32:24]

Done.

+
+    private DeviceMotionAndOrientation() {
+        mIsSpecEventActive[EventType.DEVICE_ORIENTATION.value] = false;
+        mIsSpecEventActive[EventType.DEVICE_MOTION.value] = false;
     }

[Miguel Garcia, 2013/03/12 17:00:26]

can you do this in a for loop so we don't forget to add new types eventually?

[Peter Beverloo, 2013/03/12 17:11:22]

I think we should just have two separate members.

[timvolodine, 2013/03/13 12:32:24]

Done.

[timvolodine, 2013/03/13 12:32:24]

Done.

 
     /**
      * Start listening for sensor events. If this object is already listening
      * for events, the old callback is unregistered first.
      *
      * @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 specEventType type of event to listen to, can either 0 for device

[Peter Beverloo, 2013/03/12 17:11:22]

nit: grammar (capital). s/can either/can be either/.

[timvolodine, 2013/03/13 12:32:24]

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) {
+        Set<Integer> sensorsForEvent = SENSORS_FOR_EVENT.get(eventType);
+        if (sensorsForEvent == null)
+            return false;
+
         synchronized (mNativePtrLock) {
-            stop();
-            if (registerForSensors(rateInMilliseconds)) {
+            Set<Integer> sensorsToActivate = Sets.newHashSet(sensorsForEvent);
+            sensorsToActivate.removeAll(mActiveSensors);
+            boolean success = registerForSensors(sensorsToActivate, rateInMilliseconds);
+            if (success) {
                 mNativePtr = nativePtr;
-                return true;
+                mIsSpecEventActive[eventType] = true;
             }
-            return false;
+            return success;
         }
     }
 
     /**
-     * Stop listening for sensor events. Always succeeds.
+     * Stop listening for particular sensor events. Always succeeds.
      *
+     * @param specEventType type of event to listen to, can either 0 for device

[Peter Beverloo, 2013/03/12 17:11:22]

nit: dito to line 112.

[timvolodine, 2013/03/13 12:32:24]

Done.

+     *            orientation, or 1 for device motion.
      * We strictly guarantee that nativeGotOrientation() will not be called
      * after this method returns.
      */
     @CalledByNative
-    public void stop() {
+    public void stop(int eventType) {
+        if (SENSORS_FOR_EVENT.get(eventType) == null)
+            return;
+        Set<Integer> sensorsToRemainActive = Sets.newHashSet();
+
         synchronized (mNativePtrLock) {
-            if (mNativePtr != 0) {
-                mNativePtr = 0;
-                unregisterForSensors();
+            for (EventType event : EventType.values()) {
+                if (event.value != eventType && mIsSpecEventActive[event.value]) {
+                    sensorsToRemainActive.addAll(SENSORS_FOR_EVENT.get(event.value));
+                }
             }
+            Set<Integer> sensorsToDeactivate = Sets.newHashSet(mActiveSensors);
+            sensorsToDeactivate.removeAll(sensorsToRemainActive);
+            unregisterForSensors(sensorsToDeactivate);
+            mNativePtr = 0;
+            mIsSpecEventActive[eventType] = false;
         }
     }
 
     @Override
     public void onAccuracyChanged(Sensor sensor, int accuracy) {
         // Nothing
     }
 
     @Override
     public void onSensorChanged(SensorEvent event) {
+        boolean dispatchDeviceOrientation =

[Miguel Garcia, 2013/03/12 17:00:26]

I would change this a bit so that the switch just fills in the array, then we do
a boundary check (important) then decide what method to call once we have the
sensor data.

[timvolodine, 2013/03/13 12:32:24]

Done.

+            mIsSpecEventActive[EventType.DEVICE_ORIENTATION.value];
+        boolean dispatchDeviceMotion =
+            mIsSpecEventActive[EventType.DEVICE_MOTION.value];

[Peter Beverloo, 2013/03/12 17:11:22]

When we have two separate booleans for the events, we don't need have this at
all.

[timvolodine, 2013/03/13 12:32:24]

Done.

+
         switch (event.sensor.getType()) {
             case Sensor.TYPE_ACCELEROMETER:
                 if (mGravityVector == null) {
                     mGravityVector = new float[3];
                 }
                 System.arraycopy(event.values, 0, mGravityVector, 0, mGravityVector.length);
+                if (dispatchDeviceMotion)

[Peter Beverloo, 2013/03/12 17:11:22]

nit: Please include brackets for readability. The style guide prohibits
multiple-line if statements (two, in this case) without brackets (see the "Use
Standard Brace Style" section).

[timvolodine, 2013/03/13 12:32:24]

Done.

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

[Peter Beverloo, 2013/03/12 17:11:22]

We're not using mAccelerationVector anywhere outside this block.  Please don't
copy everything and just invoke gotAcceleration(event.values[0], ...);

[timvolodine, 2013/03/13 12:32:24]

Done.

+                    mAccelerationVector = new float[3];
+                }
+                System.arraycopy(event.values, 0, mAccelerationVector, 0,
+                    mAccelerationVector.length);
+                if (dispatchDeviceMotion)

[Peter Beverloo, 2013/03/12 17:11:22]

Why do we have this if statement here?  We'll only receive linear acceleration
sensor results if device motion events are being listened to.

[timvolodine, 2013/03/13 12:32:24]

Done.

+                    gotAcceleration(mAccelerationVector[0], mAccelerationVector[1],
+                                    mAccelerationVector[2]);
+                break;

[Miguel Garcia, 2013/03/12 17:00:26]

extra blank line

[timvolodine, 2013/03/13 12:32:24]

Done.

+
+            case Sensor.TYPE_GYROSCOPE:

[Peter Beverloo, 2013/03/12 17:11:22]

This whole block: dito to the comments on line 185 and 190.

[timvolodine, 2013/03/13 12:32:24]

Done.

+                if (mGyroVector == null) {
+                    mGyroVector = new float[3];
+                }
+                System.arraycopy(event.values, 0, mGyroVector, 0, mGyroVector.length);
+                if (dispatchDeviceMotion)
+                    gotRotationRate(mGyroVector[0], mGyroVector[1], mGyroVector[2]);
                 break;
 
             case Sensor.TYPE_MAGNETIC_FIELD:
                 if (mMagneticFieldVector == null) {
                     mMagneticFieldVector = new float[3];
                 }
                 System.arraycopy(event.values, 0, mMagneticFieldVector, 0,
                                  mMagneticFieldVector.length);
                 break;
 
             default:
                 // Unexpected

[Miguel Garcia, 2013/03/12 17:00:26]

can you log/assert something here?

[timvolodine, 2013/03/13 12:32:24]

It feels a bit scary if something goes wrong and we would log at @50Hz..
On 2013/03/12 17:00:26, Miguel Garcia wrote:

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

Maybe.  We know that something is wrong when we listen to an unidentified
sensor, so it shouldn't happen.

                 return;
         }
 
-        getOrientationUsingGetRotationMatrix();
+        if (dispatchDeviceOrientation)

[Miguel Garcia, 2013/03/12 17:00:26]

I think the style guide says to use brackets on all ifs

[Peter Beverloo, 2013/03/12 17:11:22]

nit: dito to the comment on line 180.

[timvolodine, 2013/03/13 12:32:24]

Done.

[timvolodine, 2013/03/13 12:32:24]

Done.

+            getOrientationUsingGetRotationMatrix();
     }
 
     void getOrientationUsingGetRotationMatrix() {
         if (mGravityVector == null || mMagneticFieldVector == null) {
             return;
         }
 
         // Get the rotation matrix.
         // The rotation matrix that transforms from the body frame to the earth
         // frame.
@@ skipping 23 matching lines @@
         }
 
         double gamma = Math.toDegrees(rotationAngles[2]);
         while (gamma < -90.0) {
             gamma += 360.0; // [-90, 90)
         }
 
         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;
         }
         mSensorManager = (SensorManager)WeakContext.getSystemService(Context.SENSOR_SERVICE);
         return mSensorManager;
     }
 
-    void unregisterForSensors() {
-        SensorManager sensorManager = getSensorManager();
-        if (sensorManager == null) {
-            return;
+    private boolean registerForSensors(Iterable<Integer> sensorTypes, int rateInMilliseconds) {
+        for (Integer sensorType : sensorTypes) {
+            if (!mActiveSensors.contains(sensorType)) {
+                if (!registerForSensorType(sensorType, rateInMilliseconds)) {
+                    // restore the previous state upon failure
+                    unregisterForSensors(sensorTypes);

[Peter Beverloo, 2013/03/12 17:11:22]

Won't this fail if device orientation registered first (and the accelerometer
and gravity sensors are available), then device motion registers, we're trying
to register new sensors and can't register a linear acceleration sensor?  In
that case, we'd unregister all of them again, also losing device orientation
coverage.

[timvolodine, 2013/03/13 12:32:24]

this method is always called from start() with a list of inactive sensors only,
so when unregistering no active sensor should be touched.
There is a different issue here though: the method returns a boolean on success,
while the definition of success is different for device orientation/device
motion. I have added a flag and a doc to reflect that logic.

On 2013/03/12 17:11:22, Peter Beverloo wrote:

+                    return false;
+                }
+            }
+            mActiveSensors.add(sensorType);
         }
-        sensorManager.unregisterListener(this);
+        return true;
+    }
+
+    private void unregisterForSensors(Iterable<Integer> sensorTypes) {
+        for (Integer sensorType : sensorTypes) {
+            if (mActiveSensors.contains(sensorType)) {
+                getSensorManager().unregisterListener(this,
+                    getSensorManager().getDefaultSensor(sensorType));
+                mActiveSensors.remove(sensorType);
+            }
+        }
     }
 
     boolean registerForSensorType(int type, int rateInMilliseconds) {
         SensorManager sensorManager = getSensorManager();
         if (sensorManager == null) {
             return false;
         }
-        List<Sensor> sensors = sensorManager.getSensorList(type);
-        if (sensors.isEmpty()) {
+        Sensor defaultSensor = sensorManager.getDefaultSensor(type);

[Peter Beverloo, 2013/03/12 17:11:22]

I looked into doing this change myself, but I'm a bit wary because
getDefaultSensor mentions it can combine the values of multiple sensors, which
we're already doing.  Furthermore, the implementation in Android right now is a
stub, it does exactly the same to what we are doing right now.

[timvolodine, 2013/03/13 12:32:24]

ok, so should I revert to the previous solution with get(0)?
also it sounds more logical to use sensor fusion (if available) as it is as
close to hardware as possible and could possibly produce more accurate
results... e.g. the current implementation of orientation is noisy in chrome,
while that of firefox is smooth, I suspect they use Sensor.TYPE_ORIENTATION
which seems to use sensor fusion.. anyway this is probably out of scope of this
patch.

On 2013/03/12 17:11:22, Peter Beverloo wrote:

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

I do think we should revert until we properly understand what it will do.  Right
now it's undefined and we're doing our own calculations, whereas a combination
sensor (maybe similar to TYPE_ORIENTATION) will do it for us.  By using
getDefaultSensor() the behavior will change unexpectedly when Android implements
this.

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

Can you file an issue to track looking in to switching to TYPE_ORIENTATION?

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

Done.

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

will do

+        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) {
         synchronized (mNativePtrLock) {
             if (mNativePtr != 0) {
               nativeGotOrientation(mNativePtr, alpha, beta, gamma);
             }
         }
     }
 
+    void gotAcceleration(double x, double y, double z) {

[Miguel Garcia, 2013/03/12 17:00:26]

I wonder if we could not simplify this by having one single method that takes
the parameter you want (acceleration,accel + gravity, rotation rate or
orientation) and the coordinate/angles that would avoid having 4 methods
everywhere. What do you think Peter?

[Miguel Garcia, 2013/03/12 17:00:26]

can these methods be private or public if meant to be called from somewhere,
package private seems useless in this context

[Peter Beverloo, 2013/03/12 17:11:22]

We cannot be sure in regards to which sensors we can receive data from, as such,
I think having four separate methods would be best.  That allows us to determine
that we have to pass NULL to the event in the renderer process.

[Peter Beverloo, 2013/03/12 17:11:22]

They should be private.

[timvolodine, 2013/03/13 12:32:24]

Done.

[timvolodine, 2013/03/13 12:32:24]

Done.

[timvolodine, 2013/03/13 12:32:24]

Done.

[timvolodine, 2013/03/13 12:32:24]

Done.

+        synchronized (mNativePtrLock) {
+            if (mNativePtr != 0) {
+              nativeGotAcceleration(mNativePtr, x, y, z);
+            }
+        }
+    }
+
+    void gotAccelerationIncludingGravity(double x, double y, double z) {
+        synchronized (mNativePtrLock) {
+            if (mNativePtr != 0) {
+              nativeGotAccelerationIncludingGravity(mNativePtr, x, y, z);
+            }
+        }
+    }
+
+    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.
             if (mThread == null) {
                 mThread = new Thread(new Runnable() {
                     @Override
                     public void run() {
                         Looper.prepare();
                         // Our Handler doesn't actually have to do anything, because
                         // SensorManager posts directly to the underlying Looper.
@@ skipping 17 matching lines @@
     }
 
     private void setHandler(Handler handler) {
         synchronized (mHandlerLock) {
             mHandler = handler;
             mHandlerLock.notify();
         }
     }
 
     @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
      */
     private native void nativeGotOrientation(
             int nativeDataFetcherImplAndroid,
             double alpha, double beta, double gamma);
-}
+
+    /**
+     * linear acceleration w/o gravity

[Peter Beverloo, 2013/03/12 17:11:22]

nit: Please use proper grammar and spell out "without".  The
nativeGotOrientation part could use a similar comment.

[timvolodine, 2013/03/13 12:32:24]

Done.

+     */
+    private native void nativeGotAcceleration(
+            int nativeDataFetcherImplAndroid,
+            double x, double y, double z);
+
+    /**
+     * acceleration including gravity

[Peter Beverloo, 2013/03/12 17:11:22]

nit: dito.

[timvolodine, 2013/03/13 12:32:24]

Done.

+     */
+    private native void nativeGotAccelerationIncludingGravity(
+            int nativeDataFetcherImplAndroid,
+            double x, double y, double z);
+
+    /**
+     * gyroscope

[Peter Beverloo, 2013/03/12 17:11:22]

nit: dito.

[timvolodine, 2013/03/13 12:32:24]

Done.

+     */
+    private native void nativeGotRotationRate(
+            int nativeDataFetcherImplAndroid,
+            double alpha, double beta, double gamma);
+
+}