[Qt] navigator.geolocation support for qt port

• Component: Platform
• Status: RESOLVED FIXED

[Mahesh Kulkarni]

Porting navigator.geolocation using QtMobility location service. 

http://dev.w3.org/geo/api/spec-source.html

ps: proposed changes to implement this to follow after this

[Simon Hausmann]

This is also tracked by http://bugreports.qt.nokia.com/browse/QTWEBKIT-115

[Kenneth Rohde Christiansen]

Not QuickTime related :-) Changing to Qt

[Mahesh Kulkarni]

Created attachment 57751 [details]
proposed solution

Proposed fix,

1) uses qtmobility location service
2) qtmobility location service only support GPS tracking
3) QtgeolocationServicePrivate.h is introduced to hack around qtmobility incomplete signal's signature using QtMobility namespace. This would be deleted once fix is available 
4) Geolocation for  will be enabled if mobility.prf is available in QTMAKESPEC path. Any other way to check specific to location service? 

Please review.

[Mahesh Kulkarni]

Please note that I'm still doing layout/complete testing on symbian/maemo. Patch attached is to get first hand comments. 

Adding Laszlo, Yael to CC list.

[Laszlo Gombos]

Comment on attachment 57751 [details]
proposed solution

> Index: JavaScriptCore/wtf/Platform.h
> ===================================================================
> --- JavaScriptCore/wtf/Platform.h	(revision 60618)
> +++ JavaScriptCore/wtf/Platform.h	(working copy)
> @@ -616,6 +616,10 @@
>  #define WTF_PLATFORM_CF 1
>  #endif
>  
> +#if PLATFORM(QT)
> +#define ENABLE_GEOLOCATION 1
> +#endif
> +

This change enables GEOLOCATION unconditionally for QtWebKit. This might be a leftover from development, I do not think we should have this change committed to trunk.

[Kenneth Rohde Christiansen]

It should be nice if this actually compiled on the Qt bot :-)

[Kenneth Rohde Christiansen]

Comment on attachment 57751 [details]
proposed solution

WebCore/platform/qt/GeolocationServiceQt.cpp:43
 +      :m_parent(parent)
need a space after :

WebCore/platform/qt/GeolocationServiceQt.cpp:53
 +      if (gpsPos.isValid()) {
Would be better to say:

if (!gpsPos.isValid()) {
    ...
    return;
}

Then you avoid all that identation

WebCore/platform/qt/GeolocationServiceQt.cpp:61
 +                              gpsPos.attribute(QGeoPositionInfo::HorizontalAccuracy):0.0;
wrong coding style, please check the coding style guide

WebCore/platform/qt/GeolocationServiceQt.cpp:64
 +          double altitudeAccuracy = providesAltitudeAccuracy?gpsPos.attribute(QGeoPositionInfo::VerticalAccuracy):0.0;
please add spaces around the ? and the :

WebCore/platform/qt/GeolocationServiceQt.cpp:67
 +          double heading = providesHeading?gpsPos.attribute(QGeoPositionInfo::Direction):0.0;
here as well

WebCore/platform/qt/GeolocationServiceQt.cpp:70
 +          double speed = providesSpeed?gpsPos.attribute(QGeoPositionInfo::GroundSpeed):0.0;
and here

WebCore/platform/qt/GeolocationServiceQt.cpp:92
 +      m_location = QGeoPositionInfoSource::createDefaultSource(m_p);
m_p is a terrible variable name, in Qt we ALWAYS use d for the private, but then again when implementing WebCore classes we never use privates as there is no binary compatibility promise. Why do you need the private at all?

WebCore/platform/qt/GeolocationServiceQt.cpp:96
 +                  m_p, SLOT(positionUpdated(QGeoPositionInfo)));
The inner of the if spans two lines (one logically) so it needs braces. Again please read teh coding style guide.

WebCore/platform/qt/GeolocationServiceQt.cpp:98
 +          m_lastError = PositionError::create(PositionError::POSITION_UNAVAILABLE, "GPS device not found");
How do we deal with translation here? Simon?

