Bug 42629

Working on the patch.

Created attachment 81688 [details] first-look patch Need a first-hand review of the patch attached. Patch implements, 1) Enables ENABLE_CLIENT_BASED_GEOLOCATION for QtWebkit 2) Qt's WebCore::GeolocationClient class GeolocationClientQt and assigns to WebCore::PageClient for Geolocation instance. 3) Code to initialize and listening to QtMobility's geolocation service code is moved from Source/WebCore/platform/qt/GeolocationServiceQt.cpp to Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp 4) Permission API's are moved to WebCore::GeolocationClient so the QtWebkit's GeolocationPermissionClientQt calls moved from ChromeClientQt.cpp to GeolocationClientQt.cpp What patch doesnt implement/planned to implement, (Raise a new bug probably and mark it blocked to this bug? to keep the patch simple/easy to review) 1) LayoutTest implementation for Client-Based geolocation. Need to implement new Mock-Client-based test classes. Disabled Geolocation cases in Skipped file till then. 2) Cleaning up DRT Qt code related to old geolocation test framework. 3) raise a separate bug to remove/clean up GeolocationPermissionClientQt.cpp/.h code and move the logic to GeolocationClientQt.cpp itself. 4) page/ChromeClient.h functions, requestGeolocationPermissionForFrame and cancelGeolocationPermissionRequestForFrame can be disabled/put under !non-client implementation for more readability. Will raise a separate bug for this as well. Please provide your first-hand review comments before I can submit the final patch before above changes. (If chosen not to worry about patch size :)

Comment on attachment 81688 [details] first-look patch View in context: https://bugs.webkit.org/attachment.cgi?id=81688&action=review > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:89 > + m_lastPosition = GeolocationPosition::create(geoPosition.timestamp().toTime_t(), latitude, longitude, It would be great to extra resolution for the timestamp, as the DOM type, Geoposition, provides the millisecond resolution. I'm sure that there will be at least one application developer that will thank you for it! Perhaps something like: double timestamp = geoPosition.timestamp().toTime_t() + geoPosition.timestamp().time().msec() / 1000.0; > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:113 > + WebCore::Page* page = QWebPagePrivate::core(m_page); It appears that it is not possible for m_location to be NULL when startUpdating is called, and non-null at stopUpdating. Since m_location is checked for NULL in startUpdating, I think it is unnecesary to check it again here. Note that for watches, sending the error here will result in two position unavailable error messages being sent for the same cause (once in startUpdating and once in stopUpdating). I believe this is valid behaviour, but it seems like unnecessary work. > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:130 > + if (!geolocation) Geolocation delegates to GeolocationController for requestPermission and cancelPermissionRequest, so geolocation should never be NULL. It may be better to change this to an assertion to catch incorrect usage in future. > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:139 > + if (!geolocation) Same comment as for requestPermission.

Hi Mahesh, Thanks so much for tackling this work! I hope you don't mind, I did an informal review for you above. Cheers John (In reply to comment #3) > (From update of attachment 81688 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81688&action=review > > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:89 > > + m_lastPosition = GeolocationPosition::create(geoPosition.timestamp().toTime_t(), latitude, longitude, > > It would be great to extra resolution for the timestamp, as the DOM type, Geoposition, provides the millisecond resolution. I'm sure that there will be at least one application developer that will thank you for it! > > Perhaps something like: > double timestamp = geoPosition.timestamp().toTime_t() + geoPosition.timestamp().time().msec() / 1000.0; > > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:113 > > + WebCore::Page* page = QWebPagePrivate::core(m_page); > > It appears that it is not possible for m_location to be NULL when startUpdating is called, and non-null at stopUpdating. Since m_location is checked for NULL in startUpdating, I think it is unnecesary to check it again here. > > Note that for watches, sending the error here will result in two position unavailable error messages being sent for the same cause (once in startUpdating and once in stopUpdating). I believe this is valid behaviour, but it seems like unnecessary work. > > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:130 > > + if (!geolocation) > > Geolocation delegates to GeolocationController for requestPermission and cancelPermissionRequest, so geolocation should never be NULL. It may be better to change this to an assertion to catch incorrect usage in future. > > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:139 > > + if (!geolocation) > > Same comment as for requestPermission.

