[Merge] lp:~uriboni/webbrowser-app/media-access into lp:webbrowser-app

[Chris Coulson]

I've had a brief look and left a comment inline.

The application shouldn't really be storing these decisions - it doesn't work properly for media access in some circumstances anyway because Chromium needs access to these decisions for navigator.mediaDevices.enumerateDevices to work properly (the device names are scrubbed unless permission has already been granted to a domain).

(It's even worse for notifications because a site has to explicitly request permission before sending a notification. This is how we ended up with a hack in Oxide to remember these decisions for the rest of a session).

Site-specific settings really belong in Oxide (see https://blueprints.launchpad.net/oxide/+spec/site-settings, and https://docs.google.com/document/d/1EZV4KzrH9eEUMjh3G8Cdykw3JTBtqOlunEEZd1kothA/edit)

Diff comments:

> === modified file 'src/app/webbrowser/Browser.qml'
> --- src/app/webbrowser/Browser.qml	2015-09-28 08:15:10 +0000
> +++ src/app/webbrowser/Browser.qml	2015-09-30 14:10:59 +0000
> @@ -75,6 +77,49 @@
>          }
>      }
>  
> +    Connections {
> +        target: currentWebview
> +
> +        function askPermission(request) {
> +            var dialog = PopupUtils.open(mediaAccessDialogComponent, null, {
> +                request: request
> +            });
> +
> +            dialog.visibleChanged.connect(function() {
> +                if (dialog.request.isForAudio && dialog.allowAudio ||
> +                    dialog.request.isForVideo && dialog.allowVideo) dialog.request.allow()
> +                else dialog.request.deny()
> +
> +                MediaAccessModel.set(UrlUtils.extractHost(dialog.request.origin),
> +                                     (dialog.request.isForAudio) ? dialog.allowAudio : undefined,
> +                                     (dialog.request.isForVideo) ? dialog.allowVideo : undefined)
> +            })
> +        }
> +        onMediaAccessPermissionRequested: {
> +            var permissions = MediaAccessModel.get(UrlUtils.extractHost(request.origin))

This is wrong for a couple of reasons:
- If you allow device access for https://www.google.com/ but then Oxide asks for http://www.google.com, you should pop up a request again. This won't. Permissions are meant to be per-origin (which includes the scheme), not per-host.
- The embedder origin provided by Oxide is there to be used as well - if you allow device access to an origin, it doesn't necessarily mean that origin should be allowed the same access if it's embedded in another page.

> +            if (request.isForAudio && request.isForVideo) {
> +                // When isForAudio and isForVideo are true in the same request, Oxide
> +                // does not provide a way to allow or deny these requests separately, so
> +                // allow both if we have both permissions, otherwise ask permission again
> +                // (which will be for both at the same time, so the user will have to make
> +                // the choice explicitly)
> +                if (permissions.audio === true && permissions.video === true) request.allow()
> +                else if (permissions.audio === false && permissions.video === false) request.deny()
> +                else askPermission(request)
> +            } else {
> +                var permission = (request.isForAudio) ? permissions.audio : permissions.video
> +                if (permission === true) request.allow()
> +                else if (permission === false) request.deny()
> +                else askPermission(request)
> +            }
> +        }
> +    }
> +
> +    Component {
> +        id: mediaAccessDialogComponent
> +        MediaAccessDialog { }
> +    }
> +
>      actions: [
>          Actions.GoTo {
>              onTriggered: currentWebview.url = value


-- 
https://code.launchpad.net/~uriboni/webbrowser-app/media-access/+merge/272919
Your team Ubuntu Phablet Team is subscribed to branch lp:webbrowser-app.