Bug 120196

Summary: [Gtk][WK2] Use a permission-request for fullscreen requests
Product: WebKit Reporter: Ben Boeckel <mathstuf>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: UNCONFIRMED ---    
Severity: Normal CC: brian.holt, bugs-noreply, cgarcia, gns, mario, mrobinson, pnormand
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   

Description Ben Boeckel 2013-08-22 20:14:19 PDT
The documentation for the "permission-request" signal says:

> This signal is emitted when WebKit is requesting the client to decide about a permission request, such as allowing the browser to switch to fullscreen mode, sharing its location or similar operations.

However, there is the "enter-fullscreen" signal and it doesn't appear that "permission-request" is fired for this. Are the docs wrong or should "enter-fullscreen"/"leave-fullscreen" be deprecated for a new WebKitPermissionRequest implementation?
Comment 1 Brian Holt 2013-09-04 06:14:32 PDT
I know a lot of effort went into designing the permission-request mechanism so I suspect that it is the preferred option for getting permission for a fullscreen.  That said, I also think that emitting the signals for entering and leaving fullscreen mode is still useful and should be kept.

So my view is that we should create a new permission request subclass WebKitFullScreenPermissionRequest and emit it with the permission-request signal in webkitWebViewEnterFullScreen() to get the user's permission.  Adding a policy to the WebKitWebContext WebKitFullScreenPolicy with values WEBKIT_FULLSCREEN_POLICY_ASK and WEBKIT_FULLSCREEN_POLICY_ALWAYS would also make sense to me (much like TSL errors).

I'd be happy to do the implementation and propose it formally on the mailing list if there is support.
Comment 2 Mario Sanchez Prada 2013-09-04 06:35:39 PDT
(In reply to comment #1)
> [...]
> I'd be happy to do the implementation and propose it formally on the mailing list if there is support.

I think that having Philippe input on this would be very valuable before going ahead with this.

IIRC, Phil was the one adding those enter/leave fullscreen signals and I wonder whether there might be any reason why it could be better to leave them that way instead of using the permission-request API
Comment 3 Philippe Normand 2013-09-04 06:58:47 PDT
I think that permission-request signal was added after enter/leave-fullscreen?
I don't mind deprecating and switching to permission-request, for next cycle.
Comment 4 Carlos Garcia Campos 2013-09-04 08:04:43 PDT
I think we discussed it when it was added, but I don't remember why we decided to go with enter/leave signals. Maybe martin remembers. The thing is that now that we have those signals and you can prevent fullscreen from happening using that API, I see no reason for duplicating the functionality or make it it more complex with policies.
Comment 5 Brian Holt 2013-09-04 08:12:39 PDT
(In reply to comment #4)
> I think we discussed it when it was added, but I don't remember why we decided to go with enter/leave signals. Maybe martin remembers. The thing is that now that we have those signals and you can prevent fullscreen from happening using that API, I see no reason for duplicating the functionality or make it it more complex with policies.

After brainstorming a bit with Mario we reached much the same conclusion.  Unless there is some security issue about going fullscreen that we're missing, all the work to actually go fullscreen is done by the application so the existing signals are more than enough.  

Maybe I should just submit a patch updating the documentation for the permission-request signal?
Comment 6 Ben Boeckel 2013-09-04 08:16:48 PDT
(In reply to comment #4)
> I think we discussed it when it was added, but I don't remember why we decided to go with enter/leave signals. Maybe martin remembers. The thing is that now that we have those signals and you can prevent fullscreen from happening using that API, I see no reason for duplicating the functionality or make it it more complex with policies.

The problem (that I see) is that the user can't be asked on a per-request basis because to block the loading, you have to sit in the signal handler while you ask, which blocks the GUI thread.

> Unless there is some security issue about going fullscreen that we're missing, all the work to actually go fullscreen is done by the application so the existing signals are more than enough.

Security? Probably not. I do hate it when apps try to disobey my window manager though.

If permission-request isn't going to handle the fullscreen request, I guess a variable in uzbl can do the job.
Comment 7 Carlos Garcia Campos 2013-09-04 08:22:30 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > I think we discussed it when it was added, but I don't remember why we decided to go with enter/leave signals. Maybe martin remembers. The thing is that now that we have those signals and you can prevent fullscreen from happening using that API, I see no reason for duplicating the functionality or make it it more complex with policies.
> 
> The problem (that I see) is that the user can't be asked on a per-request basis because to block the loading, you have to sit in the signal handler while you ask, which blocks the GUI thread.

This is a good point, permission-request api is async, I think the internal fullscreen api is sync and that's probably the reason why we decided to use the enter/leave signals
Comment 8 Brian Holt 2013-09-04 08:28:10 PDT
> This is a good point, permission-request api is async, I think the internal fullscreen api is sync and that's probably the reason why we decided to use the enter/leave signals

So can I change the name of the bug and submit a patch to correct the documentation?
Comment 9 Mario Sanchez Prada 2013-09-04 08:35:18 PDT
(In reply to comment #8)
> > This is a good point, permission-request api is async, I think the internal fullscreen api is sync and that's probably the reason why we decided to use the enter/leave signals
> 
> So can I change the name of the bug and submit a patch to correct the documentation?

As the one who wrote the original (and misleding) documentation, I think this might be a good idea :)

When we wrote that we were thinking on examples that might make sense, and at that time the fullscreen thing looked like one of those, but after this discussion it seems obvious it's not the case so it's probably better to remove it from there to avoid more confusion.
Comment 10 Carlos Garcia Campos 2013-09-04 08:40:10 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > > This is a good point, permission-request api is async, I think the internal fullscreen api is sync and that's probably the reason why we decided to use the enter/leave signals
> > 
> > So can I change the name of the bug and submit a patch to correct the documentation?
> 
> As the one who wrote the original (and misleding) documentation, I think this might be a good idea :)
> 
> When we wrote that we were thinking on examples that might make sense, and at that time the fullscreen thing looked like one of those, but after this discussion it seems obvious it's not the case so it's probably better to remove it from there to avoid more confusion.

Well, I'm not sure, after reading the code in more detail, the fullscreen client C API implementation is GTK+ specific, so we can change it to make it async if we want. Maybe we can have a permission request object that tells you whether it's entering or leaving fullscreen that allows you to use a non-modal dialog in the UI. We can leave the signals as just notifications, for people not handling the permission-request signal, for example, and still checking the return value for backwards compatibility. The only problem is that current signal names are not appropriate for notifications, though.