83528 2012-04-09 16:56:25 -0700 Add setJSWrapperForActiveDOMNode and use it for Nodes that are also ActiveDOMObjects 2012-04-10 12:42:02 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 82256 1 adamk adamk abarth arv haraken japhet webkit.review.bot oldest_to_newest 598766 0 adamk 2012-04-09 16:56:25 -0700 Add setJSWrapperForActiveDOMNode and use it for Nodes that are also ActiveDOMObjects 598773 1 136348 adamk 2012-04-09 17:00:59 -0700 Created attachment 136348 Patch 598910 2 136348 haraken 2012-04-09 19:31:56 -0700 Comment on attachment 136348 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136348&action=review The change looks OK. > Source/WebCore/ChangeLog:15 > + No new tests, no change in behavior. Recently we've observed several changes in code generators around Nodes. Shall we add WebCore/bindings/scripts/test/TestNode.idl, so that we can test the generated code for Nodes? 599006 3 136348 haraken 2012-04-09 22:06:15 -0700 Comment on attachment 136348 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136348&action=review >> Source/WebCore/ChangeLog:15 >> + No new tests, no change in behavior. > > Recently we've observed several changes in code generators around Nodes. Shall we add WebCore/bindings/scripts/test/TestNode.idl, so that we can test the generated code for Nodes? Although I think it is basically a good idea to add TestNode.idl, but this patch requires a test case for an Active Node, which is an edge case. If we add a new test interface for these edge cases one by one, the number of Test*.idl will increase. Thus, it might be OK not to add a test case for this patch. (It's up to you.) 599046 4 haraken 2012-04-09 23:18:42 -0700 Just an out-of-curiosity question: Why does V8 use different object maps depending on Nodes, ActiveNodes, DOM objects or Active DOM objects? Is there any performance gain to separate them? JSC uses just one object map (i.e. DOMWrapperWorld::m_wrappers). 599055 5 abarth 2012-04-09 23:27:53 -0700 We should study the code, but they might be enumerated by the V8GCController in pre-GC (to make sure the wrappers for ActiveDOMObjects aren't collected if there's pending activity). 599366 6 adamk 2012-04-10 10:09:59 -0700 (In reply to comment #3) > (From update of attachment 136348 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136348&action=review > > >> Source/WebCore/ChangeLog:15 > >> + No new tests, no change in behavior. > > > > Recently we've observed several changes in code generators around Nodes. Shall we add WebCore/bindings/scripts/test/TestNode.idl, so that we can test the generated code for Nodes? > > Although I think it is basically a good idea to add TestNode.idl, but this patch requires a test case for an Active Node, which is an edge case. If we add a new test interface for these edge cases one by one, the number of Test*.idl will increase. Thus, it might be OK not to add a test case for this patch. (It's up to you.) I think adding one for Node is worthwhile, I'll do that before I land this. As for the Active Node case, there's only one at the moment: HTMLAudioElement. And I've verified manually that it's the only caller of setJSWrapperForActiveDOMNode. 599452 7 adamk 2012-04-10 11:52:06 -0700 Uploaded TestNode.idl separately in https://bugs.webkit.org/show_bug.cgi?id=83599 to avoid bloating this change. 599454 8 adamk 2012-04-10 11:55:01 -0700 (In reply to comment #5) >> Just an out-of-curiosity question: Why does V8 use different object maps depending on Nodes, ActiveNodes, DOM objects or Active DOM objects? Is there any performance gain to separate them? JSC uses just one object map (i.e. DOMWrapperWorld::m_wrappers). > We should study the code, but they might be enumerated by the V8GCController in pre-GC (to make sure the wrappers for ActiveDOMObjects aren't collected if there's pending activity). Adam's correct on the reason for these separate maps. The ActiveDOMNodeMap isn't strictly necessary, as we could do that ActiveDOMObject work only on members of the DOMNodeMap for whom isActiveNode() is true. But the ActiveDOMNodeMap acts as an optimization, allowing us to only examine active nodes during the prologue/epilogue visitors. 599526 9 136348 webkit.review.bot 2012-04-10 12:41:58 -0700 Comment on attachment 136348 Patch Clearing flags on attachment: 136348 Committed r113754: <http://trac.webkit.org/changeset/113754> 599527 10 webkit.review.bot 2012-04-10 12:42:02 -0700 All reviewed patches have been landed. Closing bug. 136348 2012-04-09 17:00:59 -0700 2012-04-10 12:41:58 -0700 Patch bug-83528-20120409170058.patch text/plain 6136 adamk U3VidmVyc2lvbiBSZXZpc2lvbjogMTEzNjI4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZTFlMTI1NjE2NGQ1ZTZk Zjc1ZTM2M2RlMGM5NjEyOWNlZGEwNzcwYi4uNDU5MjRhYThhZjQzOWQ2N2E0OTA5NDkxNjVjMTNl NTNmNmQ5M2U4YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDMyIEBACisyMDEyLTA0LTA5ICBBZGFt IEtsZWluICA8YWRhbWtAY2hyb21pdW0ub3JnPgorCisgICAgICAgIEFkZCBzZXRKU1dyYXBwZXJG b3JBY3RpdmVET01Ob2RlIGFuZCB1c2UgaXQgZm9yIE5vZGVzIHRoYXQgYXJlIGFsc28gQWN0aXZl RE9NT2JqZWN0cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9ODM1MjgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBJbnN0ZWFkIG9mIHVzaW5nIGEgcnVuLXRpbWUgY2FsbCB0byBpc0FjdGl2ZU5vZGUgdG8gZGV0 ZXJtaW5lIHdoaWNoCisgICAgICAgIG1hcCB0byBwdXQgYSBOb2RlIHdyYXBwZXIgaW4sIGdlbmVy YXRlIHRoZSBwcm9wZXIgY2FsbCBpbiB0aGUgQ29kZUdlbmVyYXRvci4KKworICAgICAgICBUaGlz IHdhcyBvcmlnaW5hbGx5IHBhcnQgb2YgcjExMjMxOCwgd2hpY2ggZ290IHJvbGxlZCBvdXQgZHVl IHRvIE9PTSBjb25jZXJucy4KKyAgICAgICAgSSdtIHNwbGl0dGluZyBpdCBpbnRvIHNtYWxsZXIg cGllY2VzIHNvIHRoYXQgZWFjaCBjYW4gYmUgbGFuZGVkIGFuZAorICAgICAgICB3YXRjaGVkIGZv ciBpc3N1ZXMgc2VwZXJhdGVseS4KKworICAgICAgICBObyBuZXcgdGVzdHMsIG5vIGNoYW5nZSBp biBiZWhhdmlvci4KKworICAgICAgICAqIGJpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4 LnBtOgorICAgICAgICAoR2VuZXJhdGVDb25zdHJ1Y3RvckNhbGxiYWNrKTogVXNlIG5ldyBHZXRE b21NYXBOYW1lIGZ1bmN0aW9uIHRvCisgICAgICAgIGZpZ3VyZSBvdXQgd2hpY2ggc2V0SlNXcmFw cGVyIHRvIGNhbGwuCisgICAgICAgIChHZW5lcmF0ZU5hbWVkQ29uc3RydWN0b3JDYWxsYmFjayk6 IGRpdHRvLgorICAgICAgICAoR2V0RG9tTWFwRnVuY3Rpb24pOiBEZWxlZ2F0ZSB0byBHZXREb21N YXBOYW1lIGZvciBsb2dpYy4KKyAgICAgICAgKEdldERvbU1hcE5hbWUpOiBOZXcgaGVscGVyIGZh Y3RvcmVkIG91dCBvZiBHZXREb21NYXBGdW5jdGlvbi4KKyAgICAgICAgKiBiaW5kaW5ncy92OC9W OERPTVdyYXBwZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6VjhET01XcmFwcGVyOjpzZXRKU1dy YXBwZXJGb3JET01Ob2RlKTogQXNzZXJ0ICFpc0FjdGl2ZU5vZGUgaW5zdGVhZCBvZiBicmFuY2hp bmcgb24gaXQuCisgICAgICAgIChXZWJDb3JlOjpWOERPTVdyYXBwZXI6OnNldEpTV3JhcHBlckZv ckFjdGl2ZURPTU5vZGUpOiBOZXcgbWV0aG9kIHNwbGl0IG91ZiBvZiB0aGUgYWJvdmUuCisgICAg ICAgIEFzc2VydCBpc0FjdGl2ZU5vZGUgaW5zdGVhZCBvZiBicmFuY2hpbmcgb24gaXQuCisgICAg ICAgICogYmluZGluZ3MvdjgvVjhET01XcmFwcGVyLmg6CisgICAgICAgIChWOERPTVdyYXBwZXIp OgorCiAyMDEyLTA0LTA5ICBUaW0gSG9ydG9uICA8dGltb3RoeV9ob3J0b25AYXBwbGUuY29tPgog CiAgICAgICAgIEZyYW1lRGF0YSBjb25zdHJ1Y3RvciB6ZXJvZXMgYWxsIGZpZWxkcywgY2F1c2lu ZyBJbWFnZU9yaWVudGF0aW9uIHRvIGJlIDAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2Jp bmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvclY4LnBtIGIvU291cmNlL1dlYkNvcmUvYmluZGlu Z3Mvc2NyaXB0cy9Db2RlR2VuZXJhdG9yVjgucG0KaW5kZXggOTcxY2ZhNjI5ODJhN2RjNzQ5YWEw NzMxZTAxODBkOTUwYzVkYWY3ZC4uZGM1MDUwMmM4OGNmYTNhNjUzMWNiMzcyYzMwYjk0NDllNDVj ZjNlMCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9Db2RlR2Vu ZXJhdG9yVjgucG0KKysrIGIvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9Db2RlR2Vu ZXJhdG9yVjgucG0KQEAgLTE3NTIsMTMgKzE3NTIsNyBAQCBFTkQKICAgICAgICAgcHVzaChAaW1w bENvbnRlbnQsICIgICAgICAgIGdvdG8gZmFpbDtcbiIpOwogICAgIH0KIAotICAgIG15ICRET01P YmplY3QgPSAiRE9NT2JqZWN0IjsKLSAgICBpZiAoSXNOb2RlU3ViVHlwZSgkZGF0YU5vZGUpKSB7 Ci0gICAgICAgICRET01PYmplY3QgPSAiRE9NTm9kZSI7Ci0gICAgfSBlbHNpZiAoJGRhdGFOb2Rl LT5leHRlbmRlZEF0dHJpYnV0ZXMtPnsiQWN0aXZlRE9NT2JqZWN0In0pIHsKLSAgICAgICAgJERP TU9iamVjdCA9ICJBY3RpdmVET01PYmplY3QiOwotICAgIH0KLQorICAgIG15ICRET01PYmplY3Qg PSBHZXREb21NYXBOYW1lKCRkYXRhTm9kZSwgJGltcGxDbGFzc05hbWUpOwogICAgIHB1c2goQGlt cGxDb250ZW50LCA8PEVORCk7CiAKICAgICBWOERPTVdyYXBwZXI6OnNldERPTVdyYXBwZXIod3Jh cHBlciwgJmluZm8sIGltcGwuZ2V0KCkpOwpAQCAtMTkzMiwxNCArMTkyNiw3IEBAIEVORAogICAg ICAgICBwdXNoKEBpbXBsQ29udGVudCwgIiAgICAgICAgZ290byBmYWlsO1xuIik7CiAgICAgfQog Ci0gICAgbXkgJERPTU9iamVjdCA9ICJET01PYmplY3QiOwotICAgICMgQSBET01PYmplY3QgdGhh dCBpcyBhbiBBY3RpdmVET01PYmplY3QgYW5kIGFsc28gYSBET01Ob2RlIHNob3VsZCBiZSB0cmVh dGVkIGFzIGFuIERPTU5vZGUgaGVyZS4KLSAgICAjIHNldEpTV3JhcHBlckZvckRPTU5vZGUoKSB3 aWxsIGxvb2sgaWYgbm9kZSBpcyBhY3RpdmUgYW5kIGNob29zZSBjb3JyZWN0IG1hcCB0byBhZGQg bm9kZSB0by4KLSAgICBpZiAoSXNOb2RlU3ViVHlwZSgkZGF0YU5vZGUpKSB7Ci0gICAgICAgICRE T01PYmplY3QgPSAiRE9NTm9kZSI7Ci0gICAgfSBlbHNpZiAoJGRhdGFOb2RlLT5leHRlbmRlZEF0 dHJpYnV0ZXMtPnsiQWN0aXZlRE9NT2JqZWN0In0pIHsKLSAgICAgICAgJERPTU9iamVjdCA9ICJB Y3RpdmVET01PYmplY3QiOwotICAgIH0KKyAgICBteSAkRE9NT2JqZWN0ID0gR2V0RG9tTWFwTmFt ZSgkZGF0YU5vZGUsICRpbXBsQ2xhc3NOYW1lKTsKICAgICBwdXNoKEBpbXBsQ29udGVudCwgPDxF TkQpOwogCiAgICAgVjhET01XcmFwcGVyOjpzZXRET01XcmFwcGVyKHdyYXBwZXIsICZWOCR7aW1w bENsYXNzTmFtZX1Db25zdHJ1Y3Rvcjo6aW5mbywgaW1wbC5nZXQoKSk7CkBAIC0zMTkwLDEzICsz MTc3LDE5IEBAIEVORAogCiBzdWIgR2V0RG9tTWFwRnVuY3Rpb24KIHsKKyAgICByZXR1cm4gImdl dCIgLiBHZXREb21NYXBOYW1lKEBfKSAuICJNYXAoKSI7Cit9CisKK3N1YiBHZXREb21NYXBOYW1l Cit7CiAgICAgbXkgJGRhdGFOb2RlID0gc2hpZnQ7CiAgICAgbXkgJHR5cGUgPSBzaGlmdDsKLSAg ICByZXR1cm4gImdldERPTVNWR0VsZW1lbnRJbnN0YW5jZU1hcCgpIiBpZiAkdHlwZSBlcSAiU1ZH RWxlbWVudEluc3RhbmNlIjsKLSAgICByZXR1cm4gImdldEFjdGl2ZURPTU5vZGVNYXAoKSIgaWYg KElzTm9kZVN1YlR5cGUoJGRhdGFOb2RlKSAmJiAkZGF0YU5vZGUtPmV4dGVuZGVkQXR0cmlidXRl cy0+eyJBY3RpdmVET01PYmplY3QifSk7Ci0gICAgcmV0dXJuICJnZXRET01Ob2RlTWFwKCkiIGlm IChJc05vZGVTdWJUeXBlKCRkYXRhTm9kZSkpOwotICAgIHJldHVybiAiZ2V0QWN0aXZlRE9NT2Jq ZWN0TWFwKCkiIGlmICRkYXRhTm9kZS0+ZXh0ZW5kZWRBdHRyaWJ1dGVzLT57IkFjdGl2ZURPTU9i amVjdCJ9OwotICAgIHJldHVybiAiZ2V0RE9NT2JqZWN0TWFwKCkiOworCisgICAgcmV0dXJuICJE T01TVkdFbGVtZW50SW5zdGFuY2UiIGlmICR0eXBlIGVxICJTVkdFbGVtZW50SW5zdGFuY2UiOwor ICAgIHJldHVybiAiQWN0aXZlRE9NTm9kZSIgaWYgKElzTm9kZVN1YlR5cGUoJGRhdGFOb2RlKSAm JiAkZGF0YU5vZGUtPmV4dGVuZGVkQXR0cmlidXRlcy0+eyJBY3RpdmVET01PYmplY3QifSk7Cisg ICAgcmV0dXJuICJET01Ob2RlIiBpZiBJc05vZGVTdWJUeXBlKCRkYXRhTm9kZSk7CisgICAgcmV0 dXJuICJBY3RpdmVET01PYmplY3QiIGlmICRkYXRhTm9kZS0+ZXh0ZW5kZWRBdHRyaWJ1dGVzLT57 IkFjdGl2ZURPTU9iamVjdCJ9OworICAgIHJldHVybiAiRE9NT2JqZWN0IjsKIH0KIAogc3ViIEdl dE5hdGl2ZVR5cGVGb3JDb252ZXJzaW9ucwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvYmlu ZGluZ3MvdjgvVjhET01XcmFwcGVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4 RE9NV3JhcHBlci5jcHAKaW5kZXggODRmZDE3MmQyMGE3MjI0NWUxNjJhNjcxNmQ5Nzg1ZGFkOWIx YjU5Zi4uMGZhZDVlODJjOGYwNzYxNmQ2M2E0ZDIxMTZlYTQyZWRkZjk4MGJhZiAxMDA2NDQKLS0t IGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvdjgvVjhET01XcmFwcGVyLmNwcAorKysgYi9Tb3Vy Y2UvV2ViQ29yZS9iaW5kaW5ncy92OC9WOERPTVdyYXBwZXIuY3BwCkBAIC03MiwxMCArNzIsMTUg QEAgbmFtZXNwYWNlIFdlYkNvcmUgewogdm9pZCBWOERPTVdyYXBwZXI6OnNldEpTV3JhcHBlckZv ckRPTU5vZGUoUGFzc1JlZlB0cjxOb2RlPiBub2RlLCB2ODo6UGVyc2lzdGVudDx2ODo6T2JqZWN0 PiB3cmFwcGVyKQogewogICAgIEFTU0VSVChtYXliZURPTVdyYXBwZXIod3JhcHBlcikpOwotICAg IGlmIChub2RlLT5pc0FjdGl2ZU5vZGUoKSkKLSAgICAgICAgZ2V0QWN0aXZlRE9NTm9kZU1hcCgp LnNldChub2RlLmxlYWtSZWYoKSwgd3JhcHBlcik7Ci0gICAgZWxzZQotICAgICAgICBnZXRET01O b2RlTWFwKCkuc2V0KG5vZGUubGVha1JlZigpLCB3cmFwcGVyKTsKKyAgICBBU1NFUlQoIW5vZGUt PmlzQWN0aXZlTm9kZSgpKTsKKyAgICBnZXRET01Ob2RlTWFwKCkuc2V0KG5vZGUubGVha1JlZigp LCB3cmFwcGVyKTsKK30KKwordm9pZCBWOERPTVdyYXBwZXI6OnNldEpTV3JhcHBlckZvckFjdGl2 ZURPTU5vZGUoUGFzc1JlZlB0cjxOb2RlPiBub2RlLCB2ODo6UGVyc2lzdGVudDx2ODo6T2JqZWN0 PiB3cmFwcGVyKQoreworICAgIEFTU0VSVChtYXliZURPTVdyYXBwZXIod3JhcHBlcikpOworICAg IEFTU0VSVChub2RlLT5pc0FjdGl2ZU5vZGUoKSk7CisgICAgZ2V0QWN0aXZlRE9NTm9kZU1hcCgp LnNldChub2RlLmxlYWtSZWYoKSwgd3JhcHBlcik7CiB9CiAKIHY4OjpMb2NhbDx2ODo6RnVuY3Rp b24+IFY4RE9NV3JhcHBlcjo6Y29uc3RydWN0b3JGb3JUeXBlKFdyYXBwZXJUeXBlSW5mbyogdHlw ZSwgRE9NV2luZG93KiB3aW5kb3cpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5n cy92OC9WOERPTVdyYXBwZXIuaCBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4RE9NV3Jh cHBlci5oCmluZGV4IGQ3NjYzMDc1NWIzZjJjYzg0MTliZTljMzRiNjVhMjMxYjcyMWIxYzkuLjE2 MzI2Yjk2NmU4ZTMzOWEwZDU4ZjBlYzFkNGVmMjFiZDMzNjYzOWUgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4RE9NV3JhcHBlci5oCisrKyBiL1NvdXJjZS9XZWJDb3Jl L2JpbmRpbmdzL3Y4L1Y4RE9NV3JhcHBlci5oCkBAIC0xMDksNiArMTA5LDcgQEAgbmFtZXNwYWNl IFdlYkNvcmUgewogICAgICAgICB0ZW1wbGF0ZTx0eXBlbmFtZSBUPiBzdGF0aWMgdm9pZCBzZXRK U1dyYXBwZXJGb3JET01PYmplY3QoUGFzc1JlZlB0cjxUPiwgdjg6OlBlcnNpc3RlbnQ8djg6Ok9i amVjdD4pOwogICAgICAgICB0ZW1wbGF0ZTx0eXBlbmFtZSBUPiBzdGF0aWMgdm9pZCBzZXRKU1dy YXBwZXJGb3JBY3RpdmVET01PYmplY3QoUGFzc1JlZlB0cjxUPiwgdjg6OlBlcnNpc3RlbnQ8djg6 Ok9iamVjdD4pOwogICAgICAgICBzdGF0aWMgdm9pZCBzZXRKU1dyYXBwZXJGb3JET01Ob2RlKFBh c3NSZWZQdHI8Tm9kZT4sIHY4OjpQZXJzaXN0ZW50PHY4OjpPYmplY3Q+KTsKKyAgICAgICAgc3Rh dGljIHZvaWQgc2V0SlNXcmFwcGVyRm9yQWN0aXZlRE9NTm9kZShQYXNzUmVmUHRyPE5vZGU+LCB2 ODo6UGVyc2lzdGVudDx2ODo6T2JqZWN0Pik7CiAKICAgICAgICAgc3RhdGljIGJvb2wgaXNWYWxp ZERPTU9iamVjdCh2ODo6SGFuZGxlPHY4OjpWYWx1ZT4pOwogCg==