Bug 41607 – DeviceOrientationEvent should use optional properties

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

The DeviceOrientation spec (http://dev.w3.org/geo/api/spec-source-orientation.html) requires the alpha, beta and gamma properties of DeviceOrientationEvent to all be optional.

[Steve Block]

Created attachment 60754 [details]
Patch

[Eric Seidel (no email)]

Attachment 60754 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3373471

[Steve Block]

Created attachment 60756 [details]
Patch

[Andreas Kling]

Comment on attachment 60756 [details]
Patch

>WebCore/dom/DeviceOrientation.h:66
> +      bool m_canProvideAlpha;
> +      double m_alpha;
> +      bool m_canProvideBeta;
> +      double m_beta;
> +      bool m_canProvideGamma;
> +      double m_gamma;
This member order will cause unnecessary padding. Better to group the doubles and bools.

Patch looks good otherwise.

[Steve Block]

Created attachment 61208 [details]
Patch

[Jeremy Orlow]

We should avoid adding new custom functions whenever practical.  It seems like it might be a bit better to add some new WebCore type that represents a double that could be null.  Or have some annotation in the IDL that says to pass a bool/enum ptr to the function and use that to pass back that the result should be null.

I'm not completely against the patch as is (hence no r-).  Adding japhet to see what he thinks.

[Steve Block]

> We should avoid adding new custom functions whenever practical.  It seems like
> it might be a bit better to add some new WebCore type that represents a double
> that could be null.  Or have some annotation in the IDL that says to pass a
> bool/enum ptr to the function and use that to pass back that the result should
> be null.
Sounds reasonable. I'm totally happy to look into this for a future patch, but it sounds like it might take some time tor reach consensus for the new type/flag. It would be good to land this patch as-is now, as it's blocking progress on DeviceOrientation.

[Jeremy Orlow]

Comment on attachment 61208 [details]
Patch


LayoutTests/fast/dom/DeviceOrientation/script-tests/optional-event-properties.js:39
 +  event.initDeviceOrientationEvent("", false, false, null, null, null);
if you used evalAndLog, then this would be printed to the console too, so it'd be easier to read without much additional effort.

WebCore/dom/DeviceOrientation.h:53
 +      DeviceOrientation()
There's enough code here I'd lean towards putting it in a .cpp file.

WebCore/bindings/js/JSDeviceOrientationEventCustom.cpp:2
 +   * Copyright 2010, The Android Open Source Project
Since these contributions are for sure from Google, should the copyright just read Google?

[Steve Block]

> LayoutTests/fast/dom/DeviceOrientation/script-tests/optional-event-properties.js:39
>  +  event.initDeviceOrientationEvent("", false, false, null, null, null);
> if you used evalAndLog, then this would be printed to the console too, so it'd be easier to read without much additional effort.
OK, will do

> WebCore/dom/DeviceOrientation.h:53
>  +      DeviceOrientation()
> There's enough code here I'd lean towards putting it in a .cpp file.
OK

> WebCore/bindings/js/JSDeviceOrientationEventCustom.cpp:2
>  +   * Copyright 2010, The Android Open Source Project
> Since these contributions are for sure from Google, should the copyright just read Google?
All of the other DeviceOrientation patches are copyright Android, so I think it's best to stick with this.

[Jeremy Orlow]

(In reply to comment #9)
> > LayoutTests/fast/dom/DeviceOrientation/script-tests/optional-event-properties.js:39
> >  +  event.initDeviceOrientationEvent("", false, false, null, null, null);
> > if you used evalAndLog, then this would be printed to the console too, so it'd be easier to read without much additional effort.
> OK, will do
> 
> > WebCore/dom/DeviceOrientation.h:53
> >  +      DeviceOrientation()
> > There's enough code here I'd lean towards putting it in a .cpp file.
> OK
> 
> > WebCore/bindings/js/JSDeviceOrientationEventCustom.cpp:2
> >  +   * Copyright 2010, The Android Open Source Project
> > Since these contributions are for sure from Google, should the copyright just read Google?
> All of the other DeviceOrientation patches are copyright Android, so I think it's best to stick with this.

Theres no reason the copyright needs to match.  There also is no strong reason why it needs to be Google rather than the android project beyond the former being closer to WebKit's conventions and the typical reasons for having the latter don't apply in this case (unless there's copied code...?).

[Steve Block]

Created attachment 61226 [details]
Patch

[Jeremy Orlow]

Comment on attachment 61226 [details]
Patch

r=me

[Steve Block]

Created attachment 61283 [details]
Patch

[Steve Block]

Created attachment 61287 [details]
Patch

[Steve Block]

Created attachment 61345 [details]
Patch

[Jeremy Orlow]

Comment on attachment 61345 [details]
Patch

rubber stamping because I already r+'ed and the diff hasn't changed much

in the future, no need to re-upload for minor changes

[Steve Block]

> rubber stamping because I already r+'ed and the diff hasn't changed much
Thanks

> in the future, no need to re-upload for minor changes
I know. I was uploading to use the try-bot to make sure I get the correct incantations for the Windows build. Hopefully this one will work!

[Steve Block]

The Windows EWS bot seems to have given up on my patch. It builds on my local machine, so will land manually and watch the bots.

[Steve Block]

Committed r63193: <http://trac.webkit.org/changeset/63193>

[WebKit Review Bot]

http://trac.webkit.org/changeset/63193 might have broken GTK Linux 64-bit Debug

[Steve Block]

Fixed with http://trac.webkit.org/changeset/63195