Bug 64595 – [Qt] Make DeviceMotion and DeviceOrientation work with WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Kenneth Rohde Christiansen]

SSIA

[Kenneth Rohde Christiansen]

Created attachment 100967 [details]
Patch

[WebKit Review Bot]

Attachment 100967 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/WebPage.cpp:97:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebPage/WebPage.cpp:98:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebPage/WebPage.cpp:160:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 100967 [details]
Patch

something went wrong with my tools :-(

[Kenneth Rohde Christiansen]

This needs to be enabled when compiling

[Kenneth Rohde Christiansen]

Created attachment 100969 [details]
Patch

[WebKit Review Bot]

Attachment 100969 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit2/WebProcess/WebPage/WebPage.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 100969 [details]
Patch

Attachment 100969 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9096243

[Kenneth Rohde Christiansen]

Created attachment 100972 [details]
Patch 3

[Early Warning System Bot]

Comment on attachment 100972 [details]
Patch 3

Attachment 100972 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9093247

[Kenneth Rohde Christiansen]

Created attachment 100975 [details]
Patch 4

Another try...

[Early Warning System Bot]

Comment on attachment 100975 [details]
Patch 4

Attachment 100975 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9095245

[Kenneth Rohde Christiansen]

Created attachment 100990 [details]
Patch

Trying to include PassOwnPtr

[Early Warning System Bot]

Comment on attachment 100990 [details]
Patch

Attachment 100990 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9093288

[Kenneth Rohde Christiansen]

Created attachment 100996 [details]
Patch

[Early Warning System Bot]

Comment on attachment 100996 [details]
Patch

Attachment 100996 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9094333

[Kenneth Rohde Christiansen]

Created attachment 100999 [details]
Patch

[Early Warning System Bot]

Comment on attachment 100999 [details]
Patch

Attachment 100999 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9095285

[Kenneth Rohde Christiansen]

I'm guess this is because of a non-clean build... it must be using the old file.

Ossy can you verify this somehow?

[Benjamin Poulain]

Comment on attachment 100999 [details]
Patch

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

Quick review.

I don't understand why you make this a Qt specific subclass of DeviceMotionClient and DeviceOrientationClient while all they do is forward call to a provider.
It looks like instead DeviceMotionClient should not be abstract, and DeviceMotionProvider should be a typedef per platform. Same for orientation.

I am not familiar with this code, is there a reason for all those indirections?

> Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp:2
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

2011 :)

> Source/WebCore/platform/qt/DeviceMotionProviderQt.h:2
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

2011

> Source/WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.cpp:24
> +#if ENABLE(DEVICE_ORIENTATION)

You should not put this check, the file is simply not compiled if DEVICE_ORIENTATION is not set.

> Source/WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.h:23
> +#if ENABLE(DEVICE_ORIENTATION)

The file is not added if ENABLE_DEVICE_ORIENTATION is not defined. I think is it more explicit to have the check on the include side than on the header side.

> Source/WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.h:30
> +#include <wtf/PassOwnPtr.h>

I am unsure why you need PassOwnPtr in this header, looks like it should be only needed in the implementation for intializing the OwnPtr(?).

> Source/WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.h:36
> +    DeviceOrientationClientQt() { }

Maybe just remove the constructor and let the compiler declare the empty constructor for you? :)

> Source/WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.h:37
>      virtual ~DeviceOrientationClientQt();

Look like this one is not needed anymore after this patch.

> Source/WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.h:47
> +    virtual void deviceOrientationControllerDestroyed();
> +
>      virtual void startUpdating();
>      virtual void stopUpdating();
> +
>      virtual DeviceOrientation* lastOrientation() const;
> -    virtual void deviceOrientationControllerDestroyed();
>  
> -public Q_SLOTS:
> -    void changeDeviceOrientation(DeviceOrientation*);
> +    virtual void setController(DeviceOrientationController*);

I would put them in the same order as DeviceOrientationClient but it does not matter much...

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:127
> +#include "DeviceMotionClientQt.h"
> +#include "DeviceOrientationClientQt.h"

If you follow my comment about checking the feature in the hader, you need #if ENABLE(DEVICE_ORIENTATION) here.

[Csaba Osztrogonác]

