Summary: | [Qt] Hook up DeviceOrientation data for Qt support | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Diego Gonzalez <diegohcg> | ||||||
Component: | WebKit Qt | Assignee: | 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
Diego Gonzalez
2010-10-02 08:48:06 PDT
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 |