Bug 223806 - Promote WKWebView getUserMedia SPI to API
Summary: Promote WKWebView getUserMedia SPI to API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
: 220416 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-03-26 11:16 PDT by youenn fablet
Modified: 2021-06-09 11:52 PDT (History)
7 users (show)

See Also:


Attachments
Patch (50.14 KB, patch)
2021-03-26 11:29 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (50.18 KB, patch)
2021-03-26 13:50 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (50.17 KB, patch)
2021-03-28 23:57 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-03-26 11:16:23 PDT
Promote WKWebView getUserMedia SPI to API
Comment 1 youenn fablet 2021-03-26 11:29:59 PDT
Created attachment 424379 [details]
Patch
Comment 2 Eric Carlson 2021-03-26 12:03:12 PDT
Comment on attachment 424379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424379&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegate.h:147
> + @discussion If not implemented, the result is the same as calling the decisionHandler with WKPermissionDecisionPrompt.
> +
> + If you do not implement this method, the web view will behave as if the user selected the Cancel button.

These don't agree with one another.

I wonder if we should mention that the call will be denied if app doesn't have the camera and/or microphone entitlement?
Comment 3 youenn fablet 2021-03-26 13:50:46 PDT
Created attachment 424400 [details]
Patch
Comment 4 Sam Weinig 2021-03-26 15:47:11 PDT
Comment on attachment 424400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424400&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegate.h:58
> +typedef NS_ENUM(NSInteger, WKMediaCaptureType) {
> +    WKMediaCaptureTypeCamera,
> +    WKMediaCaptureTypeMicrophone,
> +    WKMediaCaptureTypeCameraAndMicrophone,
> +} WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

