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

Adam Barth
Reported 2011-10-10 15:09:29 PDT
Attachments
work in progress (5.25 KB, patch)
2011-10-10 15:52 PDT, Adam Barth
no flags
Patch (6.18 KB, patch)
2011-10-10 16:35 PDT, Adam Barth
no flags
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
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.