Bug 192793 – Add a new SPI for controlling getUserMedia

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Add a new SPI for controlling getUserMedia

[youenn fablet]

Created attachment 357581 [details]
Patch

[youenn fablet]

Created attachment 357590 [details]
Patch

[youenn fablet]

Created attachment 357605 [details]
Patch

[youenn fablet]

Created attachment 357639 [details]
Patch

[Alex Christensen]

Comment on attachment 357639 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:862
>      auto decisionHandler = makeBlockPtr([protectedRequest = makeRef(request)](BOOL authorized) {

This needs a CompletionHandlerCallChecker

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:996
> +        auto decisionHandler = makeBlockPtr([protectedRequest = makeRef(request)](BOOL authorized) {

ditto

[Eric Carlson]

Comment on attachment 357639 [details]
Patch

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

> Source/WebKit/ChangeLog:16
> +        Did some refactoring by making the callbacks not returning false.

Nit: s/not returning false/not return bool/

[youenn fablet]

Created attachment 357729 [details]
Patch

[youenn fablet]

Created attachment 357745 [details]
Patch

[youenn fablet]

Created attachment 357873 [details]
Patch

[youenn fablet]

Created attachment 357885 [details]
Patch

[youenn fablet]

Created attachment 358631 [details]
Patch

[Alex Christensen]

Comment on attachment 358631 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:111
> +- (void)_webView:(WKWebView *)webView requestMediaCaptureAuthorization:(_WKCaptureDevices)devices decisionHandler:(void (^)(BOOL authorized))decisionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> +- (void)_webView:(WKWebView *)webView isMediaCaptureAuthorized:(void (^)(_WKCaptureDevices authorizedDevices))decisionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

I don't think it is a good design to have two callbacks that are this similar asking a very similar question.  It requires app developers to know lots of details about the implementation and set up a finite state machine in a certain way.

[Alex Christensen]

We discussed this, and I think it would be much more clear if one callback indicated it was for getUserMedia and one callback was for enumerateDevices.

[youenn fablet]

Yes, we can improve the name of the second callback to tie it more to enumerateDevices.
Something like canExposeMediaCaptureDeviceInformation for instance.

[youenn fablet]

Created attachment 358647 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #15)
> Created attachment 358647 [details]
> Patch

I went with the following name: queryExposableMediaCaptureDeviceInformation

[Eric Carlson]

(In reply to Alex Christensen from comment #12)
> Comment on attachment 358631 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358631&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:111
> > +- (void)_webView:(WKWebView *)webView requestMediaCaptureAuthorization:(_WKCaptureDevices)devices decisionHandler:(void (^)(BOOL authorized))decisionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> > +- (void)_webView:(WKWebView *)webView isMediaCaptureAuthorized:(void (^)(_WKCaptureDevices authorizedDevices))decisionHandler WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> I don't think it is a good design to have two callbacks that are this
> similar asking a very similar question.  

I don't agree that the questions are all that similar. One says:

    Ask the user to see if the page can access data from the
    camera/microphone right now

the other says: 

    Has this user given permission to access camera/microphone
    without a prompt at some time in the future?


> It requires app developers to know lots of details about the 
> implementation and set up a finite state machine in a certain way.

I don't understand this, why does the app need a FSM? In the first case the app prompts the user, and in the second case it checks for a stored setting.

[Eric Carlson]

(In reply to Alex Christensen from comment #13)
> We discussed this, and I think it would be much more clear if one callback
> indicated it was for getUserMedia and one callback was for enumerateDevices.

One callback is not just for enumerateDevices.


(In reply to youenn fablet from comment #16)
> (In reply to youenn fablet from comment #15)
> > Created attachment 358647 [details]
> > Patch
> 
> I went with the following name: queryExposableMediaCaptureDeviceInformation

I agree that the original name can be improved, but I don't think this name is right either. This callback is not just asking about what should be exposed by enumerateDevices, it is asking if the user has given permission to access camera/microphone without a prompt. We may *also* use this to expose device IDs etc before gUM has been called, but it is not the only use of the information provided by this callback.

How about -[_webView:hasPermissionToCaptureWithoutUserPrompt:]?

[youenn fablet]

> I agree that the original name can be improved, but I don't think this name
> is right either. This callback is not just asking about what should be
> exposed by enumerateDevices, it is asking if the user has given permission
> to access camera/microphone without a prompt. We may *also* use this to
> expose device IDs etc before gUM has been called, but it is not the only use
> of the information provided by this callback.
> 
> How about -[_webView:hasPermissionToCaptureWithoutUserPrompt:]?

Currently, we are only using the second callback to control enumerateDevices.
A callback dedicated to enumerateDevices has some benefits:
- This offers the flexibility so that one can implement Chrome, Firefox or Safari enumerateDevices  behavior
- This makes it clear to developers which callback they should implement to allow/disallow getUserMedia. Otherwise, developers might implement "hasPermissionToCaptureWithoutUserPrompt", return yes, and getUserMedia call will still fail.

[youenn fablet]

Created attachment 359296 [details]
Patch

[Eric Carlson]

Comment on attachment 359296 [details]
Patch

r=me with the completion handler change we discussed in person.

[youenn fablet]

Created attachment 359314 [details]
Patch for landing

[youenn fablet]

(In reply to Eric Carlson from comment #21)
> Comment on attachment 359296 [details]
> Patch
> 
> r=me with the completion handler change we discussed in person.

Thanks for the review.
Yes, I updated the default implementation accordingly.

[WebKit Commit Bot]

Comment on attachment 359314 [details]
Patch for landing

Rejecting attachment 359314 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 359314, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/10777103

[youenn fablet]

Created attachment 359331 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 359331 [details]
Patch for landing

Clearing flags on attachment: 359331

Committed r240100: <https://trac.webkit.org/changeset/240100>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47335825>