Bug 69737 – [Qt] GeolocationClient cleanups

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Adenilson Cavalcanti Silva]

Implementing the FIXMEs: remove of using namespaces in header file.

[Adenilson Cavalcanti Silva]

Created attachment 110324 [details]
The patch: removed using namespaces in the header file

[Mahesh Kulkarni]

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.

[Adenilson Cavalcanti Silva]

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).

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

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?

[Kenneth Rohde Christiansen]

The first patch changes just seem to make patch 2 worse

[Simon Hausmann]

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 :)

[Adenilson Cavalcanti Silva]

Created attachment 110883 [details]
Compiles with both Qt4/5, more tolerable use of ifdefs, better approach to test for Qt version

[Kenneth Rohde Christiansen]

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!

[Adenilson Cavalcanti Silva]

Created attachment 110894 [details]
Fixed pending issues.

[WebKit Review Bot]

Comment on attachment 110894 [details]
Fixed pending issues.

Clearing flags on attachment: 110894

Committed r97409: <http://trac.webkit.org/changeset/97409>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.