Add setJSWrapperForActiveDOMNode and use it for Nodes that are also ActiveDOMObjects
Created attachment 136348 [details] Patch
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 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.)
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).
(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.
Uploaded TestNode.idl separately in https://bugs.webkit.org/show_bug.cgi?id=83599 to avoid bloating this change.
(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 on attachment 136348 [details] Patch Clearing flags on attachment: 136348 Committed r113754: <http://trac.webkit.org/changeset/113754>
All reviewed patches have been landed. Closing bug.