Summary: | Provide bindings for DeviceOrientation | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Steve Block <steveblock> | ||||||||||||
Component: | New Bugs | Assignee: | 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
Steve Block
2010-05-17 02:07:16 PDT
Created attachment 56228 [details]
Patch
Created attachment 56237 [details]
Patch
Attachment 56237 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/2294223 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. Adding myself to the CC list. Created attachment 56480 [details]
Patch
Re-uploaded patch now that Bug 39222 is fixed. Note that the large size of this patch is due mainly to boilerplate and LayoutTests. Created attachment 56492 [details]
Patch
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? > 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. Created attachment 56602 [details]
Patch
Comment on attachment 56602 [details]
Patch
r=me
Comment on attachment 56602 [details] Patch Clearing flags on attachment: 56602 Committed r59847: <http://trac.webkit.org/changeset/59847> All reviewed patches have been landed. Closing bug. Why did you end up adding non-standard ondeviceorientation property to window object? (and even without webkit prefix) (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 |