Bug 59199 – [Qt][WK2] Implement geolocation provider for qt port

• Component: WebKit2
• Status: RESOLVED FIXED

[Mahesh Kulkarni]

Implement WKGeolocationProvider support geolocation for Qt port of Webkit2.

[Mahesh Kulkarni]

Created attachment 92231 [details]
patch

Original Author: [e-mail]

Changes briefly,
-          Implements default location provider from QtWebkit side for WK2 (WebGeolocationProviderQt)
-          Allows QtWebkit clients to implement/override their own provider using WK2 API’s
-          Delaying instantiating qtMobility until request from JS

[Alexis Menard (darktears)]

Comment on attachment 92231 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92231&action=review

> Source/WebKit2/ChangeLog:11
> +        Allowing QtWebkit clients to implement/override their own provider using WK2 APIâs.

Typo.

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:32
> +    return reinterpret_cast<WebGeolocationProviderQt*>(const_cast<void*>(clientInfo));

Ouch. It's not very nice. How other ports are doing?

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:92
> +        m_source = QtMobility::QGeoPositionInfoSource::createDefaultSource(this);

Why you use the namespace of QtMobility?

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:57
> +    QtMobility::QGeoPositionInfoSource* m_source;

Same here.

[Benjamin Poulain]

Comment on attachment 92231 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92231&action=review

I don't know much about the subject but there already a few good comments so I r-
Some comments on my side:

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:2
> + * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

2011

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:30
> +static WebGeolocationProviderQt* toLocationProvider(const void* clientInfo)

This is a helper that do not execute any instruction, it could probably be set explicitely inline.

>> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:32
>> +    return reinterpret_cast<WebGeolocationProviderQt*>(const_cast<void*>(clientInfo));
> 
> Ouch. It's not very nice. How other ports are doing?

I have to agree that look strange. The geolocationManager is mutable, the clientInfo isn't, it is strange we only use clientInfo there.

Why not remove the constness from the definition of WKGeolocationProvider. This make sense to have it const for other port but not in this case?

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:51
> +    : QObject()

Unnecessary.

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:95
> +            // Mobility source is available.

I think this comment is not adding much.

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:2
> +    Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)

2011

> Source/WebKit2/UIProcess/qt/WebContextQt.cpp:56
> +    static WebGeolocationProviderQt* location = WebGeolocationProviderQt::create(toAPI(geolocationManagerProxy()));
> +    Q_UNUSED(location);

This can be made cleaner. You should not require a comment when you do a static initialization, the code should be clear enough by itself.

[Mahesh Kulkarni]

Created attachment 95628 [details]
patch

second attempt. Please review.

[Yael]

Comment on attachment 95628 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95628&action=review

I am not a reviewer, these are just my observations.

> Source/WebKit2/ChangeLog:8
> +       Original Author: [e-mail]

Please fix the indentation.

> Source/WebKit2/ChangeLog:11
> +        Allowing QtWebKit clients to implement/override their own provider using WK2 APIâs.

Please fix the typo.

> Source/WebKit2/UIProcess/API/C/WKGeolocationManager.h:38
> -typedef void (*WKGeolocationProviderStartUpdatingCallback)(WKGeolocationManagerRef geolocationManager, const void* clientInfo);
> -typedef void (*WKGeolocationProviderStopUpdatingCallback)(WKGeolocationManagerRef geolocationManager, const void* clientInfo);
> +typedef void (*WKGeolocationProviderStartUpdatingCallback)(WKGeolocationManagerRef geolocationManager, void* clientInfo);
> +typedef void (*WKGeolocationProviderStopUpdatingCallback)(WKGeolocationManagerRef geolocationManager, void* clientInfo);
>  

Wouldn't removing the const be a problem for other ports?

> Source/WebKit2/UIProcess/qt/WebContextQt.cpp:57
> +#if ENABLE(GEOLOCATION)
> +    static WebGeolocationProviderQt* location = WebGeolocationProviderQt::create(toAPI(geolocationManagerProxy()));
> +    Q_UNUSED(location);
> +#endif
>  }

This code is very confusing. What is the purpose of assigning the return value to location? And how does it get deleted?
I use use DEFINE_STATIC_LOCAL for it, and not pass it directly as clientInfo.

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:57
> +    WKRetainPtr<WKGeolocationPositionRef> m_lastPosition;

Can you use RefPtr? Qt is the only port to use WKRetainPtr, and it should probably go away.

[Mahesh Kulkarni]

