Implementing the FIXMEs: remove of using namespaces in header file.
Created attachment 110324 [details] The patch: removed using namespaces in the header file
I'm not sure if we really need this bug for these changes. Because IMO 1) Qt5 is made mandatory for trunk code which means that we do not have to keep mobility hooked up in trunk anymore for location atleast. 2) https://bugs.webkit.org/show_bug.cgi?id=59199 was good enough place to hook these changes with incase if we trunk still needs to support older qt module with older mobility.
Created attachment 110560 [details] Allowing it compile with both Qt 4.x and Qt 5 (plus cleanups) The idea is to allow this WK1 code to compile with both Qt 4 and 5 (and in future to drop the ifdefs).
Comment on attachment 110324 [details] The patch: removed using namespaces in the header file View in context: https://bugs.webkit.org/attachment.cgi?id=110324&action=review Please fix changelog before landing > Source/WebKit/qt/ChangeLog:6 > + Fixmes: remove of using namespace in header file. What does this mean? The patch is imcomplete? or you fixed some fixme's?
Comment on attachment 110560 [details] Allowing it compile with both Qt 4.x and Qt 5 (plus cleanups) View in context: https://bugs.webkit.org/attachment.cgi?id=110560&action=review > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:23 > - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. Unrelated change, but ok > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:40 > +#if QT_VERSION < 0x050000 Wasn't there a nicer way of writing this like #if QT_VERSION < QT_VERSION_CHECK(5, 0, 0) > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.h:83 > +#if QT_VERSION < 0x050000 > QtMobility::QGeoPositionInfoSource* m_location; > +#elif QT_VERSION >= 0x050000 > + QGeoPositionInfoSource* m_location; > +#endif Wouldnt a "using namespace" be better when using old Qt?
The first patch changes just seem to make patch 2 worse
Comment on attachment 110560 [details] Allowing it compile with both Qt 4.x and Qt 5 (plus cleanups) View in context: https://bugs.webkit.org/attachment.cgi?id=110560&action=review Urgh, this is one mess of #ifdefs ;(. An alternative would be to split out the code into two files *Qt4 and *Qt5. Adenilson, do you mind doing another round with Kenneth's suggested changes at least? (unless you prefer the suggested split) > Source/WebKit/qt/QtWebKit.pro:287 > +qt5: QT += location Please indent this line :)
Created attachment 110883 [details] Compiles with both Qt4/5, more tolerable use of ifdefs, better approach to test for Qt version
Comment on attachment 110883 [details] Compiles with both Qt4/5, more tolerable use of ifdefs, better approach to test for Qt version View in context: https://bugs.webkit.org/attachment.cgi?id=110883&action=review Almost there! Almost there! Stay on target rogue leader... > Source/WebKit/qt/ChangeLog:10 > + Reviewed by Kenneth Rohde Christiansen. Move this up! Just after teh bugzilla url > Source/WebKit/qt/QtWebKit.pro:287 > +qt5: QT += location indentation! > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.h:50 > +// This class provides an implementation of a GeolocationService for qtWebkit. QtWebKit!
Created attachment 110894 [details] Fixed pending issues.
Comment on attachment 110894 [details] Fixed pending issues. Clearing flags on attachment: 110894 Committed r97409: <http://trac.webkit.org/changeset/97409>
All reviewed patches have been landed. Closing bug.