Android: fix the computation of device orientation angles in accordance with spec.

Android: fix the computation of device orientation angles in accordance with spec.

Currently the alpha, beta, gamma device orientation angles are
computed incorrectly in Chrome. This patch provides an implementation
for the computation of these angles according to spec, see
http://dev.w3.org/geo/api/spec-source-orientation.html. In particular
the ranges for alpha [0,2*pi), beta [-pi,pi) and gamma [-pi/2,pi/2),
with clockwise-positive rotation conventions.

At present time most browsers implement the orientation angles
incorrectly. For example Safari, Firefox and Opera all provide
different and inconsitent results. The solution in this patch could
serve as a reference implementation.

BUG=244411
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232299

[timvolodine, 2013-10-24 15:28:19]

+ Marcus

[because we need an owner ;) ]

[bulach, 2013-10-24 17:08:44]

lgtm, just nits.

also, I'd be happy to lgtm a change adding you both as OWNERS for these files!
(have to be on this same directory, and then add a per-file directive...)

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java
(right):

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:274:
values[1] = Math.asin(R[7]);           // beta (-pi/2, pi/2)
both in comments and in some real places like 294, spaces around "/" and "*"
(and the docstring above as well)

https://codereview.chromium.org/40393002/diff/1/content/public/android/javate...
File
content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java
(right):

https://codereview.chromium.org/40393002/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:222:
float[] gravity = {0, (float)Math.sin(Math.PI/4), (float)Math.cos(Math.PI/4)};
ditto, spaces around "/"

[Michael van Ouwerkerk, 2013-10-29 16:18:06]

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java
(right):

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:231:
* <b>R</b> = <b>Rz</b>(alpha)*<b>Rx</b>(beta)*<b>Ry</b>(gamma),
Perhaps all of this documentation would be easier to read without the bold and
italic tags. It seems more common to read this as plain text than as rendered
html.

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:261:
public static void getOrientationMobileDevice(float[] R, double[] values) {
Why not create the array in this method and then return it? It seems more
idiomatic for Java, and the @return documentation would make more sense.

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:261:
public static void getOrientationMobileDevice(float[] R, double[] values) {
@VisibleForTesting

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:261:
public static void getOrientationMobileDevice(float[] R, double[] values) {
How about naming this according to what it does e.g.
convertRotationMatrixToAngles or getAnglesFromRotationMatrix.

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:270:
return;
Worth calling out in the Javadoc the possibility for a 'zero angles' result if
the input is incomplete? Also worth logging this event? Anyway, when does this
happen? Should it throw an exception instead?

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:318:
gotOrientation(Math.toDegrees(rotationAngles[0]),
If degrees are needed, why not return those from the method that converts the
matrix?

https://codereview.chromium.org/40393002/diff/1/content/public/android/javate...
File
content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java
(right):

https://codereview.chromium.org/40393002/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:277:
* Helper method for veryfying angles obtained from rotation matrix.
s/veryfying/verifying/

[timvolodine, 2013-10-30 14:14:46]

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
File
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java
(right):

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:231:
* <b>R</b> = <b>Rz</b>(alpha)*<b>Rx</b>(beta)*<b>Ry</b>(gamma),
On 2013/10/29 16:18:06, Michael van Ouwerkerk wrote:
> Perhaps all of this documentation would be easier to read without the bold and
> italic tags. It seems more common to read this as plain text than as rendered
> html.

Done.

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:261:
public static void getOrientationMobileDevice(float[] R, double[] values) {
On 2013/10/29 16:18:06, Michael van Ouwerkerk wrote:
> Why not create the array in this method and then return it? It seems more
> idiomatic for Java, and the @return documentation would make more sense.

ok, I've added a return double[]. However we still need the values output
parameter for efficiency purposes, so that any code using this function can
potentially avoid using "new" for each invocation,
SensorManager.getOrientation() does this as well.

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:261:
public static void getOrientationMobileDevice(float[] R, double[] values) {
On 2013/10/29 16:18:06, Michael van Ouwerkerk wrote:
> @VisibleForTesting

Done.

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:261:
public static void getOrientationMobileDevice(float[] R, double[] values) {
On 2013/10/29 16:18:06, Michael van Ouwerkerk wrote:
> How about naming this according to what it does e.g.
> convertRotationMatrixToAngles or getAnglesFromRotationMatrix.

renamed to computeDeviceOrientationFromRotationMatrix()

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:270:
return;
On 2013/10/29 16:18:06, Michael van Ouwerkerk wrote:
> Worth calling out in the Javadoc the possibility for a 'zero angles' result if
> the input is incomplete? Also worth logging this event? Anyway, when does this
> happen? Should it throw an exception instead?

this should not happen, or it would be incorrect usage of the function. I've
added "a 3x3 rotation matrix" as a precondition.

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:274:
values[1] = Math.asin(R[7]);           // beta (-pi/2, pi/2)
On 2013/10/24 17:08:44, bulach wrote:
> both in comments and in some real places like 294, spaces around "/" and "*"
> (and the docstring above as well)

Done.
(however left some comments as is for readability)

https://codereview.chromium.org/40393002/diff/1/content/public/android/java/s...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:318:
gotOrientation(Math.toDegrees(rotationAngles[0]),
On 2013/10/29 16:18:06, Michael van Ouwerkerk wrote:
> If degrees are needed, why not return those from the method that converts the
> matrix?

yeah, but to make it more generic it would be better to use radians.
SensorManager uses radians for getOrientation() and other methods as well.

https://codereview.chromium.org/40393002/diff/1/content/public/android/javate...
File
content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java
(right):

https://codereview.chromium.org/40393002/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:222:
float[] gravity = {0, (float)Math.sin(Math.PI/4), (float)Math.cos(Math.PI/4)};
On 2013/10/24 17:08:44, bulach wrote:
> ditto, spaces around "/"

Done.

https://codereview.chromium.org/40393002/diff/1/content/public/android/javate...
content/public/android/javatests/src/org/chromium/content/browser/DeviceMotionAndOrientationTest.java:277:
* Helper method for veryfying angles obtained from rotation matrix.
On 2013/10/29 16:18:06, Michael van Ouwerkerk wrote:
> s/veryfying/verifying/

Done.

[Michael van Ouwerkerk, 2013-10-30 14:21:02]

lgtm

https://codereview.chromium.org/40393002/diff/90001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java
(right):

https://codereview.chromium.org/40393002/diff/90001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:240:
* 'pitch' angle beta is allowed to be wihtin [-pi, pi). A mobile device with
pitch angle
nit: s/wihtin/within/

[timvolodine, 2013-10-30 14:40:00]

thanks for the review guys!

https://codereview.chromium.org/40393002/diff/90001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java
(right):

https://codereview.chromium.org/40393002/diff/90001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/DeviceMotionAndOrientation.java:240:
* 'pitch' angle beta is allowed to be wihtin [-pi, pi). A mobile device with
pitch angle
On 2013/10/30 14:21:02, Michael van Ouwerkerk wrote:
> nit: s/wihtin/within/

Done.

[commit-bot: I haz the power, 2013-10-31 18:32:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/40393002/160001

[commit-bot: I haz the power, 2013-10-31 23:17:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/40393002/160001

[commit-bot: I haz the power, 2013-11-01 00:15:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/timvolodine@chromium.org/40393002/160001

[commit-bot: I haz the power, 2013-11-01 05:36:28]

Change committed as 232299