662678 - w3c device orientation spec changes

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Doug Turner (:dougt)]

Follow up to 615597.  A few more spec changes.

Basically this first patch makes devicemotion work very much like the old mozOrientation event.  It also has a bunch of support to implement the other parts of the spec such as deviceorientation and the support classes for devicemotion.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Alon Zakai (:azakai)]

I don't understand the reason for the alpha/beta/gamma => x/y/z shift - why do we want to go back to the old mozOrientationEvent behavior? The spec still uses alpha/beta/gamma I think?

Comment 3

[Doug Turner (:dougt)]

the spec has 3 parts.  the part that was implemented was deviceorientation.  it was getting the wrong data from most of the accelerometer implementations which basically send movement data (m/s).  The android one was changed such that it returned 'sorta' the right data for two of the axises.

So, basically this patch implements another part of the spec called devicemotion and *temporary* disables deviceorientation.  In a follow up patch, i will review the android sensor changes such that all accelerometers in gecko will return m/s data.

In other followups, i will implement an orientation service (if supported on the platform), which will return alpha/beta/gamma as defined by the spec.

The end result after this patch is that people can migrate from mozOrientationEvent -> devicemotion without too much pain.

Comment 4

[Alon Zakai (:azakai)]

Offline discussion summary:

1. We were receiving what amounts to accelerationPlusGravity, and inferred orientation from that.

2. We should output both orientation and motion events given that input. But, if we do get proper orientation info, we should not infer it from the accelerationPlusGravity, we should output the correct data directly.

3. The patch changes x/y/z into alpha/beta/gamma. But we will need both. In fact we will need more - x,y,z, alpha/beta/gamma, rotation changes, etc. So there are many 3-tuples of doubles here that interest us. Perhaps the ipc message should be a generic 3-tuple of doubles plus a type, for simplicity?

Comment 5

[Alon Zakai (:azakai)]

Attachment: Use orientation data on Android

Here is a patch that uses the orientation data from Android, instead of inferring it from accelerometer data.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 538126 [details] [diff] [review]
Use orientation data on Android

very cool... i think we want both (to support both devicemotion and deviceorientation).  thanks for the patch.  I will roll it into the rest of the changes I am working on (coming soon!)

Comment 7

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

if onfoo listeners should be supported, just look at nsDOMClassInfo for
things like onclick.

Comment 9

[Doug Turner (:dougt)]

smaug - yup, to make webkit behavior.  i will do this in a follow up patch.  I'd like to get the bigger changes in as soon as possible.

Comment 10

[Alon Zakai (:azakai)]

Comment on attachment 538608 [details] [diff] [review]
patch v.2

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

Overall looks good. Various minor nits.

Only substantial questions are

1. Why remove the hasNewListener, that was throttling duplicate values?
2. We should rename a lot. Can do it here or in a followup bug. Maybe makes sense to leave for a followup, and consider things carefully there.

::: content/events/src/nsDOMCompassNeedsCalibrationEvent.cpp
@@ +14,5 @@
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + * Doug Turner <[e-mail]>
> + * Portions created by the Initial Developer are Copyright (C) 2009

Copyright should be 2011. Ditto in other new files here. Are they copied from the latest official boilerplate?

http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

::: content/events/src/nsDOMDeviceMotionEvent.cpp
@@ +135,5 @@
> +}
> +
> +NS_IMETHODIMP nsDOMDeviceAcceleration::GetX(double *aX)
> +{
> +  *aX = mX;

NS_ENSURE_ARG_POINTER (also two places below)

@@ +174,5 @@
> +}
> +
> +NS_IMETHODIMP nsDOMDeviceRotationRate::GetAlpha(double *aAlpha)
> +{
> +  *aAlpha = mAlpha;

NS_ENSURE_ARG_POINTER and also two places below

::: content/events/src/nsDOMDeviceMotionEvent.h
@@ +70,5 @@
> +  double mX, mY, mZ;
> +};
> +
> +class nsDOMDeviceMotionEvent : public nsDOMEvent,
> +                              public nsIDOMDeviceMotionEvent

add 1 space for indentation please

