Bug 39210 – Provide bindings for DeviceOrientation

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

[Steve Block]

Provide bindings for DeviceOrientation

[Steve Block]

Created attachment 56228 [details]
Patch

[Steve Block]

Created attachment 56237 [details]
Patch

[WebKit Review Bot]

Attachment 56237 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2294223

[Steve Block]

The Chromium and qt build failures seem to be due to the V8 code generator failing to guard some code with ENABLE(DEVICE_ORIENTATION), which causes problems when the feature is disabled.

Have filed Bug 39222 to track this.

[Hans Wennborg]

Adding myself to the CC list.

[Steve Block]

Created attachment 56480 [details]
Patch

[Steve Block]

Re-uploaded patch now that Bug 39222 is fixed.

Note that the large size of this patch is due mainly to boilerplate and LayoutTests.

[Steve Block]

Created attachment 56492 [details]
Patch

[Jeremy Orlow]

Comment on attachment 56492 [details]
Patch

Looking pretty good.


WebCore/Android.derived.v8bindings.mk:34
 +  FEATURE_DEFINES += V8_BINDING
Did you mean for this to be here?

WebCore/config.h:109
 +  #define ENABLE_DEVICE_ORIENTATION 1
Is this supposed to be here?

WebCore/dom/DeviceOrientationEvent.h:46
 +      double alpha() { return m_alpha; }
) const {

WebCore/dom/DeviceOrientationEvent.h:47
 +      double beta() { return m_beta; }
ditto

WebCore/dom/DeviceOrientationEvent.h:48
 +      double gamma() { return m_gamma; }
ditto

LayoutTests/platform/gtk/Skipped:1182
 +  fast/dom/DeviceOrientation/window-property.html
Why not the whole directory?

LayoutTests/platform/chromium/test_expectations.txt:2843
 +  DEFER SKIP : fast/dom/DeviceOrientation = TEXT
What's defer mean?

I made a bug for this.  s/DEFER/BUG44654/

WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops:57
 +  		PerformEnvironmentSet="true"
Is this supposed to be true?

WebKitLibraries/win/tools/vsprops/FeatureDefines.vsprops:57
 +  		PerformEnvironmentSet="true"
Is this supposed to be true?

LayoutTests/fast/dom/DeviceOrientation/script-tests/window-property.js:5
 +      for (var property in window) {
Why not just 'if ('ondeviceorientation' in window)' ?

LayoutTests/fast/dom/DeviceOrientation/script-tests/window-property.js:17
 +  window.jsTestIsAsync = false;
is this necessary?

[Steve Block]

> WebCore/Android.derived.v8bindings.mk:34
>  +  FEATURE_DEFINES += V8_BINDING
> Did you mean for this to be here?
Yes - I split this out onto its own line because it's not really a feature like the others are.

> WebCore/config.h:109
>  +  #define ENABLE_DEVICE_ORIENTATION 1
It doesn't really matter - removed for now.

> WebCore/dom/DeviceOrientationEvent.h:46
>  +      double alpha() { return m_alpha; }
> ) const {
Done

> LayoutTests/platform/gtk/Skipped:1182
>  +  fast/dom/DeviceOrientation/window-property.html
> Why not the whole directory?
That's what the comments at the top of this file require - 'no grouping should occur. Every test for itself'

> LayoutTests/platform/chromium/test_expectations.txt:2843
>  +  DEFER SKIP : fast/dom/DeviceOrientation = TEXT
> What's defer mean?
From the comments as the top of the file - 'DEFER: For tests we'll fix in the future.'

> I made a bug for this.  s/DEFER/BUG44654/
Done

> WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops:57
>  +          PerformEnvironmentSet="true"
> Is this supposed to be true?
All of the flags in this file seem to set this.

> WebKitLibraries/win/tools/vsprops/FeatureDefines.vsprops:57
>  +          PerformEnvironmentSet="true"
> Is this supposed to be true?
Ditto

> LayoutTests/fast/dom/DeviceOrientation/script-tests/window-property.js:5
>  +      for (var property in window) {
> Why not just 'if ('ondeviceorientation' in window)' ?
We already test this on line 14. The difference is that properties which are marked 'DontEnum' will pass 'if (foo in bar)' but won't be enumerated with 'for (var x in bar)'. This test is copied from fast/dom/Geolocation/resources/enabled.js

> LayoutTests/fast/dom/DeviceOrientation/script-tests/window-property.js:17
>  +  window.jsTestIsAsync = false;
> is this necessary?
No, fixed.

[Steve Block]

Created attachment 56602 [details]
Patch

[Jeremy Orlow]

Comment on attachment 56602 [details]
Patch

r=me

[Steve Block]

Comment on attachment 56602 [details]
Patch

Clearing flags on attachment: 56602

Committed r59847: <http://trac.webkit.org/changeset/59847>

[Steve Block]

All reviewed patches have been landed.  Closing bug.

[Olli Pettay (:smaug)]

Why did you end up adding non-standard ondeviceorientation property
to window object? (and even without webkit prefix)

[Andrei Popescu]

(In reply to comment #15)
> Why did you end up adding non-standard ondeviceorientation property
> to window object? (and even without webkit prefix)

Hi Olli

Device Orientation is a specification published by the Geolocation WG at the W3C:

http://dev.w3.org/geo/api/spec-source-orientation.html

We've started the process to publish the FPWD last week. Doug Turner is Mozilla's representative in the Geolocation WG.

Thanks,
Andrei