Bug 39210

Summary: Provide bindings for DeviceOrientation
Product: WebKit Reporter: Steve Block <steveblock>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andreip, android-webkit-unforking, bolsinga, bugs, ddkilzer, dglazkov, eric, hans, jorlow, steveblock, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 39222    
Bug Blocks: 30335    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Steve Block 2010-05-17 02:07:16 PDT
Provide bindings for DeviceOrientation
Comment 1 Steve Block 2010-05-17 02:42:04 PDT
Created attachment 56228 [details]
Patch
Comment 2 Steve Block 2010-05-17 06:27:55 PDT
Created attachment 56237 [details]
Patch
Comment 3 WebKit Review Bot 2010-05-17 07:12:49 PDT
Attachment 56237 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2294223
Comment 4 Steve Block 2010-05-17 09:32:25 PDT
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.
Comment 5 Hans Wennborg 2010-05-18 04:40:38 PDT
Adding myself to the CC list.
Comment 6 Steve Block 2010-05-19 04:30:01 PDT
Created attachment 56480 [details]
Patch
Comment 7 Steve Block 2010-05-19 04:31:39 PDT
Re-uploaded patch now that Bug 39222 is fixed.

Note that the large size of this patch is due mainly to boilerplate and LayoutTests.
Comment 8 Steve Block 2010-05-19 08:30:38 PDT
Created attachment 56492 [details]
Patch
Comment 9 Jeremy Orlow 2010-05-20 09:03:35 PDT
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?
Comment 10 Steve Block 2010-05-20 09:39:25 PDT
> 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.
Comment 11 Steve Block 2010-05-20 09:43:26 PDT
Created attachment 56602 [details]
Patch
Comment 12 Jeremy Orlow 2010-05-20 10:30:21 PDT
Comment on attachment 56602 [details]
Patch

r=me
Comment 13 Steve Block 2010-05-20 10:37:51 PDT
Comment on attachment 56602 [details]
Patch

Clearing flags on attachment: 56602

Committed r59847: <http://trac.webkit.org/changeset/59847>
Comment 14 Steve Block 2010-05-20 10:38:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Olli Pettay (:smaug) 2011-06-10 15:17:06 PDT
Why did you end up adding non-standard ondeviceorientation property
to window object? (and even without webkit prefix)
Comment 16 Andrei Popescu 2011-06-13 02:05:25 PDT
(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