[webkit-reviews] review granted: [Bug 229308] Use UserMediaRequestIdentifier in WebKit rather than a mysterious uint64_t : [Attachment 435938] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Aug 20 00:29:02 PDT 2021
youenn fablet <youennf at gmail.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 229308: Use UserMediaRequestIdentifier in WebKit rather than a mysterious
uint64_t
https://bugs.webkit.org/show_bug.cgi?id=229308
Attachment 435938: Patch
https://bugs.webkit.org/attachment.cgi?id=435938&action=review
--- Comment #5 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 435938
--> https://bugs.webkit.org/attachment.cgi?id=435938
Patch
Looks good.
Can you put UserMediaRequestIdentifier in its own UserMediaRequestIdentifier.h
and split m_pendingDeviceRequests in another patch with another type?
View in context: https://bugs.webkit.org/attachment.cgi?id=435938&action=review
> Source/WebCore/ChangeLog:11
> + * Modules/mediastream/UserMediaRequest.h:
It is probably best to put UserMediaRequestIdentifier in its own
UserMediaRequestIdentifier.h header.
That will reduce includes and reduce the patch size as well.
> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:649
> + // This sends UserMediaRequestIdentifierType of 0 because this does not
correspond to a UserMediaPermissionRequest in web process.
s/This sends/We use/
> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:689
> + auto requestID = UserMediaRequestIdentifier::generate();
request is not a user media request but a permission request.
I am not sure we should use the same identifier type as it makes things more
difficult to read.
We could use its own ObjectIdentifier<UserMediaPermissionRequest> type.
> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:804
> + auto requestID = UserMediaRequestIdentifier::generate();
Ditto
> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.h:155
> + HashSet<WebCore::UserMediaRequestIdentifier> m_pendingDeviceRequests;
Ditto.
> Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.h:23
> +#include "IdentifierTypes.h"
Why do we need this one?
More information about the webkit-reviews
mailing list