search

sailfishos / sailfish-components-webview

Mozilla Public License 2.0
13 stars 23 forks source link

[sailfish-components-webview] Only populate permissions model with known permissions. Contributes to JB#56450 OMP#JOLLA-550 #140

Closed chriadam closed 2 years ago

chriadam commented 2 years ago

If a permission isn't one of the permissions we are aware of (and have an icon for) don't include it in the permissions model which exposes it to QML.

Contributes to JB#56450

chriadam commented 2 years ago

This is still WIP. Need to confirm that this approach is valid, and that we cannot enumerate the supported permissions programmatically.

llewelld commented 2 years ago

I should add that I tested this and found that it addressed the problem: the "Site permissions" button now appears on the sites where it was missing before (at least for the few I tested).

llewelld commented 2 years ago

One small point: the commit title needs updating to include OMP#JOLLA-550.

chriadam commented 2 years ago

Thanks, updated commit message with bug tag.

Do you have a suggestion for: a) whether we should centralise the "known permission types" b) how to offer that as API which can be queries in the appropriate places

Or, do you think we should leave this as-is for now, to avoid unnecessary complexity?

llewelld commented 2 years ago

a) whether we should centralise the "known permission types" b) how to offer that as API which can be queries in the appropriate places

I'm not sure. Off the top of my head, it probably doesn't make sense to change anything in the privileged javascript (gecko + embedlite-components) layers where they're all handled as strings already, since this approach comes from upstream.

That leaves the QML layer, where it might make more sense to provide an enum (maybe exposed through the PermissionModel here?) instead of using strings.

If there's a very specific set of permissions that the UI is willing/able to handle (which seems to be the case, and also seems unlikely to change), then using an enum in the QML layer would make sense in my opinion.

Or, do you think we should leave this as-is for now, to avoid unnecessary complexity?

Unless it's a very quick task, I'd personally be inclined to create another task for it. In my opinion your PR here already fixes the issue for the user.

Eventually we may need to provide an interface to the permissions as part of the PopupInterface. So it might be worth thinking about whether it would be better to provide this using an enum or a string. Maybe that could also fall under the task.