[webkit-reviews] review granted: [Bug 99129] [V8] PerContextEnabled methods should be installed when the constructor is created : [Attachment 168362] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has granted Hajime Morrita
<morrita at google.com>'s request for review:
Bug 99129: [V8] PerContextEnabled methods should be installed when the
constructor is created
https://bugs.webkit.org/show_bug.cgi?id=99129

Attachment 168362: Patch
https://bugs.webkit.org/attachment.cgi?id=168362&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=168362&action=review


This patch will affect the performance of toV8Slow(). Before landing, please
confirm that this patch (+ other patches you're trying to land) won't regress
performance of Bindings/create-element.html and Dromaeo DOM tests.

> Source/WebCore/ChangeLog:16
> +	   generated method, which is desined to be called for each time when
the prototype

Nit: desined => designed.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:845
> +    if ($implClassName eq "DOMWindow") {
> +	   push(@implContentDecls, <<END);
> +    return perContextData->constructorForType(WrapperTypeInfo::unwrap(data),
V8DOMWindow::toNative(info.Holder())->document());
> +END
> +    } elsif ($implClassName eq "WorkerContext") {
> +	   push(@implContentDecls, <<END);
> +    return perContextData->constructorForType(WrapperTypeInfo::unwrap(data),
V8WorkerContext::toNative(info.Holder()));
> +END
> +    } else {
> +	   die "Unknown Context ${implClassName}"
> +    }
> +    push(@implContentDecls, <<END);
> +}

Nit: I would prefer push(@implContentDecls, "...") rather than here documents
for one-line code.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2066
>> +WrapperTypeInfo V8${implClassName}Constructor::info = {
V8${implClassName}Constructor::GetTemplate, V8${implClassName}::derefObject,
V8${implClassName}::toActiveDOMObject, 0,
V8${implClassName}::installPerContextPrototypeProperties, 0,
WrapperTypeObjectPrototype };
> 
> Can't you pass 0 instead of passing an empty function? Then you can avoid
generating empty functions.

I discussed with morrita-san offline. I understood why you want to pass an
empty function.