Make it possible for WebCore to check to see if persistent permission to access media capture devices has been granted to the page's origin.
Created attachment 267052 [details] Preliminary patch for the bots. Patch without useful ChangeLog entries for the bots to mull over.
Attachment 267052 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 44 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 267056 [details] Patch.
Comment on attachment 267056 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=267056&action=review > Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:86 > + m_hasUserMediaPermission = access; Is storing in m_hasUserMediaPermission thread-safe given the callOnMainThread() below? > Source/WebCore/Modules/mediastream/UserMediaController.h:67 > +inline void UserMediaController::checkUserMediaPermission(Ref<UserMediaPermissionCheck>&& request) I don't really like this model of a "check" function taking ownership of the UserMediaPermissionCheck passed in. > Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.cpp:43 > +RefPtr<UserMediaPermissionCheck> UserMediaPermissionCheck::create(Document& document, UserMediaPermissionCheckClient& client) This should return a Ref<> > Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.cpp:81 > + m_protector = this; Weird. Why doesn't someone own this explicitly? > Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.h:69 > + RefPtr<UserMediaPermissionCheck> m_protector; Odd to have this here.
Comment on attachment 267056 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=267056&action=review >> Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:86 >> + m_hasUserMediaPermission = access; > > Is storing in m_hasUserMediaPermission thread-safe given the callOnMainThread() below? Yes, this is the callback from the permission checker so it is called exactly once. >> Source/WebCore/Modules/mediastream/UserMediaController.h:67 >> +inline void UserMediaController::checkUserMediaPermission(Ref<UserMediaPermissionCheck>&& request) > > I don't really like this model of a "check" function taking ownership of the UserMediaPermissionCheck passed in. The check function doesn't take ownership, it passes it along to the client (the WK1 or WK2 WebUserMediaClient ) which stores it in a map and calls the UI delegate (WK1) or page proxy (WK2). >> Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.cpp:43 >> +RefPtr<UserMediaPermissionCheck> UserMediaPermissionCheck::create(Document& document, UserMediaPermissionCheckClient& client) > > This should return a Ref<> Oops, will fix. >> Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.cpp:81 >> + m_protector = this; > > Weird. Why doesn't someone own this explicitly? Actually, the checker is owned by a MediaDevicesRequest so this isn't necessary. Will remove.
Comment on attachment 267056 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=267056&action=review >>> Source/WebCore/Modules/mediastream/UserMediaController.h:67 >>> +inline void UserMediaController::checkUserMediaPermission(Ref<UserMediaPermissionCheck>&& request) >> >> I don't really like this model of a "check" function taking ownership of the UserMediaPermissionCheck passed in. > > The check function doesn't take ownership, it passes it along to the client (the WK1 or WK2 WebUserMediaClient ) which stores it in a map and calls the UI delegate (WK1) or page proxy (WK2). So the argument type should just be UserMediaPermissionCheck& instead of Ref&&. Same for requestUserMediaAccess.
(In reply to comment #6) > Comment on attachment 267056 [details] > Patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=267056&action=review > > >>> Source/WebCore/Modules/mediastream/UserMediaController.h:67 > >>> +inline void UserMediaController::checkUserMediaPermission(Ref<UserMediaPermissionCheck>&& request) > >> > >> I don't really like this model of a "check" function taking ownership of the UserMediaPermissionCheck passed in. > > > > The check function doesn't take ownership, it passes it along to the client (the WK1 or WK2 WebUserMediaClient ) which stores it in a map and calls the UI delegate (WK1) or page proxy (WK2). > > So the argument type should just be UserMediaPermissionCheck& instead of > Ref&&. Same for requestUserMediaAccess. OK, fixed.
Created attachment 267102 [details] Updated patch.
Comment on attachment 267102 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=267102&action=review r=me with comments. > Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:84 > +void MediaDevicesRequest::didCompleteCheck(bool access) nit: I think we usually name booleans so that they start with something like is/can/should for clarity. > Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:91 > + RefPtr<MediaDevicesRequest> protectedThis(this); Hmm, this looks unsafe. MediaDevicesRequest is not ThreadSafeRefCounted. > Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.cpp:75 > + Document* document = downcast<Document>(scriptExecutionContext()); nit: I would prefer: auto& document = downcast<Document>(*scriptExecutionContext()); > Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.h:50 > +class UserMediaPermissionCheck : public ContextDestructionObserver, public RefCounted<UserMediaPermissionCheck> { Could be final ? > Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.h:41 > + static PassRefPtr<UserMediaPermissionCheckProxy> create(UserMediaPermissionRequestManagerProxy& manager, uint64_t userMediaID) We don't use PassRefPtr anymore. This should return a Ref<> > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:63 > +PassRefPtr<UserMediaPermissionCheckProxy> UserMediaPermissionRequestManagerProxy::createUserMediaPermissionCheck(uint64_t userMediaID) Should return a Ref<>. > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:65 > + RefPtr<UserMediaPermissionCheckProxy> request = UserMediaPermissionCheckProxy::create(*this, userMediaID); Ref<> > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:66 > + m_pendingDeviceRequests.add(userMediaID, request.get()); request.ptr() > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:40 > + PassRefPtr<UserMediaPermissionCheckProxy> createUserMediaPermissionCheck(uint64_t userMediaID); Ref<> > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:90 > +void UserMediaPermissionRequestManager::startUserMediaPermissionCheck(WebCore::UserMediaPermissionCheck& request) You don't need the WebCore:: > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:92 > + Document* document = downcast<Document>(request.scriptExecutionContext()); auto* > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:84 > + DEPRECATED_DEFINE_STATIC_LOCAL(UserMediaCheckMap, requests, ()); static NeverDestroyed<> ? > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:119 > +void WebUserMediaClient::requestUserMediaAccess(UserMediaRequest& prpRequest) We should get rid of the prp prefix > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:123 > + UserMediaRequest* request = &prpRequest; We don't really need this local variable anymore I believe. > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:153 > +void WebUserMediaClient::checkUserMediaPermission(UserMediaPermissionCheck& prpRequest) We should drop the prp prefix > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:157 > + UserMediaPermissionCheck* request = &prpRequest; Not needed? > Tools/WebKitTestRunner/TestController.h:289 > + typedef Vector<std::pair<WKRetainPtr<WKSecurityOriginRef>, WKRetainPtr<WKUserMediaPermissionRequestRef>>> PermissionRequestMap; Calling this "Map" is a tad confusing. Maybe List or Vector?
Comment on attachment 267102 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=267102&action=review > Source/WebCore/Modules/mediastream/UserMediaPermissionCheck.h:34 > +#include <wtf/PassRefPtr.h> Useless include. > Source/WebKit2/UIProcess/UserMediaPermissionCheckProxy.h:31 > +#include <wtf/PassRefPtr.h> Useless include.
Created attachment 267116 [details] Patch for landing.
Attachment 267116 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:25: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 267121 [details] Another updated patch for landing.
Committed r193944: https://trac.webkit.org/r193944