Bug 192793

Created attachment 357581 [details] Patch

Created attachment 357590 [details] Patch

Created attachment 357605 [details] Patch

Created attachment 357639 [details] Patch

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

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/

Created attachment 357729 [details] Patch

Created attachment 357745 [details] Patch

Created attachment 357873 [details] Patch

Created attachment 357885 [details] Patch

Created attachment 358631 [details] Patch

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.

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.

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

Created attachment 358647 [details] Patch

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

(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.

(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:]?

> 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.

Created attachment 359296 [details] Patch

Comment on attachment 359296 [details] Patch r=me with the completion handler change we discussed in person.

Created attachment 359314 [details] Patch for landing

(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.

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

Created attachment 359331 [details] Patch for landing

Comment on attachment 359331 [details] Patch for landing Clearing flags on attachment: 359331 Committed r240100: <https://trac.webkit.org/changeset/240100>

All reviewed patches have been landed. Closing bug.

<rdar://problem/47335825>