Bug 110396

Summary: [v8] Fix an erroneous WrapperGrouper call in preparation for refactoring
Product: WebKit Reporter: Adam Klein <adamk>
Component: New BugsAssignee: Adam Klein <adamk>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, haraken, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Adam Klein 2013-02-20 15:23:18 PST
[v8] Fix an erroneous WrapperGrouper call in preparation for refactoring
Comment 1 Adam Klein 2013-02-20 15:25:04 PST
Created attachment 189393 [details]
Patch
Comment 2 Kentaro Hara 2013-02-20 15:33:30 PST
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?
Comment 3 Adam Klein 2013-02-20 15:40:03 PST
(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).
Comment 4 Adam Klein 2013-02-20 15:40:37 PST
As for why I'm making this change, see the FIXME in V8GCController a few lines above the lines I'm changing.
Comment 5 Kentaro Hara 2013-02-20 15:42:55 PST
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 6 WebKit Review Bot 2013-02-20 16:02:18 PST
Comment on attachment 189393 [details]
Patch

Clearing flags on attachment: 189393

Committed r143526: <http://trac.webkit.org/changeset/143526>
Comment 7 WebKit Review Bot 2013-02-20 16:02:22 PST
All reviewed patches have been landed.  Closing bug.