79319 2012-02-22 19:58:55 -0800 Cleanup RenderBlock::moveChildrenTo 2012-02-23 11:11:48 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED P3 Normal --- 1 jchaffraix jchaffraix eric inferno ojan tony webkit.review.bot oldest_to_newest 562739 0 jchaffraix 2012-02-22 19:58:55 -0800 Seen during a review, the function is pretty difficult to read but also has some redundant code hidden in it. Patch forthcoming. 562748 1 128376 jchaffraix 2012-02-22 20:03:44 -0800 Created attachment 128376 Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo. 562843 2 inferno 2012-02-22 22:32:50 -0800 (In reply to comment #1) > Created an attachment (id=128376) [details] > Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo. Yay! now i can just fix stuff in central RenderBlock::moveChildTo 562846 3 128376 eric 2012-02-22 22:38:32 -0800 Comment on attachment 128376 Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo. View in context: https://bugs.webkit.org/attachment.cgi?id=128376&action=review I wonder if this will have a performance effect. > Source/WebCore/rendering/RenderBlock.cpp:992 > + for (RenderObject* child = startChild; child && child != endChild; ) { > + // Save our next sibling as moveChildTo will clear it. > + RenderObject* nextSibling = child->nextSibling(); > + moveChildTo(toBlock, child, beforeChild, fullRemoveInsert); > + child = nextSibling; > } Just to make sure... there are no mutation events (or other JavaScript) run by moveChildTo such that we would need to hold a RefPtr for child? 562848 4 inferno 2012-02-22 22:42:49 -0800 (In reply to comment #3) > (From update of attachment 128376 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128376&action=review > > I wonder if this will have a performance effect. > > > Source/WebCore/rendering/RenderBlock.cpp:992 > > + for (RenderObject* child = startChild; child && child != endChild; ) { > > + // Save our next sibling as moveChildTo will clear it. > > + RenderObject* nextSibling = child->nextSibling(); > > + moveChildTo(toBlock, child, beforeChild, fullRemoveInsert); > > + child = nextSibling; > > } > > Just to make sure... there are no mutation events (or other JavaScript) run by moveChildTo such that we would need to hold a RefPtr for child? No Refptrs in the rendering world :( 563159 5 128376 jchaffraix 2012-02-23 09:22:49 -0800 Comment on attachment 128376 Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo. View in context: https://bugs.webkit.org/attachment.cgi?id=128376&action=review > I wonder if this will have a performance effect. I was wondering that too. Some archaeology did not show the split was done for performance so we should be fine. In case, we can always inline moveChildTo. >>> Source/WebCore/rendering/RenderBlock.cpp:992 >>> } >> >> Just to make sure... there are no mutation events (or other JavaScript) run by moveChildTo such that we would need to hold a RefPtr for child? > > No Refptrs in the rendering world :( Indeed, but more importantly, the operations are atomic from a JS perspective. 563271 6 128376 webkit.review.bot 2012-02-23 11:11:43 -0800 Comment on attachment 128376 Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo. Clearing flags on attachment: 128376 Committed r108645: <http://trac.webkit.org/changeset/108645> 563272 7 webkit.review.bot 2012-02-23 11:11:48 -0800 All reviewed patches have been landed. Closing bug. 128376 2012-02-22 20:03:44 -0800 2012-02-23 11:11:43 -0800 Simple cleanup, removed unneeded inline check, while -> for and use moveChildTo. bug-79319-20120222200343.patch text/plain 2532 jchaffraix U3VidmVyc2lvbiBSZXZpc2lvbjogMTA4NTkzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDhjN2U1Y2ZjMDcxMzkx OTM3N2Y2ZjJiMTUzYWEzYzc5ZGYzZGZhNS4uZTFmZGRiM2Y3OTYwNmVhYTA1NTYzMzExZDMwZGM0 MTc1OTEzYzE2NCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEyLTAyLTIyICBKdWxp ZW4gQ2hhZmZyYWl4ICA8amNoYWZmcmFpeEB3ZWJraXQub3JnPgorCisgICAgICAgIENsZWFudXAg UmVuZGVyQmxvY2s6Om1vdmVDaGlsZHJlblRvCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD03OTMxOQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIFJlZmFjdG9yaW5nLCBubyBjaGFuZ2UgaW4gYmVoYXZpb3IgZXhw ZWN0ZWQuCisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyQmxvY2suY3BwOgorICAgICAgICAo V2ViQ29yZTo6UmVuZGVyQmxvY2s6Om1vdmVDaGlsZHJlblRvKToKKyAgICAgICAgUmVtb3ZlZCBp bmxpbmUgY2hlY2sgdGhhdCB3YXMgcmVkdW5kYW50LCBzd2l0Y2hlZCB0byB8d2hpbGV8IHRvIGEg fGZvcnwKKyAgICAgICAgdG8gc2hvdyB3aGF0J3MgZ29pbmcgb24gbW9yZSBjbG9zZWx5IGFuZCBh ZGRzIGEgY2FsbCB0byBtb3ZlQ2hpbGRUbyB0bworICAgICAgICBzaGFyZSBtb3JlIGNvZGUgYmV0 d2VlbiB0aGUgMiBmdW5jdGlvbnMuCisKIDIwMTItMDItMjIgIEFkcmllbm5lIFdhbGtlciAgPGVu bmVAZ29vZ2xlLmNvbT4KIAogICAgICAgICBbY2hyb21pdW1dIFVucmV2aWV3ZWQgc3BlY3VsYXRp dmUgY2hyb21pdW0tbWFjIGJ1aWxkIGZpeC4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3Jl bmRlcmluZy9SZW5kZXJCbG9jay5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVy QmxvY2suY3BwCmluZGV4IGM1ZTk0YzFkZGJmMWU0NGU0Zjk1N2ZmY2FhMWFjMWQwZTZkNjUwZjIu LjM3NGRmZDlkMzQ5ODY3OTY4MDIzYTdjNmM0ZmNmMzA1ODU2YWQxMjUgMTAwNzU1Ci0tLSBhL1Nv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJCbG9jay5jcHAKKysrIGIvU291cmNlL1dlYkNv cmUvcmVuZGVyaW5nL1JlbmRlckJsb2NrLmNwcApAQCAtOTgzLDE4ICs5ODMsMTIgQEAgdm9pZCBS ZW5kZXJCbG9jazo6bW92ZUNoaWxkVG8oUmVuZGVyQmxvY2sqIHRvQmxvY2ssIFJlbmRlck9iamVj dCogY2hpbGQsIFJlbmRlck8KIHZvaWQgUmVuZGVyQmxvY2s6Om1vdmVDaGlsZHJlblRvKFJlbmRl ckJsb2NrKiB0b0Jsb2NrLCBSZW5kZXJPYmplY3QqIHN0YXJ0Q2hpbGQsIFJlbmRlck9iamVjdCog ZW5kQ2hpbGQsIFJlbmRlck9iamVjdCogYmVmb3JlQ2hpbGQsIGJvb2wgZnVsbFJlbW92ZUluc2Vy dCkKIHsKICAgICBBU1NFUlQoIWJlZm9yZUNoaWxkIHx8IHRvQmxvY2sgPT0gYmVmb3JlQ2hpbGQt PnBhcmVudCgpKTsKLSAgICBSZW5kZXJPYmplY3QqIG5leHRDaGlsZCA9IHN0YXJ0Q2hpbGQ7Ci0g ICAgd2hpbGUgKG5leHRDaGlsZCAmJiBuZXh0Q2hpbGQgIT0gZW5kQ2hpbGQpIHsKLSAgICAgICAg UmVuZGVyT2JqZWN0KiBjaGlsZCA9IG5leHRDaGlsZDsKLSAgICAgICAgbmV4dENoaWxkID0gY2hp bGQtPm5leHRTaWJsaW5nKCk7Ci0gICAgICAgIGlmIChmdWxsUmVtb3ZlSW5zZXJ0KSB7Ci0gICAg ICAgICAgICAvLyBUYWtlcyBjYXJlIG9mIGFkZGluZyB0aGUgbmV3IGNoaWxkIGNvcnJlY3RseSBp ZiB0b0Jsb2NrIGFuZCBmcm9tQmxvY2sKLSAgICAgICAgICAgIC8vIGhhdmUgZGlmZmVyZW50IGtp bmQgb2YgY2hpbGRyZW4gKGJsb2NrIHZzIGlubGluZSkuCi0gICAgICAgICAgICB0b0Jsb2NrLT5h ZGRDaGlsZElnbm9yaW5nQ29udGludWF0aW9uKGNoaWxkcmVuKCktPnJlbW92ZUNoaWxkTm9kZSh0 aGlzLCBjaGlsZCksIGJlZm9yZUNoaWxkKTsKLSAgICAgICAgfSBlbHNlCi0gICAgICAgICAgICB0 b0Jsb2NrLT5jaGlsZHJlbigpLT5pbnNlcnRDaGlsZE5vZGUodG9CbG9jaywgY2hpbGRyZW4oKS0+ cmVtb3ZlQ2hpbGROb2RlKHRoaXMsIGNoaWxkLCBmYWxzZSksIGJlZm9yZUNoaWxkLCBmYWxzZSk7 Ci0gICAgICAgIGlmIChjaGlsZCA9PSBlbmRDaGlsZCkKLSAgICAgICAgICAgIHJldHVybjsKKwor ICAgIGZvciAoUmVuZGVyT2JqZWN0KiBjaGlsZCA9IHN0YXJ0Q2hpbGQ7IGNoaWxkICYmIGNoaWxk ICE9IGVuZENoaWxkOyApIHsKKyAgICAgICAgLy8gU2F2ZSBvdXIgbmV4dCBzaWJsaW5nIGFzIG1v dmVDaGlsZFRvIHdpbGwgY2xlYXIgaXQuCisgICAgICAgIFJlbmRlck9iamVjdCogbmV4dFNpYmxp bmcgPSBjaGlsZC0+bmV4dFNpYmxpbmcoKTsKKyAgICAgICAgbW92ZUNoaWxkVG8odG9CbG9jaywg Y2hpbGQsIGJlZm9yZUNoaWxkLCBmdWxsUmVtb3ZlSW5zZXJ0KTsKKyAgICAgICAgY2hpbGQgPSBu ZXh0U2libGluZzsKICAgICB9CiB9CiAK