[v8] Fix an erroneous WrapperGrouper call in preparation for refactoring
Created attachment 189393 [details] Patch
Comment on attachment 189393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189393&action=review > Source/WebCore/bindings/v8/V8GCController.cpp:314 > - m_grouper.addNodeToGroup(V8GCController::opaqueRootForGC(*it, m_isolate), wrapper); > + m_grouper.addObjectToGroup(V8GCController::opaqueRootForGC(*it, m_isolate), wrapper); *it is a Node, and opaqueRootForGC() returns a Node, so I'm confused. Would you elaborate on why you want to make this change?
(In reply to comment #2) > (From update of attachment 189393 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189393&action=review > > > Source/WebCore/bindings/v8/V8GCController.cpp:314 > > - m_grouper.addNodeToGroup(V8GCController::opaqueRootForGC(*it, m_isolate), wrapper); > > + m_grouper.addObjectToGroup(V8GCController::opaqueRootForGC(*it, m_isolate), wrapper); > > *it is a Node, and opaqueRootForGC() returns a Node, so I'm confused. Would you elaborate on why you want to make this change? Note that all the analogous calls to this one are from the CodeGenerator, in GenerateOpaqueRootForGC, and the return value of that is always passed to addObjectToGroup() (by V8GCController). Adam Barth may be able to shine more light on this, but it appears to me that "addNodeToGroup" refers to the wrapper being a Node wrapper, not the root being a Node (though that's definitely not obvious from the type signatures of the functions, as you point out).
As for why I'm making this change, see the FIXME in V8GCController a few lines above the lines I'm changing.
Comment on attachment 189393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189393&action=review >>> Source/WebCore/bindings/v8/V8GCController.cpp:314 >>> + m_grouper.addObjectToGroup(V8GCController::opaqueRootForGC(*it, m_isolate), wrapper); >> >> *it is a Node, and opaqueRootForGC() returns a Node, so I'm confused. Would you elaborate on why you want to make this change? > > Note that all the analogous calls to this one are from the CodeGenerator, in GenerateOpaqueRootForGC, and the return value of that is always passed to addObjectToGroup() (by V8GCController). Adam Barth may be able to shine more light on this, but it appears to me that "addNodeToGroup" refers to the wrapper being a Node wrapper, not the root being a Node (though that's definitely not obvious from the type signatures of the functions, as you point out). Thanks, understood. Makes sense to me. I'll rename add{Node,Object}ToGroup to add{Node,Object}WrapperToGroup in a follow-up patch.
Comment on attachment 189393 [details] Patch Clearing flags on attachment: 189393 Committed r143526: <http://trac.webkit.org/changeset/143526>
All reviewed patches have been landed. Closing bug.