Bug 46810 - [Qt] Review the setUserPermission & friends API
Summary: [Qt] Review the setUserPermission & friends API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Simon Hausmann
URL:
Keywords: Qt, QtTriaged
Depends on: 46814
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-29 08:04 PDT by Andreas Kling
Modified: 2010-12-02 12:28 PST (History)
7 users (show)

See Also:


Attachments
Patch (8.04 KB, patch)
2010-11-23 05:06 PST, Simon Hausmann
vestbo: review+
Details | Formatted Diff | Diff
Patch (17.83 KB, patch)
2010-11-23 05:06 PST, Simon Hausmann
vestbo: review+
Details | Formatted Diff | Diff
Patch (7.29 KB, patch)
2010-11-23 05:07 PST, Simon Hausmann
vestbo: review+
Details | Formatted Diff | Diff
Patch (8.68 KB, patch)
2010-11-23 05:33 PST, Simon Hausmann
vestbo: review+
Details | Formatted Diff | Diff
Patch for symbian DEF file (96.17 KB, patch)
2010-11-23 15:49 PST, Norbert Leser
no flags Details | Formatted Diff | Diff
Patch for qtwebkit 2.1 (DEF file for winscw target) (186.27 KB, patch)
2010-12-02 12:28 PST, Norbert Leser
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-09-29 08:04:01 PDT
We need to look over the new user permission API before releasing QtWebKit 2.1
Comment 1 Simon Hausmann 2010-10-29 02:01:55 PDT
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);
Comment 2 Yael 2010-10-29 05:43:40 PDT
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.
Comment 3 Jocelyn Turcotte 2010-10-29 06:29:28 PDT
(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.
Comment 4 Simon Hausmann 2010-10-29 08:05:11 PDT
(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.
Comment 5 Simon Hausmann 2010-10-29 08:05:57 PDT
(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...
Comment 6 Jocelyn Turcotte 2010-10-29 14:12:53 PDT
(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.
Comment 7 Simon Hausmann 2010-11-16 08:29:48 PST
Committed r72101: <http://trac.webkit.org/changeset/72101>
Comment 8 Simon Hausmann 2010-11-23 05:06:07 PST
Created attachment 74646 [details]
Patch
Comment 9 Simon Hausmann 2010-11-23 05:06:47 PST
Created attachment 74647 [details]
Patch
Comment 10 Simon Hausmann 2010-11-23 05:07:29 PST
Created attachment 74648 [details]
Patch
Comment 11 Ademar Reis 2010-11-23 05:25:40 PST
Please don't forget to update the symbian .def files :-)
Comment 12 Simon Hausmann 2010-11-23 05:33:50 PST
Created attachment 74651 [details]
Patch
Comment 13 Simon Hausmann 2010-11-23 06:24:54 PST
Committed r72600: <http://trac.webkit.org/changeset/72600>
Comment 14 Simon Hausmann 2010-11-23 06:25:13 PST
Committed r72601: <http://trac.webkit.org/changeset/72601>
Comment 15 Simon Hausmann 2010-11-23 06:25:32 PST
Committed r72602: <http://trac.webkit.org/changeset/72602>
Comment 16 Simon Hausmann 2010-11-23 06:25:51 PST
Committed r72603: <http://trac.webkit.org/changeset/72603>
Comment 17 Simon Hausmann 2010-11-23 06:57:31 PST
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>
Comment 18 Norbert Leser 2010-11-23 15:49:59 PST
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.
Comment 19 Laszlo Gombos 2010-11-23 19:53:57 PST
Comment on attachment 74706 [details]
Patch for symbian DEF file

LGTM, r+.
Comment 20 Andreas Kling 2010-11-25 10:22:03 PST
Comment on attachment 74706 [details]
Patch for symbian DEF file

Clearing flags on attachment: 74706

Committed r72739: <http://trac.webkit.org/changeset/72739>
Comment 21 Ademar Reis 2010-11-25 11:38:12 PST
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)
Comment 22 Norbert Leser 2010-11-29 04:29:23 PST
(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.
Comment 23 Ademar Reis 2010-11-29 06:58:01 PST
(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).
Comment 24 Norbert Leser 2010-12-02 12:28:19 PST
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.)