Bug 229308 - Use UserMediaRequestIdentifier in WebKit rather than a mysterious uint64_t
Summary: Use UserMediaRequestIdentifier in WebKit rather than a mysterious uint64_t
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on: 229278
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-19 14:48 PDT by Simon Fraser (smfr)
Modified: 2021-08-20 20:28 PDT (History)
15 users (show)

See Also:


Attachments
Patch (45.89 KB, patch)
2021-08-19 14:51 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (46.00 KB, patch)
2021-08-19 15:52 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (45.94 KB, patch)
2021-08-19 21:14 PDT, Simon Fraser (smfr)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (48.19 KB, patch)
2021-08-19 21:39 PDT, Simon Fraser (smfr)
youennf: review+
Details | Formatted Diff | Diff
Patch (55.84 KB, patch)
2021-08-20 14:20 PDT, Simon Fraser (smfr)
youennf: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2021-08-19 14:48:45 PDT
Use UserMediaRequestIdentifier in WebKit rather than a mysterious uint64_t
Comment 1 Simon Fraser (smfr) 2021-08-19 14:51:55 PDT
Created attachment 435908 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-08-19 15:52:38 PDT
Created attachment 435911 [details]
Patch
Comment 3 Simon Fraser (smfr) 2021-08-19 21:14:18 PDT
Created attachment 435937 [details]
Patch
Comment 4 Simon Fraser (smfr) 2021-08-19 21:39:56 PDT
Created attachment 435938 [details]
Patch
Comment 5 youenn fablet 2021-08-20 00:29:02 PDT
Comment on attachment 435938 [details]
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?
Comment 6 Simon Fraser (smfr) 2021-08-20 14:20:02 PDT
Created attachment 436027 [details]
Patch
Comment 7 youenn fablet 2021-08-20 14:43:23 PDT
Comment on attachment 436027 [details]
Patch

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

> Source/WebKit/Scripts/webkit/messages.py:763
> +        'WebCore::UserMediaRequestIdentifier': ['<WebCore/UserMediaRequestIdentifier.h>'],

Is it actually needed now that we have its own header?
Comment 8 Simon Fraser (smfr) 2021-08-20 20:27:34 PDT
https://trac.webkit.org/changeset/281360/webkit
Comment 9 Radar WebKit Bug Importer 2021-08-20 20:28:18 PDT
<rdar://problem/82195936>