Bug 83528

Summary: Add setJSWrapperForActiveDOMNode and use it for Nodes that are also ActiveDOMObjects
Product: WebKit Reporter: Adam Klein <adamk>
Component: New BugsAssignee: Adam Klein <adamk>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, arv, haraken, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 82256    
Attachments:
Description Flags
Patch none

Description Adam Klein 2012-04-09 16:56:25 PDT
Add setJSWrapperForActiveDOMNode and use it for Nodes that are also ActiveDOMObjects
Comment 1 Adam Klein 2012-04-09 17:00:59 PDT
Created attachment 136348 [details]
Patch
Comment 2 Kentaro Hara 2012-04-09 19:31:56 PDT
Comment on attachment 136348 [details]
Patch

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

The change looks OK.

> Source/WebCore/ChangeLog:15
> +        No new tests, no change in behavior.

Recently we've observed several changes in code generators around Nodes. Shall we add WebCore/bindings/scripts/test/TestNode.idl, so that we can test the generated code for Nodes?
Comment 3 Kentaro Hara 2012-04-09 22:06:15 PDT
Comment on attachment 136348 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        No new tests, no change in behavior.
> 
> Recently we've observed several changes in code generators around Nodes. Shall we add WebCore/bindings/scripts/test/TestNode.idl, so that we can test the generated code for Nodes?

Although I think it is basically a good idea to add TestNode.idl, but this patch requires a test case for an Active Node, which is an edge case. If we add a new test interface for these edge cases one by one, the number of Test*.idl will increase. Thus, it might be OK not to add a test case for this patch. (It's up to you.)
Comment 4 Kentaro Hara 2012-04-09 23:18:42 PDT
Just an out-of-curiosity question: Why does V8 use different object maps depending on Nodes, ActiveNodes, DOM objects or Active DOM objects? Is there any performance gain to separate them? JSC uses just one object map (i.e. DOMWrapperWorld::m_wrappers).
Comment 5 Adam Barth 2012-04-09 23:27:53 PDT
We should study the code, but they might be enumerated by the V8GCController in pre-GC (to make sure the wrappers for ActiveDOMObjects aren't collected if there's pending activity).
Comment 6 Adam Klein 2012-04-10 10:09:59 PDT
(In reply to comment #3)
> (From update of attachment 136348 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136348&action=review
> 
> >> Source/WebCore/ChangeLog:15
> >> +        No new tests, no change in behavior.
> > 
> > Recently we've observed several changes in code generators around Nodes. Shall we add WebCore/bindings/scripts/test/TestNode.idl, so that we can test the generated code for Nodes?
> 
> Although I think it is basically a good idea to add TestNode.idl, but this patch requires a test case for an Active Node, which is an edge case. If we add a new test interface for these edge cases one by one, the number of Test*.idl will increase. Thus, it might be OK not to add a test case for this patch. (It's up to you.)

I think adding one for Node is worthwhile, I'll do that before I land this.

As for the Active Node case, there's only one at the moment: HTMLAudioElement. And I've verified manually that it's the only caller of setJSWrapperForActiveDOMNode.
Comment 7 Adam Klein 2012-04-10 11:52:06 PDT
Uploaded TestNode.idl separately in https://bugs.webkit.org/show_bug.cgi?id=83599 to avoid bloating this change.
Comment 8 Adam Klein 2012-04-10 11:55:01 PDT
(In reply to comment #5)
>> Just an out-of-curiosity question: Why does V8 use different object maps depending on Nodes, ActiveNodes, DOM objects or Active DOM objects? Is there any performance gain to separate them? JSC uses just one object map (i.e. DOMWrapperWorld::m_wrappers).

> We should study the code, but they might be enumerated by the V8GCController in pre-GC (to make sure the wrappers for ActiveDOMObjects aren't collected if there's pending activity).

Adam's correct on the reason for these separate maps. The ActiveDOMNodeMap isn't strictly necessary, as we could do that ActiveDOMObject work only on members of the DOMNodeMap for whom isActiveNode() is true. But the ActiveDOMNodeMap acts as an optimization, allowing us to only examine active nodes during the prologue/epilogue visitors.
Comment 9 WebKit Review Bot 2012-04-10 12:41:58 PDT
Comment on attachment 136348 [details]
Patch

Clearing flags on attachment: 136348

Committed r113754: <http://trac.webkit.org/changeset/113754>
Comment 10 WebKit Review Bot 2012-04-10 12:42:02 PDT
All reviewed patches have been landed.  Closing bug.