Created attachment 99976 [details]
patch

Thanks Yael for review. Updated as per comments from Yael.


> > Source/WebKit2/UIProcess/API/C/WKGeolocationManager.h:38
> > -typedef void (*WKGeolocationProviderStartUpdatingCallback)(WKGeolocationManagerRef geolocationManager, const void* clientInfo);
> > -typedef void (*WKGeolocationProviderStopUpdatingCallback)(WKGeolocationManagerRef geolocationManager, const void* clientInfo);
> > +typedef void (*WKGeolocationProviderStartUpdatingCallback)(WKGeolocationManagerRef geolocationManager, void* clientInfo);
> > +typedef void (*WKGeolocationProviderStopUpdatingCallback)(WKGeolocationManagerRef geolocationManager, void* clientInfo);
> >  
> 
> Wouldn't removing the const be a problem for other ports?

No, because no other ports have geolocation provider implemented yet.

> > Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:57
> > +    WKRetainPtr<WKGeolocationPositionRef> m_lastPosition;
> 
> Can you use RefPtr? Qt is the only port to use WKRetainPtr, and it should probably go away.

The class isn't RefCounted yet. Shall take up this separately.

[WebKit Review Bot]

Attachment 99976 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/qt/WebContextQt.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mahesh Kulkarni]

Created attachment 99979 [details]
patch

fixed style issue

[Alexis Menard (darktears)]

Comment on attachment 99979 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99979&action=review

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:54
> +        0,      /* version */

I'm not sure those comments are following the WebKit code style.

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:108
> +    m_source->startUpdates();

This code is wrong and will crash in the case of QGeoPositionInfoSource::createDefaultSource(this); return 0. You will enter in the else and then call m_source->startUpdates(); and m_source is null. You should probably return in the else.

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:113
> +    m_source->stopUpdates();

Can this be called if m_source is 0?

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:34
> +// Below using is needed, because the implementation signal is defined without the namespace.

I fail to understand why you need this, especially in a header file.

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:51
> +    void positionUpdated(const QGeoPositionInfo&);

couldn't it be QtMobility::QGeoPositionInfo?

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:59
> +    QGeoPositionInfoSource* m_source;

Ditto.

[Adenilson Cavalcanti Silva]

Created attachment 110320 [details]
Fixed crash condition, mobility classes forward declarations, comments, inline function

[Kenneth Rohde Christiansen]

Comment on attachment 110320 [details]
Fixed crash condition, mobility classes forward declarations, comments, inline function

View in context: https://bugs.webkit.org/attachment.cgi?id=110320&action=review

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:55
> +    QtMobility::QGeoPositionInfoSource* m_source;

Shouldn't this be an OwnPtr?

[Alexis Menard (darktears)]

Comment on attachment 110320 [details]
Fixed crash condition, mobility classes forward declarations, comments, inline function

View in context: https://bugs.webkit.org/attachment.cgi?id=110320&action=review

This patch seems correct except for the fac that we use QtMobility as a dependency. For WK2 we are based on Qt5 and most of the mobility stuff is back in Qt. 

For example : http://doc.qt.nokia.com/qt5-snapshot/qgeolocation.html

So I guess we should start with those now.

>> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:55
>> +    QtMobility::QGeoPositionInfoSource* m_source;
> 
> Shouldn't this be an OwnPtr?

yep

[Adenilson Cavalcanti Silva]

Kenneth & Alexis
Thanks for the review, I will make the changes related to the OwnPtr.

Alexis
Concerning the dependency on Mobility, initially I thought about using the class in Qt5. But this will require a change on Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp (since it is using Mobility too).

So, what if we first address the issue of current bug and then open a new issue for refactoring both files to use qt5 class?

[Adenilson Cavalcanti Silva]

Maybe this bug could be used for the refactoring task:
https://bugs.webkit.org/show_bug.cgi?id=69737

[Igor Trindade Oliveira]

