Use the PropertyName parameter passed to custom getters/setters rather than a const char* in DOM attribute prologues
Created attachment 423792 [details] Patch
Comment on attachment 423792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=423792&action=review Nicely done, r=me with minor nits. I especially like the `if constexpr` bits. > Source/JavaScriptCore/ChangeLog:8 > + Add throwVMDOMAttributeSetterTypeError to match existing throwVMDOMAttributeSetterTypeError, and move typo: to match existing *throwVMDOMAttributeGetterTypeError* > Source/JavaScriptCore/ChangeLog:9 > + additional helpers used by WebCore here to avoid redudent work. typo: *redundant* > Source/WebCore/ChangeLog:19 > + - Remove AttributeSetter::call as it was redudent with invokeFunctorPropagatingExceptionIfNecessary. typo: *redundant* > Source/WebCore/ChangeLog:286 > + * bindings/scripts/test/JS/JSWorkletGlobalScope.cpp: unrelated: it would nice if ChangeLog tooling collapsed all this into * bindings/scripts/test/JS/*: Updated. > Source/WebCore/bindings/js/JSDOMAttribute.h:78 > + RELEASE_AND_RETURN(throwScope, rejectPromiseWithGetterTypeError(lexicalGlobalObject, JSClass::info()->className, attributeName)); nit: throwDOMAttributeGetterTypeError() / throwDOMAttributeSetterTypeError() use `classInfo` only to get `className`. Passing it directly would IMO make helpers look simpler from the call site, and align prototypes of rejectPromiseWithGetterTypeError() / throwVMDOMAttributeGetterTypeError(). > Source/WebCore/bindings/js/JSDOMAttribute.h:80 > + return JSC::JSValue::encode(JSC::jsUndefined()); nit: WebKit code style guide seems to forbid `else` after `return` (https://webkit.org/code-style-guidelines/#linebreaking-else-if).
(In reply to Alexey Shvayka from comment #2) > Comment on attachment 423792 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=423792&action=review > > Nicely done, r=me with minor nits. > I especially like the `if constexpr` bits. > > > Source/JavaScriptCore/ChangeLog:8 > > + Add throwVMDOMAttributeSetterTypeError to match existing throwVMDOMAttributeSetterTypeError, and move > > typo: to match existing *throwVMDOMAttributeGetterTypeError* Fixed. > > > Source/JavaScriptCore/ChangeLog:9 > > + additional helpers used by WebCore here to avoid redudent work. > > typo: *redundant* Fixed. > > > Source/WebCore/ChangeLog:19 > > + - Remove AttributeSetter::call as it was redudent with invokeFunctorPropagatingExceptionIfNecessary. > > typo: *redundant* Fixed. > > > Source/WebCore/ChangeLog:286 > > + * bindings/scripts/test/JS/JSWorkletGlobalScope.cpp: > > unrelated: it would nice if ChangeLog tooling collapsed all this into > * bindings/scripts/test/JS/*: Updated. Fixed. > > > Source/WebCore/bindings/js/JSDOMAttribute.h:78 > > + RELEASE_AND_RETURN(throwScope, rejectPromiseWithGetterTypeError(lexicalGlobalObject, JSClass::info()->className, attributeName)); > > nit: throwDOMAttributeGetterTypeError() / throwDOMAttributeSetterTypeError() > use `classInfo` only to get `className`. > Passing it directly would IMO make helpers look simpler from the call site, > and align prototypes of rejectPromiseWithGetterTypeError() / > throwVMDOMAttributeGetterTypeError(). > Fixed. > > Source/WebCore/bindings/js/JSDOMAttribute.h:80 > > + return JSC::JSValue::encode(JSC::jsUndefined()); > > nit: WebKit code style guide seems to forbid `else` after `return` > (https://webkit.org/code-style-guidelines/#linebreaking-else-if). For if constexpr, I tend to ignore this rule, as the semantics are actually a little different.
Created attachment 423819 [details] Patch
Committed r274770: <https://commits.webkit.org/r274770> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423819 [details].
<rdar://problem/75702190>