WebCore/platform/qt/GeolocationServiceQtPrivate.h:43
 +  class GeolocationServiceQtPrivate : public QObject {
Given the comment, the name *Private is probably not the best. You need to come up with a better name

WebCore/platform/qt/GeolocationServiceQtPrivate.h:48
 +  public slots:
please add a new line before 'public slots'. Also you cannot use 'slots' as the user might use Boost, thus please use 'public Q_SLOTS' instead

WebCore/platform/qt/GeolocationServiceQtPrivate.h:50
 +      void positionUpdated(const QGeoPositionInfo &);
no space needed before &

[Laszlo Gombos]

According to http://doc.qt.nokia.com/qtmobility-1.0/index.html#platform-compatability Location API is supported (as a tier 2 platform) on desktop platforms as well. 

Ossy what do you think about installing these modules on the buildbot (Linux and Win) ?

[Mahesh Kulkarni]

Created attachment 57846 [details]
proposed solution

(In reply to comment #7)

Thanks Kenneth and Laszlo for your review comments. 


> (From update of attachment 57751 [details])
> WebCore/platform/qt/GeolocationServiceQt.cpp:43
>  +      :m_parent(parent)
> need a space after :
> 
> WebCore/platform/qt/GeolocationServiceQt.cpp:53
>  +      if (gpsPos.isValid()) {
> Would be better to say:
> 
> if (!gpsPos.isValid()) {
>     ...
>     return;
> }
> 
> Then you avoid all that identation

Done

> 
> WebCore/platform/qt/GeolocationServiceQt.cpp:61
>  +                              gpsPos.attribute(QGeoPositionInfo::HorizontalAccuracy):0.0;
> wrong coding style, please check the coding style guide
> 
> WebCore/platform/qt/GeolocationServiceQt.cpp:64
>  +          double altitudeAccuracy = providesAltitudeAccuracy?gpsPos.attribute(QGeoPositionInfo::VerticalAccuracy):0.0;
> please add spaces around the ? and the :
> 
> WebCore/platform/qt/GeolocationServiceQt.cpp:67
>  +          double heading = providesHeading?gpsPos.attribute(QGeoPositionInfo::Direction):0.0;
> here as well
> 
> WebCore/platform/qt/GeolocationServiceQt.cpp:70
>  +          double speed = providesSpeed?gpsPos.attribute(QGeoPositionInfo::GroundSpeed):0.0;
> and here
> 

Done


> WebCore/platform/qt/GeolocationServiceQt.cpp:92
>  +      m_location = QGeoPositionInfoSource::createDefaultSource(m_p);
> m_p is a terrible variable name, in Qt we ALWAYS use d for the private, but then again when implementing WebCore classes we never use privates as there is no binary compatibility promise. Why do you need the private at all?
> 

this member is removed, read further for reason

> WebCore/platform/qt/GeolocationServiceQt.cpp:96
>  +                  m_p, SLOT(positionUpdated(QGeoPositionInfo)));
> The inner of the if spans two lines (one logically) so it needs braces. Again please read teh coding style guide.
> 

Done.

> WebCore/platform/qt/GeolocationServiceQt.cpp:98
>  +          m_lastError = PositionError::create(PositionError::POSITION_UNAVAILABLE, "GPS device not found");
> How do we deal with translation here? Simon?

I just introduced a tr() to it for now. Actually geolocation common code takes care of all error messages given back to JS, m_lasterror is filled with this error message only for a special use case where updatePostion is called with a invalid GeoPostion value.

> 
> WebCore/platform/qt/GeolocationServiceQtPrivate.h:43
>  +  class GeolocationServiceQtPrivate : public QObject {
> Given the comment, the name *Private is probably not the best. You need to come up with a better name
> 
> WebCore/platform/qt/GeolocationServiceQtPrivate.h:48
>  +  public slots:
> please add a new line before 'public slots'. Also you cannot use 'slots' as the user might use Boost, thus please use 'public Q_SLOTS' instead
> 
> WebCore/platform/qt/GeolocationServiceQtPrivate.h:50
>  +      void positionUpdated(const QGeoPositionInfo &);
> no space needed before &

private class is just to define a slot which connects to qtmobility signal. Because the signal is not defined with QtMobility (not fully qualified signal) I need to use "using namespace" in header file as work around until mobility fixes the issue. Patch has been sent for the same to mobility team as well. 

New patch has no GeoLocationQtServicePrivate.h file. geolocationserviceQt.h it self has "using namespace" instead. Which will be reverted as per webkit coding guidelines once the mobility fix is available.

[Csaba Osztrogonác]

(In reply to comment #8)
> According to http://doc.qt.nokia.com/qtmobility-1.0/index.html#platform-compatability Location API is supported (as a tier 2 platform) on desktop platforms as well. 
> 
> Ossy what do you think about installing these modules on the buildbot (Linux and Win) ?

Good idea. I played with it, but I have some build problem. I can continue it next week.

[Simon Hausmann]

What about using WebCore/platform/mock/GeolocationServiceMock.cpp for the layout testing?

[Simon Hausmann]

Comment on attachment 57846 [details]
proposed solution

WebCore/WebCore.pri:113
 +      exists($$[QMAKE_MKSPECS]/features/mobility.prf) {
I think we should fix the mobility detection separately first. The last comment in bug #39357 points out a new way of detecting the availability of Qt Mobility and its individual modules.

[Laszlo Gombos]

Comment on attachment 57846 [details]
proposed solution

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 60655)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,27 @@
> [phone]  Mahesh Kulkarni  <[e-mail]>
> +
> +        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.

[Mahesh Kulkarni]

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.

[Csaba Osztrogonác]

Qt Mobility API 1.0.0 is installed on "Qt Linux Release", 
"Qt Windows 32-bit Release", "Qt Windows 32-bit Debug"
buildbots, feel free to experiment with this API.

[Simon Hausmann]

(In reply to comment #15)
> Qt Mobility API 1.0.0 is installed on "Qt Linux Release", 
> "Qt Windows 32-bit Release", "Qt Windows 32-bit Debug"
> buildbots, feel free to experiment with this API.

For the detection of QtMobility - load(mobilityconfig) - to work, QtMobility 1.0.1 is needed :-/

[Mahesh Kulkarni]

Created attachment 58367 [details]
solution with layout testing

This patch,

1) Takes care all of comments from Laszlo in prev comment
2) Enables layout cases for geolocation for all qt platform
3) Implementation for geolocation cases under DumpRenderTreeQt
4) One case is failing and it is skipped for now (will raise a different bug and analyze) 
5) Does *not* include symbian def files which will follow in a separate patch

