[webkit-reviews] review granted: [Bug 171637] Hoist DOM binding attribute getter prologue into JavaScriptCore taking advantage of DOMJIT / CheckSubClass : [Attachment 311958] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 171637: Hoist DOM binding attribute getter prologue into JavaScriptCore
taking advantage of DOMJIT / CheckSubClass
https://bugs.webkit.org/show_bug.cgi?id=171637

Attachment 311958: Patch

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




--- Comment #61 from Darin Adler <darin at apple.com> ---
Comment on attachment 311958
  --> https://bugs.webkit.org/attachment.cgi?id=311958
Patch

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

Looks good.

> Source/JavaScriptCore/bytecode/AccessCase.cpp:556
> +	       if
(!structure()->classInfo()->isSubClassOf(access.domAttribute()->classInfo)) {

Unfortunate naming on this existing "is sub class of" function, since
"subclass" is a single word. It should be "is subclass of", therefore
"isSubclassOf".

> Source/JavaScriptCore/domjit/DOMJITGetterSetter.h:55
> +    Ref<DOMJIT::CallDOMGetterSnippet> compile() const
> +    {
> +	   return m_compiler();
> +    }

Seems like this could be a on-liner like the functions above. But also, we
could omit this and the callers could just say compiler()(), unless we think
that’s too confusing and non-obvious.

> Source/JavaScriptCore/runtime/DOMAttributeGetterSetter.h:32
> +namespace JSC {
> +namespace DOMJIT {

I suggest a blank line here.

> Source/JavaScriptCore/runtime/JSCustomGetterSetterFunction.cpp:61
>      CustomGetterSetter::CustomGetter getter = customGetterSetter->getter();
> -    ASSERT(getter);
> -    return getter(exec, JSValue::encode(exec->thisValue()),
customGetterSetterFunction->propertyName());
> +    return getter(exec, JSValue::encode(thisValue),
customGetterSetterFunction->propertyName());

Why not do it all on one line? I figured the local variable was so we could
assert it, but if not, then seems like we don’t need a type and name for it.

> Source/JavaScriptCore/runtime/PropertySlot.cpp:45
> +	   auto scope = DECLARE_THROW_SCOPE(vm);

Why is the scope outside the if?

> Source/JavaScriptCore/runtime/PropertySlot.cpp:59
> +	   auto scope = DECLARE_THROW_SCOPE(vm);

Why is the scope outside the if?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1209
> +    # If we use CustomGetterSetter in IDL code generator, we cannot skip
type check.

No need for comma here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1213
> +    # If the interface has special logic of casting, we cannot hoist type
check to JSC.

Should say "for casting" rather than "of casting". No need for comma here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1215
> +    return 0 if $interface->extendedAttributes->{"ImplicitThis"};
> +    return 0 if $interface->extendedAttributes->{"CustomProxyToJSObject"};

No quotation marks needed here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1223
> +    return 0 if $attribute->extendedAttributes->{"DOMJIT"};

No quotation marks needed here.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3381
> +	       # assert("Only DOMJIT=Getter is supported for attributes")
unless
$codeGenerator->ExtendedAttributeContains($attribute->extendedAttributes->{DOMJ
IT}, "Getter");

I don’t understand the status of this IDL compilation error checking. Why are
we taking it out?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3384
> +	       push(@implContent, "#if ${conditionalString}\n") if
$conditionalString;

Should have two "\n" here so we don’t wrap the code too tightly.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3385
> +	       $implIncludes{"<wtf/NeverDestroyed.h>"} = 1;

This doesn’t seem to be needed. I don’t see use of NeverDestroyed in the
generated code.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3386
> +	       $implIncludes{"DOMJITIDLTypeFilter.h"} = 1;

Setting this to 1 doesn’t seem quite right and the old code didn’t do that.
Don’t we want something more like AddToImplIncludes("DOMJITIDLTypeFilter.h",
$conditionalString)?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3391
> +	       # my $setter = IsReadonly($attribute) ? "nullptr" :
GetAttributeSetterName($interface, $generatorName, $attribute);

Is this commented-out code helpful? I don’t have a strong objection to leaving
it in, but I do have a mild objection.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3396
> +	       push(@implContent, "static const JSC::DOMJIT::GetterSetter
DOMJITAttributeFor${generatorName} {\n");

I’m not sure I understand the use of DOMJIT types when ENABLE(JIT) is false.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3406
> +	       push(@implContent, "#endif\n") if $conditionalString;
> +	       push(@implContent, "\n");

Should have two "\n" in the conditional string line and not have the second
push. No need for an extra "\n" if we are not adding an #endif.