Implement WKGeolocationProvider support geolocation for Qt port of Webkit2.
Created attachment 92231 [details] patch Original Author: kenneth.r.christiansen@nokia.com 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
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.
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.
Created attachment 95628 [details] patch second attempt. Please review.
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: kenneth.r.christiansen@nokia.com 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.
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.
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.
Created attachment 99979 [details] patch fixed style issue
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.
Created attachment 110320 [details] Fixed crash condition, mobility classes forward declarations, comments, inline function
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?
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
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?
Maybe this bug could be used for the refactoring task: https://bugs.webkit.org/show_bug.cgi?id=69737
(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?
(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.
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.
(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.
(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.
(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.
(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.
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
Created attachment 110568 [details] Previous fixes plus using Qt5 location. This patch depends on https://bugs.webkit.org/show_bug.cgi?id=69737
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: kenneth.r.christiansen@nokia.com 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.
Created attachment 111059 [details] Setting to mutable the GeoPositionSource class member (thus respecting WKGeolocationManager const pointers)
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 <hausmann@webkit.org> This line needs to be omitted until the patch gets a proper r+. > Source/WebKit2/ChangeLog:12 > + > + One newline is enough here.
Created attachment 111065 [details] Setting to mutable the GeoPositionSource class member (thus respecting WKGeolocationManager const pointers), fixed new line in Changelog
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 <kenneth.r.christiansen@nokia.com> 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?
Created attachment 111134 [details] Previous fixes, plus virtual destructor/no tmp variable
Created attachment 111272 [details] Previous fixes, plus removed an unnecessary header file.
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.
Comment on attachment 111272 [details] Previous fixes, plus removed an unnecessary header file. r=me
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>
All reviewed patches have been landed. Closing bug.