Bug 41413 – [Qt] QtWebKit needs public API for Notifications.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Yael]

Current implementation of Notifications in QtWebKit uses a private API. Clients of QtWebKit need a public API to avoid hacks/workarounds.

[Yael]

Created attachment 60128 [details]
Patch.

This patch introduces a new QWebPage API to replace the private DumpRenderTree API.
The same API can be reused for other features that require user consent, such as GeoLocation API.

[Kenneth Rohde Christiansen]

Comment on attachment 60128 [details]
Patch.

WebKit/qt/Api/qwebpage.h:204
 +      enum UserPermissionType {
Isn't that more domain than type? PermissionDomain ?

WebKit/qt/Api/qwebpage.h:198
 +      enum UserPermission {
PermissionPolicy ?

WebKit/qt/Api/qwebpage.h:303
 +      void setUserPermission(UserPermissionType, const QString&, UserPermission);
void setUserPermission(PermissionDomain domain, const QString&, PermissionPolicy policy) ?

WebKit/qt/Api/qwebpage.h:199
 +          UserPermissionAllowed,
maybe Granded. You grand permission I believe.


Are all these other changes really related to the public API?

[Yael]

(In reply to comment #2)
> (From update of attachment 60128 [details])
> WebKit/qt/Api/qwebpage.h:204
>  +      enum UserPermissionType {
> Isn't that more domain than type? PermissionDomain ?
> 
> WebKit/qt/Api/qwebpage.h:198
>  +      enum UserPermission {
> PermissionPolicy ?
> 
> WebKit/qt/Api/qwebpage.h:303
>  +      void setUserPermission(UserPermissionType, const QString&, UserPermission);
> void setUserPermission(PermissionDomain domain, const QString&, PermissionPolicy policy) ?
> 
> WebKit/qt/Api/qwebpage.h:199
>  +          UserPermissionAllowed,
> maybe Granded. You grand permission I believe.
> 
> 
> Are all these other changes really related to the public API?

Kenneth, thanks for reviewing the API proposal. Unfortunately, all the changes are related to the new API, including WebKit/chromium changes. The current NotificationPresenter interface does not give us enough information to know which page initiated the notification, so if we wanted to show a UI, we would not know which tab/window should show that UI.
In Chromium this is not a problem because they know which process initiated the IPC.

[Yael]

Created attachment 60152 [details]
Patch

Rename parameters as suggested by Kenneth in comment #2.

[Simon Hausmann]

Comment on attachment 60152 [details]
Patch

Some comments:

WebKit/qt/Api/qwebpage.h:303
 +      void setUserPermission(PermissionDomain, const QString&, PermissionPolicy);
In the Qt API we tend to always give names to the parameters.

WebKit/qt/Api/qwebpage.h:386
 +      void requestPermissionFromUser(QWebPage::PermissionDomain domain, const QString& origin);
I think in this API the origin parameter is significant and should come first. The same applies to the other functions.

The use of the origin as string is nice, that's nice. But is it very descriptive? Does it allow the UI on top of QtWebKit to provide enough information to the user? I wonder if these permissions can always be tied to a QWebFrame instead (as first parameter)?


Conceptually I think it would be nice if the API allowed the developer to react immediately if he wants to but he also
has the option of reacting later. Immediate reaction is useful for programmatic setups, like the DRT.

I like the approach of emitting a signal and I think it would be nice if in the slot all the parameters are in place
to make granting or denying the permissions a simple task. So an object as parameter that allows setting the permission
would achieve that, such as QWebFrame or a more specialized object.

[Laszlo Gombos]

(In reply to comment #5)
> (From update of attachment 60152 [details])
> Some comments:

> The use of the origin as string is nice, that's nice. But is it very descriptive? Does it allow the UI on top of QtWebKit to provide enough information to the user? I wonder if these permissions can always be tied to a QWebFrame instead (as first parameter)?
> 

I agree as well the QWebFrame could be more useful; we do not to be careful with the life cycle of the objects:
 - notification itself could outlive the frame by design, but the permission request should be invalidated if the originating frame dies.

 - in general if the context for the permission is a Worker than we have a URL but not a frame, but so far we do not support this case.

[Yael]

Comment on attachment 60152 [details]
Patch

Thank you, Simon and Laszlo, for reviewing the patch. I will update it with your comments after the (4th of July) holiday weekend.

[Yael]

Simon, I tested Chromium and FIreFox's behavior of prompts for permission, and both browsers show only the origin when they prompt the user for permission, regardless of what kind of permission they need (GeoLocation, appcache, notifications and so on). 

The biggest problem I see with passing QWebFrame in the API is the following test that I did using Chromium browser:

1) Load this page:

<html>
<script>
function remove() {
document.getElementById("par").innerHTML="Do not crash";
}
</script>
Click this button to remove the iframe <button onclick="remove();">click me</button><br>
<div id="par"><iframe src="http://slides.html5rocks.com/#slide12" width=800 height=800></iframe></div>
</html>

2) click on "Set notification premissions ..."
3) click on "click me".

With the proposed change, the client is now left with a deleted QWebFrame .

[Mahesh Kulkarni]

(In reply to comment #8)
> Simon, I tested Chromium and FIreFox's behavior of prompts for permission, and both browsers show only the origin when they prompt the user for permission, regardless of what kind of permission they need (GeoLocation, appcache, notifications and so on). 
> 
> The biggest problem I see with passing QWebFrame in the API is the following test that I did using Chromium browser:
> 
> 1) Load this page:
> 
> <html>
> <script>
> function remove() {
> document.getElementById("par").innerHTML="Do not crash";
> }
> </script>
> Click this button to remove the iframe <button onclick="remove();">click me</button><br>
> <div id="par"><iframe src="http://slides.html5rocks.com/#slide12" width=800 height=800></iframe></div>
> </html>
> 
> 2) click on "Set notification premissions ..."
> 3) click on "click me".
> 
> With the proposed change, the client is now left withwith a deleted QWebFrame .

I think we would need one more api something like, QwebPage::cancelRequestForPermission() so that client can discard any UI pending for user request. 

Incase of geolocation, ChromeClient::cancelGeolocationPermissionRequestForFrame is triggered on disconnectFrame(). This still needs to be redirected to client. 

We would still end up with deleted QwebFrame client in above case until cancel request arrives and handled from client side itself

[Yael]

Created attachment 61178 [details]
Patch

This patch removed DumpRenderTree private API and introduced new public API.
It also implements the cancel API for notifications permission requests.

A follow-up patch should move some of the printf code out of NotificationsPresenterClientQt and into DumpRenderTreeQt.

[Yael]

Created attachment 61296 [details]
Patch

As suggested by Kenneth on IRC, make enum names more inline with Qt naming conventions.
Kenneth, sorry for the choppy IRC connection, we had multiple power failures today due to the heat wave.

[Kenneth Rohde Christiansen]

Comment on attachment 61296 [details]
Patch

WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.h:113
 +      QWebPage* page(ScriptExecutionContext*);
Is this like a cast? if so call it toPage, that seems to be the standard in WebKit. In Qt we often use that for methods doing for instance conversion.

WebKitTools/DumpRenderTree/qt/DumpRenderTreeQt.h:196
 +      void cancelRequestsForPermission(QWebFrame* frame, QWebPage::PermissionDomain domain);
Looks OK to me, but I would like to know what Simon thinks, especially about the API part.

[Yael]

Created attachment 61313 [details]
Patch

Changed page() and frame() to toPage() and toFrame() as suggested in comment #12.

[Laszlo Gombos]

Comment on attachment 61313 [details]
Patch

> Index: WebKit/qt/Api/qwebpage.h
> ===================================================================
> --- WebKit/qt/Api/qwebpage.h	(revision 63134)
> +++ WebKit/qt/Api/qwebpage.h	(working copy)
> @@ -195,6 +195,16 @@ public:
>          WebModalDialog
>      };
>  
> +    enum PermissionPolicy {
> +        PermissionGranted,
> +        PermissionIgnored,
> +        PermissionDenied
> +    };
> +

