Get DeviceOrientation necessary data via Qt mobility library using a provider class.
Created attachment 69577 [details] Patch
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 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.
Are you going to implement DeviceMotionClientQt.cpp using QAccelerometerReading ?
(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!
(In reply to comment #4) > Are you going to implement DeviceMotionClientQt.cpp using QAccelerometerReading ? Yes, almost done
Created attachment 69581 [details] Patch v2
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 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!
Thanks guys
Comment on attachment 69581 [details] Patch v2 Clearing flags. Patch landed at r69024