Bug 69792

Summary:

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

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

WebCore JavaScript

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, antonm, enal, simon.fraser, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

66878

Attachments:

Description	Flags
work in progress	none
Patch	none

Adam Barth

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

Attachments
work in progress (5.25 KB, patch) 2011-10-10 15:52 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch (6.18 KB, patch) 2011-10-10 16:35 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2011-10-10 15:52:18 PDT

Created attachment 110425 [details] work in progress

Adam Barth

Comment 2 2011-10-10 16:35:11 PDT

Created attachment 110432 [details] Patch

Nate Chapin

Comment 3 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?

Adam Barth

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

WebKit Review Bot

Comment 5 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>

WebKit Review Bot

Comment 6 2011-10-10 17:10:29 PDT

All reviewed patches have been landed. Closing bug.

Simon Fraser (smfr)

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

Adam Barth

Comment 8 2011-10-10 20:59:58 PDT

Will fix.

anton muhin

Comment 9 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?

Adam Barth

Comment 10 2011-10-11 10:07:17 PDT

> may this go into common code? I'm not sure I understand your question.

anton muhin

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

Adam Barth

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

anton muhin

Comment 13 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

Note You need to log in before you can comment on or make changes to this bug.