WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
110396
[v8] Fix an erroneous WrapperGrouper call in preparation for refactoring
https://bugs.webkit.org/show_bug.cgi?id=110396
Summary
[v8] Fix an erroneous WrapperGrouper call in preparation for refactoring
Adam Klein
Reported
2013-02-20 15:23:18 PST
[v8] Fix an erroneous WrapperGrouper call in preparation for refactoring
Attachments
Patch
(1.92 KB, patch)
2013-02-20 15:25 PST
,
Adam Klein
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Adam Klein
Comment 1
2013-02-20 15:25:04 PST
Created
attachment 189393
[details]
Patch
Kentaro Hara
Comment 2
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?
Adam Klein
Comment 3
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).
Adam Klein
Comment 4
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.
Kentaro Hara
Comment 5
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.
WebKit Review Bot
Comment 6
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
>
WebKit Review Bot
Comment 7
2013-02-20 16:02:22 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug