Bug 222165 – Allow to use BigInt as key identifier

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Allow to use BigInt as key identifier

[youenn fablet]

Created attachment 420953 [details]
Patch

[Darin Adler]

Comment on attachment 420953 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransform.idl:53
> +    [Custom] Promise<undefined> setEncryptionKey(CryptoKey key, optional any keyID);

Why do we need to use [Custom] and not union types for this?

[youenn fablet]

<rdar://74487156>

[youenn fablet]

(In reply to Darin Adler from comment #2)
> Comment on attachment 420953 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420953&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransform.idl:53
> > +    [Custom] Promise<undefined> setEncryptionKey(CryptoKey key, optional any keyID);
> 
> Why do we need to use [Custom] and not union types for this?

We could do that, I am guessing that we might want to use only one of these types in the end, but this might be worth discussing in standard body first.

[youenn fablet]

rdar has some context as well.

[youenn fablet]

Created attachment 420976 [details]
Patch

[Darin Adler]

Comment on attachment 420976 [details]
Patch

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

> Source/WebCore/Modules/mediastream/RTCRtpSFrameTransform.idl:53
> +    [Custom] Promise<undefined> setEncryptionKey(CryptoKey key, optional any keyID);

I’d like to eventually make this a non-custom binding.

> Source/WebCore/bindings/js/JSRTCRtpSFrameTransformCustom.cpp:36
> +using namespace JSC;

In new code I suggest we not use "using namespace" like this.

[youenn fablet]

(In reply to Darin Adler from comment #7)
> Comment on attachment 420976 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420976&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCRtpSFrameTransform.idl:53
> > +    [Custom] Promise<undefined> setEncryptionKey(CryptoKey key, optional any keyID);
> 
> I’d like to eventually make this a non-custom binding.

Agreed.

> > Source/WebCore/bindings/js/JSRTCRtpSFrameTransformCustom.cpp:36
> > +using namespace JSC;
> 
> In new code I suggest we not use "using namespace" like this.

Why?

[EWS]

Committed r273158: <https://commits.webkit.org/r273158>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420976 [details].

[Eric Carlson]

Comment on attachment 420976 [details]
Patch

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

> Source/WebCore/bindings/js/JSRTCRtpSFrameTransformCustom.cpp:59
> +                throwException(&lexicalGlobalObject, throwScope, createDOMException(&lexicalGlobalObject, RangeError, "Not a 64 bits integer"_s));

s/64 bits/64 bit/

[Darin Adler]

Comment on attachment 420976 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSRTCRtpSFrameTransformCustom.cpp:36
>>> +using namespace JSC;
>> 
>> In new code I suggest we not use "using namespace" like this.
> 
> Why?

There’s a consensus that it’s clearer to use the namespace in front of the individual identifiers. Both the auto keyword and context-dependent lookup reduce how often the prefix has to be used so it doesn’t make code a lot harder to read and write. There are also many problems with how "using namespace" affects code unintentionally. I *think* that at least doing it inside a "namespace WebCore { }" doesn’t affect the multiple files in a unified build, but otherwise that is a problem.