Bug 47052 – [Qt] Hook up DeviceOrientation data for Qt support

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

[Diego Gonzalez]

Get DeviceOrientation necessary data via Qt mobility library using a provider class.

[Diego Gonzalez]

Created attachment 69577 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 69577 [details]
Patch

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

> WebCore/WebCore.pro:3266
> +        ../WebKit/qt/WebCoreSupport/DeviceOrientationProviderQt.h \

None of this depends directly on QtMobility?

> WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.cpp:41
> +    m_provider = 0;
> +    delete m_provider;

This is wrong! you are leaking!

> WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.cpp:72
> +    if (m_controller)
> +        m_controller->didChangeDeviceOrientation(orientation);

I like the following more

if (!m_controller)
    return;

m_controller->...

[Kenneth Rohde Christiansen]

Comment on attachment 69577 [details]
Patch

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

> WebKit/qt/WebCoreSupport/DeviceOrientationProviderQt.cpp:60
> +    // Qt mobility provide these data via QRatationSensor using the

R_o_tation!

> WebKit/qt/WebCoreSupport/DeviceOrientationProviderQt.cpp:69
> +            true, reading->x(), true, reading->y());

What is true here? Please add a /* ... */ comment in front like /* strict */ true... etc

> WebKit/qt/WebCoreSupport/DeviceOrientationProviderQt.h:26
> +#include <QRotationFilter>

So I guess this is QtMobility dependent... I guess that needs some qmake changes then.

[Kenneth Rohde Christiansen]

Are you going to implement DeviceMotionClientQt.cpp using QAccelerometerReading ?

[Diego Gonzalez]

(In reply to comment #2)
> (From update of attachment 69577 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69577&action=review
> 
> > WebCore/WebCore.pro:3266
> > +        ../WebKit/qt/WebCoreSupport/DeviceOrientationProviderQt.h \
> 
> None of this depends directly on QtMobility?
> 

It will be added in WebCore.pro by the patch in bug 47051:
+    CONFIG += mobility
+    MOBILITY += sensors

> > WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.cpp:41
> > +    m_provider = 0;
> > +    delete m_provider;
> 
> This is wrong! you are leaking!
> 

Will check this.

> > WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.cpp:72
> > +    if (m_controller)
> > +        m_controller->didChangeDeviceOrientation(orientation);
> 
> I like the following more
> 
> if (!m_controller)
>     return;
> 
> m_controller->...

OK!

[Diego Gonzalez]

(In reply to comment #4)
> Are you going to implement DeviceMotionClientQt.cpp using QAccelerometerReading ?

Yes, almost done

[Diego Gonzalez]

Created attachment 69581 [details]
Patch v2

[Andreas Kling]

Comment on attachment 69581 [details]
Patch v2

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

Otherwise LGTM, leaving final review for Kenneth.

> WebKit/qt/WebCoreSupport/DeviceOrientationProviderQt.h:51
> +    PassRefPtr<DeviceOrientation> m_orientation;

This should be a RefPtr.

[Kenneth Rohde Christiansen]

Comment on attachment 69581 [details]
Patch v2

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

r+ if you fix the RefPtr issue.

> WebCore/WebCore.pro:@
>  contains(DEFINES, ENABLE_DEVICE_ORIENTATION=1) {

This is disabled by default? Would be nice to have enabled on the buildbots

>> WebKit/qt/WebCoreSupport/DeviceOrientationProviderQt.h:51
>> +    PassRefPtr<DeviceOrientation> m_orientation;
> 
> This should be a RefPtr.

This will need to be fixed before landing!

[Diego Gonzalez]

Thanks guys

[Diego Gonzalez]

Comment on attachment 69581 [details]
Patch v2

Clearing flags. Patch landed at r69024