Bug 191227 – [MediaStream] User should not be prompted again after denying getDisplayMedia request

• Component: WebRTC
• Status: RESOLVED FIXED

[Eric Carlson]

As is the case with camera and microphone access, a user should not be prompted again if they deny a page's request for screen capture.

[Radar WebKit Bug Importer]

<rdar://problem/45784512>

[Eric Carlson]

Created attachment 353773 [details]
Patch

[youenn fablet]

Comment on attachment 353773 [details]
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?

[Eric Carlson]

Comment on attachment 353773 [details]
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;

True, we shouldn't get this far if there are not capable video devices. Added and ASSERT.

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

Good idea, I added a helper function.

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

Fixed.

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

Great suggestion, changed.

[Eric Carlson]

Comment on attachment 353773 [details]
Patch

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

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

This variable isn't actually needed with the change suggested below, removed.

[Eric Carlson]

Created attachment 353804 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 353804 [details]
Patch for landing.

Clearing flags on attachment: 353804

Committed r237784: <https://trac.webkit.org/changeset/237784>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

Some ports not ENABLE(MEDIA_STREAM) can't build.

https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20Ubuntu%20LTS%20%28Build%29/builds/16960

> FAILED: /usr/lib/ccache/clang++  (...) -c DerivedSources/WebKit/unified-sources/UnifiedSource26.cpp
> In file included from DerivedSources/WebKit/unified-sources/UnifiedSource26.cpp:6:
> ../../Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:275:9: error: use of undeclared identifier 'wasRequestDenied'
>     if (wasRequestDenied(frameID, userMediaDocumentOrigin, topLevelDocumentOrigin, requestingMicrophone, requestingCamera, requestingScreenCapture))
>         ^
> ../../Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:281:12: error: use of undeclared identifier 'searchForGrantedRequest'
>     return searchForGrantedRequest(frameID, userMediaDocumentOrigin, topLevelDocumentOrigin, requestingMicrophone, requestingCamera) ? RequestAction::Grant : RequestAction::Prompt;
>            ^
> 2 errors generated.

[Fujii Hironori]

Committed r237788: <https://trac.webkit.org/changeset/237788>

[Eric Carlson]

(In reply to Fujii Hironori from comment #10)
> Committed r237788: <https://trac.webkit.org/changeset/237788>

Thank you!

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #7)
> Committed r237784: <https://trac.webkit.org/changeset/237784>

The following layout tests are failing after this change:
fast/mediastream/getUserMedia-deny-persistency.html
fast/mediastream/getUserMedia-deny-persistency2.html
fast/mediastream/getUserMedia-deny-persistency3.html

--- /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/fast/mediastream/getUserMedia-deny-persistency-expected.txt
+++ /Volumes/Data/slave/sierra-release-tests-wk2/build/layout-test-results/fast/mediastream/getUserMedia-deny-persistency-actual.txt
@@ -1,3 +1,3 @@
 
-PASS Testing same page getUserMedia deny persistency with audio denied 
+FAIL Testing same page getUserMedia deny persistency with audio denied assert_equals: expected "NotAllowedError" but got "TypeError"
 
https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r237785%20(12559)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 191270

[Eric Carlson]

Created attachment 353981 [details]
Updated patch

[WebKit Commit Bot]

Comment on attachment 353981 [details]
Updated patch

Clearing flags on attachment: 353981

Committed r237879: <https://trac.webkit.org/changeset/237879>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.