[Webkit-unassigned] [Bug 39724] [Qt] navigator.geolocation support for qt port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39724





--- Comment #13 from Laszlo Gombos <laszlo.1.gombos at nokia.com>  2010-06-07 18:00:23 PST ---
(From update of attachment 57846)
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 60655)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,27 @@
> +2010-06-04  Mahesh Kulkarni  <mahesh.kulkarni at nokia.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt] navigator.geolocation support for qt port
> +        https://bugs.webkit.org/show_bug.cgi?id=39724
> +
> +        Added support for navigator.geolocation in qtwebkit using qtmobility location service
> +
> +        Test: No new cases added. Ran existing geolocation test cases.

Have you actually run and _pass_ all the fast/dom/Geolocation and fast/dom/Window/window-properties-geolocation.html ?

If so perhaps you should mention that you not only run but also _passed_ the tests. Also please mention the platform where you passed them (e.g. Linux) and also mention the reason why you have not removed these tests from the Skipped list (perhaps because the buildbot does not have support for QtMobility. 

> Index: WebCore/WebCore.pri
> ===================================================================
> --- WebCore/WebCore.pri	(revision 60618)
> +++ WebCore/WebCore.pri	(working copy)
> @@ -109,6 +109,12 @@ greaterThan(QT_MINOR_VERSION, 5) {
>      else:DEFINES += ENABLE_XSLT=0
>  }
>  
> +!CONFIG(QTDIR_build):!contains(DEFINES, ENABLE_GEOLOCATION=.) {
> +    exists($$[QMAKE_MKSPECS]/features/mobility.prf) {
> +       DEFINES += ENABLE_GEOLOCATION=1
> +    }
> +}
> +

As discussed this is not a reliable way to detect if Geolocation QtMobility API is supported. This should be replaced with contains(MOBILITY_CONFIG, location).

>  contains(DEFINES, ENABLE_QT_BEARER=1) {
> -    HEADERS += \
> -        platform/network/qt/NetworkStateNotifierPrivate.h
> -
> -    SOURCES += \
> -        platform/network/qt/NetworkStateNotifierQt.cpp
> -
> +    message("bearer exists")
> +    HEADERS += platform/network/qt/NetworkStateNotifierPrivate.h
> +    SOURCES += platform/network/qt/NetworkStateNotifierQt.cpp
>      CONFIG += mobility
>      MOBILITY += bearer
>  }

This is unrelated change and I prefer to keep it as it was as that makes it easier to add new files to the list (and make patches more readable). Please remove this change.

>  contains(DEFINES, ENABLE_SVG=1) {
>      SOURCES += \
>  # TODO: this-one-is-not-auto-added! FIXME! tmp/SVGElementFactory.cpp \
> Index: WebCore/platform/qt/GeolocationServiceQt.cpp
> ===================================================================
> --- WebCore/platform/qt/GeolocationServiceQt.cpp	(revision 0)
> +++ WebCore/platform/qt/GeolocationServiceQt.cpp	(revision 0)
> @@ -0,0 +1,111 @@

[...]

> +    if (m_location)
> +        connect(m_location, SIGNAL(positionUpdated(QGeoPositionInfo)), this, SLOT(positionUpdated(QGeoPositionInfo)));
> +    else
> +        m_lastError = PositionError::create(PositionError::POSITION_UNAVAILABLE, tr("GPS device not found"));

The platform independent WebKit code is using "Failed to start Geolocation service" error message (see Geolocation.cpp). Perhaps we should do the same here. I particularly dislike GPS in the error message as GPS is not the only location provider that QtMobility supports (I hope).

> +void GeolocationServiceQt::positionUpdated(const QGeoPositionInfo &gpsPos)

As noted above GPS is not the only source for location data. Can we use a more generic variable name - perhaps geoPos.

> Index: WebCore/platform/qt/GeolocationServiceQt.h
> ===================================================================
> --- WebCore/platform/qt/GeolocationServiceQt.h	(revision 0)
> +++ WebCore/platform/qt/GeolocationServiceQt.h	(revision 0)
> +// FIXME: Remove usage of "usinb namespace" in a header file.

Misspelled using. 

> +// There is bug in qtMobility signal names are not full qualified when used with namespace
> +// QtMobility namespace in slots throws up error and its required to be fixed in qtmobility.

Maybe mention the QtMobility version - is it v1.0 ? 

> +// nmea simulation will read static positions from a log file

This is only relevant for Symbian; does not have much value in WebKit code I think. 

> Index: WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
> ===================================================================
> --- WebKit/qt/tests/qwebpage/tst_qwebpage.cpp	(revision 60618)
> +++ WebKit/qt/tests/qwebpage/tst_qwebpage.cpp	(working copy)
> @@ -77,6 +77,7 @@ private slots:
>  

[...]

>  void tst_QWebPage::infiniteLoopJS()
>  {
>      JSTestPage* newPage = new JSTestPage(m_view);
>      m_view->setPage(newPage);
> -    m_view->setHtml(QString("<html><bodytest</body></html>"), QUrl());
> +    m_view->setHtml(QString("<html><body>test</body></html>"), QUrl());

Unrelated change, please remove.

>      m_view->page()->mainFrame()->evaluateJavaScript("var run = true;var a = 1;while(run){a++;}");
> +    delete newPage;

This looks like a good change but you should really have a separate patch for this.

This is really close for r+. Hope you find some time to address this minor issues.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.