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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 8 04:32:12 PDT 2010


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





--- Comment #14 from maheshK <mahesh.kulkarni at nokia.com>  2010-06-08 04:32:10 PST ---
Thanks Laszlo for review comments. 

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


I forgot to update my comment a day before that I'm working on Geolocation layout test cases using GeolocationServiceMock and will upload a new patch soon with the results. 

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

I have included this changes. 

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


Bearer changes are checked in as separate bug. I have removed this as well. 


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


Actually QtMobility location service only supports GPS as of now. They have no  plans of supporting other measures in near future :( 
But I think your comment to make webkit changes more generic makes more sense. I will incorporate these change. 


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

My bad :( 
Done.


> > +// 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. 

Sorry what exactly is only Symbian you meant here? 
nmea simulation reads positions from a log file and simulates similar behavior of GPS positioning. This helps in testing geolocation on any platform. It would be good to have this in webkit code but not sure if this is right?



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

Will do that. 

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


More information about the webkit-unassigned mailing list