[V8] More efficient wrapper dispatch
Created attachment 173888 [details] Patch
Comment on attachment 173888 [details] Patch 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.
/me is rebasing to top of tree.
Created attachment 173929 [details] Patch for landing
I'm going to land this patch because I'd like to build upon it.
Committed r134447: <http://trac.webkit.org/changeset/134447>
(In reply to comment #6) > Committed r134447: <http://trac.webkit.org/changeset/134447> It broke the Qt build :-/ [1;31mgenerated/JSSVGElementWrapperFactory.cpp:711: error: expected '}' at end of input[0m
(In reply to comment #7) > (In reply to comment #6) > > Committed r134447: <http://trac.webkit.org/changeset/134447> > > It broke the Qt build :-/ > > [1;31mgenerated/JSSVGElementWrapperFactory.cpp:711: error: expected '}' at end of input[0m /me will fix.
Attempted fix in http://trac.webkit.org/changeset/134457
Comment on attachment 173888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=173888&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:498 >> + 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. No good reason. Copy and paste issues. Will fix. >> 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). Yeah, me too, I just didn't want to do all the renaming in one patch as I wanted the patch to focus on the change in wrap. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:553 >> + return v8::Handle<v8::Object>(); > > Can this ever actually happen? I don't know. I needed to copy the functionality of toV8, which has the check, but without returning v8::Null. >> 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. okay. I wasn't sure what any of that meant
(In reply to comment #5) > I'm going to land this patch because I'd like to build upon it. I'll try to respond to all the above comments in a fix up patch tomorrow. Did you change anything?
> wrapSlow -> createWrapper > dispatchWrap -> wrap > dispatchWrapCustom -> wrap okay, I just didn't want anyone using those functions, so I figured unusual names > 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. At first I wanted the asserts autogenerated, but I later added the same assert to wrapSlow. I can drop it and just go back to one non member function called wrap.
> Did you change anything? I just addressed the createV8HTMLWrapper / createV8SVGWrapper nit.
(In reply to comment #9) > Attempted fix in http://trac.webkit.org/changeset/134457 Thanks for the quick fix.
This change appears to have caused an infinite recursion problem in V8 when the <video> tag is used with MediaPlayer disabled. See https://bugs.webkit.org/show_bug.cgi?id=103431.