Bug 46344 - Update DeviceMotionEvent to spec
Summary: Update DeviceMotionEvent to spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dean Jackson
URL: http://dev.w3.org/geo/api/spec-source...
Keywords:
: 44569 (view as bug list)
Depends on: 46449
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-23 02:57 PDT by Hans Wennborg
Modified: 2010-09-24 08:34 PDT (History)
10 users (show)

See Also:


Attachments
Patch (52.97 KB, patch)
2010-09-23 04:21 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (52.70 KB, patch)
2010-09-23 12:46 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (52.70 KB, patch)
2010-09-23 13:13 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
updated patch (51.85 KB, patch)
2010-09-23 22:52 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (59.76 KB, patch)
2010-09-24 06:26 PDT, Hans Wennborg
steveblock: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2010-09-23 02:57:05 PDT
Update DeviceMotionEvent to spec
Comment 1 Hans Wennborg 2010-09-23 03:00:28 PDT
The spec (see URL) for DeviceMotionEvent has changed a bit since this was first implemented in Bug 42865. We should update the implementation accordingly.
Comment 2 Hans Wennborg 2010-09-23 04:21:45 PDT
Created attachment 68508 [details]
Patch
Comment 3 Dean Jackson 2010-09-23 10:14:23 PDT
*** Bug 44569 has been marked as a duplicate of this bug. ***
Comment 4 Dean Jackson 2010-09-23 10:18:15 PDT
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 5 Simon Fraser (smfr) 2010-09-23 12:08:36 PDT
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
Comment 6 Hans Wennborg 2010-09-23 12:46:11 PDT
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.
Comment 7 Hans Wennborg 2010-09-23 12:46:45 PDT
Created attachment 68567 [details]
Patch
Comment 8 Simon Fraser (smfr) 2010-09-23 12:57:41 PDT
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.
Comment 9 Hans Wennborg 2010-09-23 13:13:01 PDT
Created attachment 68576 [details]
Patch

Returning raw const pointers
Comment 10 WebKit Review Bot 2010-09-23 14:02:47 PDT
Attachment 68576 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4084068
Comment 11 WebKit Commit Bot 2010-09-23 14:31:57 PDT
Comment on attachment 68576 [details]
Patch

Clearing flags on attachment: 68576

Committed r68197: <http://trac.webkit.org/changeset/68197>
Comment 12 WebKit Commit Bot 2010-09-23 14:32:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 WebKit Review Bot 2010-09-23 14:45:43 PDT
http://trac.webkit.org/changeset/68197 might have broken Chromium Linux Release
Comment 14 Andrew Wilson 2010-09-23 15:04:36 PDT
Reverted this patch because it did not compile on chromium-linux (note the red in the cr-linux badge on the patch).
Comment 15 Dean Jackson 2010-09-23 22:52:42 PDT
Created attachment 68649 [details]
updated patch

Trying to compile on cr-linux
Comment 16 Dean Jackson 2010-09-23 23:06:40 PDT
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
Comment 17 WebKit Review Bot 2010-09-23 23:17:02 PDT
http://trac.webkit.org/changeset/68236 might have broken Chromium Linux Release
Comment 18 Steve Block 2010-09-24 02:51:08 PDT
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.
Comment 19 Hans Wennborg 2010-09-24 06:26:11 PDT
(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.
Comment 20 Hans Wennborg 2010-09-24 06:26:47 PDT
Created attachment 68669 [details]
Patch
Comment 21 Steve Block 2010-09-24 07:26:05 PDT
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
Comment 22 Hans Wennborg 2010-09-24 07:38:34 PDT
Committed r68252: <http://trac.webkit.org/changeset/68252>
Comment 23 anton muhin 2010-09-24 07:45:10 PDT
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
Comment 24 Hans Wennborg 2010-09-24 08:34:55 PDT
(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.