(In reply to comment #13)
> Kenneth & Alexis
> Thanks for the review, I will make the changes related to the OwnPtr.
> 
> Alexis
> Concerning the dependency on Mobility, initially I thought about using the class in Qt5. But this will require a change on Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp (since it is using Mobility too).
> 

Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp is using Qt 4.X so it should use Mobility.

> So, what if we first address the issue of current bug and then open a new issue for refactoring both files to use qt5 class?

[Mahesh Kulkarni]

(In reply to comment #13)
> Kenneth & Alexis
> Thanks for the review, I will make the changes related to the OwnPtr.
> 
> Alexis
> Concerning the dependency on Mobility, initially I thought about using the class in Qt5. But this will require a change on Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp (since it is using Mobility too).
> 
> So, what if we first address the issue of current bug and then open a new issue for refactoring both files to use qt5 class?

Thanks Adenilson for the patch. FYI http://gitorious.net/+mob-team/webkit/mob-team-qtwebkit2 is branch that I'm working on where most of these fixes are made. For example using Qt's Location instead of QtMobilities for Qt version more than 5.0 and crash fix.

[Mahesh Kulkarni]

Comment on attachment 99979 [details]
patch

Making my patch obsolete. Adenilson you can refer mob-team-qtwebkit2 for using qt5 location mobule instead if you like.

[Alexis Menard (darktears)]

(In reply to comment #15)
> (In reply to comment #13)
> > Kenneth & Alexis
> > Thanks for the review, I will make the changes related to the OwnPtr.
> > 
> > Alexis
> > Concerning the dependency on Mobility, initially I thought about using the class in Qt5. But this will require a change on Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp (since it is using Mobility too).

Thing is QtMobility is not tested against Qt5 and most probably will not work. So we need to use Qt's module when WK2 + Qt5 because that's the only supported code path.

> > 
> 
> Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp is using Qt 4.X so it should use Mobility.

It can work with both playing around with ifdef.

> 
> > So, what if we first address the issue of current bug and then open a new issue for refactoring both files to use qt5 class?

No this is the other way around. We don't land something that may be broken or we know doesn't make sense.

[Alexis Menard (darktears)]

(In reply to comment #18)
> (In reply to comment #15)
> > (In reply to comment #13)
> > > Kenneth & Alexis
> > > Thanks for the review, I will make the changes related to the OwnPtr.
> > > 
> > > Alexis
> > > Concerning the dependency on Mobility, initially I thought about using the class in Qt5. But this will require a change on Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp (since it is using Mobility too).
> 
> Thing is QtMobility is not tested against Qt5 and most probably will not work. So we need to use Qt's module when WK2 + Qt5 because that's the only supported code path.
> 
> > > 
> > 
> > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp is using Qt 4.X so it should use Mobility.
> 
> It can work with both playing around with ifdef.

Or we can add a file in the WebCoreSupport of the WebProcess to only have Qt5 support. (duplicate code for sure).

Though in the first place it seems wrong we use stuff in Source/WebKit/qt/WebCoreSupport for WebKit2 isn't it?

> 
> > 
> > > So, what if we first address the issue of current bug and then open a new issue for refactoring both files to use qt5 class?
> 
> No this is the other way around. We don't land something that may be broken or we know doesn't make sense.

[Alexis Menard (darktears)]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #15)
> > > (In reply to comment #13)
> > > > Kenneth & Alexis
> > > > Thanks for the review, I will make the changes related to the OwnPtr.
> > > > 
> > > > Alexis
> > > > Concerning the dependency on Mobility, initially I thought about using the class in Qt5. But this will require a change on Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp (since it is using Mobility too).
> > 
> > Thing is QtMobility is not tested against Qt5 and most probably will not work. So we need to use Qt's module when WK2 + Qt5 because that's the only supported code path.
> > 
> > > > 
> > > 
> > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp is using Qt 4.X so it should use Mobility.
> > 
> > It can work with both playing around with ifdef.
> 
> Or we can add a file in the WebCoreSupport of the WebProcess to only have Qt5 support. (duplicate code for sure).
> 
> Though in the first place it seems wrong we use stuff in Source/WebKit/qt/WebCoreSupport for WebKit2 isn't it?

Ok got it, it's the WK1 stuff that is built.

> 
> > 
> > > 
> > > > So, what if we first address the issue of current bug and then open a new issue for refactoring both files to use qt5 class?
> > 
> > No this is the other way around. We don't land something that may be broken or we know doesn't make sense.

[Mahesh Kulkarni]

(In reply to comment #20)
> (In reply t/qt/WebCoreSupport for WebKit2 isn't it?
> 
> Ok got it, it's the WK1 stuff that is built.
> 
> > 

Yes it is.

Copying my comment from: https://bugs.webkit.org/show_bug.cgi?id=69737

 Qt5 is made mandatory for trunk code which means that we do not have to keep mobility hooked up in trunk anymore and use qt5 location instead.

[Adenilson Cavalcanti Silva]

I will 'port' the patch to use Qt5, but meanwhile GeolocationClient (WK1 code) needs to compile with both Qt4 and Qt5.

The issue has the required patch: https://bugs.webkit.org/show_bug.cgi?id=69737

[Adenilson Cavalcanti Silva]

Created attachment 110568 [details]
Previous fixes plus using Qt5 location.

This patch depends on https://bugs.webkit.org/show_bug.cgi?id=69737

[Simon Hausmann]

Comment on attachment 110568 [details]
Previous fixes plus using Qt5 location.

View in context: https://bugs.webkit.org/attachment.cgi?id=110568&action=review

r- because of the const stuff. Otherwise the patch looks good to me.

> Source/WebKit2/ChangeLog:6
> +        Author: [e-mail]

If you want to, you could also write:

"Based on patch by Kenneth Christiansen <his email here>"

> Source/WebKit2/UIProcess/API/C/WKGeolocationManager.h:-43
> -typedef void (*WKGeolocationProviderStartUpdatingCallback)(WKGeolocationManagerRef geolocationManager, const void* clientInfo);
> -typedef void (*WKGeolocationProviderStopUpdatingCallback)(WKGeolocationManagerRef geolocationManager, const void* clientInfo);
> +typedef void (*WKGeolocationProviderStartUpdatingCallback)(WKGeolocationManagerRef geolocationManager, void* clientInfo);
> +typedef void (*WKGeolocationProviderStopUpdatingCallback)(WKGeolocationManagerRef geolocationManager, void* clientInfo);
>  
>  struct WKGeolocationProvider {
> -    int                                                                 version;
> -    const void *                                                        clientInfo;
> -    WKGeolocationProviderStartUpdatingCallback                          startUpdating;
> -    WKGeolocationProviderStopUpdatingCallback                           stopUpdating;

I do think that removing the const is a problem here. I bet Safari 5 is using this API to implement gelocation. Also the const - as ugly as it is - is the pattern used in all of the public WK2 C API.

[Adenilson Cavalcanti Silva]

Created attachment 111059 [details]
Setting to mutable the GeoPositionSource class member (thus respecting WKGeolocationManager const pointers)

[Andreas Kling]

Comment on attachment 111059 [details]
Setting to mutable the GeoPositionSource class member (thus respecting WKGeolocationManager const pointers)

View in context: https://bugs.webkit.org/attachment.cgi?id=111059&action=review

> Source/WebKit2/ChangeLog:6
> +        Reviewed by Simon Hausmann <[e-mail]>

This line needs to be omitted until the patch gets a proper r+.

> Source/WebKit2/ChangeLog:12
> +
> +

One newline is enough here.

[Adenilson Cavalcanti Silva]

Created attachment 111065 [details]
Setting to mutable the GeoPositionSource class member (thus respecting WKGeolocationManager const pointers), fixed new line in Changelog

[Kenneth Rohde Christiansen]

Comment on attachment 111065 [details]
Setting to mutable the GeoPositionSource class member (thus respecting WKGeolocationManager const pointers), fixed new line in Changelog

View in context: https://bugs.webkit.org/attachment.cgi?id=111065&action=review

> Source/WebKit2/ChangeLog:8
> +        Based on patch by Kenneth Christiansen <[e-mail]>

Add newline after this

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:32
> +    const WebGeolocationProviderQt *tmp = static_cast<const WebGeolocationProviderQt* >(clientInfo);

* alignment! also an extra space after the Qt* I don't see why this tmp is needed if you are not asserting! also tmp is a bad name

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.cpp:97
> +        if (!(m_source = QGeoPositionInfoSource::createDefaultSource(const_cast<WebGeolocationProviderQt* >(this)))) {

Space after * again

> Source/WebKit2/UIProcess/qt/WebGeolocationProviderQt.h:38
> +    ~WebGeolocationProviderQt();

virtual?

[Adenilson Cavalcanti Silva]

Created attachment 111134 [details]
Previous fixes, plus virtual destructor/no tmp variable

[Adenilson Cavalcanti Silva]

Created attachment 111272 [details]
Previous fixes, plus removed an unnecessary header file.

[WebKit Review Bot]

Attachment 111272 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Hausmann]

Comment on attachment 111272 [details]
Previous fixes, plus removed an unnecessary header file.

r=me

[WebKit Review Bot]

Comment on attachment 111272 [details]
Previous fixes, plus removed an unnecessary header file.

Clearing flags on attachment: 111272

Committed r97733: <http://trac.webkit.org/changeset/97733>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.