Bug 47052

Summary: [Qt] Hook up DeviceOrientation data for Qt support
Product: WebKit Reporter: Diego Gonzalez <diegohcg>
Component: WebKit QtAssignee: Diego Gonzalez <diegohcg>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, kenneth, kling, tonikitoo
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 47050    
Attachments:
Description Flags
Patch
kenneth: review-
Patch v2 none

Description Diego Gonzalez 2010-10-02 08:48:06 PDT
Get DeviceOrientation necessary data via Qt mobility library using a provider class.
Comment 1 Diego Gonzalez 2010-10-02 09:11:09 PDT
Created attachment 69577 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2010-10-02 11:55:36 PDT
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->...
Comment 3 Kenneth Rohde Christiansen 2010-10-02 12:13:47 PDT
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.
Comment 4 Kenneth Rohde Christiansen 2010-10-02 12:15:28 PDT
Are you going to implement DeviceMotionClientQt.cpp using QAccelerometerReading ?
Comment 5 Diego Gonzalez 2010-10-02 12:20:38 PDT
(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!
Comment 6 Diego Gonzalez 2010-10-02 12:21:32 PDT
(In reply to comment #4)
> Are you going to implement DeviceMotionClientQt.cpp using QAccelerometerReading ?

Yes, almost done
Comment 7 Diego Gonzalez 2010-10-02 14:17:41 PDT
Created attachment 69581 [details]
Patch v2
Comment 8 Andreas Kling 2010-10-02 16:13:54 PDT
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.
Comment 9 Kenneth Rohde Christiansen 2010-10-02 18:50:07 PDT
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!
Comment 10 Diego Gonzalez 2010-10-04 12:48:47 PDT
Thanks guys
Comment 11 Diego Gonzalez 2010-10-04 12:50:20 PDT
Comment on attachment 69581 [details]
Patch v2

Clearing flags. Patch landed at r69024