[Qt] QWebPage::allowGeolocationRequest should be async API. Same should apply for other APIs that are requesting permission e.g. for notification.
This bug could also be used to support ChromeClient::cancelGeolocationPermissionRequestForFrame
IMO, The API we define for GeoLocation should be as similar as possible to the one for Notification. Please take a look at https://bugs.webkit.org/show_bug.cgi?id=41413 for my proposal for Notifications API.
*** Bug 42630 has been marked as a duplicate of this bug. ***
- Provides a new async API for geolocation permission. - Using Notification API approach from qtwebkit. GeolocationPermissionClientQt maintains list of pending requests from WebCore - Intimates WebCore pending geolocation notifiers when client either allows/denies the request. - Implements ChromeClientQt::cancelGeolocationPermissionRequestForFrame which removes - DumpRenderTree implements the async request from QtWebkit - WebPage maintains list of geolocation permission request QtWebkit and set's when LayoutTestController gets the access from test JS Please review.
Created attachment 64424 [details] patch (In reply to comment #4) > - Provides a new async API for geolocation permission. > - Using Notification API approach from qtwebkit. GeolocationPermissionClientQt maintains list of pending requests from WebCore > - Intimates WebCore pending geolocation notifiers when client either allows/denies the request. > - Implements ChromeClientQt::cancelGeolocationPermissionRequestForFrame which removes > - DumpRenderTree implements the async request from QtWebkit > - WebPage maintains list of geolocation permission request QtWebkit and set's when LayoutTestController gets the access from test JS > > > Please review. Right now 4 layoutTest cases are blocked by this bug and these cases will be available once patches for bug 42811 and bug 40002 gets in. I will attach one more patch followed by this one to enable all above 4 cases.
IMO, QWebPage::requestPermissionFromUser() followed by QWebPAge::setPermission is enough to export (also cancelRequestsForPermission()) and remove QWebPage::checkPermissionFromUser() for two reasons, 1) checkPermissionFromUser and setPermission confuses clients 2) checkPermissionFromUser is over ahead to call/maintain from WebCore side As requestPermission is async, client can maintain pending requests and call QWebPAge::setPermission() once the permission is acquired.
Attachment 64424 [details] did not build on qt: Build output: http://queues.webkit.org/results/3785057
Created attachment 64426 [details] patch Fixed build problem with previous patch. Previous patch was missing DumpRenderTree changes. Also updated unit test case of qwebpage with new async api for geolocation.
Created attachment 64428 [details] patch Opps my bad, last patch had wrong ChangeLog message. Removed in this one. Please review!
Attachment 64426 [details] did not build on qt: Build output: http://queues.webkit.org/results/3747162
Attachment 64428 [details] did not build on qt: Build output: http://queues.webkit.org/results/3714171
(In reply to comment #6) > IMO, QWebPage::requestPermissionFromUser() followed by QWebPAge::setPermission is enough to export (also cancelRequestsForPermission()) and remove > QWebPage::checkPermissionFromUser() for two reasons, > 1) checkPermissionFromUser and setPermission confuses clients > 2) checkPermissionFromUser is over ahead to call/maintain from WebCore side > > As requestPermission is async, client can maintain pending requests and call QWebPAge::setPermission() once the permission is acquired. Laszlo & I discussed this to great detail. You may not need the sync API in order to support Geolocation, but both are needed for the Notification use case. Please don't remove the sync API :-)
Comment on attachment 64428 [details] patch !GEOLOCATION system isn't building with my patch. Will upload fixed patch. Obsoleting latest patch.
Created attachment 64442 [details] patch hopefully this will fix build for !GEOLOCATION env. Please refer comment 4 and comment 5 for changes description.
*** Bug 43478 has been marked as a duplicate of this bug. ***
Comment on attachment 64442 [details] patch WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:234 + setUserPermission(frame, domain, PermissionDenied); In the case where the permission has already been set from JavaScript, does this mean that the permission request from the Geolocation object is answered completely synchronously? Is your implementation of these permissions for the browser synchronous or asynchronous? WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:278 + setUserPermission(frame, domain, PermissionDenied); It looks like this will pass the permissions to WebCore synchronously. If the browser permissions are asynchronous, you might want to consider making this asynchronous too to match the 'real life' code path better, as Mac does. Do the delayed permission tests pass with this synchronous code? WebCore/ChangeLog:8 + Adding GeolocationPermissionClientQt.cpp/h to build under geolocation flag. This change should remove the delayed permission test from the skipped list, if possible. Also, we should probably mention here that this change provides functionality for those tests.
(In reply to comment #16) > (From update of attachment 64442 [details]) > WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:234 > + setUserPermission(frame, domain, PermissionDenied); > In the case where the permission has already been set from JavaScript, does this mean that the permission request from the Geolocation object is answered completely synchronously? Is your implementation of these permissions for the browser synchronous or asynchronous? > In case of javascript setting permission in DRT case, LayouController would retain the permissions. When Geolocation request is made from any frame, LayoutController permission will be given to all frames. When permission request is made (QWebpage::requestPermissionFromUser), one more setUserPermission() will be called with permission set from LayoutControl. Btw, There are no layout cases where permission for each frame differs. This should probably be raised as a bug? > WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.cpp:278 > + setUserPermission(frame, domain, PermissionDenied); > It looks like this will pass the permissions to WebCore synchronously. If the browser permissions are asynchronous, you might want to consider making this asynchronous too to match the 'real life' code path better, as Mac does. Do the delayed permission tests pass with this synchronous code? > Yes they all passed because of above reason. We could probably change GeolocationPermissionClientQt to maintain a set of <Frames, permission> much before requests are made. (i.e when setUserPermission sync API called by client without any request). Cache the permission when any frames are created. This way clients can grand geolocation permission by default to certain frames. > WebCore/ChangeLog:8 > + Adding GeolocationPermissionClientQt.cpp/h to build under geolocation flag. > This change should remove the delayed permission test from the skipped list, if possible. Also, we should probably mention here that this change provides functionality for those tests. Sorry above line is invalid as GeolocationPermissionClientQt.cpp/h are not under the flag anymore. I will upload the new patch with this corrected. One more mistake in patch, GeolocationPermissionClientQt.h are included twice in place of .cpp.
> Btw, There are no layout cases where permission for each frame differs. This > should probably be raised as a bug? Yes, please do. CC me and I'll review any patches you send. > Yes they all passed because of above reason. OK This change looks fine to me but I'll leave it to somebody familiar with Qt to give the r+.
Created attachment 64756 [details] patch This patch includes changes described in comment 4 and comment 5 and 1) cache client permission from call QWebPage::setUserPermission() even without any geolocation requests. When Webcore makes a geolocation permission request, cached permission will be passed on. This way clients can set default sync permission to geolocation. 2) Enabled all geolocation delayed layout test cases to Qt port as the permission API is now async Please review
Created attachment 64757 [details] patch Mistakenly enabled wrong layout test case. Please refer prev comment for change description.
Bug #42630 - closed as duplicate of this one - was blocking Bug #39121 (QtWebKit 2.1 release). Should this one block Bug #39121 as well? That is, is this a requirement for QtWebKit 2.1?
Comment on attachment 64757 [details] patch > WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.cpp:44 > +GeolocationPermissionClientQt* s_geolocationPermission= 0; This global variable should be marked "static" and doesn't need a 0 initialization then. You could however also make it a class variable (in which case IIRC it needs an initialization :) > WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.cpp:67 > + setPermission(webFrame, m_permissionForFrames.value(webFrame)); These two lines perform an identical map lookup. Wouldn't it be simpler to just look up the iterator once and use it's value if it's not end()? > WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.cpp:78 > +void GeolocationPermissionClientQt::cancelGeolocationPermissionRequestForFrame(QWebFrame* webFrame, Geolocation* listener) Is this function also called when navigating from one origin to another within the same frame? I.e. how does WebCore ensure that when I grant maps.ovi.com geolocation permission and then navigate to maps.evilh4x0r.com, the latter site won't be able to read my GPS? > WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.cpp:90 > + m_permissionForFrames.remove(webFrame); Ahh, I guess this part answers my previous question ;-) > WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.h:55 > + QHash<QWebFrame*, Geolocation*> m_pendingPermissionRequests; Hmm, instead of storing a separate hash, wouldn't it be simpler to store the information in QWebFramePrivate directly? In general I like the direction this patch is going very much, it simplifies the API :-)
(In reply to comment #22) > > WebKit/qt/WebCoreSupport/GeolocationPermissionClientQt.h:55 > > + QHash<QWebFrame*, Geolocation*> m_pendingPermissionRequests; > Hmm, instead of storing a separate hash, wouldn't it be simpler to store the information in QWebFramePrivate directly? > As per our discussion with tronical in IRC we decided to *not* to cache permission from user, set even before call for requestPermission() is made. in WebCore geolocation permission is tied up with frame where as permission is given per domain. Right now Qtwebkit clients needs handle persisting per domain permission. QtWebkit will make request (first time) for every frame making request for location. My next patch will have that fix. (Chrome handles webkit layer side to allow permission host/domain based by storing domain. This actually has a problem with ChromeClient::cancelGeolocationPermissionRequestForFrame as it passes frame pointer, because, if two frames requesting for permission (both having domain say, x.myxyz.com) and say second frame redirects to x.myabc.com, then we have to remove the pending permission listeners for second frame. IMO chrome port missing this handling. Steve? )
Created attachment 66042 [details] patch Removed : cache client permission from call QWebPage::setUserPermission() even without any geolocation requests. When Webcore makes a geolocation permission request, cached permission will be passed on. This way clients can set default sync permission to geolocation. Please review
> (Chrome handles webkit layer side to allow permission host/domain based by > storing domain. This actually has a problem with > ChromeClient::cancelGeolocationPermissionRequestForFrame as it passes frame > pointer, because, if two frames requesting for permission (both having domain > say, x.myxyz.com) and say second frame redirects to x.myabc.com, then we have > to remove the pending permission listeners for second frame. IMO chrome port > missing this handling. Steve? ) I'm not familiar with the Chrome port - adding Marcus and Joth who are.
I think the ChangeLog needs to be updated as well as setUserPermission no longer caches the permission data. The following sentence should be removed from WebKit/qt/ChangeLog In case client call QWebPage::setUserPermission without any request, cache the permission for the frame and use it when WebCore makes a request. Also just a nit, but it would be good to keep the file list sorted in WebCore.pro.
Created attachment 66173 [details] patch Thanks for the review Laszlo. Incorporated changes as per comment #26
Comment on attachment 66173 [details] patch Looks good to me. All review feedback is addressed, r+. Great patch, thanks.
Comment on attachment 66173 [details] patch Clearing flags on attachment: 66173 Committed r66597: <http://trac.webkit.org/changeset/66597>
All reviewed patches have been landed. Closing bug.
Thank you Laszlo/Simon/Steve.
Revision r66597 cherry-picked into qtwebkit-2.1 with commit 5da1b423861c071a2ea012e62d2a37603d96ab68