Bug 41364 – [Qt] QWebPage::allowGeolocationRequest should be async API

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Laszlo Gombos]

[Qt] QWebPage::allowGeolocationRequest should be async API. Same should apply for other APIs that are requesting permission e.g. for notification.

[Mahesh Kulkarni]

This bug could also be used to support ChromeClient::cancelGeolocationPermissionRequestForFrame

[Yael]

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.

[Mahesh Kulkarni]

*** Bug 42630 has been marked as a duplicate of this bug. ***

[Mahesh Kulkarni]

 - 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.

[Mahesh Kulkarni]

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.

[Mahesh Kulkarni]

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.

[Early Warning System Bot]

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

[Mahesh Kulkarni]

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.

[Mahesh Kulkarni]

Created attachment 64428 [details]
patch

Opps my bad, last patch had wrong ChangeLog message. Removed in this one. Please review!

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Yael]

(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 :-)

[Mahesh Kulkarni]

Comment on attachment 64428 [details]
patch

!GEOLOCATION system isn't building with my patch. Will upload fixed patch. Obsoleting latest patch.

[Mahesh Kulkarni]

Created attachment 64442 [details]
patch

hopefully this will fix build for !GEOLOCATION env. Please refer comment 4 and comment 5 for changes description.

[Mahesh Kulkarni]

*** Bug 43478 has been marked as a duplicate of this bug. ***

[Steve Block]

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.

[Mahesh Kulkarni]

(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.

[Steve Block]

> 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+.

[Mahesh Kulkarni]

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

[Mahesh Kulkarni]

Created attachment 64757 [details]
patch

Mistakenly enabled wrong layout test case. Please refer prev comment for change description.

[Ademar Reis]

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?

[Simon Hausmann]

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 :-)

[Mahesh Kulkarni]

(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? )

[Mahesh Kulkarni]

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

[Steve Block]

> (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.

[Laszlo Gombos]

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.

[Mahesh Kulkarni]

Created attachment 66173 [details]
patch

Thanks for the review Laszlo.

Incorporated changes as per comment #26

[Laszlo Gombos]

Comment on attachment 66173 [details]
patch

Looks good to me. All review feedback is addressed, r+. 

Great patch, thanks.

[WebKit Commit Bot]

Comment on attachment 66173 [details]
patch

Clearing flags on attachment: 66173

Committed r66597: <http://trac.webkit.org/changeset/66597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mahesh Kulkarni]

Thank you Laszlo/Simon/Steve.

[Ademar Reis]

Revision r66597 cherry-picked into qtwebkit-2.1 with commit 5da1b423861c071a2ea012e62d2a37603d96ab68