110396 2013-02-20 15:23:18 -0800 [v8] Fix an erroneous WrapperGrouper call in preparation for refactoring 2013-02-20 16:02:22 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 adamk adamk abarth haraken japhet webkit.review.bot oldest_to_newest 837829 0 adamk 2013-02-20 15:23:18 -0800 [v8] Fix an erroneous WrapperGrouper call in preparation for refactoring 837830 1 189393 adamk 2013-02-20 15:25:04 -0800 Created attachment 189393 Patch 837837 2 189393 haraken 2013-02-20 15:33:30 -0800 Comment on attachment 189393 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? 837843 3 adamk 2013-02-20 15:40:03 -0800 (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). 837844 4 adamk 2013-02-20 15:40:37 -0800 As for why I'm making this change, see the FIXME in V8GCController a few lines above the lines I'm changing. 837849 5 189393 haraken 2013-02-20 15:42:55 -0800 Comment on attachment 189393 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. 837870 6 189393 webkit.review.bot 2013-02-20 16:02:18 -0800 Comment on attachment 189393 Patch Clearing flags on attachment: 189393 Committed r143526: <http://trac.webkit.org/changeset/143526> 837871 7 webkit.review.bot 2013-02-20 16:02:22 -0800 All reviewed patches have been landed. Closing bug. 189393 2013-02-20 15:25:04 -0800 2013-02-20 16:02:17 -0800 Patch bug-110396-20130220152125.patch text/plain 1962 adamk U3VidmVyc2lvbiBSZXZpc2lvbjogMTQzNDgzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTNlMTBjYjdiOWFkMmYz ODkyNTBiYmY5OWFhNGMxNzZjMjAwMTY4Ny4uZWE4MzgwMzkxOTc1ZDkyMDdiY2U2NjRmY2ViM2M0 M2VmZDg2ZmUwNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDEzLTAyLTIwICBBZGFt IEtsZWluICA8YWRhbWtAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFt2OF0gRml4IGFuIGVycm9u ZW91cyBXcmFwcGVyR3JvdXBlciBjYWxsIGluIHByZXBhcmF0aW9uIGZvciByZWZhY3RvcmluZwor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTEwMzk2CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhpcyBpcyBp biBwcmVwYXJhdGlvbiBmb3IgYSByZWZhY3RvciB0byBleHBvc2UgYSBzaW1wbGlmaWVkCisgICAg ICAgIFdyYXBwZXJHcm91cGVyIGludGVyZmFjZSB0byBWOCB3cmFwcGVyIGNsYXNzZXMgZW5hYmxp bmcgdGhlbSB0bworICAgICAgICBzcGVjaWZ5IG11bHRpcGxlIHJvb3RzIHBlciB3cmFwcGVyIG9i amVjdC4gCisKKyAgICAgICAgKiBiaW5kaW5ncy92OC9WOEdDQ29udHJvbGxlci5jcHA6IFNpbmNl IE11dGF0aW9uT2JzZXJ2ZXJzIGFyZSBub3QgTm9kZXMsIHRoZSBjb3JyZWN0IGNhbGwgaGVyZSBp cyBhZGRPYmplY3RUb0dyb3VwLCBhcyBpdCBpcyBmb3IgYWxsIG90aGVyIG5vbi1Ob2RlIHdyYXBw ZXJzLgorCiAyMDEzLTAyLTIwICBSb2JlcnQgSG9nYW4gIDxyb2JlcnRAd2Via2l0Lm9yZz4KIAog ICAgICAgICBObyBjYXJldCBvbiBlbXB0eSBjb250ZW50ZWRpdGFibGUgZWxlbWVudCB3aXRoIG5l Z2F0aXZlIHRleHQtaW5kZW50CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy92 OC9WOEdDQ29udHJvbGxlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy92OC9WOEdDQ29u dHJvbGxlci5jcHAKaW5kZXggMzMyNjg3OTYyMTJmY2MwNjg5NDNiMGY3NmExMTViNjExOTk5ZGUz Yi4uNTgzNDA1ZDhhNDkwODRkMjg0OTcyNWFiOWM5Yzg2YThhOTNhMzA2NCAxMDA2NDQKLS0tIGEv U291cmNlL1dlYkNvcmUvYmluZGluZ3MvdjgvVjhHQ0NvbnRyb2xsZXIuY3BwCisrKyBiL1NvdXJj ZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4R0NDb250cm9sbGVyLmNwcApAQCAtMzExLDcgKzMxMSw3 IEBAIHB1YmxpYzoKICAgICAgICAgICAgIE11dGF0aW9uT2JzZXJ2ZXIqIG9ic2VydmVyID0gc3Rh dGljX2Nhc3Q8TXV0YXRpb25PYnNlcnZlcio+KG9iamVjdCk7CiAgICAgICAgICAgICBIYXNoU2V0 PE5vZGUqPiBvYnNlcnZlZE5vZGVzID0gb2JzZXJ2ZXItPmdldE9ic2VydmVkTm9kZXMoKTsKICAg ICAgICAgICAgIGZvciAoSGFzaFNldDxOb2RlKj46Oml0ZXJhdG9yIGl0ID0gb2JzZXJ2ZWROb2Rl cy5iZWdpbigpOyBpdCAhPSBvYnNlcnZlZE5vZGVzLmVuZCgpOyArK2l0KQotICAgICAgICAgICAg ICAgIG1fZ3JvdXBlci5hZGROb2RlVG9Hcm91cChWOEdDQ29udHJvbGxlcjo6b3BhcXVlUm9vdEZv ckdDKCppdCwgbV9pc29sYXRlKSwgd3JhcHBlcik7CisgICAgICAgICAgICAgICAgbV9ncm91cGVy LmFkZE9iamVjdFRvR3JvdXAoVjhHQ0NvbnRyb2xsZXI6Om9wYXF1ZVJvb3RGb3JHQygqaXQsIG1f aXNvbGF0ZSksIHdyYXBwZXIpOwogICAgICAgICB9IGVsc2UgewogICAgICAgICAgICAgQWN0aXZl RE9NT2JqZWN0KiBhY3RpdmVET01PYmplY3QgPSB0eXBlLT50b0FjdGl2ZURPTU9iamVjdCh3cmFw cGVyKTsKICAgICAgICAgICAgIGlmIChhY3RpdmVET01PYmplY3QgJiYgYWN0aXZlRE9NT2JqZWN0 LT5oYXNQZW5kaW5nQWN0aXZpdHkoKSkK