Bug 42865 – Implement DeviceMotionEvent

• Component: DOM
• Status: RESOLVED FIXED

[Dean Jackson]

Implement AccelerometerEvents, as defined by the linked spec.

[Dean Jackson]

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.

[Dean Jackson]

Referencing bug that will help with Page constructor

[Steve Block]

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.

[Dean Jackson]

Created attachment 62637 [details]
updated patch with new testcases

[Dean Jackson]

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

[Steve Block]

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

[Dean Jackson]

The patch still has an extra "class Page;" in AccelerometerController.h that I'll remove.

[Dean Jackson]

Created attachment 63296 [details]
Updated patch after the Page constructor changes

[Steve Block]

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.

[Dean Jackson]

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

[Dean Jackson]

Created attachment 63374 [details]
Updated patch

post review patch

[Steve Block]

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.

[Dean Jackson]

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.

[Dean Jackson]

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.

[Dean Jackson]

Created attachment 63547 [details]
Same as last patch without typo

[WebKit Review Bot]

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.

[Dean Jackson]

Created attachment 63550 [details]
Stylebot fixes, plus VS project file was missing a file

[Early Warning System Bot]

Attachment 63550 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3589894

[WebKit Review Bot]

Attachment 63550 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3608689

[Dean Jackson]

Created attachment 63555 [details]
QT and crlinux build fixes

[WebKit Review Bot]

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.

[Dean Jackson]

Created attachment 63556 [details]
yet another style fix

[Simon Fraser (smfr)]

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.

[Dean Jackson]

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.

[Adam Barth]

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

[Steve Block]

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.

[Dean Jackson]

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

[Dean Jackson]

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

[WebKit Review Bot]

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

[Steve Block]

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

[Simon Fraser (smfr)]

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

[Dean Jackson]

It seems the QT Windows build bot is out of disk space.

[Eric Seidel (no email)]

Ossy is your man!

[Csaba Osztrogonác]

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

[Jeremy Orlow]

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.

[Steve Block]

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