[webkit-reviews] review granted: [Bug 102082] [V8] More efficient wrapper dispatch : [Attachment 173888] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dan Carney <dcarney at google.com>'s
request for review:
Bug 102082: [V8] More efficient wrapper dispatch
https://bugs.webkit.org/show_bug.cgi?id=102082

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173888&action=review


This looks great.  I'm not 100% sure about the names.  Here's a recommendation:


wrapSlow -> createWrapper
dispatchWrap -> wrap
dispatchWrapCustom -> wrap

When you have a CustomToJSObject, can't we just skip generating the
implementation of wrap (aka dispatchWrap)?  I don't see what having the thunk
buys us.  In any case, feel free to land the patch with the current names.  We
can iterate from there if needed.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:498
> +    if ($interfaceName eq "HTMLElement") {
> +	   push(@headerContent, <<END);
> +    friend v8::Handle<v8::Object> createV8HTMLWrapper(HTMLElement*,
v8::Handle<v8::Object> creationContext, v8::Isolate*);
> +END
> +    } elsif ($interfaceName eq "SVGElement") {
> +	   push(@headerContent, <<END);
> +    friend v8::Handle<v8::Object> createV8SVGWrapper(SVGElement* element,
v8::Handle<v8::Object> creationContext, v8::Isolate* isolate);

Why does the HTML version have named parameters but the SVG version does not? 
We should probably make this consistent.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:519
>      static v8::Handle<v8::Object> wrapSlow(${wrapSlowArgumentType},
v8::Handle<v8::Object> creationContext, v8::Isolate*);

I was thinking of renaming wrapSlow to createWrapper (assuming it no longer
checks the cache).

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:553
> +    if (UNLIKELY(!impl))
> +	   return v8::Handle<v8::Object>();

Can this ever actually happen?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3425
>  v8::Handle<v8::Object> ${className}::wrapSlow(${wrapSlowArgumentType} impl,
v8::Handle<v8::Object> creationContext, v8::Isolate* isolate)

Yeah, this always creates a wrapper now, right?  We should name it
createWrapper.	(We can do that in a separate patch if you like.)

> Source/WebCore/bindings/scripts/IDLAttributes.txt:122
> +V8NoWrapperCache=*

I think you want this to be just "V8NoWrapperCache" because the attribute
doesn't take any arguments.