[webkit-reviews] review granted: [Bug 223542] Use the PropertyName parameter passed to custom getters/setters rather than a redundant const char* in DOM attribute prologues : [Attachment 423792] Patch

[bugzilla-daemon at webkit.org]

Alexey Shvayka <shvaikalesh at gmail.com> has granted Sam Weinig
<sam at webkit.org>'s request for review:
Bug 223542: Use the PropertyName parameter passed to custom getters/setters
rather than a redundant const char* in DOM attribute prologues
https://bugs.webkit.org/show_bug.cgi?id=223542

Attachment 423792: Patch

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




--- Comment #2 from Alexey Shvayka <shvaikalesh at gmail.com> ---
Comment on attachment 423792
  --> https://bugs.webkit.org/attachment.cgi?id=423792
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).