[webkit-reviews] review granted: [Bug 174313] Implement EventTarget constructor : [Attachment 390761] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 174313: Implement EventTarget constructor
https://bugs.webkit.org/show_bug.cgi?id=174313

Attachment 390761: Patch

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




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

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

> Source/WebCore/bindings/js/JSDOMWrapperCache.h:204
> +template<typename DOMClass> inline void
setSubclassStructureIfNeeded(JSC::JSGlobalObject* lexicalGlobalObject,
JSC::CallFrame* callFrame, JSC::JSValue jsValue)

I suggest changing the JSValue argument to a JSObject* argument. The code that
knows the value is an object is presumably at the call site; it’s best to have
the typecast where the knowledge is rather than elsewhere.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7366
> +	       push(@$outputArray, "	auto jsValue =
toJSNewlyCreated<${IDLType}>(" . join(", ", @constructionConversionArguments) .
");\n");

Seems like toJSNewlyCreated should return a JSObject* instead of a JSValue if
it’s always going to be a JSObject. Don’t have to make that change now, but
it’s better than returning something and knowing you can typecast back.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7368
> +	       push(@$outputArray, "   
setSubclassStructureIfNeeded<${implType}>(lexicalGlobalObject, callFrame,
jsValue);\n");

Looks like this might slow down all other cases just to make the EventTarget
case work. Maybe only a tiny bit, though. Is there some way to avoid that?

> Source/WebCore/dom/make_event_factory.pl:138
> +    print F "    default:\n";

I would suggest instead adding a case for EventTargetInterfaceType here?

    print F "	case EventTargetInterfaceType:\n";
    print F "	    break;\n";

It’s nice to get a warning if we accidentally have any unhandled type, and this
approach preserves it and also make the change to the script slightly
simpler/smaller.