Bug 83528 – Add setJSWrapperForActiveDOMNode and use it for Nodes that are also ActiveDOMObjects

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Klein]

Add setJSWrapperForActiveDOMNode and use it for Nodes that are also ActiveDOMObjects

[Adam Klein]

Created attachment 136348 [details]
Patch

[Kentaro Hara]

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?

[Kentaro Hara]

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

[Kentaro Hara]

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

[Adam Barth]

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 Klein]

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

[Adam Klein]

Uploaded TestNode.idl separately in https://bugs.webkit.org/show_bug.cgi?id=83599 to avoid bloating this change.

[Adam Klein]

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

[WebKit Review Bot]

Comment on attachment 136348 [details]
Patch

Clearing flags on attachment: 136348

Committed r113754: <http://trac.webkit.org/changeset/113754>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.