Bug 69792 – [V8] CodeGeneratorV8 shouldn't hardcode the list of ActiveDOMObjects

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Adam Barth]

http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm#L2860

sub IsActiveDomType needs to die.

[Adam Barth]

Created attachment 110425 [details]
work in progress

[Adam Barth]

Created attachment 110432 [details]
Patch

[Nate Chapin]

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?

[Adam Barth]

> 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.

[WebKit Review Bot]

Comment on attachment 110432 [details]
Patch

Clearing flags on attachment: 110432

Committed r97109: <http://trac.webkit.org/changeset/97109>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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

[Adam Barth]

Will fix.

[anton muhin]

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?

[Adam Barth]

> may this go into common code?

I'm not sure I understand your question.

[anton muhin]

(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.

[Adam Barth]

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.

[anton muhin]

(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