79724 2012-02-27 17:32:40 -0800 Stop doubling maximalOutlineSize during painting 2012-02-29 12:50:06 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 jchaffraix jchaffraix robert tony webkit.review.bot oldest_to_newest 566186 0 jchaffraix 2012-02-27 17:32:40 -0800 Several call sites are wrongly doubling the maximal outline when manipulating the repaint rectangle: * RenderReplaced * RenderTableSection * RenderTableCell There is no historical explanation for the doubling. Moreover the rest of the code handle the maximalOutlineSize by just inflating the repaint rectangle once so it looks like a bug! 566938 1 129303 jchaffraix 2012-02-28 11:55:43 -0800 Created attachment 129303 Proposed refactoring: remove the doubling and introduce some localRepaintRect to simplify the logic. 567709 2 129303 tony 2012-02-29 10:27:07 -0800 Comment on attachment 129303 Proposed refactoring: remove the doubling and introduce some localRepaintRect to simplify the logic. View in context: https://bugs.webkit.org/attachment.cgi?id=129303&action=review > Source/WebCore/ChangeLog:8 > + Refactoring only, covered by existing tests (mostly repaint ones). Can you name the pixel tests that need a rebaseline from this? Maybe we can pre-emptively mark them in test_expectations.txt so the bots don't go red. 567716 3 129303 jchaffraix 2012-02-29 10:32:47 -0800 Comment on attachment 129303 Proposed refactoring: remove the doubling and introduce some localRepaintRect to simplify the logic. View in context: https://bugs.webkit.org/attachment.cgi?id=129303&action=review >> Source/WebCore/ChangeLog:8 >> + Refactoring only, covered by existing tests (mostly repaint ones). > > Can you name the pixel tests that need a rebaseline from this? Maybe we can pre-emptively mark them in test_expectations.txt so the bots don't go red. There is none that needs to be rebaselined. However there are several tests involving tables and replaced elements in our tree. The repaint tests are the one that checks that I did not break our repainting by "shrinking" the outline. I can change the wording if you want. 567746 4 129303 tony 2012-02-29 11:04:53 -0800 Comment on attachment 129303 Proposed refactoring: remove the doubling and introduce some localRepaintRect to simplify the logic. View in context: https://bugs.webkit.org/attachment.cgi?id=129303&action=review >>> Source/WebCore/ChangeLog:8 >>> + Refactoring only, covered by existing tests (mostly repaint ones). >> >> Can you name the pixel tests that need a rebaseline from this? Maybe we can pre-emptively mark them in test_expectations.txt so the bots don't go red. > > There is none that needs to be rebaselined. However there are several tests involving tables and replaced elements in our tree. The repaint tests are the one that checks that I did not break our repainting by "shrinking" the outline. I can change the wording if you want. I see, I was confused by "Stop doubling maximalOutlineSize during painting", which sounds like a different behavior. I think the current working is OK. Feel free to cq+. 567774 5 129303 jchaffraix 2012-02-29 11:32:08 -0800 Comment on attachment 129303 Proposed refactoring: remove the doubling and introduce some localRepaintRect to simplify the logic. View in context: https://bugs.webkit.org/attachment.cgi?id=129303&action=review >>>> Source/WebCore/ChangeLog:8 >>>> + Refactoring only, covered by existing tests (mostly repaint ones). >>> >>> Can you name the pixel tests that need a rebaseline from this? Maybe we can pre-emptively mark them in test_expectations.txt so the bots don't go red. >> >> There is none that needs to be rebaselined. However there are several tests involving tables and replaced elements in our tree. The repaint tests are the one that checks that I did not break our repainting by "shrinking" the outline. I can change the wording if you want. > > I see, I was confused by "Stop doubling maximalOutlineSize during painting", which sounds like a different behavior. I think the current working is OK. Feel free to cq+. OK , cqinating :-) 567841 6 129303 webkit.review.bot 2012-02-29 12:50:00 -0800 Comment on attachment 129303 Proposed refactoring: remove the doubling and introduce some localRepaintRect to simplify the logic. Clearing flags on attachment: 129303 Committed r109246: <http://trac.webkit.org/changeset/109246> 567842 7 webkit.review.bot 2012-02-29 12:50:06 -0800 All reviewed patches have been landed. Closing bug. 129303 2012-02-28 11:55:43 -0800 2012-02-29 12:50:00 -0800 Proposed refactoring: remove the doubling and introduce some localRepaintRect to simplify the logic. bug-79724-20120228115542.patch text/plain 4904 jchaffraix U3VidmVyc2lvbiBSZXZpc2lvbjogMTA5MTIzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZGQxZDFlMTczMTU2NTE5 MWUxODdmZWMwZDhiOTA0YWVlODRhMzNjMS4uYjczNWI1YThiYjhjMzQ0ZDhjYWNkNjZhMTRhODY0 ODVhNTc4MTk1MiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI0IEBACisyMDEyLTAyLTI4ICBKdWxp ZW4gQ2hhZmZyYWl4ICA8amNoYWZmcmFpeEB3ZWJraXQub3JnPgorCisgICAgICAgIFN0b3AgZG91 YmxpbmcgbWF4aW1hbE91dGxpbmVTaXplIGR1cmluZyBwYWludGluZworICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Nzk3MjQKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBSZWZhY3RvcmluZyBvbmx5LCBjb3ZlcmVk IGJ5IGV4aXN0aW5nIHRlc3RzIChtb3N0bHkgcmVwYWludCBvbmVzKS4KKworICAgICAgICAqIHJl bmRlcmluZy9SZW5kZXJSZXBsYWNlZC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJSZXBs YWNlZDo6c2hvdWxkUGFpbnQpOgorICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJUYWJsZUNlbGwu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyVGFibGVDZWxsOjpwYWludENvbGxhcHNlZEJv cmRlcnMpOgorICAgICAgICBJbnRyb2R1Y2UgYSBsb2NhbCByZXBhaW50IHJlY3RhbmdsZSB0aGF0 IHdlIGluZmxhdGUgYnkgdGhlIG1heGltYWxPdXRsaW5lU2l6ZQorICAgICAgICB0byBzaW1wbGlm eSB0aGUgY29tcGFyaXNvbiBsb2dpYy4gQWxzbyB0cmllZCB0byBtYWtlIGl0IGNsZWFyZXIgd2hh dCdzIGdvaW5nIG9uCisgICAgICAgIGJ5IHR3ZWFraW5nIHRoZSBleGlzdGluZyBjb2RlLgorCisg ICAgICAgICogcmVuZGVyaW5nL1JlbmRlclRhYmxlU2VjdGlvbi5jcHA6CisgICAgICAgIChXZWJD b3JlOjpSZW5kZXJUYWJsZVNlY3Rpb246OnBhaW50T2JqZWN0KToKKyAgICAgICAgUmVtb3ZlIHRo ZSBkb3VibGluZy4KKwogMjAxMi0wMi0yOCAgQWRhbSBLbGVpbiAgPGFkYW1rQGNocm9taXVtLm9y Zz4KIAogICAgICAgICBVbnJldmlld2VkLCBzcGVjdWxhdGl2ZSB0ZXN0IGZpeCBhZnRlciByMTA5 MDE2LgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclJlcGxhY2Vk LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJSZXBsYWNlZC5jcHAKaW5kZXgg MzYwOGEwN2ZlZTUzYWY1NmJlOWM0OWNlZWVlNzQ4NDA5MzI1MTFjYS4uNWRmZDA5OTRkOTJmYjZh ODFkMjI0NmYyNTYyYzA2ZDgxNmRmMDczZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcmVu ZGVyaW5nL1JlbmRlclJlcGxhY2VkLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcv UmVuZGVyUmVwbGFjZWQuY3BwCkBAIC0xOTYsMTAgKzE5NiwxMiBAQCBib29sIFJlbmRlclJlcGxh Y2VkOjpzaG91bGRQYWludChQYWludEluZm8mIHBhaW50SW5mbywgY29uc3QgTGF5b3V0UG9pbnQm IHBhaW50TwogICAgICAgICBib3R0b20gPSBtYXgoc2VsQm90dG9tLCBib3R0b20pOwogICAgIH0K ICAgICAKLSAgICBMYXlvdXRVbml0IG9zID0gMiAqIG1heGltYWxPdXRsaW5lU2l6ZShwYWludElu Zm8ucGhhc2UpOwotICAgIGlmIChhZGp1c3RlZFBhaW50T2Zmc2V0LngoKSArIG1pblhWaXN1YWxP dmVyZmxvdygpID49IHBhaW50SW5mby5yZWN0Lm1heFgoKSArIG9zIHx8IGFkanVzdGVkUGFpbnRP ZmZzZXQueCgpICsgbWF4WFZpc3VhbE92ZXJmbG93KCkgPD0gcGFpbnRJbmZvLnJlY3QueCgpIC0g b3MpCisgICAgTGF5b3V0UmVjdCBsb2NhbFJlcGFpbnRSZWN0ID0gcGFpbnRJbmZvLnJlY3Q7Cisg ICAgbG9jYWxSZXBhaW50UmVjdC5pbmZsYXRlKG1heGltYWxPdXRsaW5lU2l6ZShwYWludEluZm8u cGhhc2UpKTsKKyAgICBpZiAoYWRqdXN0ZWRQYWludE9mZnNldC54KCkgKyBtaW5YVmlzdWFsT3Zl cmZsb3coKSA+PSBsb2NhbFJlcGFpbnRSZWN0Lm1heFgoKSB8fCBhZGp1c3RlZFBhaW50T2Zmc2V0 LngoKSArIG1heFhWaXN1YWxPdmVyZmxvdygpIDw9IGxvY2FsUmVwYWludFJlY3QueCgpKQogICAg ICAgICByZXR1cm4gZmFsc2U7Ci0gICAgaWYgKHRvcCA+PSBwYWludEluZm8ucmVjdC5tYXhZKCkg KyBvcyB8fCBib3R0b20gPD0gcGFpbnRJbmZvLnJlY3QueSgpIC0gb3MpCisKKyAgICBpZiAodG9w ID49IGxvY2FsUmVwYWludFJlY3QubWF4WSgpIHx8IGJvdHRvbSA8PSBsb2NhbFJlcGFpbnRSZWN0 LnkoKSkKICAgICAgICAgcmV0dXJuIGZhbHNlOwogCiAgICAgcmV0dXJuIHRydWU7CmRpZmYgLS1n aXQgYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGFibGVDZWxsLmNwcCBiL1NvdXJj ZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUYWJsZUNlbGwuY3BwCmluZGV4IDQzNGFjMGQ2OTVi YTkyM2FhMDcxZTAwNTAxNWMxMDNmZjgzOWU0NDkuLmQ3MGRhNjI0MDFjZjEzNDgxZGI5MjFmNzZj NDUxYzRlNTJkMmU3MDUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5k ZXJUYWJsZUNlbGwuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUYWJs ZUNlbGwuY3BwCkBAIC05NjAsMTggKzk2MCwyMCBAQCB2b2lkIFJlbmRlclRhYmxlQ2VsbDo6cGFp bnRDb2xsYXBzZWRCb3JkZXJzKFBhaW50SW5mbyYgcGFpbnRJbmZvLCBjb25zdCBMYXlvdXRQbwog ICAgIGlmICghcGFpbnRJbmZvLnNob3VsZFBhaW50V2l0aGluUm9vdCh0aGlzKSB8fCBzdHlsZSgp LT52aXNpYmlsaXR5KCkgIT0gVklTSUJMRSkKICAgICAgICAgcmV0dXJuOwogCi0gICAgTGF5b3V0 UG9pbnQgYWRqdXN0ZWRQYWludE9mZnNldCA9IHBhaW50T2Zmc2V0ICsgbG9jYXRpb24oKTsKLSAg ICBMYXlvdXRVbml0IG9zID0gMiAqIG1heGltYWxPdXRsaW5lU2l6ZShwYWludEluZm8ucGhhc2Up OwotICAgIGlmICghKGFkanVzdGVkUGFpbnRPZmZzZXQueSgpIC0gdGFibGUoKS0+b3V0ZXJCb3Jk ZXJUb3AoKSA8IHBhaW50SW5mby5yZWN0Lm1heFkoKSArIG9zCi0gICAgICAgICYmIGFkanVzdGVk UGFpbnRPZmZzZXQueSgpICsgaGVpZ2h0KCkgKyB0YWJsZSgpLT5vdXRlckJvcmRlckJvdHRvbSgp ID4gcGFpbnRJbmZvLnJlY3QueSgpIC0gb3MpKQorICAgIExheW91dFJlY3QgbG9jYWxSZXBhaW50 UmVjdCA9IHBhaW50SW5mby5yZWN0OworICAgIGxvY2FsUmVwYWludFJlY3QuaW5mbGF0ZShtYXhp bWFsT3V0bGluZVNpemUocGFpbnRJbmZvLnBoYXNlKSk7CisKKyAgICBMYXlvdXRSZWN0IHBhaW50 UmVjdCA9IExheW91dFJlY3QocGFpbnRPZmZzZXQgKyBsb2NhdGlvbigpLCBzaXplKCkpOworICAg IGlmIChwYWludFJlY3QueSgpIC0gdGFibGUoKS0+b3V0ZXJCb3JkZXJUb3AoKSA+PSBsb2NhbFJl cGFpbnRSZWN0Lm1heFkoKSkKKyAgICAgICAgcmV0dXJuOworCisgICAgaWYgKHBhaW50UmVjdC5t YXhZKCkgKyB0YWJsZSgpLT5vdXRlckJvcmRlckJvdHRvbSgpIDw9IGxvY2FsUmVwYWludFJlY3Qu eSgpKQogICAgICAgICByZXR1cm47CiAKICAgICBHcmFwaGljc0NvbnRleHQqIGdyYXBoaWNzQ29u dGV4dCA9IHBhaW50SW5mby5jb250ZXh0OwogICAgIGlmICghdGFibGUoKS0+Y3VycmVudEJvcmRl clZhbHVlKCkgfHwgZ3JhcGhpY3NDb250ZXh0LT5wYWludGluZ0Rpc2FibGVkKCkpCiAgICAgICAg IHJldHVybjsKIAotICAgIExheW91dFJlY3QgcGFpbnRSZWN0ID0gTGF5b3V0UmVjdChhZGp1c3Rl ZFBhaW50T2Zmc2V0LCBzaXplKCkpOwotCiAgICAgUmVuZGVyU3R5bGUqIHRhYmxlU3R5bGUgPSB0 YWJsZSgpLT5zdHlsZSgpOwogICAgIENvbGxhcHNlZEJvcmRlclZhbHVlIGxlZnRWYWwgPSBjYWNo ZWRDb2xsYXBzZWRMZWZ0Qm9yZGVyKHRhYmxlU3R5bGUpOwogICAgIENvbGxhcHNlZEJvcmRlclZh bHVlIHJpZ2h0VmFsID0gY2FjaGVkQ29sbGFwc2VkUmlnaHRCb3JkZXIodGFibGVTdHlsZSk7CmRp ZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGFibGVTZWN0aW9uLmNw cCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUYWJsZVNlY3Rpb24uY3BwCmluZGV4 IDY0NzVmODcwMmU4MzVjNDQ2Y2UwMjE0NjMxOWVkY2I2MzJjOTc4M2QuLmZkZjVlYWRkYjdjZWU0 NjI2ZDg1MzJiZjdhMjVhOGU2NTFkZGZjOWUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3Jl bmRlcmluZy9SZW5kZXJUYWJsZVNlY3Rpb24uY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRl cmluZy9SZW5kZXJUYWJsZVNlY3Rpb24uY3BwCkBAIC0xMDYwLDkgKzEwNjAsNyBAQCB2b2lkIFJl bmRlclRhYmxlU2VjdGlvbjo6cGFpbnRPYmplY3QoUGFpbnRJbmZvJiBwYWludEluZm8sIGNvbnN0 IExheW91dFBvaW50JiBwYQogICAgICAgICAgICAgbG9jYWxSZXBhaW50UmVjdC5zZXRYKHdpZHRo KCkgLSBsb2NhbFJlcGFpbnRSZWN0Lm1heFgoKSk7CiAgICAgfQogCi0gICAgLy8gRklYTUU6IFdo eSBkbyB3ZSBkb3VibGUgdGhlIG91dGxpbmUgc2l6ZT8KLSAgICBMYXlvdXRVbml0IG91dGxpbmVT aXplID0gMiAqIG1heGltYWxPdXRsaW5lU2l6ZShwYWludFBoYXNlKTsKLSAgICBsb2NhbFJlcGFp bnRSZWN0LmluZmxhdGUob3V0bGluZVNpemUpOworICAgIGxvY2FsUmVwYWludFJlY3QuaW5mbGF0 ZShtYXhpbWFsT3V0bGluZVNpemUocGFpbnRQaGFzZSkpOwogCiAgICAgQ2VsbFNwYW4gZGlydGll ZFJvd3MgPSB0aGlzLT5kaXJ0aWVkUm93cyhsb2NhbFJlcGFpbnRSZWN0KTsKICAgICBDZWxsU3Bh biBkaXJ0aWVkQ29sdW1ucyA9IHRoaXMtPmRpcnRpZWRDb2x1bW5zKGxvY2FsUmVwYWludFJlY3Qp Owo=