Closed
Bug 662678
Opened 13 years ago
Closed 13 years ago
w3c device orientation spec changes
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
mozilla6
Tracking | Status | |
---|---|---|
firefox6 | --- | fixed |
People
(Reporter: dougt, Assigned: dougt)
References
Details
(Keywords: dev-doc-needed)
Attachments
(2 files, 5 obsolete files)
114.40 KB,
patch
|
smaug
:
review+
asa
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
5.63 KB,
patch
|
dougt
:
review+
asa
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee: nobody → doug.turner
Assignee | ||
Updated•13 years ago
|
Attachment #537903 -
Flags: superreview?(Olli.Pettay)
Attachment #537903 -
Flags: review?(azakai)
Comment 2•13 years ago
|
||
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?
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Updated•13 years ago
|
Attachment #537903 -
Flags: superreview?(Olli.Pettay)
Attachment #537903 -
Flags: review?(azakai)
Comment 4•13 years ago
|
||
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•13 years ago
|
||
Here is a patch that uses the orientation data from Android, instead of inferring it from accelerometer data.
Attachment #538126 -
Flags: review?(doug.turner)
Assignee | ||
Comment 6•13 years ago
|
||
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!)
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #537903 -
Attachment is obsolete: true
Attachment #538126 -
Attachment is obsolete: true
Attachment #538126 -
Flags: review?(doug.turner)
Assignee | ||
Updated•13 years ago
|
Attachment #538608 -
Flags: review?(azakai)
Assignee | ||
Updated•13 years ago
|
Attachment #538608 -
Flags: superreview?(Olli.Pettay)
Comment 8•13 years ago
|
||
if onfoo listeners should be supported, just look at nsDOMClassInfo for things like onclick.
Assignee | ||
Comment 9•13 years ago
|
||
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•13 years ago
|
||
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 <dougt@dougt.org> > + * 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?
Attachment #538608 -
Flags: review?(azakai) → review+
Assignee | ||
Comment 11•13 years ago
|
||
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.
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #538608 -
Attachment is obsolete: true
Attachment #538608 -
Flags: superreview?(Olli.Pettay)
Attachment #539099 -
Flags: review?(Olli.Pettay)
Comment 13•13 years ago
|
||
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.
Attachment #539099 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #539099 -
Attachment is obsolete: true
Attachment #539585 -
Flags: review?(Olli.Pettay)
Comment 15•13 years ago
|
||
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; }
Attachment #539585 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 16•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/d48792c8de67
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•13 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 17•13 years ago
|
||
for reference, the bug that will track optional/null attributes is bug 665409.
Comment 18•13 years ago
|
||
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.
Assignee | ||
Comment 19•13 years ago
|
||
:( I did. followup coming!
Assignee | ||
Comment 20•13 years ago
|
||
Attachment #540467 -
Flags: review?(ventnor.bugzilla)
Comment 21•13 years ago
|
||
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.
Attachment #540467 -
Flags: review?(ventnor.bugzilla) → review+
Assignee | ||
Comment 22•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/98334d656ac4
Assignee | ||
Comment 23•13 years ago
|
||
Attachment #540467 -
Attachment is obsolete: true
Attachment #540512 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Attachment #539585 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•13 years ago
|
Attachment #540512 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 24•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d48792c8de67 waiting on followup patch...
Comment 25•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e8f3a7dd12b8
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Version: unspecified → Trunk
Comment 26•13 years ago
|
||
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?
Assignee | ||
Comment 27•13 years ago
|
||
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.
Updated•13 years ago
|
Attachment #539585 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•13 years ago
|
Attachment #540512 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 28•13 years ago
|
||
http://hg.mozilla.org/releases/mozilla-aurora/rev/5f396320042f
status-firefox6:
--- → fixed
Assignee | ||
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 29•13 years ago
|
||
This is landed into Firefox 6, so target is mozilla6.
Target Milestone: mozilla7 → mozilla6
Comment 30•13 years ago
|
||
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•13 years ago
|
||
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•12 years ago
|
||
We really need to document the devicemotion thing and link to it from https://developer.mozilla.org/en/detecting_device_orientation and the like)...
You need to log in
before you can comment on or make changes to this bug.
Description
•