[Csaba Osztrogonác]

> For the detection of QtMobility - load(mobilityconfig) - to work, QtMobility 1.0.1 is needed :-/

:-/ I asked Laszlo if 1.0.0 is good for us ...

When I asked, I can't build the Qt Mobility trunk,
only 1.0.0. And I haven't find 1.0.1 release anywhere.

Fortunately build problem was fixed, and now the
trunk of Qt Mobility API is buildable.

Will 2e9e10ec43b2ca22823ab162ba8a52d3ef9da106 good for us?

[Laszlo Gombos]

Comment on attachment 58367 [details]
solution with layout testing

> +## load mobilityconfig if mobility is available 
> +load(mobilityconfig, true)

I nit - I think we should test to make sure that the mobility.prf exists before loading it ? 

Patch otherwise looks good to me.

Simon, Kenneth - are you in agreement with the API proposed  ?

[Laszlo Gombos]

(In reply to comment #19)
> (From update of attachment 58367 [details])
> > +## load mobilityconfig if mobility is available 
> > +load(mobilityconfig, true)
> 
> I nit - I think we should test to make sure that the mobility.prf exists before loading it ? 

From bug 39357

A better fix is to replace

load(mobilityconfig)

with

load(mobilityconfig,true)

where the second argument (true) means: ignore errors when loading

I will mark https://bug-39357-attachments.webkit.org/attachment.cgi?id=58142 obsolete as this patch should take care of this as well.

[Laszlo Gombos]

(In reply to comment #18)
> > For the detection of QtMobility - load(mobilityconfig) - to work, QtMobility 1.0.1 is needed :-/
> 
> :-/ I asked Laszlo if 1.0.0 is good for us ...
> 
> When I asked, I can't build the Qt Mobility trunk,
> only 1.0.0. And I haven't find 1.0.1 release anywhere.
> 
> Fortunately build problem was fixed, and now the
> trunk of Qt Mobility API is buildable.
> 
> Will 2e9e10ec43b2ca22823ab162ba8a52d3ef9da106 good for us?

On IRC we agreed to pursue 1.0.0 for now until there is another official QtMobility release.

<lgombos> Ossy: I was going to ask you if you have Qt 4.6 or 4.7 on the bot ? 
<Ossy> lgombos, on "Qt Linux Release" 4.6.2
<Ossy> lgombos, but we have a Qt-4.7.0 bot on webkit.sed.hu
<lgombos> Ossy: I see - so so far "official" bots are running official Qt releases
<Ossy> lgombos, yes
<lgombos> Ossy: I'm a bit nervous adding unofficial Qt snapshot to the "official" bots
<Ossy> lgombos, agree
<lgombos> Ossy: instead what i was thinking of is to use 1.0 QtMobility release but when we build webkit the bot would force geolocation - e.g. build-webkit --geolocation
<lgombos> Ossy: do you think this is a good idea ? 
<Ossy> lgombos, I think we can do it with BUILD_WEBKIT_ARGS, I'll try if it is buildable with --geolocation option

[Csaba Osztrogonác]

"solution with layout testing"

I tested this patch on the bot with GeoLocation 
enabled, it works without any regression.

After the patch landed I will add WEBKIT_BUILD_ARGS=--geolocation
environment variable to the bot to force enabling GeoLocation.

[Laszlo Gombos]

Mahesh, what do you think about moving allowGeolocationRequest to QWebFrame instead of QWebPage ?

[Simon Hausmann]

(In reply to comment #23)
> Mahesh, what do you think about moving allowGeolocationRequest to QWebFrame instead of QWebPage ?

Since QWebFrame cannot be subclassed, I think QWebPage is the right location.

However we're getting more and more of these kinds of callbacks and I'd really like to find a nicer solution there. But let that be our worry, I don't want to stall this great patch because of that :)

[Laszlo Gombos]

Comment on attachment 58367 [details]
solution with layout testing

API is agreed, so this looks good to me, r+.

Marking cq- as buildbot needs to enable geolocation in order to pass the LayoutTests.

[Laszlo Gombos]

Comment on attachment 58367 [details]
solution with layout testing

Ossy promised to land and adjust the bot to enable Geolocation at the same time.

[Csaba Osztrogonác]

(In reply to comment #26)
> (From update of attachment 58367 [details])
> Ossy promised to land and adjust the bot to enable Geolocation at the same time.

I landed the patch manually in http://trac.webkit.org/changeset/61117.
(Before it I resolved two trivial conflicts. And fixed qt and QT typos
in changelogs to Qt.)

[Mahesh Kulkarni]

Thanks a lot Laszlo, Simon, Kenneth and Csaba! 

Special thanks to Jayakishore Thunga for the equal contribution with the patch!

[Yael]

I think the API that was defined for the permission is inadequate. Other browsers display a bar at the top and if/when the user wants, he will accept/reject/ignore the question. The API that this patch introduces does not allow the user to ignore the question.

[Laszlo Gombos]

(In reply to comment #29)
> I think the API that was defined for the permission is inadequate. Other browsers display a bar at the top and if/when the user wants, he will accept/reject/ignore the question. The API that this patch introduces does not allow the user to ignore the question.

I agree - opened a new bug for it - https://bugs.webkit.org/show_bug.cgi?id=41364.

[Mahesh Kulkarni]

(In reply to comment #30)
> (In reply to comment #29)
> > I think the API that was defined for the permission is inadequate. Other browsers display a bar at the top and if/when the user wants, he will accept/reject/ignore the question. The API that this patch introduces does not allow the user to ignore the question.
> 
> I agree - opened a new bug for it - https://bugs.webkit.org/show_bug.cgi?id=41364.

Yes async for geolocation request API is required and also need to add  ChromeClient::cancelGeolocationPermissionRequestForFrame to cancel the same.

[Yael]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > I think the API that was defined for the permission is inadequate. Other browsers display a bar at the top and if/when the user wants, he will accept/reject/ignore the question. The API that this patch introduces does not allow the user to ignore the question.
> > 
> > I agree - opened a new bug for it - https://bugs.webkit.org/show_bug.cgi?id=41364.
> 
> Yes async for geolocation request API is required and also need to add  ChromeClient::cancelGeolocationPermissionRequestForFrame to cancel the same.

Looking at the Chromium implementation of GeoLocation, they ask the user for permission before starting the GeoLocation service, while the QtWebKit implementation asks the user for permission only after it receives an update from the service. Should we change the behavior in GeoLocationServiceQt.cpp as well?