Thanks for the review John! You are spot on with the comments and I'm updating all of them now. My bad with timestamp tobe miliseconds change because it is already present in trunk code with non-client based implementation and looks like I merged older code :( Updating patch. (In reply to comment #3) > (From update of attachment 81688 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81688&action=review > > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:89 > > + m_lastPosition = GeolocationPosition::create(geoPosition.timestamp().toTime_t(), latitude, longitude, > > It would be great to extra resolution for the timestamp, as the DOM type, Geoposition, provides the millisecond resolution. I'm sure that there will be at least one application developer that will thank you for it! > > Perhaps something like: > double timestamp = geoPosition.timestamp().toTime_t() + geoPosition.timestamp().time().msec() / 1000.0; > > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:113 > > + WebCore::Page* page = QWebPagePrivate::core(m_page); > > It appears that it is not possible for m_location to be NULL when startUpdating is called, and non-null at stopUpdating. Since m_location is checked for NULL in startUpdating, I think it is unnecesary to check it again here. > > Note that for watches, sending the error here will result in two position unavailable error messages being sent for the same cause (once in startUpdating and once in stopUpdating). I believe this is valid behaviour, but it seems like unnecessary work. > > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:130 > > + if (!geolocation) > > Geolocation delegates to GeolocationController for requestPermission and cancelPermissionRequest, so geolocation should never be NULL. It may be better to change this to an assertion to catch incorrect usage in future. > > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:139 > > + if (!geolocation) > > Same comment as for requestPermission.

