Bug 41607

Created attachment 60754 [details] Patch

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

Created attachment 60756 [details] Patch

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.

Created attachment 61208 [details] Patch

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.

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

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?

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

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

Created attachment 61226 [details] Patch

Comment on attachment 61226 [details] Patch r=me

Created attachment 61283 [details] Patch

Created attachment 61287 [details] Patch

Created attachment 61345 [details] Patch

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

> 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!

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.

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

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

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