I think it is important to find a more descriptive name than PermissionIgnored. I suggest PermissionUnknown instead of PermissionIgnored. This is also inline with a recent discussion about a generic Permission API discussed in public-webapps.

[Yael]

Created attachment 62289 [details]
Patch

Updated the patch to latest trunk, and modified PermissionIgnored to PermissionUnknown, per comment 14.
Also added https://bugs.webkit.org/show_bug.cgi?id=42798 to Chromium's test expectations file, as suggested by Tor Arne on IRC.

[Simon Hausmann]

Comment on attachment 62289 [details]
Patch

WebKit/qt/Api/qwebpage.h:387
 +      void checkPermissionFromUser(QWebFrame* frame, QWebPage::PermissionDomain domain, QWebPage::PermissionPolicy& policy);
We usually don't use synchronous signals with out values in the Qt API.

Is this needed in places where a synchronous response is needed from the user and we can't fall back to the requestPermission -> setUserPermission flow?

[Yael]

(In reply to comment #16)
> (From update of attachment 62289 [details])
Thanks for the review, Simon!
> WebKit/qt/Api/qwebpage.h:387
>  +      void checkPermissionFromUser(QWebFrame* frame, QWebPage::PermissionDomain domain, QWebPage::PermissionPolicy& policy);
> We usually don't use synchronous signals with out values in the Qt API.
> 
"policy" is the return value of this signal. Ideally, this would be a virtual function, but since we cannot add virtual functions to QWebPage, I made it a signal.
> Is this needed in places where a synchronous response is needed from the user and we can't fall back to the requestPermission -> setUserPermission flow?
yes.
Notifications have 2 types of checks. One is synchronous and one is asynchronous. The asynchronous check is expected to show a UI and the synchronous is not expected to show a UI. We cannot call the asynchronous API when the synchronous API is expected. 

I will create a new bug for the documentation of this new API and hope that with the documentation it will become more clear.

[Laszlo Gombos]

Comment on attachment 62289 [details]
Patch

This looks good to me; r+.

I expect some more discussion about the exposed API but in order to do that I'd like to see if we can make some progress of hooking up Geolocation support behind the same permission API.

[WebKit Commit Bot]

Comment on attachment 62289 [details]
Patch

Clearing flags on attachment: 62289

Committed r63921: <http://trac.webkit.org/changeset/63921>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r63921 cherry-picked into qtwebkit-2.1 with commit 235046a1fbe4cae2938ffe6538e324035a4297e4