Bug 47051 - [Qt] Provide Qt support for DeviceMotion/Orientation clients
Summary: [Qt] Provide Qt support for DeviceMotion/Orientation clients
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Diego Gonzalez
URL:
Keywords: Qt
Depends on:
Blocks: 47050
  Show dependency treegraph
 
Reported: 2010-10-02 08:42 PDT by Diego Gonzalez
Modified: 2010-10-02 14:20 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.76 KB, patch)
2010-10-02 08:56 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff
Patch (13.76 KB, patch)
2010-10-02 08:58 PDT, Diego Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Gonzalez 2010-10-02 08:42:38 PDT
Implement client classes to make possible to hook up motion/orientation
data in further implementations.
Comment 1 Diego Gonzalez 2010-10-02 08:56:18 PDT
Created attachment 69574 [details]
Patch
Comment 2 WebKit Review Bot 2010-10-02 08:56:52 PDT
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.
Comment 3 Diego Gonzalez 2010-10-02 08:58:54 PDT
Created attachment 69575 [details]
Patch

Correcting style
Comment 4 Kenneth Rohde Christiansen 2010-10-02 12:08:32 PDT
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 *
Comment 5 Diego Gonzalez 2010-10-02 12:36:27 PDT
(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
Comment 6 Diego Gonzalez 2010-10-02 14:19:27 PDT
Comment on attachment 69575 [details]
Patch

Clearing flags. Patch landed at r68978