Bug 46810 – [Qt] Review the setUserPermission & friends API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Andreas Kling]

We need to look over the new user permission API before releasing QtWebKit 2.1

[Simon Hausmann]

Proposal from Andreas and me:

1) Remove synchronous checkPermission() signal - cache result inside WebKit instead

Justification: This seems like a premature API in the light of the currently ongoing permissions API work (http://dev.w3.org/2006/webapi/WebNotifications/publish/FeaturePermissions.html ). In addition we don't use out arguments for signals in Qt APIs.

2) Extend PermissionPolicy to the following values:


    enum PermissionPolicy {
        PermissionUnknown,
        PermissionGrantedByUser,
        PermissionGrantedByDefault,        
        PermissionDeniedByUser,        
        PermissionDeniedByDefault
    };


This makes it easy to distinguish between permissions granted by the user and defaults the programmer sets, when reading the code.

3) Rename PermissionsDomain to the following enum:

    enum FeaturePermission {
        NotificationFeature,
        GeolocationFeature
    };

This is shorter and removes the redundant use of Permission. Feature is also a simpler word instead of domain.

4) Rename setUserPermission to setFeaturePermission


We think that after these proposed changes the resulting user code is more readable and it is easier to implement permission handling in the application. It is only necessary to respond to two signals (always asynchronously) and the setFeaturePermission function allows setting defaults as well as responding to requests.

Example of the old code:

page->setUserPermission(frame, NotificationPermissionDomain, PermissionAllowed);

Example of the improved API:

page->setFeaturePermission(frame, NotificationFeature, PermissionAllowedByUser);

[Yael]

This proposal looks good to me.
I will be OOO next week, but if there is a consensus about a new API after I get back (11/8), I can work on this.

[Jocelyn Turcotte]

(In reply to comment #1)

> 1) Remove synchronous checkPermission() signal - cache result inside WebKit instead
> 
> Justification: This seems like a premature API in the light of the currently ongoing permissions API work (http://dev.w3.org/2006/webapi/WebNotifications/publish/FeaturePermissions.html ). In addition we don't use out arguments for signals in Qt APIs.

Agreed, and what is the need for cancelRequestsForPermission, is there cases where there is no other way to prevent accessing a destroyed object? Couldn't we swallow the response in setUserPermission instead?


> 3) Rename PermissionsDomain to the following enum:
> 
>     enum FeaturePermission {
>         NotificationFeature,
>         GeolocationFeature
>     };

Maybe we could additionally remove "Permission" from the enum name, or replace it with another word. In case the feature IDs starts getting used in non-permission related APIs later.

[Simon Hausmann]

