[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