Summary: | Allow to use BigInt as key identifier | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | cdumez, darin, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, hta, jer.noble, kondapallykalyan, philipj, sam, sergio, tommyw, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Local Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
youenn fablet
2021-02-19 03:21:34 PST
Created attachment 420953 [details]
Patch
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? (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. rdar has some context as well. Created attachment 420976 [details]
Patch
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. (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? Committed r273158: <https://commits.webkit.org/r273158> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420976 [details]. 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/ 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. |