(In reply to comment #3)
> (In reply to comment #1)
> 
> > 1) Remove synchronous checkPermission() signal - cache result inside WebKit instead
> > 
> > Justification: This seems like a premature API in the light of the currently ongoing permissions API work (http://dev.w3.org/2006/webapi/WebNotifications/publish/FeaturePermissions.html ). In addition we don't use out arguments for signals in Qt APIs.
> 
> Agreed, and what is the need for cancelRequestsForPermission, is there cases where there is no other way to prevent accessing a destroyed object? Couldn't we swallow the response in setUserPermission instead?

In the slot connected to the cancelRequestForPermission() you would be able to close a non-modal permission dialog that may still be visible.

[Simon Hausmann]

(In reply to comment #3)
> > 3) Rename PermissionsDomain to the following enum:
> > 
> >     enum FeaturePermission {
> >         NotificationFeature,
> >         GeolocationFeature
> >     };
> 
> Maybe we could additionally remove "Permission" from the enum name, or replace it with another word. In case the feature IDs starts getting used in non-permission related APIs later.

Good point, the enum _values_ are generic and don't have permission in them, so maybe the enum itself shouldn't have that either. Hmmm...

[Jocelyn Turcotte]

(In reply to comment #4)
> > Agreed, and what is the need for cancelRequestsForPermission, is there cases where there is no other way to prevent accessing a destroyed object? Couldn't we swallow the response in setUserPermission instead?
> 
> In the slot connected to the cancelRequestForPermission() you would be able to close a non-modal permission dialog that may still be visible.

I didn't see a cancelRequest() method on the script side of features and QWebPage::cancelRequestForPermission seems to be emitted only in subcalls of disconnectFrame() in Frame::~Frame().

In that case, what are the cases where you would use this signal instead of QWebFrame::destroyed()? I see the reason why you would use it, but it's not clear what is the purpose to expose this callback in the API.

[Simon Hausmann]

Committed r72101: <http://trac.webkit.org/changeset/72101>

[Simon Hausmann]

Created attachment 74646 [details]
Patch

[Simon Hausmann]

Created attachment 74647 [details]
Patch

[Simon Hausmann]

Created attachment 74648 [details]
Patch

[Ademar Reis]

Please don't forget to update the symbian .def files :-)

[Simon Hausmann]

Created attachment 74651 [details]
Patch

[Simon Hausmann]

Committed r72600: <http://trac.webkit.org/changeset/72600>

[Simon Hausmann]

Committed r72601: <http://trac.webkit.org/changeset/72601>

[Simon Hausmann]

Committed r72602: <http://trac.webkit.org/changeset/72602>

[Simon Hausmann]

Committed r72603: <http://trac.webkit.org/changeset/72603>

[Simon Hausmann]

Revision r72101 cherry-picked into qtwebkit-2.1 with commit e645d43 <http://gitorious.org/webkit/qtwebkit/commit/e645d43>
Revision r72600 cherry-picked into qtwebkit-2.1 with commit 2290254 <http://gitorious.org/webkit/qtwebkit/commit/2290254>
Revision r72601 cherry-picked into qtwebkit-2.1 with commit 68124d7 <http://gitorious.org/webkit/qtwebkit/commit/68124d7>
Revision r72602 cherry-picked into qtwebkit-2.1 with commit aa7ca33 <http://gitorious.org/webkit/qtwebkit/commit/aa7ca33>
Revision r72603 cherry-picked into qtwebkit-2.1 with commit df0e5bc <http://gitorious.org/webkit/qtwebkit/commit/df0e5bc>

[Norbert Leser]

Created attachment 74706 [details]
Patch for symbian DEF file

Patch for Symbian .def file for ARM target, to reflect the API changes of previous patches to this bug.

Note that svn diff shows complete file replacement (due to .def file type), while in fact only 4 methods were declared ABSENT and 3 new ones added.

[Laszlo Gombos]

Comment on attachment 74706 [details]
Patch for symbian DEF file

LGTM, r+.

[Andreas Kling]

Comment on attachment 74706 [details]
Patch for symbian DEF file

Clearing flags on attachment: 74706

Committed r72739: <http://trac.webkit.org/changeset/72739>

[Ademar Reis]

I updated the symbian .def files on 2.1:

http://gitorious.org/webkit/qtwebkit/commit/29553ffa6d3ffaa84a3360f26bdb51cec72ab624
(includes other pending updates on the .def file)

I wonder if we should update the qt/symbian/bwins/QtWebKitu.def as well? (for winscw)

[Norbert Leser]

(In reply to comment #21)
> I updated the symbian .def files on 2.1:
> 
> http://gitorious.org/webkit/qtwebkit/commit/29553ffa6d3ffaa84a3360f26bdb51cec72ab624
> (includes other pending updates on the .def file)
> 
> I wonder if we should update the qt/symbian/bwins/QtWebKitu.def as well? (for winscw)

Right, that one exists but has been stale for a while.

Not sure, what we should best do about it. It does not have any relevance for backward binary compatibility, thus any freshly compiled content, regardless of ordinal numbers etc., would be sufficient.

One could delete that file, but the disadvantage is that anyone who wants to compile for both targets at the same time, arm and winscw, would have to recreate the def file. If we keep it, we should keep the bwins/QtWebKitu.def valid.

I will post a patch of a new valid bwins/QtWebKitu.def once I have successfully compiled for winscw.

[Ademar Reis]

(In reply to comment #22)
<snip>
> 
> I will post a patch of a new valid bwins/QtWebKitu.def once I have successfully compiled for winscw.

You won't be able to build trunk using winscw. You need a few intrusive patches which are present only on 2.1: 00ea9ca8dd57338d2fdf50ec42faac886a0124e7,  ec96dfdc8601a98299b04affc819a84b9e2e6d1f and 1be7a0eaab0df955130791dcfe43dd5b6ed2c452.

In other words, we don't support winscw on trunk, so I guess we could remove the respective .def file from there (but an updated .def file for 2.1 is still necessary and we're missing that).

[Norbert Leser]

Created attachment 75405 [details]
Patch for qtwebkit 2.1 (DEF file for winscw target)

This patch contains the updated DEF file for winscw target.
That one is only valid for qtwebkit 2.1 and needs to be merged there.

(I will upload another patch later that deletes the bwins/QtWebKitu.def for webkit trunk, since currently the winscw target cannot be built there anyway.)