::: content/events/src/nsDOMOrientationEvent.h
@@ +44,1 @@
>                                public nsIDOMDeviceOrientationEvent

needs some spaces to align with previous line

::: content/events/src/nsEventListenerManager.cpp
@@ +500,5 @@
>                                     kAllMutationBits :
>                                     MutationBitForEventType(aType));
>      }
> +  } else if (aTypeAtom == nsGkAtoms::ondeviceorientation ||
> +             aTypeAtom == nsGkAtoms::ondevicemotion) {

We might want to split up the OrientationEventListener into orientation/motion. Or at least rename it somehow. Followup bug?

::: dom/base/nsGlobalWindow.cpp
@@ +829,5 @@
>      mShowFocusRings(PR_TRUE),
>  #endif
>      mShowFocusRingForContent(PR_FALSE),
>      mFocusByKeyOccurred(PR_FALSE),
> +    mHasDeviceMotion(PR_FALSE),

This is used for both motion and orientation, I think? Perhaps we should rename it to mHasDeviceSensor?

@@ +9955,5 @@
>    PRBool shouldResume = (mTimeoutsSuspendDepth == 0);
>    nsresult rv;
>  
>    if (shouldResume) {
> +    EnableDeviceMotionUpdates();

EnableDeviceSensorUpdates perhaps?

::: dom/system/Makefile.in
@@ +46,5 @@
>  LIBXUL_LIBRARY  = 1
>  
>  ifneq (,$(filter qt gtk2,$(MOZ_WIDGET_TOOLKIT)))
>  DIRS = unix
> +	endif

unneeded indentation

::: dom/system/cocoa/nsAccelerometerSystem.h
@@ +1,1 @@
> +

unneeded line

::: dom/system/nsAccelerometer.cpp
@@ +62,3 @@
>  
>  protected:
> +  unsigned long mType;

What is mType?

@@ +73,5 @@
> +NS_INTERFACE_MAP_BEGIN(nsDeviceMotionData)
> +NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDeviceMotionData)
> +NS_INTERFACE_MAP_END
> +
> +NS_IMPL_THREADSAFE_ADDREF(nsDeviceMotionData)

Why threadsafe?

@@ +118,5 @@
>  {
>    nsCOMPtr<nsIPrefBranch> prefSrv = do_GetService(NS_PREFSERVICE_CONTRACTID);
>    if (prefSrv) {
>      PRInt32 value;
> +    nsresult rv = prefSrv->GetIntPref("device.motion.update.interval", &value);

Should a default value for these be defined in the prefs files?

@@ +123,5 @@
>      if (NS_SUCCEEDED(rv))
>        mUpdateInterval = value;
>  
>      PRBool bvalue;
> +    rv = prefSrv->GetBoolPref("device.motion.enabled", &bvalue);

ditto

@@ -166,5 @@
>      return NS_OK; // already exists
>  
>    if (mStarted == PR_FALSE) {
>      mStarted = PR_TRUE;
> -    mNewListener = PR_TRUE;

We were using this before to stop sending unneeded updates. Do we not want to do that anymore?

@@ +240,5 @@
> +      nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(mWindowListeners[i]);
> +      if (type == nsIDeviceMotionData::TYPE_ACCELERATION)
> +        FireDOMMotionEvent(domdoc, target, x, y, z);
> +      else if (type == nsIDeviceMotionData::TYPE_ORIENTATION)
> +        FireDOMOrientationEvent(domdoc, target, x, y, z);

We pass x,y,z here into params called alpha/beta/gamma. This is potentially confusing. At least add a comment here?

::: dom/system/nsAccelerometer.h
@@ +47,1 @@
>  { 0xecba5203, 0x77da, 0x465a, \

Refresh CID? Not sure if necessary.

::: dom/system/unix/nsAccelerometerSystem.cpp
@@ +194,2 @@
>  {
> +  // DeviceMotions in Linux are used by reading a file (yay UNIX!), which is

Arguably in this function, all DeviceMotions should remain Accelerometers, since that is what they really are.

Perhaps nsDeviceMotionSystem should be renamed somehow. I am not sure how though. In any case, this and all other renaming things can be left for a followup I guess.

::: embedding/android/GeckoAppShell.java
@@ +581,5 @@
>  
> +    static Sensor gAccelerometerSensor = null;
> +    static Sensor gOrientationSensor = null;
> +
> +    public static void enableDeviceMotion(boolean enable) {

gAccelerometerSensor, gOrientationSensor - so perhaps call this function enableDeviceSensors?

@@ +589,5 @@
>  
> +        if (gAccelerometerSensor == null || gOrientationSensor == null) {
> +            gAccelerometerSensor = sm.getDefaultSensor(Sensor.TYPE_ACCELEROMETER);
> +            gOrientationSensor   = sm.getDefaultSensor(Sensor.TYPE_ORIENTATION);
> +        }

Long term, not sure we always want to listen to them both at once. Perhaps add a comment here.

Why refresh both if only one is null?

@@ +595,2 @@
>          if (enable) {
> +            sm.registerListener(GeckoApp.surfaceView, gAccelerometerSensor, SensorManager.SENSOR_DELAY_GAME);

need checks for null, like we had before

::: embedding/android/GeckoEvent.java
@@ +58,5 @@
>      public static final int INVALID = -1;
>      public static final int NATIVE_POKE = 0;
>      public static final int KEY_EVENT = 1;
>      public static final int MOTION_EVENT = 2;
> +    public static final int OREINTATION_EVENT = 3;

typo, OREI=>ORIE

@@ +149,5 @@
> +            mY = s.values[1] / SensorManager.GRAVITY_EARTH;
> +            mZ = s.values[2] / SensorManager.GRAVITY_EARTH;
> +        }
> +        else {
> +            mType = OREINTATION_EVENT;

typo in OREI

@@ +160,5 @@
> +                                             s.values[1] * s.values[1] + 
> +                                             s.values[2] * s.values[2]);
> +            mAlpha = 0; // This should be null; we do not have enough info to calculate it
> +            mBeta = Math.toDegrees(Math.asin(s.values[1] / magnitude));
> +            mGamma = -Math.toDegrees(Math.asin(s.values[0] / magnitude));

Comment about how we need to stop doing this approach?

Comment 11

[Doug Turner (:dougt)]

Alon and I discussed the naming throughout these changes.  Basically we needed a term that encompassed both orientation, rotation, and acceleration changes.  The least bad term I thought of was DeviceMotion.  Of course, it doesn't really apply to device orientation -- it is good enough.  Plus, this interface is gecko internal, so it probably doesn't matter in any case.


The question about mType -- mType is the kind of device motion.  See the nsIDeviceMotionData interface for the consts.


We also discussed why I removed mNewListener.  I do not think we want to filter anymore.  The Android API allows you to specify the frequency / accuracy.  I think we just want the change (if 

New patch coming up.

Comment 12

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 539099 [details] [diff] [review]
patch v.3


>--- /dev/null
>+++ b/content/events/src/nsDOMCompassNeedsCalibrationEvent.h

The whole compassneedscalibration interface is useless.
Please remove it.


>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.
Mozilla Foundation


>+
>+NS_IMPL_ADDREF_INHERITED(nsDOMDeviceMotionEvent, nsDOMEvent)
>+NS_IMPL_RELEASE_INHERITED(nsDOMDeviceMotionEvent, nsDOMEvent)
>+
>+DOMCI_DATA(DeviceMotionEvent, nsDOMDeviceMotionEvent)
>+
>+NS_INTERFACE_MAP_BEGIN(nsDOMDeviceMotionEvent)
>+NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMDeviceMotionEvent)
>+NS_INTERFACE_MAP_ENTRY(nsIDOMDeviceMotionEvent)
>+NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(DeviceMotionEvent)
>+NS_INTERFACE_MAP_END_INHERITING(nsDOMEvent)
2 space indentation after _BEING and before _END_



>+
>+NS_IMETHODIMP nsDOMDeviceMotionEvent::InitDeviceMotionEvent(const nsAString & aEventTypeArg,
NS_IMETHODIMP to its own line, please.
Same also elsewhere


>+                                                            PRBool aCanBubbleArg,
>+                                                            PRBool aCancelableArg,
>+                                                            nsIDOMDeviceAcceleration* aAcceleration,
>+                                                            nsIDOMDeviceAcceleration* aAccelerationIncludingGravity,
>+                                                            nsIDOMDeviceRotationRate* aRotationRate,
>+                                                            double aInterval)
>+{
>+  nsresult rv = nsDOMEvent::InitEvent(aEventTypeArg, aCanBubbleArg, aCancelableArg);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  mAcceleration = aAcceleration;
>+  mAccelerationIncludingGravity = aAccelerationIncludingGravity;
>+  mRotationRate = aRotationRate;
>+  mInterval = aInterval;
>+  return NS_OK;
>+}

mAcceleration
mAccelerationIncludingGravity
mRotationRateake
should be cycle collected.

Per spec the init*** method takes Acceleration, not DeviceAcceleration.
Please rename the interfaces.

>-	nsIDOMDeviceOrientationEvent.idl			\
>+	nsIDOMDeviceOrientationEvent.idl	    \
>+	nsIDOMDeviceMotionEvent.idl			    \
>+	nsIDOMCompassNeedsCalibrationEvent.idl	\
Something strange with \


I'd like to see a new patch.

Comment 14

[Doug Turner (:dougt)]

Attachment: patch v.4

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 539585 [details] [diff] [review]
patch v.4

>+NS_NewDOMDeviceMotionEvent(nsIDOMEvent** aInstancePtrResult,
>+                           nsPresContext* aPresContext,
>+                           nsEvent *aEvent) 
>+{
>+  NS_ENSURE_ARG_POINTER(aInstancePtrResult);
>+
>+  nsDOMDeviceMotionEvent* it = new nsDOMDeviceMotionEvent(aPresContext, aEvent);
>+  if (nsnull == it) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
No need for OOM check.

>+class nsDOMDeviceRotationRate : public nsIDOMDeviceRotationRate
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIDOMDEVICEROTATIONRATE
>+
>+    nsDOMDeviceRotationRate(double aAlpha, double aBeta, double aGamma);
too many spaces


>+class nsDOMDeviceAcceleration : public nsIDOMDeviceAcceleration
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIDOMDEVICEACCELERATION
>+
>+    nsDOMDeviceAcceleration(double aX, double aY, double aZ);
ditto


>-NS_IMETHODIMP nsDOMOrientationEvent::InitDeviceOrientationEvent(const nsAString & aEventTypeArg,
>+NS_IMETHODIMP nsDOMDeviceOrientationEvent::InitDeviceOrientationEvent(const nsAString & aEventTypeArg,
>                                                                 PRBool aCanBubbleArg,
>                                                                 PRBool aCancelableArg,
>                                                                 double aAlpha,
>                                                                 double aBeta,
>                                                                 double aGamma,
>                                                                 PRBool aAbsolute)
align parameters please.
NS_IMETHODIMP could be in its own line. Same thing also elsewhere.


>+++ b/content/events/test/Makefile.in
>@@ -99,16 +99,17 @@ _TEST_FILES = \
> 		test_bug624127.html \
> 		test_bug650493.html \
> 		test_bug641477.html \
> 		test_bug648573.html \
> 		test_bug615597.html \
> 		test_bug656379-1.html \
> 		test_bug656379-2.html \
> 		test_bug656954.html \
>+        test_bug662678.html \
You need to use tabs here

>   readonly attribute double alpha;
>   readonly attribute double beta;
>   readonly attribute double gamma;
>   readonly attribute boolean absolute;
>-  readonly attribute boolean compassCalibrated;
Please file a followup to implement double? attributes.
I don't think our idl can handle that syntax yet.


>+nsDeviceMotion::FireDOMOrientationEvent(nsIDOMDocument *domdoc,
>+                                         nsIDOMEventTarget *target,
>+                                         double alpha,
>+                                         double beta,
>+                                         double gamma) {
{ should be in the next line.
And the indentation of the parameters is strange.


>+  if (!oe)
>+    return;
if (expr) {
  stmt;
}

Comment 16

[Doug Turner (:dougt)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d48792c8de67

Comment 17

[Doug Turner (:dougt)]

for reference, the bug that will track optional/null attributes is bug 665409.

Comment 18

[Michael Ventnor]

Comment on attachment 539585 [details] [diff] [review]
patch v.4

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

::: dom/system/unix/nsAccelerometerSystem.cpp
@@ +194,2 @@
>  {
> +  // DeviceMotions in Linux are used by reading a file (yay UNIX!), which is

Did you just do a global find-replace of "Accelerometer" with "DeviceMotion"?

::: dom/system/windows/nsAccelerometerSystem.cpp
@@ +85,5 @@
>    if (!mLibrary)
>      return PR_FALSE;
>  
> +  gShockproofGetDeviceMotionData = (ShockproofGetDeviceMotionData)
> +    GetProcAddress(mLibrary, "ShockproofGetDeviceMotionData");

...because unless this function exists, it seems like this is now broken.

Comment 19

[Doug Turner (:dougt)]

:(  I did.  followup coming!

Comment 20

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 21

[Michael Ventnor]

Comment on attachment 540467 [details] [diff] [review]
patch v.1

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

::: dom/system/unix/nsDeviceMotionSystem.cpp
@@ +191,5 @@
>  }
>  
>  void nsDeviceMotionSystem::Startup()
>  {
> +  // Accelerometer in Linux are used by reading a file (yay UNIX!), which is

Accelerometers.

r=me otherwise.

Comment 22

[Doug Turner (:dougt)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/98334d656ac4

Comment 23

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 24

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/d48792c8de67

waiting on followup patch...

Comment 25

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/mozilla-central/rev/e8f3a7dd12b8

Comment 26

[Asa Dotzler [:asa]]

I assume this impacts all Firefox and not just mobile. What's the value of taking this into Aurora over waiting for the next uplift and what are the chances that something breaks here?

Comment 27

[Doug Turner (:dougt)]

Asa, correct - this is a platform change.

The value is that we'd have website compat with existing webkit implementations that support these device events.  I believe that iphone/ipad already support these events.

It also moves us from our somewhat broken implementation, to something that we want to support long term.

As for risk, I do not think that you will see anything.  I think any bustage will be from sites that used the old event and having migrated.

Comment 28

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/5f396320042f

Comment 29

[Makoto Kato [:m_kato]]

This is landed into Firefox 6, so target is mozilla6.

Comment 30

[Eric Shepherd [:sheppy]]

I've started documenting this and am going to start pasting in URLs. They're not done yet but I need to keep track of them for later review purposes, so here we go:

https://developer.mozilla.org/en/DOM/DeviceMotionEvent
https://developer.mozilla.org/en/DOM/DeviceMotionEvent.acceleration
https://developer.mozilla.org/en/DOM/DeviceMotionEvent.accelerationIncludingGravity
https://developer.mozilla.org/en/DOM/DeviceMotionEvent.interval
https://developer.mozilla.org/en/DOM/DeviceMotionEvent.rotationRate

https://developer.mozilla.org/en/DOM/RotationRate
https://developer.mozilla.org/en/DOM/RotationRate.alpha
https://developer.mozilla.org/en/DOM/RotationRate.beta
https://developer.mozilla.org/en/DOM/RotationRate.gamma

https://developer.mozilla.org/en/DOM/Orientation_and_motion_data_explained

I'm working on the last one still, and once it's done, changes will percolate upward through all those others with cross-referencing helping to fill in some of the holes in the content. Then I'll work on DeviceOrientation, and once that's done, I'm going to put together an example and that should let me finish it all up.

Comment 31

[Eric Shepherd [:sheppy]]

More documents started:

https://developer.mozilla.org/en/DOM/DeviceOrientationEvent
https://developer.mozilla.org/en/DOM/DeviceOrientationEvent.absolute
https://developer.mozilla.org/en/DOM/DeviceOrientationEvent.alpha
https://developer.mozilla.org/en/DOM/DeviceOrientationEvent.beta
https://developer.mozilla.org/en/DOM/DeviceOrientationEvent.gamma

More to come.

Comment 32

[Boris Zbarsky [:bzbarsky]]

We really need to document the devicemotion thing and link to it from https://developer.mozilla.org/en/detecting_device_orientation and the like)...