[Webkit-unassigned] [Bug 102082] [V8] More efficient wrapper dispatch

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=102082


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #173888|review?                     |review+
               Flag|                            |




--- Comment #2 from Adam Barth <abarth at webkit.org>  2012-11-13 08:19:35 PST ---
(From update of attachment 173888)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.