This seems like something an NS_OPTIONS style bit mask would be better suited for, and therefore you wouldn't need an explicit WKMediaCaptureTypeCameraAndMicrophone value.
Comment 5 youenn fablet 2021-03-27 06:05:01 PDT
(In reply to Sam Weinig from comment #4)
> Comment on attachment 424400 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424400&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegate.h:58
> > +typedef NS_ENUM(NSInteger, WKMediaCaptureType) {
> > +    WKMediaCaptureTypeCamera,
> > +    WKMediaCaptureTypeMicrophone,
> > +    WKMediaCaptureTypeCameraAndMicrophone,
> > +} WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> This seems like something an NS_OPTIONS style bit mask would be better
> suited for, and therefore you wouldn't need an explicit
> WKMediaCaptureTypeCameraAndMicrophone value.

We could go both ways. I
went this way for two reasons:
1. With a bit field, there is the theoretical possibility of asking for 'no camera' and 'no microphone'. Patch shows an API test previously dealing with this case.
2. Enum works fine with the usual cases for this API:
a. Deny always: return WKPermissionDecisionDeny
b. Grant always: return WKPermissionDecisionGrant
c. Grant only microphone: return type == WKMediaCaptureTypeMicrophone ? WKPermissionDecisionGrant : WKPermissionDecisionDeny
d. Grant only camera: return type == WKMediaCaptureTypeCamera ? WKPermissionDecisionGrant : WKPermissionDecisionDeny
3. Enum works fine when implementing prompting:
switch (type) {
case WKMediaCaptureTypeCamera:
    return "Allow access to camera(s)";
case WKMediaCaptureTypeMicrophone:
    return "Allow access to microphone(s)";
case WKMediaCaptureTypeCameraAndMicrophone:
    return "Allow access to camera(s) and microphone(s)";
}
Comment 6 youenn fablet 2021-03-27 06:05:19 PDT
Comment on attachment 424400 [details]
Patch

WPE error unrelated
Comment 7 Eric Carlson 2021-03-27 07:46:49 PDT
Comment on attachment 424400 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424400&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:377
> + @param state state to apply for capture.

s/state state/state State/

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:378
> + @param completionHandler A block to invoke with the return value of the function call.

The function has not return value, so maybe "A block to invoke after the camera state has been changed.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:388
> + @param state state to apply for capture.
> + @param completionHandler A block to invoke with the return value of the function call.

Ditto the above.
Comment 8 Sam Weinig 2021-03-27 10:51:49 PDT
(In reply to youenn fablet from comment #5)
> (In reply to Sam Weinig from comment #4)
> > Comment on attachment 424400 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=424400&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegate.h:58
> > > +typedef NS_ENUM(NSInteger, WKMediaCaptureType) {
> > > +    WKMediaCaptureTypeCamera,
> > > +    WKMediaCaptureTypeMicrophone,
> > > +    WKMediaCaptureTypeCameraAndMicrophone,
> > > +} WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > 
> > This seems like something an NS_OPTIONS style bit mask would be better
> > suited for, and therefore you wouldn't need an explicit
> > WKMediaCaptureTypeCameraAndMicrophone value.
> 
> We could go both ways. I
> went this way for two reasons:
> 1. With a bit field, there is the theoretical possibility of asking for 'no
> camera' and 'no microphone'. Patch shows an API test previously dealing with
> this case.
> 2. Enum works fine with the usual cases for this API:
> a. Deny always: return WKPermissionDecisionDeny
> b. Grant always: return WKPermissionDecisionGrant
> c. Grant only microphone: return type == WKMediaCaptureTypeMicrophone ?
> WKPermissionDecisionGrant : WKPermissionDecisionDeny
> d. Grant only camera: return type == WKMediaCaptureTypeCamera ?
> WKPermissionDecisionGrant : WKPermissionDecisionDeny
> 3. Enum works fine when implementing prompting:
> switch (type) {
> case WKMediaCaptureTypeCamera:
>     return "Allow access to camera(s)";
> case WKMediaCaptureTypeMicrophone:
>     return "Allow access to microphone(s)";
> case WKMediaCaptureTypeCameraAndMicrophone:
>     return "Allow access to camera(s) and microphone(s)";
> }

The biggest problem with this approach is it leads no real room for new types in the future without exponential items to cover all combinations. That seems like a bigger deal than documenting that you must pass in at least one type for it to be meaningful.
Comment 9 youenn fablet 2021-03-27 11:21:35 PDT
> The biggest problem with this approach is it leads no real room for new
> types in the future without exponential items to cover all combinations.
> That seems like a bigger deal than documenting that you must pass in at
> least one type for it to be meaningful.

This delegate is scoped to getUserMedia which by design is dedicated to microphone and/or camera. Hence why the initial proposal was booleans. I changed to an enum as it is as nice or nicer to developers and might still allow some extension, should we see the need in the future.

I am not sure which additional future type we are talking about here.
One known feature in that area is screen sharing.
Though we have no concrete plan yet on how to control screen sharing, we think another delegate might be a better fit.

We plan to have specific delegates for geolocation or device motion orientation as well.
Comment 10 Sam Weinig 2021-03-27 17:45:58 PDT
(In reply to youenn fablet from comment #9)
> > The biggest problem with this approach is it leads no real room for new
> > types in the future without exponential items to cover all combinations.
> > That seems like a bigger deal than documenting that you must pass in at
> > least one type for it to be meaningful.
> 
> This delegate is scoped to getUserMedia which by design is dedicated to
> microphone and/or camera. Hence why the initial proposal was booleans. I
> changed to an enum as it is as nice or nicer to developers and might still
> allow some extension, should we see the need in the future.

getUserMedia is scoped to microphone and/or camera *today*. There may very well be other sensors getUserMedia will deal with in the future. When designing APIs, we should be cognizant that we don't know what the future brings, and design APIs to accommodate.
Comment 11 youenn fablet 2021-03-28 02:08:00 PDT
> getUserMedia is scoped to microphone and/or camera *today*.

Right and getUserMedia model is far from being perfect so I hope we keep it that way.

> There may very well be other sensors getUserMedia will deal with in the future.

It would not be great to ask users a question like: give me "camera AND microphone AND xray AND telescope" or fail.
CameraAndMicrophone combo is an exception, given how this API is biased towards video conferencing.
Comment 12 Sam Weinig 2021-03-28 14:26:03 PDT
Ok, that’s fair I guess.
Comment 13 youenn fablet 2021-03-28 23:57:12 PDT
Created attachment 424530 [details]
Patch for landing
Comment 14 EWS 2021-03-29 09:33:04 PDT
Committed r275162: <https://commits.webkit.org/r275162>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424530 [details].
Comment 15 Radar WebKit Bug Importer 2021-03-29 09:34:19 PDT
<rdar://problem/75957792>
Comment 16 youenn fablet 2021-05-20 05:57:28 PDT
*** Bug 220416 has been marked as a duplicate of this bug. ***
Comment 17 Kyle Dunn 2021-05-20 08:17:56 PDT
Youenn, thanks for getting this resolved! Is there a way for me to determine when this will land in a public iOS build? Or will you update this issue as the time comes?
Comment 18 Kyle Dunn 2021-06-09 11:52:07 PDT
FYI - this appears to have landed in iOS 15. The particulars for this addition are demo'd @ 14:40 here: https://developer.apple.com/videos/play/wwdc2021/10032/