[webkit-reviews] review granted: [Bug 185761] [GTK][WPE] Add mediaDevices.enumerateDevices support : [Attachment 351952] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 185761: [GTK][WPE] Add mediaDevices.enumerateDevices support
https://bugs.webkit.org/show_bug.cgi?id=185761

Attachment 351952: Patch

https://bugs.webkit.org/attachment.cgi?id=351952&action=review




--- Comment #61 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 351952
  --> https://bugs.webkit.org/attachment.cgi?id=351952
Patch

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

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:35
> +#include <wtf/text/WTFString.h>

Some of these includes are not needed right now.
WTFString.h is probably already included in FileSystem.h or StrginBuilder.h

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:50
> +String
DeviceIdHashSaltStorage::getDeviceIdHashSaltForOrigin(UserMediaPermissionCheckP
roxy& request)

s/getDeviceIdHashSaltForOrigin/deviceIdHashSaltForOrigin/
Ditto below.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:67
> +String DeviceIdHashSaltStorage::getDeviceIdHashSaltForOrigin(SecurityOrigin&
documentOrigin)

can it return a const String&

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:95
> +    RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler
= WTFMove(completionHandler)]() mutable {

For now, we are only main thread based, so we can directly call
completionHandler.
If we start do these things in a background thread, we might need to isolate
copy origins.

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:106
> +    RunLoop::main().dispatch(WTFMove(completionHandler));

completionHandler() for now?

> Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:65
> +    String getDeviceIdHashSaltForOrigin(WebCore::SecurityOrigin&);

I wonder whether we should not expose directly this one and not the one taking
a UserMediaPermissionCheckProxy.