[V8] use toV8Fast in all relevant getters
Created attachment 171640 [details] Patch
Comment on attachment 171640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171640&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:297 > + die("IsDOMNodeType is out of date with respect to $interfaceName") if IsDOMNodeType($interfaceName) != IsNodeSubType($dataNode); Why are both IsDOMNodeType() and IsNodeSubType() needed, given that they should return the same result? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1121 > + } elsif (IsDOMNodeType($attrType) && IsDOMNodeType($interfaceName)) { > + AddToImplIncludes($attrType.".h"); > push(@implContentDecls, " return toV8Fast($result, info, imp);\n"); Some Node-type classes have custom implementation of toV8() in bindings/v8/custom/*. Wouldn't it cause a problem? I mean, if you call toV8Fast() here, you might end up failing to call custom toV8(). > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-4031 > - return 1 if $type eq 'HTMLElement'; > - return 1 if $type eq 'HTMLUnknownElement'; > - return 1 if $type eq 'HTMLFormElement'; > - return 1 if $type eq 'HTMLTableCaptionElement'; > - return 1 if $type eq 'HTMLTableSectionElement'; Isn't IsDOMNodeType() used by other places? I want to make sure that this change won't affect other places.
Comment on attachment 171640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171640&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:297 >> + die("IsDOMNodeType is out of date with respect to $interfaceName") if IsDOMNodeType($interfaceName) != IsNodeSubType($dataNode); > > Why are both IsDOMNodeType() and IsNodeSubType() needed, given that they should return the same result? One is only useful for the currently executing dataNode. The other can check the DOMNode-iness of anything based on name. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1121 >> push(@implContentDecls, " return toV8Fast($result, info, imp);\n"); > > Some Node-type classes have custom implementation of toV8() in bindings/v8/custom/*. Wouldn't it cause a problem? I mean, if you call toV8Fast() here, you might end up failing to call custom toV8(). Yep. Fix on its way. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-4031 >> - return 1 if $type eq 'HTMLTableSectionElement'; > > Isn't IsDOMNodeType() used by other places? I want to make sure that this change won't affect other places. It's used in a few other places. I can check it out, but it's pretty poorly named if this update is not correct.
Created attachment 171854 [details] Patch
Comment on attachment 171854 [details] Patch This looks great!
Comment on attachment 171854 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171854&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:527 > + my $has_empty_toV8 = 0; > + my $has_fast_toV8 = 0; WebKit uses camelCase for variable names. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:604 > + die("HasFastToV8 is out of sync with respect to interface $interfaceName [$has_fast_toV8]\n") if HasFastToV8($interfaceName) != $has_fast_toV8; > + die("HasEmptyToV8 is out of sync with respect to interface $interfaceName [$has_fast_toV8]\n") if HasEmptyToV8($interfaceName) != $has_empty_toV8; Basically we don't want to introduce hard-coded class lists (i.e. HasFastToV8() and HasEmptyToV8()). See below comments. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4060 > +sub HasEmptyToV8 > +{ > + my $type = shift; > + > + return 1 if $type eq 'Element'; > + return 1 if $type eq 'SVGRenderingIntent'; > + return 1 if $type eq 'SVGUnitTypes'; > + return 1 if $type eq 'SVGZoomAndPan'; > + > + return 0; > +} What is this? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4071 > + return 0 if $type eq 'Document'; > + return 0 if $type eq 'Element'; > + return 0 if $type eq 'HTMLDocument'; > + return 0 if $type eq 'HTMLElement'; > + return 0 if $type eq 'SVGDocument'; > + return 0 if $type eq 'SVGElement'; Wouldn't it be possible to rename toV8() in custom bindings to toV8Fast() and thus remove this hard-coded class list from CodeGeneratorV8.pm? (It would be better if we could make the implementation of custom toV8Fast() *fast*er, but let's do it in follow-up patches.) > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4076 > sub IsDOMNodeType Can't we just replace IsDOMNodeType() with IsNodeSubType(), and thus kill IsDOMNodeType() ?
View in context: https://bugs.webkit.org/attachment.cgi?id=171854&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:545 > +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* node, const v8::AccessorInfo& info, Node* holder) It's unfortunate that we now have two copies of this function definition. Maybe we could make a helper "generate" function to share the code?
(In reply to comment #7) > View in context: https://bugs.webkit.org/attachment.cgi?id=171854&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:545 > > +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* node, const v8::AccessorInfo& info, Node* holder) > > It's unfortunate that we now have two copies of this function definition. Maybe we could make a helper "generate" function to share the code? Or can we just generate nothing so that the virtualized Node::toV8Fast() is called ? dcarney: Sorry for setting my r? by mistake... please reset it:)
Comment on attachment 171854 [details] Patch Please address haraken's comments before landing. It might be worth posting another iteration of the patch for review.
Comment on attachment 171854 [details] Patch I discussed with dcarney a bit more offline. To clean up confusing toV8(), toV8Fast(), toV8Slow() and custom toV8(), we decided to land the patch step by step. For now marking r-.
Created attachment 174099 [details] Patch
(In reply to comment #11) > Created an attachment (id=174099) [details] > Patch As per Kentaro's request, we now have toV8Fast on all Node subclasses which dispatches correctly in all cases (in some cases to regular toV8). toV8Slow was removed in another patch.
Comment on attachment 174099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174099&action=review Looks good to me as an incremental step to implement toV8Fast() and kill toV8Slow(). Just to clarify, would you explain what code will be executed for HTMLDivElement::toV8Fast() by this change? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:606 > + V8DOMWindowShell* context = V8DOMWindowShell::getEntered(); > + ASSERT(context); > + v8::Handle<v8::Object> wrapper = context->world()->isolatedWorldDOMDataStore()->get(impl); Consider a change from abarth (https://bugs.webkit.org/show_bug.cgi?id=102156).
(In reply to comment #13) > (From update of attachment 174099 [details]) > Looks good to me as an incremental step to implement toV8Fast() and kill toV8Slow(). > > Just to clarify, would you explain what code will be executed for HTMLDivElement::toV8Fast() by this change? Maybe you need to update run-bindings-tests results.
Created attachment 174117 [details] Patch
(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 174099 [details] [details]) > > Looks good to me as an incremental step to implement toV8Fast() and kill toV8Slow(). > > > > Just to clarify, would you explain what code will be executed for HTMLDivElement::toV8Fast() by this change? > > Maybe you need to update run-bindings-tests results. fixed
(In reply to comment #13) > (From update of attachment 174099 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174099&action=review > > Looks good to me as an incremental step to implement toV8Fast() and kill toV8Slow(). > > Just to clarify, would you explain what code will be executed for HTMLDivElement::toV8Fast() by this change? it will call toV8Fast(HTMLDivElement*... which will perform the fast check and on cache miss, dispatch the wrapping call through dispatchWrap(HTMLDivElement*... instead of dispatchWrap(Node*..., which would eventually call dispatchWrap(HTMLDivElement*... anyway
Comment on attachment 174117 [details] Patch Thanks!
Comment on attachment 174117 [details] Patch Clearing flags on attachment: 174117 Committed r134588: <http://trac.webkit.org/changeset/134588>
All reviewed patches have been landed. Closing bug.