Update DeviceMotionEvent to spec
The spec (see URL) for DeviceMotionEvent has changed a bit since this was first implemented in Bug 42865. We should update the implementation accordingly.
Created attachment 68508 [details] Patch
*** Bug 44569 has been marked as a duplicate of this bug. ***
The specification defines the Acceleration and RotationRate interfaces in the IDL. I wonder if we should do that as well? Unfortunately, this would mean adding new files which is always annoying. Especially in this case where they are simple structures.
Comment on attachment 68508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68508&action=review > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:38 > +namespace { Why not just make these static functions? > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:40 > +RefPtr<DeviceMotionData::Acceleration> readAccelerationArgument(JSValue value, ExecState* exec) This should return a PassRefPtr > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:52 > + double x = static_cast<double>(xValue.toNumber(exec)); alphaValue.toNumber() returns a double. No need to do any casting here. > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:78 > +RefPtr<DeviceMotionData::RotationRate> readRotationRateArgument(JSValue value, ExecState* exec) This should return a PassRefPtr > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:119 > + object->putDirect(Identifier(exec, stringToUString("x")), acceleration->canProvideX() ? jsNumber(exec, acceleration->x()) : jsNull()); No need for stringToUString; see Identifier(ExecState* exec, const char* s) > WebCore/dom/DeviceMotionData.cpp:85 > + : m_acceleration() > + , m_accelerationIncludingGravity() > + , m_rotationRate() No need to initialize these. > WebCore/dom/DeviceMotionData.h:62 > + bool m_canProvideX; > + bool m_canProvideY; > + bool m_canProvideZ; > + > + double m_x; > + double m_y; > + double m_z; Put the bools after the doubles to minimize padding. > WebCore/dom/DeviceMotionData.h:90 > + bool m_canProvideAlpha; > + bool m_canProvideBeta; > + bool m_canProvideGamma; > + > + double m_alpha; > + double m_beta; > + double m_gamma; Ditto
Thanks for the review. (In reply to comment #5) > (From update of attachment 68508 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68508&action=review > > > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:38 > > +namespace { > > Why not just make these static functions? Done. > > > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:40 > > +RefPtr<DeviceMotionData::Acceleration> readAccelerationArgument(JSValue value, ExecState* exec) > > This should return a PassRefPtr Oops. Done. > > > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:52 > > + double x = static_cast<double>(xValue.toNumber(exec)); > > alphaValue.toNumber() returns a double. No need to do any casting here. Done. > > > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:78 > > +RefPtr<DeviceMotionData::RotationRate> readRotationRateArgument(JSValue value, ExecState* exec) > > This should return a PassRefPtr Done. > > > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:119 > > + object->putDirect(Identifier(exec, stringToUString("x")), acceleration->canProvideX() ? jsNumber(exec, acceleration->x()) : jsNull()); > > No need for stringToUString; see Identifier(ExecState* exec, const char* s) Ah, thanks. Done. > > > WebCore/dom/DeviceMotionData.cpp:85 > > + : m_acceleration() > > + , m_accelerationIncludingGravity() > > + , m_rotationRate() > > No need to initialize these. Done. > > > WebCore/dom/DeviceMotionData.h:62 > > + bool m_canProvideX; > > + bool m_canProvideY; > > + bool m_canProvideZ; > > + > > + double m_x; > > + double m_y; > > + double m_z; > > Put the bools after the doubles to minimize padding. Done. > > > WebCore/dom/DeviceMotionData.h:90 > > + bool m_canProvideAlpha; > > + bool m_canProvideBeta; > > + bool m_canProvideGamma; > > + > > + double m_alpha; > > + double m_beta; > > + double m_gamma; > > Ditto Done.
Created attachment 68567 [details] Patch
Comment on attachment 68567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68567&action=review > WebCore/dom/DeviceMotionData.h:101 > + PassRefPtr<Acceleration> acceleration() const { return m_acceleration; } > + PassRefPtr<Acceleration> accelerationIncludingGravity() const { return m_accelerationIncludingGravity; } > + PassRefPtr<RotationRate> rotationRate() const { return m_rotationRate; } These should not return PassRefPtr<Acceleration>; that implies transfer of ownership. You should return raw const pointers here. Sorry i missed this last time.
Created attachment 68576 [details] Patch Returning raw const pointers
Attachment 68576 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4084068
Comment on attachment 68576 [details] Patch Clearing flags on attachment: 68576 Committed r68197: <http://trac.webkit.org/changeset/68197>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/68197 might have broken Chromium Linux Release
Reverted this patch because it did not compile on chromium-linux (note the red in the cr-linux badge on the patch).
Created attachment 68649 [details] updated patch Trying to compile on cr-linux
build bots are slow so I'm committing and crossing my fingers Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog M LayoutTests/fast/dom/DeviceMotion/create-event-expected.txt M LayoutTests/fast/dom/DeviceMotion/optional-event-properties-expected.txt M LayoutTests/fast/dom/DeviceMotion/script-tests/create-event.js M LayoutTests/fast/dom/DeviceMotion/script-tests/optional-event-properties.js M LayoutTests/fast/dom/script-tests/prototype-inheritance.js M WebCore/ChangeLog M WebCore/WebCore.exp.in M WebCore/bindings/js/JSDeviceMotionEventCustom.cpp M WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp M WebCore/dom/DeviceMotionData.cpp M WebCore/dom/DeviceMotionData.h M WebCore/dom/DeviceMotionEvent.idl Committed r68236
http://trac.webkit.org/changeset/68236 might have broken Chromium Linux Release
Comment on attachment 68649 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=68649&action=review Sorry for the late review! > LayoutTests/fast/dom/DeviceMotion/create-event-expected.txt:-30 > - Is deleting these trailing lines intentional? I'm intrigued as to why this has changed? > LayoutTests/fast/dom/script-tests/prototype-inheritance.js:26 > + "DeviceMotionEvent", Can you explain why this is now required? How was the test passing before? If it's unrelated to the rest of the patch, it might warrant a comment in the ChangeLog or even a separate patch. > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:47 > + if (exec->hadException()) I think we should be testing that when the JS getter for the these properties throw, we still throw the exception to script. See the Geolocation test fast/dom/Geolocation/script-tests/argument-types.js. This can be a separate patch if you like. > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:49 > + bool canProvideX = !xValue.isUndefined(); I think this should be isNullOrUndefined(). It would be good to add test cases for acceleration.x etc being null or undefined, as we do with acceleration itself. > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:119 > + object->putDirect(Identifier(exec, "z"), acceleration->canProvideZ() ? jsNumber(exec, acceleration->z()) : jsNull()); I think this is the right choice. We could use an Acceleration.idl to do this, but we'd end up having to write much of this by hand anyway to provide custom setters to handle the 'may be null' semantics. I think there was talk of adding a 'may be null' type to IDL, which would simplify all this. > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:169 > + // Spare comment symbol > WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:168 > + intervalProvided, interval); There's no hard maximum line length, so don't break lines unless they get really long. Breaking into groups as you've done elsewhere makes sense in that case.
(In reply to comment #18) > (From update of attachment 68649 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68649&action=review > > Sorry for the late review! > > > LayoutTests/fast/dom/DeviceMotion/create-event-expected.txt:-30 > > - > > Is deleting these trailing lines intentional? I'm intrigued as to why this has changed? I just copied the results I got from DRT (on Linux) into the expectations files. Putting the trailing lines back in, they seem to be there on the other expectations files. > > > LayoutTests/fast/dom/script-tests/prototype-inheritance.js:26 > > + "DeviceMotionEvent", > > Can you explain why this is now required? How was the test passing before? If it's unrelated to the rest of the patch, it might warrant a comment in the ChangeLog or even a separate patch. As long as ENABLE(DEVICE_ORIENTATION) is false (or the runtime flag is off), this test is unaffected. I added this because I think the layout tests should pass for those that enable the flag. Extending the comment in the ChangeLog. If you think it's too early, I'm happy to punt it to a later patch. > > > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:47 > > + if (exec->hadException()) > > I think we should be testing that when the JS getter for the these properties throw, we still throw the exception to script. See the Geolocation test fast/dom/Geolocation/script-tests/argument-types.js. This can be a separate patch if you like. Thanks for pointing this out; the V8 code was failing at this. Tests added. > > > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:49 > > + bool canProvideX = !xValue.isUndefined(); > > I think this should be isNullOrUndefined(). It would be good to add test cases for acceleration.x etc being null or undefined, as we do with acceleration itself. Done. > > > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:119 > > + object->putDirect(Identifier(exec, "z"), acceleration->canProvideZ() ? jsNumber(exec, acceleration->z()) : jsNull()); > > I think this is the right choice. We could use an Acceleration.idl to do this, but we'd end up having to write much of this by hand anyway to provide custom setters to handle the 'may be null' semantics. I think there was talk of adding a 'may be null' type to IDL, which would simplify all this. > > > WebCore/bindings/js/JSDeviceMotionEventCustom.cpp:169 > > + // > > Spare comment symbol Done. > > > WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:168 > > + intervalProvided, interval); > > There's no hard maximum line length, so don't break lines unless they get really long. Breaking into groups as you've done elsewhere makes sense in that case. Done. Joined some lines.
Created attachment 68669 [details] Patch
Comment on attachment 68669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68669&action=review > WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:114 > + return 0; 4 space indent
Committed r68252: <http://trac.webkit.org/changeset/68252>
Comment on attachment 68669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68669&action=review minor drive by nits > WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:72 > + double x = static_cast<double>(xValue->NumberValue()); nit: apparently you don't need static_cast<double> here > WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:78 > + double y = static_cast<double>(yValue->NumberValue()); ditto
(In reply to comment #23) > (From update of attachment 68669 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68669&action=review > > minor drive by nits > > > WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:72 > > + double x = static_cast<double>(xValue->NumberValue()); > > nit: apparently you don't need static_cast<double> here > > > WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:78 > > + double y = static_cast<double>(yValue->NumberValue()); > > ditto Thanks for pointing this out. Fixing in Bug 46466.