(In reply to comment #18)
> I'm guess this is because of a non-clean build... it must be using the old file.
> 
> Ossy can you verify this somehow?

No, it isn't a non-clean build problem.

Now there are two header files:
 - WebCore/platform/qt/DeviceOrientationProviderQt.h
 - WebKit/qt/WebCoreSupport/DeviceOrientationClientQt.h

And I guess that the cpp file includes the wrong one.

[Csaba Osztrogonác]

Comment on attachment 100999 [details]
Patch

r- now, because it doesn't build

[Luiz Agostini]

Created attachment 102211 [details]
patch

[Luiz Agostini]

(In reply to comment #19)
> (From update of attachment 100999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100999&action=review
> 
> Quick review.
> 
> I don't understand why you make this a Qt specific subclass of DeviceMotionClient and DeviceOrientationClient while all they do is forward call to a provider.
> It looks like instead DeviceMotionClient should not be abstract, and DeviceMotionProvider should be a typedef per platform. Same for orientation.
> 
> I am not familiar with this code, is there a reason for all those indirections?

I think that what we could do to avoid all those indirections is to have no provider.

[Alexis Menard (darktears)]

Comment on attachment 102211 [details]
patch

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

> Source/WebCore/platform/qt/DeviceOrientationProviderQt.cpp:2
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

2011

> Source/WebCore/platform/qt/DeviceOrientationProviderQt.h:2
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

Still 2011

[Kenneth Rohde Christiansen]

> > I am not familiar with this code, is there a reason for all those indirections?
> 
> I think that what we could do to avoid all those indirections is to have no provider.

There were possible a reason for this at some point, but not anymore. I wouldn't mind that being another patch though.

[Kenneth Rohde Christiansen]

(In reply to comment #25)
> > > I am not familiar with this code, is there a reason for all those indirections?
> > 
> > I think that what we could do to avoid all those indirections is to have no provider.
> 
> There were possible a reason for this at some point, but not anymore. I wouldn't mind that being another patch though.

One of the reasons that there is a provider is that one is used by both DeviceMotion and DeviceOrientation.

[Eric Seidel (no email)]

This looks abandoned.  Should this be r-'d?  Closed?

[Kenneth Rohde Christiansen]

No, this will be done at some point. I can probably review the patch on monday.

[Allan Sandfeld Jensen]

The patch is pretty old, maybe it needs to be rebased first?

[Lars Knudsen]

(In reply to comment #29)
> The patch is pretty old, maybe it needs to be rebased first?

I'll take a shot at it now

[Lars Knudsen]

Created attachment 139614 [details]
Patch

[WebKit Review Bot]

Attachment 139614 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lars Knudsen]

Created attachment 139616 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 139616 [details]
Patch

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

> Source/WebCore/platform/qt/DeviceMotionClientQt.cpp:2
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

If you change any of these files, please update copyright date

> Source/WebCore/platform/qt/DeviceMotionProviderQt.cpp:76
> +            false, 0 /* The interval is treated internally by Qt mobility */);

Nothing is called Qt Mobility in Qt5. QtSensors I believe

[Early Warning System Bot]

Comment on attachment 139616 [details]
Patch

Attachment 139616 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12586608

[Early Warning System Bot]

Comment on attachment 139616 [details]
Patch

Attachment 139616 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12584619

[Lars Knudsen]

Created attachment 139622 [details]
Patch

[Early Warning System Bot]

Comment on attachment 139622 [details]
Patch

Attachment 139622 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12587557

[Lars Knudsen]

Created attachment 139626 [details]
Patch

[Early Warning System Bot]

Comment on attachment 139626 [details]
Patch

Attachment 139626 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12589559

[Lars Knudsen]

Created attachment 139696 [details]
Patch

[Simon Hausmann]

Comment on attachment 139696 [details]
Patch

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

> Source/WebCore/Target.pri:3439
> +    haveQt(5) {
> +        QT += sensors
> +    } else {
> +        CONFIG *= mobility
> +        MOBILITY *= sensors
> +    }

I don't think this block is needed given that it's also in WebCore.pri.

> Source/WebKit2/Target.pri:794
> +contains(DEFINES, ENABLE_DEVICE_ORIENTATION=1) {
> +    haveQt(5): QT += sensors
> +}

Why is this needed? WebCore.pri has it and this Target.pri has WEBKIT += webcore, which should pull in WebCore.pri through Tools/qmake/mkspecs/modules/webcore.prf

[Lars Knudsen]

Created attachment 139767 [details]
Patch

[Lars Knudsen]

(In reply to comment #42)
> (From update of attachment 139696 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139696&action=review
> 
> > Source/WebCore/Target.pri:3439
> > +    haveQt(5) {
> > +        QT += sensors
> > +    } else {
> > +        CONFIG *= mobility
> > +        MOBILITY *= sensors
> > +    }
> 
> I don't think this block is needed given that it's also in WebCore.pri.
> 
> > Source/WebKit2/Target.pri:794
> > +contains(DEFINES, ENABLE_DEVICE_ORIENTATION=1) {
> > +    haveQt(5): QT += sensors
> > +}
> 
> Why is this needed? WebCore.pri has it and this Target.pri has WEBKIT += webcore, which should pull in WebCore.pri through Tools/qmake/mkspecs/modules/webcore.prf

True, done - thanks! :)

[Kenneth Rohde Christiansen]

[10:38] <simon_> laknudse, kenneth: the pattern in if (!m_provider && controller) looks a bit dangerous to me :)
[10:38] <simon_> if !m_provider and !controller, then setController crashes
[10:39] * simon_ would remove the controller part of the if()

[Lars Knudsen]

Created attachment 139785 [details]
Patch

[WebKit Review Bot]

Comment on attachment 139785 [details]
Patch

Clearing flags on attachment: 139785

Committed r115812: <http://trac.webkit.org/changeset/115812>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke the builds with Qt Mobility:
(Qt Windows, Qt MIPS, Qt SH4 bots)

In file included from ../../../Source/WebCore/platform/qt/DeviceMotionClientQt.h:27:0,
                 from ../../../Source/WebKit/qt/Api/qwebpage.cpp:48:
../../../Source/WebCore/platform/qt/DeviceMotionProviderQt.h:25:32: fatal error: QAccelerometerFilter: No such file or directory

[Csaba Osztrogonác]

Created attachment 139807 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 139807 [details]
Patch

Clearing flags on attachment: 139807

Committed r115828: <http://trac.webkit.org/changeset/115828>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.