| Summary: | [Qt] Make DeviceMotion and DeviceOrientation work with WebKit2 | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kenneth Rohde Christiansen <kenneth> | ||||||||||||||||||||||||||||||||||
| Component: | WebKit2 | Assignee: | Lars Knudsen <larsgk> | ||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | ahf, allan.jensen, benjamin, eric, hausmann, larsgk, laszlo.gombos, luiz, menard, ossy, webkit.review.bot, yael, zoltan | ||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||
|
Description
Kenneth Rohde Christiansen
2011-07-15 06:08:08 PDT
Created attachment 100967 [details]
Patch
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.
Comment on attachment 100967 [details]
Patch
something went wrong with my tools :-(
This needs to be enabled when compiling Created attachment 100969 [details]
Patch
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.
Comment on attachment 100969 [details] Patch Attachment 100969 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9096243 Created attachment 100972 [details]
Patch 3
Comment on attachment 100972 [details] Patch 3 Attachment 100972 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9093247 Created attachment 100975 [details]
Patch 4
Another try...
Comment on attachment 100975 [details] Patch 4 Attachment 100975 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9095245 Created attachment 100990 [details]
Patch
Trying to include PassOwnPtr
Comment on attachment 100990 [details] Patch Attachment 100990 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9093288 Created attachment 100996 [details]
Patch
Comment on attachment 100996 [details] Patch Attachment 100996 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9094333 Created attachment 100999 [details]
Patch
Comment on attachment 100999 [details] Patch Attachment 100999 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9095285 I'm guess this is because of a non-clean build... it must be using the old file. Ossy can you verify this somehow? 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. (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. Comment on attachment 100999 [details]
Patch
r- now, because it doesn't build
Created attachment 102211 [details]
patch
(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. 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 > > 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.
(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. This looks abandoned. Should this be r-'d? Closed? No, this will be done at some point. I can probably review the patch on monday. The patch is pretty old, maybe it needs to be rebased first? (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 Created attachment 139614 [details]
Patch
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.
Created attachment 139616 [details]
Patch
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 Comment on attachment 139616 [details] Patch Attachment 139616 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12586608 Comment on attachment 139616 [details] Patch Attachment 139616 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12584619 Created attachment 139622 [details]
Patch
Comment on attachment 139622 [details] Patch Attachment 139622 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12587557 Created attachment 139626 [details]
Patch
Comment on attachment 139626 [details] Patch Attachment 139626 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12589559 Created attachment 139696 [details]
Patch
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 Created attachment 139767 [details]
Patch
(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! :) [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() Created attachment 139785 [details]
Patch
Comment on attachment 139785 [details] Patch Clearing flags on attachment: 139785 Committed r115812: <http://trac.webkit.org/changeset/115812> All reviewed patches have been landed. Closing bug. 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
Created attachment 139807 [details]
Patch
Comment on attachment 139807 [details] Patch Clearing flags on attachment: 139807 Committed r115828: <http://trac.webkit.org/changeset/115828> All reviewed patches have been landed. Closing bug. |