Bug 41364

Summary: [Qt] QWebPage::allowGeolocationRequest should be async API
Product: WebKit Reporter: Laszlo Gombos <laszlo.gombos>
Component: WebKit QtAssignee: Mahesh Kulkarni <maheshk>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, bulach, commit-queue, hausmann, joth, laszlo.gombos, maheshk, steveblock, webkit-ews, yael
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch
none
patch
none
patch
none
patch
none
patch none

Description Laszlo Gombos 2010-06-29 11:35:55 PDT
[Qt] QWebPage::allowGeolocationRequest should be async API. Same should apply for other APIs that are requesting permission e.g. for notification.
Comment 1 Mahesh Kulkarni 2010-06-29 11:46:28 PDT
This bug could also be used to support ChromeClient::cancelGeolocationPermissionRequestForFrame
Comment 2 Yael 2010-07-02 05:14:02 PDT
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.
Comment 3 Mahesh Kulkarni 2010-07-23 12:14:49 PDT
*** Bug 42630 has been marked as a duplicate of this bug. ***
Comment 4 Mahesh Kulkarni 2010-08-14 13:08:52 PDT
 - 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.
Comment 5 Mahesh Kulkarni 2010-08-14 13:19:28 PDT
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.
Comment 6 Mahesh Kulkarni 2010-08-14 13:20:23 PDT
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.
Comment 7 Early Warning System Bot 2010-08-14 13:28:52 PDT
Attachment 64424 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3785057
Comment 8 Mahesh Kulkarni 2010-08-14 14:01:13 PDT
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.
Comment 9 Mahesh Kulkarni 2010-08-14 14:05:08 PDT
Created attachment 64428 [details]
patch

Opps my bad, last patch had wrong ChangeLog message. Removed in this one. Please review!
Comment 10 Early Warning System Bot 2010-08-14 14:10:37 PDT
Attachment 64426 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3747162
Comment 11 Early Warning System Bot 2010-08-14 14:17:12 PDT
Attachment 64428 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3714171
Comment 12 Yael 2010-08-14 16:55:12 PDT
(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 13 Mahesh Kulkarni 2010-08-14 19:53:28 PDT
Comment on attachment 64428 [details]
patch

!GEOLOCATION system isn't building with my patch. Will upload fixed patch. Obsoleting latest patch.
Comment 14 Mahesh Kulkarni 2010-08-15 04:33:36 PDT
Created attachment 64442 [details]
patch

hopefully this will fix build for !GEOLOCATION env. Please refer comment 4 and comment 5 for changes description.
Comment 15 Mahesh Kulkarni 2010-08-15 21:42:09 PDT
*** Bug 43478 has been marked as a duplicate of this bug. ***
Comment 16 Steve Block 2010-08-18 02:05:59 PDT
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.
Comment 17 Mahesh Kulkarni 2010-08-18 04:09:43 PDT
(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.
Comment 18 Steve Block 2010-08-18 05:12:33 PDT
> 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+.
Comment 19 Mahesh Kulkarni 2010-08-18 12:32:53 PDT
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
Comment 20 Mahesh Kulkarni 2010-08-18 12:35:26 PDT
Created attachment 64757 [details]
patch

Mistakenly enabled wrong layout test case. Please refer prev comment for change description.
Comment 21 Ademar Reis 2010-08-25 10:59:29 PDT
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 22 Simon Hausmann 2010-08-30 00:44:54 PDT
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 :-)
Comment 23 Mahesh Kulkarni 2010-08-31 06:14:48 PDT
(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? )
Comment 24 Mahesh Kulkarni 2010-08-31 06:17:59 PDT
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
Comment 25 Steve Block 2010-08-31 10:45:01 PDT
> (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.
Comment 26 Laszlo Gombos 2010-08-31 22:17:15 PDT
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.
Comment 27 Mahesh Kulkarni 2010-09-01 00:15:39 PDT
Created attachment 66173 [details]
patch

Thanks for the review Laszlo.

Incorporated changes as per comment #26
Comment 28 Laszlo Gombos 2010-09-01 05:01:26 PDT
Comment on attachment 66173 [details]
patch

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

Great patch, thanks.
Comment 29 WebKit Commit Bot 2010-09-01 05:31:43 PDT
Comment on attachment 66173 [details]
patch

Clearing flags on attachment: 66173

Committed r66597: <http://trac.webkit.org/changeset/66597>
Comment 30 WebKit Commit Bot 2010-09-01 05:31:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Mahesh Kulkarni 2010-09-01 20:49:37 PDT
Thank you Laszlo/Simon/Steve.
Comment 32 Ademar Reis 2010-09-02 07:42:55 PDT
Revision r66597 cherry-picked into qtwebkit-2.1 with commit 5da1b423861c071a2ea012e62d2a37603d96ab68