Implement AccelerometerEvents, as defined by the linked spec.
Created attachment 62435 [details] Patch I won't be surprised when this fails to build on some platforms, since the Page constructor has been changed.
Referencing bug that will help with Page constructor
Comment on attachment 62435 [details] Patch LayoutTests/fast/dom/Window/window-properties-device-orientation.html:1 + <p>This test dumps all of the properties that are reachable from the window.DeviceOrientationEvent, window.ondeviceorientation, window.AccelerometerEvent and window.onaccelerometer objects, along with their types.</p> Might be worth adding a note that these are all guarded by the same enable flag, so are tested together. LayoutTests/ChangeLog:14 + * fast/dom/Window/window-properties.html: I think you should also add the Accelerometer equivalent of the tests in fast/dom/DeviceOrientation too. WebCore/WebCore.exp.in:522 + __ZN7WebCore4PageC1EPNS_12ChromeClientEPNS_17ContextMenuClientEPNS_12EditorClientEPNS_10DragClientEPNS_15InspectorClientEPNS_18PluginHalterClientEPNS_27GeolocationControllerClientEPNS_23DeviceOrientationClientEPNS_19AccelerometerClientEPNS_27BackForwardControllerClientE It would be good to wait until my patch to add the OptionalClient parameter lands, to avoid this extra churn. WebCore/Android.v8bindings.mk:88 + bindings/v8/custom/V8AccelerometerEventCustom.cpp \ I don't see this file in the patch? WebCore/dom/AccelerometerController.h:41 + AccelerometerController(Page*, AccelerometerClient*); What is the Page needed for? I don't think DeviceOrientationController needs the Page either. WebCore/dom/Event.h:144 + virtual bool isAccelerometerEvent() const; There are lots of places where Acceleromter and DeviceOrientation appear in a pair. It would be good to be consistent with ordering. WebCore/page/DOMWindow.cpp:1436 + frame()->page()->deviceOrientationController()->removeListener(this); Thanks! WebCore/dom/AccelerometerClient.h:40 + virtual ~AccelerometerClient() {} I don't think this should be protected, as it's likely that we'll need to hold references to the client via this interface in order to implement mock injection for testing.
Created attachment 62637 [details] updated patch with new testcases
(In reply to comment #3) > (From update of attachment 62435 [details]) > LayoutTests/fast/dom/Window/window-properties-device-orientation.html:1 > + <p>This test dumps all of the properties that are reachable from the window.DeviceOrientationEvent, window.ondeviceorientation, window.AccelerometerEvent and window.onaccelerometer objects, along with their types.</p> > Might be worth adding a note that these are all guarded by the same enable flag, so are tested together. Done > > LayoutTests/ChangeLog:14 > + * fast/dom/Window/window-properties.html: > I think you should also add the Accelerometer equivalent of the tests in fast/dom/DeviceOrientation too. Done. By the way, I think there are errors in the DeviceOrientation ones. Specifically, the existence of the DeviceOrientationEvent property on window - it's a link to the constructor object, not a function. You can see the differences in the ones I've attached for Accelerometer. > > WebCore/WebCore.exp.in:522 > + __ZN7WebCore4PageC1EPNS_12ChromeClientEPNS_17ContextMenuClientEPNS_12EditorClientEPNS_10DragClientEPNS_15InspectorClientEPNS_18PluginHalterClientEPNS_27GeolocationControllerClientEPNS_23DeviceOrientationClientEPNS_19AccelerometerClientEPNS_27BackForwardControllerClientE > It would be good to wait until my patch to add the OptionalClient parameter lands, to avoid this extra churn. Yes, I will wait. > > WebCore/Android.v8bindings.mk:88 > + bindings/v8/custom/V8AccelerometerEventCustom.cpp \ > I don't see this file in the patch? Sorry, I forgot. I've added one now but have not compiled it to test. > > WebCore/dom/AccelerometerController.h:41 > + AccelerometerController(Page*, AccelerometerClient*); > What is the Page needed for? I don't think DeviceOrientationController needs the Page either. Removed. I'll leave it to you to remove it from DeviceOrientation. > > WebCore/dom/Event.h:144 > + virtual bool isAccelerometerEvent() const; > There are lots of places where Acceleromter and DeviceOrientation appear in a pair. It would be good to be consistent with ordering. Fixed in a lot of places. I use alpha order. > > WebCore/page/DOMWindow.cpp:1436 > + frame()->page()->deviceOrientationController()->removeListener(this); > Thanks! > > WebCore/dom/AccelerometerClient.h:40 > + virtual ~AccelerometerClient() {} > I don't think this should be protected, as it's likely that we'll need to hold references to the client via this interface in order to implement mock injection for testing. Fixed.
> By the way, I think there are errors in the DeviceOrientation ones. > Specifically, the existence of the DeviceOrientationEvent property on window > - it's a link to the constructor object, not a function. You can see the > differences in the ones I've attached for Accelerometer. Ah yes, the difference is due to the use of JSC vs V8. I guess that JSC should 'win' here for the generic expected result, and we should add V8-expected expected results for Chromium and Android. I've filed Bug 43025.
The patch still has an extra "class Page;" in AccelerometerController.h that I'll remove.
Created attachment 63296 [details] Updated patch after the Page constructor changes
Comment on attachment 63296 [details] Updated patch after the Page constructor changes LayoutTests/fast/dom/Window/window-properties-device-orientation.html:2 + These properties are currently guarded by ENABLE_DEVICE_ORIENTATION.</p> Looks like this last sentence isn't in the expected result. LayoutTests/platform/gtk/Skipped:1096 + fast/dom/Accelerometer/window-property.html alpha ordering WebCore/WebCore.gypi:525 + 'bindings/js/JSAccelerometerEventCustom.cpp', alpha ordering WebCore/WebCore.pro:286 + bindings/js/JSAccelerometerEventCustom.cpp \ alpha ordering WebCore/WebCore.xcodeproj/project.pbxproj: + 59A86007119DAFA100DEF1EF /* JSDeviceOrientationEvent.h */, Is this intentional? WebKitTools/Scripts/build-webkit:102 + This isn't needed. It was added in http://trac.webkit.org/changeset/64270/trunk/WebKitTools/Scripts/build-webkit WebCore/dom/AccelerometerController.cpp:29 + #if ENABLE(DEVICE_ORIENTATION) I'm not sure there's a formal policy for using guards, but my approach has been to use as few as possible to make sure the feature isn't exposed when it's not required. I find this keeps things as simple as possible. In particular, there should be no need to guard the contents of these cpp files. LayoutTests/ChangeLog:27 + * fast/dom/Accelerometer/window-property.html: Added. Could you file a bug to track the fact that this new test won't pass with V8. CC me and I'll provide the V8-specific result.
(In reply to comment #9) > (From update of attachment 63296 [details]) > LayoutTests/fast/dom/Window/window-properties-device-orientation.html:2 > + These properties are currently guarded by ENABLE_DEVICE_ORIENTATION.</p> > Looks like this last sentence isn't in the expected result. Added. > > LayoutTests/platform/gtk/Skipped:1096 > + fast/dom/Accelerometer/window-property.html > alpha ordering Fixed > > WebCore/WebCore.gypi:525 > + 'bindings/js/JSAccelerometerEventCustom.cpp', > alpha ordering This is because the file already has case ordering issues. I put the new file at the correct place. > > WebCore/WebCore.pro:286 > + bindings/js/JSAccelerometerEventCustom.cpp \ > alpha ordering Ditto. > > WebCore/WebCore.xcodeproj/project.pbxproj: > + 59A86007119DAFA100DEF1EF /* JSDeviceOrientationEvent.h */, > Is this intentional? Yes. There was a case ordering issue of the device orientation files in the xcode project. You'll see I've moved them slightly. I felt ok changing this file, but didn't want to muck around too much in the build files of a platform I can't test. > > WebKitTools/Scripts/build-webkit:102 > + > This isn't needed. It was added in http://trac.webkit.org/changeset/64270/trunk/WebKitTools/Scripts/build-webkit I missed this. Removed. > > WebCore/dom/AccelerometerController.cpp:29 > + #if ENABLE(DEVICE_ORIENTATION) > I'm not sure there's a formal policy for using guards, but my approach has been to use as few as possible to make sure the feature isn't exposed when it's not required. I find this keeps things as simple as possible. In particular, there should be no need to guard the contents of these cpp files. > Removed. > LayoutTests/ChangeLog:27 > + * fast/dom/Accelerometer/window-property.html: Added. > Could you file a bug to track the fact that this new test won't pass with V8. CC me and I'll provide the V8-specific result. Yes.
Created attachment 63374 [details] Updated patch post review patch
Comment on attachment 63374 [details] Updated patch WebCore/Configurations/FeatureDefines.xcconfig:64 + ENABLE_DEVICE_ORIENTATION = ENABLE_DEVICE_ORIENTATION; If you want to change this file, you must also change the corresponding file in JavaScriptCore and WebKit/mac. See the header of this file. WebCore/bindings/v8/custom/V8AccelerometerEventCustom.cpp:41 + INC_STATS("DOM.DeviceOrientationEvent.x._get"); Should be AccelerometerEvent Looks good other than the two minor points above. It would be good if you could rebase your patch before you upload next time, so that it can build on the EWS bots. It's easy to introduce a build break with a patch like this that adds new files.
Thanks. I'm actually going to rename AccelerometerEvent to DeviceMotionEvent, after the discussion on the public Geolocation list. I don't want to land this and then have to change everything.
Created attachment 63546 [details] New patch - Accelerometer is now DeviceMotion This patch now uses DeviceMotionEvent rather than AccelerometerEvent to reflect the fact that the event encapsulates accelerometer and gyroscopic data. It seems the Geolocation WG have accepted the change, but the specification needs to be updated. I've rebased just now so hopefully it will patch against the current tree. I was only a few hours out of date last time :) I didn't mean to change the FeatureDefines in the previous patch. It's no longer here. The custom v8 implementation is also changed. The major change other than naming was to move the accessor functions in DeviceMotionData to be inline. They are all one-liners. Note that there are new results for the window-properties-device-orientation test. Since this is skipped it would only be turned on when there is a client implementation, and in that case you won't get the empty result. This will need a V8 specific results file too.
Created attachment 63547 [details] Same as last patch without typo
Attachment 63547 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/dom/DeviceMotionController.cpp:30: Alphabetical sorting problem. [build/include_order] [4] WebCore/dom/DeviceMotionEvent.cpp:27: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/dom/DeviceMotionEvent.cpp:28: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] WebCore/dom/DeviceMotionData.cpp:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 4 in 54 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 63550 [details] Stylebot fixes, plus VS project file was missing a file
Attachment 63550 [details] did not build on qt: Build output: http://queues.webkit.org/results/3589894
Attachment 63550 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3608689
Created attachment 63555 [details] QT and crlinux build fixes
Attachment 63555 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/bindings/js/JSEventCustom.cpp:43: "JSDeviceMotionEvent.h" already included at WebCore/bindings/js/JSEventCustom.cpp:38 [build/include] [4] Total errors found: 1 in 56 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 63556 [details] yet another style fix
Comment on attachment 63556 [details] yet another style fix > + https://bugs.webkit.org/show_bug.cgi?id=42865 > + Implement AccelerometerEvent You mean the DeviceMotion event? > + Implement the DeviceMotionEvent interface as defined > + in http://dev.w3.org/geo/api/spec-source-orientation.html > + This is currently an empty implementation, in that there is > + no motion client connected - the platform implementations > + will need to do that. > + > + Note also that at this time the specified event is > + called Accelerometer, but this will change to DeviceMotion > + with the inclusion of gyroscopic results. I think you should remove this from the Changelog; it will go stale as soon as the spec changes. > <File > + RelativePath="$(WebKitOutputDir)\obj\$(ProjectName)\DerivedSources\JSDeviceMotionEvent.cpp" > + > > + <FileConfiguration > + Name="Debug|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> > + <FileConfiguration > + Name="Release|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> > + <FileConfiguration > + Name="Debug_Internal|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> > + <FileConfiguration > + Name="Debug_Cairo|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> > + <FileConfiguration > + Name="Release_Cairo|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> > + <FileConfiguration > + Name="Debug_All|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> Not sure if you should include all that gubbins. > + <FileConfiguration > + Name="Debug|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> > + <FileConfiguration > + Name="Release|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> > + <FileConfiguration > + Name="Debug_Internal|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> > + <FileConfiguration > + Name="Debug_Cairo|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> > + <FileConfiguration > + Name="Release_Cairo|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> > + <FileConfiguration > + Name="Debug_All|Win32" > + ExcludedFromBuild="true" > + > > + <Tool > + Name="VCCLCompilerTool" > + /> > + </FileConfiguration> Ditto. > diff --git a/WebCore/WebCore.xcodeproj/project.pbxproj b/WebCore/WebCore.xcodeproj/project.pbxproj > isa = PBXProject; > buildConfigurationList = 149C284308902B11008A9EFC /* Build configuration list for PBXProject "WebCore" */; > compatibilityVersion = "Xcode 2.4"; > + developmentRegion = English; > hasScannedForEncodings = 1; Spurious change (but harmless). > diff --git a/WebCore/dom/DeviceMotionClient.h b/WebCore/dom/DeviceMotionClient.h > + virtual DeviceMotionData* currentDeviceMotion() const = 0; I think this should return a const DeviceMotionData*, or const DeviceMotionData&, or something refcounted. > diff --git a/WebCore/dom/EventNames.h b/WebCore/dom/EventNames.h > index 6620cec..235988a 100644 > --- a/WebCore/dom/EventNames.h > +++ b/WebCore/dom/EventNames.h > @@ -50,6 +50,7 @@ namespace WebCore { > macro(copy) \ > macro(cut) \ > macro(dblclick) \ > + macro(devicemotion) \ I wonder if this should have a "webkit" prefix, since things are still in flux.
I'd like to see why the windows bot can't apply the patch. I don't have windows to test why the VS project is conflicting.
(In reply to comment #24) > I'd like to see why the windows bot can't apply the patch. I don't have windows to test why the VS project is conflicting. https://webkit-commit-queue.appspot.com/results/3585983 Looks like a problem in the vcproj. You don't need windows to see the problem. Just try ./WebKitTools/Script/webkit-patch apply-attachment 63556 [details] --non-interactive (The --non-interactive flag causes webkit-patch to pass --force to svn-apply.)
Comment on attachment 63556 [details] yet another style fix LayoutTests/fast/dom/Window/window-properties-device-orientation-expected.txt:31 + window.DeviceOrientationEvent.prototype [object DeviceOrientationEventPrototype] Why have the expected results for DeviceOrientation changed? Was this test failing on Mac? If so, it must be another V8 vs JSC difference. Please update Bug 43443 to reflect this. WebCore/dom/DeviceMotionEvent.idl:36 + readonly attribute [Custom] double zRotationRate; Presumably these are the rotations about the body x, y and z axes. I wonder if we should use the names of the angles used in DeviceOrientation - betaRate, gammaRate, alphaRate - for consistency? Let's not hold up this patch on this, but we should discuss on the spec mailing list and possibly rename them. WebCore/dom/DeviceOrientationController.cpp:41 + m_client->setController(this); This shouldn't be changed. Optional features like DeviceOrientation should not be enabled unless a client is provided and we don't want to have to make the entire implementation robust to not having a client. See Bug 43533.
(In reply to comment #26) > (From update of attachment 63556 [details]) > LayoutTests/fast/dom/Window/window-properties-device-orientation-expected.txt:31 > + window.DeviceOrientationEvent.prototype [object DeviceOrientationEventPrototype] > Why have the expected results for DeviceOrientation changed? Was this test failing on Mac? If so, it must be another V8 vs JSC difference. Please update Bug 43443 to reflect this. I mentioned the reason up above (somewhere). I'll revert the change though. It's not a big deal. > WebCore/dom/DeviceMotionEvent.idl:36 > + readonly attribute [Custom] double zRotationRate; > Presumably these are the rotations about the body x, y and z axes. I wonder if we should use the names of the angles used in DeviceOrientation - betaRate, gammaRate, alphaRate - for consistency? Let's not hold up this patch on this, but we should discuss on the spec mailing list and possibly rename them. Yes, I don't mind either way. I find x, y, z easier because the accelerometer is defined that way. Other people might use pitch, yaw, roll. > > WebCore/dom/DeviceOrientationController.cpp:41 > + m_client->setController(this); > This shouldn't be changed. Optional features like DeviceOrientation should not be enabled unless a client is provided and we don't want to have to make the entire implementation robust to not having a client. See Bug 43533. OK.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M LayoutTests/ChangeLog A LayoutTests/fast/dom/DeviceMotion/create-event-expected.txt A LayoutTests/fast/dom/DeviceMotion/create-event.html A LayoutTests/fast/dom/DeviceMotion/optional-event-properties-expected.txt A LayoutTests/fast/dom/DeviceMotion/optional-event-properties.html A LayoutTests/fast/dom/DeviceMotion/script-tests/TEMPLATE.html A LayoutTests/fast/dom/DeviceMotion/script-tests/create-event.js A LayoutTests/fast/dom/DeviceMotion/script-tests/optional-event-properties.js A LayoutTests/fast/dom/DeviceMotion/script-tests/window-property.js A LayoutTests/fast/dom/DeviceMotion/window-property-expected.txt A LayoutTests/fast/dom/DeviceMotion/window-property.html M LayoutTests/fast/dom/Window/window-properties-device-orientation-expected.txt M LayoutTests/fast/dom/Window/window-properties-device-orientation.html M LayoutTests/fast/dom/Window/window-properties.html M LayoutTests/platform/gtk/Skipped M LayoutTests/platform/mac/Skipped M LayoutTests/platform/qt/Skipped M LayoutTests/platform/win/Skipped M WebCore/Android.derived.jscbindings.mk M WebCore/Android.derived.v8bindings.mk M WebCore/Android.jscbindings.mk M WebCore/Android.mk M WebCore/Android.v8bindings.mk M WebCore/CMakeLists.txt M WebCore/ChangeLog M WebCore/DerivedSources.make M WebCore/GNUmakefile.am M WebCore/WebCore.exp.in M WebCore/WebCore.gypi M WebCore/WebCore.pri M WebCore/WebCore.pro M WebCore/WebCore.vcproj/WebCore.vcproj M WebCore/WebCore.xcodeproj/project.pbxproj M WebCore/bindings/generic/RuntimeEnabledFeatures.cpp M WebCore/bindings/generic/RuntimeEnabledFeatures.h A WebCore/bindings/js/JSDeviceMotionEventCustom.cpp M WebCore/bindings/js/JSEventCustom.cpp A WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp A WebCore/dom/DeviceMotionClient.h A WebCore/dom/DeviceMotionController.cpp A WebCore/dom/DeviceMotionController.h A WebCore/dom/DeviceMotionData.cpp A WebCore/dom/DeviceMotionData.h A WebCore/dom/DeviceMotionEvent.cpp A WebCore/dom/DeviceMotionEvent.h A WebCore/dom/DeviceMotionEvent.idl M WebCore/dom/Document.cpp M WebCore/dom/Event.cpp M WebCore/dom/Event.h M WebCore/dom/EventNames.h M WebCore/page/DOMWindow.cpp M WebCore/page/DOMWindow.h M WebCore/page/DOMWindow.idl M WebCore/page/Page.cpp M WebCore/page/Page.h Committed r64845
http://trac.webkit.org/changeset/64845 might have broken Qt Windows 32-bit Debug The following changes are on the blame list: http://trac.webkit.org/changeset/64845 http://trac.webkit.org/changeset/64846
> > + virtual DeviceMotionData* currentDeviceMotion() const = 0; > > I think this should return a const DeviceMotionData*, or const DeviceMotionData&, > or something refcounted. I think this is OK as it is - the caller can use a RefPtr to ref count the result if required for long-term use, or use the raw pointer directly if they just need the result at the call site.
(In reply to comment #30) > > > + virtual DeviceMotionData* currentDeviceMotion() const = 0; > > > > I think this should return a const DeviceMotionData*, or const DeviceMotionData&, > > or something refcounted. > I think this is OK as it is - the caller can use a RefPtr to ref count the result if required for long-term use, or use the raw pointer directly if they just need the result at the call site. I'm worried about a caller mutating the returned DeviceMotionData.
It seems the QT Windows build bot is out of disk space.
Ossy is your man!
(In reply to comment #32) > It seems the QT Windows build bot is out of disk space. Sorry about it, I enlarged the ramdisk size of Qt Windows debug bot.
Sorry for the (very) late comment. But I'm wondering if there's a reason you didn't implement a mock device like geolocation/device orientation (https://bugs.webkit.org/show_bug.cgi?id=39589) so that more of the plumbing can be exercised. My (admittedly limited) understanding is that such things are fairly common for APIs like this.
> Sorry for the (very) late comment. But I'm wondering if there's a reason you > didn't implement a mock device like geolocation/device orientation Yes, we should have a mock to test this. My assumption when I reviewed this patch was that the mock would be implemented in a separate patch, as was the case for DeviceOrientation.