feat: route frame based permission checks through our permission check handler #14569
Conversation
MarshallOfSound
force-pushed
the
route-permission-checks
branch
from
f946f6c
to
93e1e21
Compare
| @@ -245,9 +246,13 @@ bool AtomPermissionManager::CheckPermissionWithDetails( | |||
| blink::mojom::PermissionStatus | |||
| AtomPermissionManager::GetPermissionStatusForFrame( | |||
| content::PermissionType permission, | |||
| content::RenderFrameHost* render_frame_host, | |||
| content::RenderFrameHost* rfh, | |||
There was a problem hiding this comment.
👎
Abbreviations are evil.
|
|
||
| Sets the handler which can be used to respond to permission checks for the `session`. | ||
| Returning `true` will allow the permission and `false` will reject it. | ||
| To clear the handler, call `setPermissionCheckHandler(null)`. | ||
|
|
||
| Please note not all syncronous permission checks are passed through this handler, | ||
| for instance `Notification.granted` is not routed but `new Notification` will cause | ||
| the `permissionRequestHandler` to be fired. |
There was a problem hiding this comment.
This is confusing. Is returning false from this function enough to prevent usage of the feature? If not, what else do I have to do?
There was a problem hiding this comment.
@nornagon In some cases returning false here will block usage of the feature, in other cases you'll have to return false here and block it in the request handler as well
There was a problem hiding this comment.
As a developer, that's totally useless information to me unless the docs specifically describe what this will prevent.
I understand if the intention is that ultimately, returning false from this function will block the usage of that feature the same as if the user had clicked 'block' in the browser, but it's not currently true because we haven't finished implementing the feature. If that's the case, then we should mark this function experimental.
There was a problem hiding this comment.
@nornagon I'm not sure how to phrase this but basically there are 3 types of checks coming through / not coming through this handler now
- A check coming through the handler that results in a feature being enabled / disabled (see the
mediapermission) - A check coming through the handler that results in a feature "appearing" to be enabled / disabled (
Notification.granted) but require the request handler to actually allow / deny the request. I.e. Denying the check but allowing the request would make the feature active. - Checks not coming through the handler right now (notification checks aren't router here yet)
There was a problem hiding this comment.
will block the usage of that feature the same as if the user had clicked 'block' in the browser
That functionality comes from setPermissionRequestHandler, it's asyncronous
There was a problem hiding this comment.
By using "for instance" in docs here, it is not clear which checks will come through the handler and which will not.
I think we should list all checks that will not come through the handler, and mark that type of check as experimental.
|
|
||
| Sets the handler which can be used to respond to permission checks for the `session`. | ||
| Returning `true` will allow the permission and `false` will reject it. | ||
| To clear the handler, call `setPermissionCheckHandler(null)`. | ||
|
|
||
| Please note not all syncronous permission checks are passed through this handler, | ||
| for instance `Notification.granted` is not routed but `new Notification` will cause | ||
| the `permissionRequestHandler` to be fired. |
There was a problem hiding this comment.
By using "for instance" in docs here, it is not clear which checks will come through the handler and which will not.
I think we should list all checks that will not come through the handler, and mark that type of check as experimental.
|
@MarshallOfSound Do you still want to work on this? |
|
Closing as all activities on this PR seem to be dead. |
|
@codebytere I thought #18773 was a backport of #18757? The title looks similar though. I merged #18773 because it was marked fast-track. |
Fixes #14544
There is a TODO remaining because the legacy chromium permission status API does not pass a RFH or any way to get a web contents so it can't be emitted on a session object. Current plan is to leave it there till chromium removes that API. Chromium is already suggesting the frame based API over the old origin based one.
https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_manager.h?dr=CSs&g=0&l=71
Notes: Add more of chromiums permission checks to the permission check handler API