[webkit-reviews] review granted: [Bug 191227] [MediaStream] User should not be prompted again after denying getDisplayMedia request : [Attachment 353773] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 191227: [MediaStream] User should not be prompted again after denying
getDisplayMedia request
https://bugs.webkit.org/show_bug.cgi?id=191227

Attachment 353773: Patch

https://bugs.webkit.org/attachment.cgi?id=353773&action=review




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 353773
  --> https://bugs.webkit.org/attachment.cgi?id=353773
Patch

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

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:289
> +	   bool requestingScreenCapture = localUserRequest.type ==
MediaStreamRequest::Type::DisplayMedia && !videoDevices.isEmpty();

Do we need !videoDevices.isEmpty()?
I would have believed the following:
bool requestingScreenCapture = localUserRequest.type ==
MediaStreamRequest::Type::DisplayMedia;

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:290
> +	   bool requestingCamera = !requestingScreenCapture &&
!videoDevices.isEmpty();

It seems we should also introduce:
requestingMicrophone = !requestingScreenCapture && !audioDevices.isEmpty();

I wonder whether we should introduce a new utility function that would return
whether the request is denied, granted or requires prompt.
Then we could write this method as:
if (localUserRequest.type == MediaStreamRequest::Type::DisplayMedia)
    return Prompt;
...


Another possibility would to make it return an std::optional<DeniedOrGranted>.
We would then write:
if (localUserRequest.type == MediaStreamRequest::Type::DisplayMedia)
    return { };
...

And in the caller:
if (auto decision = checkRequestDecision(...)) {
    // deny or grant
    return;
}
// Do the prompt dance.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/GetDisplayMedia.mm:161
> +    int m_denialCount { 0 };

s/int/unsigned/

> Tools/TestWebKitAPI/Tests/WebKitCocoa/GetDisplayMedia.mm:184
> +    promptForCapture(@"{ video: true }", false);

Should we set shoulDeny = false here or maybe in the callback that is checking
it?