Bug 223542 – Use the PropertyName parameter passed to custom getters/setters rather than a redundant const char* in DOM attribute prologues

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Use the PropertyName parameter passed to custom getters/setters rather than a const char* in DOM attribute prologues

[Sam Weinig]

Created attachment 423792 [details]
Patch

[Alexey Shvayka]

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).

[Sam Weinig]

(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.

[Sam Weinig]

Created attachment 423819 [details]
Patch

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/75702190>