Bug 47051 – [Qt] Provide Qt support for DeviceMotion/Orientation clients

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Diego Gonzalez]

Implement client classes to make possible to hook up motion/orientation
data in further implementations.

[Diego Gonzalez]

Created attachment 69574 [details]
Patch

[WebKit Review Bot]

Attachment 69574 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/qt/WebCoreSupport/DeviceMotionClientQt.h:24:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Diego Gonzalez]

Created attachment 69575 [details]
Patch

Correcting style

[Kenneth Rohde Christiansen]

Comment on attachment 69575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69575&action=review

r+ with the below fixes. Please make it clear that these are dummy impls so far.

> WebCore/WebCore.pro:3266
> +        bindings/generic/RuntimeEnabledFeatures.h

Is this one really device orientation dependent?

> WebKit/qt/WebCoreSupport/DeviceMotionClientQt.cpp:44
> +void DeviceMotionClientQt::startUpdating()
> +{
> +}
> +
> +void DeviceMotionClientQt::stopUpdating()
> +{
> +}

why are these empty? no comment, no nothing...

> WebKit/qt/WebCoreSupport/DeviceMotionClientQt.cpp:49
> +DeviceMotionData* DeviceMotionClientQt::currentDeviceMotion() const
> +{
> +    return 0;
> +}

Is this just a dummy implementation? You should make that more clear in the ChangeLog

> WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.cpp:27
> +DeviceOrientationClientQt::DeviceOrientationClientQt(QWebPage *page)

Wrong placement of *

[Diego Gonzalez]

(In reply to comment #4)
> (From update of attachment 69575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69575&action=review
> 
> r+ with the below fixes. Please make it clear that these are dummy impls so far.
> 
> > WebCore/WebCore.pro:3266
> > +        bindings/generic/RuntimeEnabledFeatures.h
> 
> Is this one really device orientation dependent?

Yes, Page.cpp uses it:
#if ENABLE(DEVICE_ORIENTATION) 
    , m_deviceMotionController(RuntimeEnabledFeatures::deviceMotionEnabled() ? new DeviceMotionController(pageClients.deviceMotionClient) : 0)
    , m_deviceOrientationController(RuntimeEnabledFeatures::deviceOrientationEnabled() ? new DeviceOrientationController(this, pageClients.deviceOrientationClient) : 0)
#endif

> 
> > WebKit/qt/WebCoreSupport/DeviceMotionClientQt.cpp:44
> > +void DeviceMotionClientQt::startUpdating()
> > +{
> > +}
> > +
> > +void DeviceMotionClientQt::stopUpdating()
> > +{
> > +}
> 
> why are these empty? no comment, no nothing...

Sure, I will add the comments

> > WebKit/qt/WebCoreSupport/DeviceMotionClientQt.cpp:49
> > +DeviceMotionData* DeviceMotionClientQt::currentDeviceMotion() const
> > +{
> > +    return 0;
> > +}
> 
> Is this just a dummy implementation? You should make that more clear in the ChangeLog

Sure

> > WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.cpp:27
> > +DeviceOrientationClientQt::DeviceOrientationClientQt(QWebPage *page)
> 
> Wrong placement of *

OK

[Diego Gonzalez]

Comment on attachment 69575 [details]
Patch

Clearing flags. Patch landed at r68978