Bug 46344 – Update DeviceMotionEvent to spec

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

Update DeviceMotionEvent to spec

[Hans Wennborg]

The spec (see URL) for DeviceMotionEvent has changed a bit since this was first implemented in Bug 42865. We should update the implementation accordingly.

[Hans Wennborg]

Created attachment 68508 [details]
Patch

[Dean Jackson]

*** Bug 44569 has been marked as a duplicate of this bug. ***

[Dean Jackson]

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.

[Simon Fraser (smfr)]

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

[Hans Wennborg]

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.

[Hans Wennborg]

Created attachment 68567 [details]
Patch

[Simon Fraser (smfr)]

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.

[Hans Wennborg]

Created attachment 68576 [details]
Patch

Returning raw const pointers

[WebKit Review Bot]

Attachment 68576 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4084068

[WebKit Commit Bot]

Comment on attachment 68576 [details]
Patch

Clearing flags on attachment: 68576

Committed r68197: <http://trac.webkit.org/changeset/68197>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/68197 might have broken Chromium Linux Release

[Andrew Wilson]

Reverted this patch because it did not compile on chromium-linux (note the red in the cr-linux badge on the patch).

[Dean Jackson]

Created attachment 68649 [details]
updated patch

Trying to compile on cr-linux

[Dean Jackson]

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

[WebKit Review Bot]

http://trac.webkit.org/changeset/68236 might have broken Chromium Linux Release

[Steve Block]

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.

[Hans Wennborg]

(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.

[Hans Wennborg]

Created attachment 68669 [details]
Patch

[Steve Block]

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

[Hans Wennborg]

Committed r68252: <http://trac.webkit.org/changeset/68252>

[anton muhin]

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

[Hans Wennborg]

(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.