Bug 69792

Summary: [V8] CodeGeneratorV8 shouldn't hardcode the list of ActiveDOMObjects
Product: WebKit Reporter: Adam Barth <abarth>
Component: WebCore JavaScriptAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, antonm, enal, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66878    
Attachments:
Description Flags
work in progress
none
Patch none

Description Adam Barth 2011-10-10 15:09:29 PDT
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm#L2860

sub IsActiveDomType needs to die.
Comment 1 Adam Barth 2011-10-10 15:52:18 PDT
Created attachment 110425 [details]
work in progress
Comment 2 Adam Barth 2011-10-10 16:35:11 PDT
Created attachment 110432 [details]
Patch
Comment 3 Nate Chapin 2011-10-10 16:41:40 PDT
Comment on attachment 110432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110432&action=review

:-D

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:438
> +ALWAYS_INLINE v8::Handle<v8::Object> ${className}::existingWrapper(${nativeType}* impl)
> +{
> +END
> +    my $getWrapper = IsNodeSubType($dataNode) ? "V8DOMWrapper::getWrapper(impl)" : "${domMapFunction}.get(impl)";
> +    push(@headerContent, <<END);
> +    return ${getWrapper};
> +}

I assume this is just for readability?
Comment 4 Adam Barth 2011-10-10 16:45:28 PDT
> I assume this is just for readability?

At this call site:

- my $domMapFunction = GetDomMapFunction(0, $returnType);

It's hard to figure out whether $returnType is an active DOM object, so we let the C++ compiler figure it out for us using the new existingWrapper function.
Comment 5 WebKit Review Bot 2011-10-10 17:10:25 PDT
Comment on attachment 110432 [details]
Patch

Clearing flags on attachment: 110432

Committed r97109: <http://trac.webkit.org/changeset/97109>
Comment 6 WebKit Review Bot 2011-10-10 17:10:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Simon Fraser (smfr) 2011-10-10 20:44:02 PDT
Looks like this caused a bunch of "binding tests failed" on the bots:
http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Tests%29/builds/34194
Comment 8 Adam Barth 2011-10-10 20:59:58 PDT
Will fix.
Comment 9 anton muhin 2011-10-11 09:22:05 PDT
Comment on attachment 110432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110432&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:432
> +ALWAYS_INLINE v8::Handle<v8::Object> ${className}::existingWrapper(${nativeType}* impl)

may this go into common code?
Comment 10 Adam Barth 2011-10-11 10:07:17 PDT
> may this go into common code?

I'm not sure I understand your question.
Comment 11 anton muhin 2011-10-11 11:15:44 PDT
(In reply to comment #10)
> > may this go into common code?
> 
> I'm not sure I understand your question.

Dreadful sorry, Adam.

I meant may we have this existingWrapper as a template function somewhere in Source/WebCore/bindings/v8 instead in generated files.

The goal is to have as minimum of logic in generated code as possible.

Feel free to ignore.
Comment 12 Adam Barth 2011-10-11 12:24:18 PDT
Ah, I see.  The advantage of having it in the generated code is that it depends on the ActiveDOMObject IDL attribute.  In some sense, I would have preferred to generate the code inline at the call sites in the code generator, but not all the type information was available, so I made the C++ compiler do that work.

I definitely agree that we should prefer to put as much in non-generated code as possible, but I don't see a clean way of doing that in this case.
Comment 13 anton muhin 2011-10-11 12:31:10 PDT
(In reply to comment #12)
> Ah, I see.  The advantage of having it in the generated code is that it depends on the ActiveDOMObject IDL attribute.  In some sense, I would have preferred to generate the code inline at the call sites in the code generator, but not all the type information was available, so I made the C++ compiler do that work.
> 
> I definitely agree that we should prefer to put as much in non-generated code as possible, but I don't see a clean way of doing that in this case.

ok