However, (In reply to comment #3) > (From update of attachment 81688 [details]) > It appears that it is not possible for m_location to be NULL when startUpdating is called, and non-null at stopUpdating. Since m_location is checked for NULL in startUpdating, I think it is unnecesary to check it again here. > > Note that for watches, sending the error here will result in two position unavailable error messages being sent for the same cause (once in startUpdating and once in stopUpdating). I believe this is valid behaviour, but it seems like unnecessary work. > m_location can result in NULL when QtMobility see no source of GPS on given device. For example on my linux desktop machine! So it is required to check for NULL both in startUpdate and stopUpdate as both gets called for either watch or single shot request (incase of page leave).

Created attachment 81993 [details] updated patch Updated patch as per comment #3. From previous patch, ----------------------------- Patch implements, 1) Enables ENABLE_CLIENT_BASED_GEOLOCATION for QtWebkit 2) Qt's WebCore::GeolocationClient class GeolocationClientQt and assigns to WebCore::PageClient for Geolocation instance. 3) Code to initialize and listening to QtMobility's geolocation service code is moved from Source/WebCore/platform/qt/GeolocationServiceQt.cpp to Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp 4) Permission API's are moved to WebCore::GeolocationClient so the QtWebkit's GeolocationPermissionClientQt calls moved from ChromeClientQt.cpp to GeolocationClientQt.cpp What patch doesnt implement/planned to implement, (Raise a new bug probably and mark it blocked to this bug? to keep the patch simple/easy to review) 1) LayoutTest implementation for Client-Based geolocation. Need to implement new Mock-Client-based test classes. Disabled Geolocation cases in Skipped file till then. 2) Cleaning up DRT Qt code related to old geolocation test framework. 3) raise a separate bug to remove/clean up GeolocationPermissionClientQt.cpp/.h code and move the logic to GeolocationClientQt.cpp itself. 4) page/ChromeClient.h functions, requestGeolocationPermissionForFrame and cancelGeolocationPermissionRequestForFrame can be disabled/put under !non-client implementation for more readability. Will raise a separate bug for this as well. Please provide your first-hand review comments before I can submit the final patch before above changes. (If chosen not to worry about patch size :) --------------------------------

Adding Kenneth since he's been poking at this kind of stuff lately.

Comment on attachment 81993 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=81993&action=review > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:90 > + double timestamp = geoPosition.timestamp().toMSecsSinceEpoch(); Sorry for the previous confusing comment. This timestamp should be in seconds (not milliseconds) -- see Geolocation.cpp line 61 createGeoposition(GeolocationPosition *position) Suggest: double timestamp = geoPosition.timestamp().toMSecsSinceEpoch() / 1000.0; > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:93 > + double timestamp = (datetime.toTime_t() * 1000.00) + datetime.time().msec(); Same here. Suggest: double timestamp = datetime.toTime_t() + datetime.time().msec() / 1000.0;

(In reply to comment #9) > (From update of attachment 81993 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81993&action=review > > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:90 > > + double timestamp = geoPosition.timestamp().toMSecsSinceEpoch(); > > Sorry for the previous confusing comment. This timestamp should be in seconds (not milliseconds) -- see Geolocation.cpp line 61 createGeoposition(GeolocationPosition *position) > Suggest: > double timestamp = geoPosition.timestamp().toMSecsSinceEpoch() / 1000.0; > > > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:93 > > + double timestamp = (datetime.toTime_t() * 1000.00) + datetime.time().msec(); > > Same here. Suggest: > double timestamp = datetime.toTime_t() + datetime.time().msec() / 1000.0; This change was made sometime back in QtWebkit because spec says so. According to HTML5 GeoLocation standards, (Geo)Position interface’s timestamp should be of DOMTimestamp type and DOMTimestamp represents a number of milliseconds. Ref: http://dev.w3.org/geo/api/spec-source.html#position_interface http://www.w3.org/TR/DOM-Level-3-Core/core.html#Core-DOMTimeStamp More info: https://bugs.webkit.org/show_bug.cgi?id=51100

Comment on attachment 81993 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=81993&action=review >>> Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:90 >>> + double timestamp = geoPosition.timestamp().toMSecsSinceEpoch(); >> >> Sorry for the previous confusing comment. This timestamp should be in seconds (not milliseconds) -- see Geolocation.cpp line 61 createGeoposition(GeolocationPosition *position) >> Suggest: >> double timestamp = geoPosition.timestamp().toMSecsSinceEpoch() / 1000.0; > > This change was made sometime back in QtWebkit because spec says so. > > According to HTML5 GeoLocation standards, (Geo)Position interface’s timestamp should be of DOMTimestamp type and DOMTimestamp represents a number of milliseconds. > > Ref: http://dev.w3.org/geo/api/spec-source.html#position_interface > http://www.w3.org/TR/DOM-Level-3-Core/core.html#Core-DOMTimeStamp > > More info: https://bugs.webkit.org/show_bug.cgi?id=51100 Agreed. But notice that GeolocationPosition is a different class to Geoposition. GeolocationPosition expects seconds and will convert it to a DOMTimeStamp in milliseconds for the Geoposition (DOM) class.

(In reply to comment #11) > (From update of attachment 81993 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81993&action=review > > >>> Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:90 > >>> + double timestamp = geoPosition.timestamp().toMSecsSinceEpoch(); > >> > >> Sorry for the previous confusing comment. This timestamp should be in seconds (not milliseconds) -- see Geolocation.cpp line 61 createGeoposition(GeolocationPosition *position) > >> Suggest: > >> double timestamp = geoPosition.timestamp().toMSecsSinceEpoch() / 1000.0; > > > > This change was made sometime back in QtWebkit because spec says so. > > > > According to HTML5 GeoLocation standards, (Geo)Position interface’s timestamp should be of DOMTimestamp type and DOMTimestamp represents a number of milliseconds. > > > > Ref: http://dev.w3.org/geo/api/spec-source.html#position_interface > > http://www.w3.org/TR/DOM-Level-3-Core/core.html#Core-DOMTimeStamp > > > > More info: https://bugs.webkit.org/show_bug.cgi?id=51100 > > Agreed. But notice that GeolocationPosition is a different class to Geoposition. GeolocationPosition expects seconds and will convert it to a DOMTimeStamp in milliseconds for the Geoposition (DOM) class. Ah right. That change has to be cherry picked to our release branch! Thanks for the info.

(In reply to comment #11) > (From update of attachment 81993 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81993&action=review > > >>> Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:90 > >>> + double timestamp = geoPosition.timestamp().toMSecsSinceEpoch(); > >> > >> Sorry for the previous confusing comment. This timestamp should be in seconds (not milliseconds) -- see Geolocation.cpp line 61 createGeoposition(GeolocationPosition *position) > >> Suggest: > >> double timestamp = geoPosition.timestamp().toMSecsSinceEpoch() / 1000.0; > > > > This change was made sometime back in QtWebkit because spec says so. > > > > According to HTML5 GeoLocation standards, (Geo)Position interface’s timestamp should be of DOMTimestamp type and DOMTimestamp represents a number of milliseconds. > > > > Ref: http://dev.w3.org/geo/api/spec-source.html#position_interface > > http://www.w3.org/TR/DOM-Level-3-Core/core.html#Core-DOMTimeStamp > > > > More info: https://bugs.webkit.org/show_bug.cgi?id=51100 > > Agreed. But notice that GeolocationPosition is a different class to Geoposition. GeolocationPosition expects seconds and will convert it to a DOMTimeStamp in milliseconds for the Geoposition (DOM) class. John, I looked at the changes you suggested. Don't you think we will miss out on the milliseconds details (by rounding off the time by 1000) specially when we get the exact time details from location source? Atleast in case of QtMobility we do get exact time details and same details we would endup not giving to javascript developer. If this is true for most of ports we can create a different bug fix this with a dedicated test case?

(In reply to comment #13) > > John, I looked at the changes you suggested. Don't you think we will miss out on the milliseconds details (by rounding off the time by 1000) specially when we get the exact time details from location source? Atleast in case of QtMobility we do get exact time details and same details we would endup not giving to javascript developer. If this is true for most of ports we can create a different bug fix this with a dedicated test case? Mahesh, I don't believe details will effectively be lost. The timestamp is divided by a double (1000.0) and stored as a double, and then convertSecondsToDOMTimeStamp is multiplying by the same (1000.0). If you're worried about losing floating point precision in hardware rounding off the double, then I suppose that is a warranted concern...but effectively it's only going to lose +/- 1 millisecond if ever.

I agree with Jarred, I think that any floating point error introduced here will be very minor indeed. However, it could be an argument for changing GeolocationPosition to storing time as an integer number of in milliseconds, to match Geoposition. I think that should be a separate bug though. > Mahesh, I don't believe details will effectively be lost. The timestamp is divided by a double (1000.0) and stored as a double, and then convertSecondsToDOMTimeStamp is multiplying by the same (1000.0). If you're worried about losing floating point precision in hardware rounding off the double, then I suppose that is a warranted concern...but effectively it's only going to lose +/- 1 millisecond if ever.

(In reply to comment #15) > However, it could be an argument for changing GeolocationPosition to storing time as an integer number of in milliseconds, to match Geoposition. I think that should be a separate bug though. +1 for that match up. It would make sense to store in milliseconds so that it is an integer and so that it's the "lowest common denominator" and no chance of precision loss. Not to mention, it's more consistent and understandable given that the DOM spec is talking about milliseconds. We won't have to worry while coding if we're in seconds or milliseconds.

(In reply to comment #16) > (In reply to comment #15) > > However, it could be an argument for changing GeolocationPosition to storing time as an integer number of in milliseconds, to match Geoposition. I think that should be a separate bug though. > > +1 for that match up. It would make sense to store in milliseconds so that it is an integer and so that it's the "lowest common denominator" and no chance of precision loss. Not to mention, it's more consistent and understandable given that the DOM spec is talking about milliseconds. We won't have to worry while coding if we're in seconds or milliseconds. Thanks John, Jarred for your comments! Yes approach would make this easier to deal with. Which as you said should be raised and fixed in a diff bug. I will raise a different error. updating patch as per john suggestion and uploading

Created attachment 82587 [details] patch Updated as per comment 13 and comment 14. Follow up bugs raised, 1. Implementing client-based layout test for qtwebkit - https://bugs.webkit.org/show_bug.cgi?id=54334 2. Cleanup/Move GeolocationPermissionClientQt.cpp/h implementation to GeolocationClientQt - https://bugs.webkit.org/show_bug.cgi?id=54338 3. GeolocationPosition to store timestamp as integer in miliseconds - https://bugs.webkit.org/show_bug.cgi?id=54529

Attachment 82587 [details] did not build on qt: Build output: http://queues.webkit.org/results/7920152

(In reply to comment #19) > Attachment 82587 [details] did not build on qt: > Build output: http://queues.webkit.org/results/7920152 False alarm! This patch needs a clean build. Can anyone point to someone who can issue a clean build?

(In reply to comment #20) > (In reply to comment #19) > > Attachment 82587 [details] [details] did not build on qt: > > Build output: http://queues.webkit.org/results/7920152 > > False alarm! This patch needs a clean build. Can anyone point to someone who can issue a clean build? Just ping me on #qtwebkit before landing to make a clean build on bots. I'm inspecting this incremental build problem now.

Qt build is OK with a clean build. I'll try to figure out why incremental build failed and try to fix build system to avoid similar problems.

I got it. :) s_factoryFunction, positionChanged, errorOccurred, setError, ... are defined in an ENABLE_CLIENT_BASED_GEOLOCATION ifdef block. But ufortunately GNU make can't trigger rebuild if a value of this macro changes. :( Clean build would help for this problem, but I prefer touching WebCore/page/Geolocation.h and WebCore/platform/GeolocationService.h (eg. add a new line at the end of files), because it will solve the incremental build problem for all developers, not only for the buildbots.

Created attachment 82654 [details] patch updated patch as per Ossy suggestion comment #23 Touched couple of headers to act like a clean build

Comment on attachment 82654 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=82654&action=review > Source/WebKit/qt/WebCoreSupport/ChromeClientQt.h:187 > + virtual void requestGeolocationPermissionForFrame(Frame*, Geolocation*) {} > + virtual void cancelGeolocationPermissionRequestForFrame(Frame*, Geolocation*) {} wrong style, needs a space in the {}. ie { } > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:50 > + m_location = QGeoPositionInfoSource::createDefaultSource(this); Can't this be in the above initializer? ie , m_location(QGeolo.... Also now we will create a GeolocationClientQt even if it is useless. Maybe you can let the constructor take the QGeoPositionInfoSource*. But please take care of the right ownership so that we dont start leaking > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:55 > + connect(m_location, SIGNAL(positionUpdated(QGeoPositionInfo)), this, SLOT(positionUpdated(QGeoPositionInfo))); > + > +} unneeded newline > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:90 > + double timestamp = geoPosition.timestamp().toMSecsSinceEpoch() / 1000; maybe rename to timeStampInSeconds. > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:96 > + m_lastPosition = GeolocationPosition::create(timestamp, latitude, longitude, Shouldn't we free the previous position? > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.h:67 > + RefPtr<GeolocationPosition> m_lastPosition; Ah it is a refptr, ok! :-)

Created attachment 82823 [details] patch Thanks for review Kenneth. Updated patch as per comment #25 Now QGeoPositionInfoSource is created only when location request is made, 1) Instance of location service isnt created if the page doesn't need it. 2) If a page fails to get a instance of location service, it retries on next request.

Comment on attachment 82823 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=82823&action=review > Source/WebKit/qt/ChangeLog:9 > + Implements client based geolocation for qtwebkit. New client based geolocation contains permission API's as well > + so removed the implementation from ChromeClientQt.cpp. Please try to keep these within 80 chars in the ChangeLogs > Source/WebKit/qt/WebCoreSupport/GeolocationClientQt.cpp:63 > +void GeolocationClientQt::positionUpdated(const QGeoPositionInfo &geoPosition) wrong & alignment

Comment on attachment 82823 [details] patch Removing auto commit as made few changes as per comment 27. Will manually check-it in with some committers help!

Landed in http://trac.webkit.org/changeset/79028