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.
<rdar://problem/45784512>
Created attachment 353773 [details] Patch
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?
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.
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.
Created attachment 353804 [details] Patch for landing.
Comment on attachment 353804 [details] Patch for landing. Clearing flags on attachment: 353804 Committed r237784: <https://trac.webkit.org/changeset/237784>
All reviewed patches have been landed. Closing bug.
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.
Committed r237788: <https://trac.webkit.org/changeset/237788>
(In reply to Fujii Hironori from comment #10) > Committed r237788: <https://trac.webkit.org/changeset/237788> Thank you!
(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
Re-opened since this is blocked by bug 191270
Created attachment 353981 [details] Updated patch
Comment on attachment 353981 [details] Updated patch Clearing flags on attachment: 353981 Committed r237879: <https://trac.webkit.org/changeset/237879>