Bug 110396 – [v8] Fix an erroneous WrapperGrouper call in preparation for refactoring

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Klein]

[v8] Fix an erroneous WrapperGrouper call in preparation for refactoring

[Adam Klein]

Created attachment 189393 [details]
Patch

[Kentaro Hara]

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?

[Adam Klein]

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

[Adam Klein]

As for why I'm making this change, see the FIXME in V8GCController a few lines above the lines I'm changing.

[Kentaro Hara]

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.

[WebKit Review Bot]

Comment on attachment 189393 [details]
Patch

Clearing flags on attachment: 189393

Committed r143526: <http://trac.webkit.org/changeset/143526>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.