71517 2011-11-03 16:18:29 -0700 RenderLayer::styleChanged invalidates the GraphicsLayer needlessly 2011-11-04 11:53:16 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED http://jsbin.com/ulofav P2 Normal --- 1 jchaffraix jchaffraix enne jamesr simon.fraser webkit.review.bot oldest_to_newest 495853 0 jchaffraix 2011-11-03 16:18:29 -0700 Currently we invalidate the whole GraphicsLayer with: void RenderLayer::styleChanged(StyleDifference diff, const RenderStyle* oldStyle) ... if (m_backing && diff >= StyleDifferenceRepaint) m_backing->setContentsNeedDisplay(); This does not seem to take into account that the repainting logic will invalidate the needed part of the layer. 495870 1 113573 jchaffraix 2011-11-03 16:30:22 -0700 Created attachment 113573 Proposed change: remove the invalidation code so that we rely on repainting properly. 495912 2 113573 darin 2011-11-03 17:26:22 -0700 Comment on attachment 113573 Proposed change: remove the invalidation code so that we rely on repainting properly. View in context: https://bugs.webkit.org/attachment.cgi?id=113573&action=review > Source/WebCore/ChangeLog:8 > + Refactoring covered by existing tests. I don’t think this meets the definition of “refactoring”. The claim here is that we are removing unneeded code. 495924 3 simon.fraser 2011-11-03 17:31:59 -0700 It's going to be hard to find out whether this causes painting regressions. 495949 4 jchaffraix 2011-11-03 17:50:31 -0700 (In reply to comment #3) > It's going to be hard to find out whether this causes painting regressions. What is sure is that we are over-invalidating and this is hurting us (see http://code.google.com/p/chromium/issues/detail?id=99814 for an example where we repaint each frame for no good reason). This patch is maybe going too far (on purpose) and the scope can be reduced for safety. It may also be a good experiment to assess and extend our repaint coverage. 495989 5 113573 simon.fraser 2011-11-03 18:51:25 -0700 Comment on attachment 113573 Proposed change: remove the invalidation code so that we rely on repainting properly. I think it's OK to check this in, but we should be on the lookout for repainting regressions. Do all the pixel tests pass? 496006 6 jchaffraix 2011-11-03 19:51:49 -0700 (In reply to comment #5) > (From update of attachment 113573 [details]) > I think it's OK to check this in, but we should be on the lookout for repainting regressions. Do all the pixel tests pass? Yes, all the pixel tests passed on Chromium Linux (the EWS confirmed that I did not miss some tests). I have tested also a bit on Mac but I have run into other issues at the time (what I got looked fine but it was noisy). I have to redo the ChangeLog to account for my sloppy use of the word 'refactoring' so I will re-run the tests on Mac before landing. 496310 7 113681 jchaffraix 2011-11-04 11:00:25 -0700 Created attachment 113681 Patch for landing 496318 8 jchaffraix 2011-11-04 11:06:30 -0700 There was still a lot of noise when running the tests on Mac. Most of them are because NRWT does not pick up the custom scrollbars or some missing rebaselines. One or 2 were looking weird but did not look like repainting issue. I will watch the bots and see what they think. 496366 9 113681 webkit.review.bot 2011-11-04 11:53:11 -0700 Comment on attachment 113681 Patch for landing Clearing flags on attachment: 113681 Committed r99307: <http://trac.webkit.org/changeset/99307> 496367 10 webkit.review.bot 2011-11-04 11:53:16 -0700 All reviewed patches have been landed. Closing bug. 113573 2011-11-03 16:30:22 -0700 2011-11-04 11:00:22 -0700 Proposed change: remove the invalidation code so that we rely on repainting properly. bug-71517-20111103163021.patch text/plain 2118 jchaffraix U3VidmVyc2lvbiBSZXZpc2lvbjogOTkyMDgKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA4YTJmMDY2YTM4ZmQ4NzQz YWQ3YjhmZGUyYTY2ZjQxNGM2NDhlZTdkLi4xYjdhZDg0YzJiMWNkN2M0ZWU2MDZkNDYxNjMyMDAy MzE3Mjg2ODc0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjAgQEAKKzIwMTEtMTEtMDMgIEp1bGll biBDaGFmZnJhaXggIDxqY2hhZmZyYWl4QHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmVuZGVyTGF5 ZXI6OnN0eWxlQ2hhbmdlZCBpbnZhbGlkYXRlcyB0aGUgR3JhcGhpY3NMYXllciBuZWVkbGVzc2x5 CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03MTUxNwor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJlZmFjdG9y aW5nIGNvdmVyZWQgYnkgZXhpc3RpbmcgdGVzdHMuCisKKyAgICAgICAgVGhlIGN1cnJlbnQgY29k ZSBpcyBpbnZhbGlkYXRpbmcgdGhlIHdob2xlIFJlbmRlckxheWVyQmFja2luZyBhcyBwYXJ0IG9m CisgICAgICAgIGEgc3R5bGUgY2hhbmdlLiBIb3dldmVyIHRoaXMgaXMgcmVkdW5kYW50IHdpdGgg dGhlIHJlcGFpbnRpbmcgbG9naWMgd2hpY2gKKyAgICAgICAgd291bGQgaW52YWxpZGF0ZSBvbmx5 IHRoZSBuZWNlc3NhcnkgYml0cy4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJMYXllci5j cHA6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJMYXllcjo6c3R5bGVDaGFuZ2VkKToKKyAgICAg ICAgUmVtb3ZlIHRoZSBpbnZhbGlkYXRpb24gY29kZSBhcyBpdCBkdXBsaWNhdGVzIGFuZCBkZWZl YXRzIHRoZSByZXBhaW50IGxvZ2ljLgorCiAyMDExLTExLTAzICBBbmRyZWFzIEtsaW5nICA8a2xp bmdAd2Via2l0Lm9yZz4KIAogICAgICAgICBDU1NSdWxlTGlzdDogTW92ZSBydWxlIG9ycGhhbmlu ZyBmcm9tIGRlbGV0ZVJ1bGUoKSBvdXQgdG8gY2FsbGVycy4KZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJMYXllci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJp bmcvUmVuZGVyTGF5ZXIuY3BwCmluZGV4IDVhNDgzYjIxZjBlODI2YmI4YTBlYzkyMDYxNzQ1Mzkx MDUxYmE3ZDguLjkwZTRhZTBiYTE5NGUwYjU0NWY3MzE2MDRlODUzOTk4ZTk4ODhiZTQgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJMYXllci5jcHAKKysrIGIvU291 cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckxheWVyLmNwcApAQCAtNDE3Miw3ICs0MTcyLDcg QEAgYm9vbCBSZW5kZXJMYXllcjo6aXNTZWxmUGFpbnRpbmdMYXllcigpIGNvbnN0CiAgICAgICAg IHx8IHJlbmRlcmVyKCktPmlzUmVuZGVySUZyYW1lKCk7CiB9CiAKLXZvaWQgUmVuZGVyTGF5ZXI6 OnN0eWxlQ2hhbmdlZChTdHlsZURpZmZlcmVuY2UgZGlmZiwgY29uc3QgUmVuZGVyU3R5bGUqIG9s ZFN0eWxlKQordm9pZCBSZW5kZXJMYXllcjo6c3R5bGVDaGFuZ2VkKFN0eWxlRGlmZmVyZW5jZSwg Y29uc3QgUmVuZGVyU3R5bGUqIG9sZFN0eWxlKQogewogICAgIGJvb2wgaXNOb3JtYWxGbG93T25s eSA9IHNob3VsZEJlTm9ybWFsRmxvd09ubHkoKTsKICAgICBpZiAoaXNOb3JtYWxGbG93T25seSAh PSBtX2lzTm9ybWFsRmxvd09ubHkpIHsKQEAgLTQyMzUsMTEgKzQyMzUsNiBAQCB2b2lkIFJlbmRl ckxheWVyOjpzdHlsZUNoYW5nZWQoU3R5bGVEaWZmZXJlbmNlIGRpZmYsIGNvbnN0IFJlbmRlclN0 eWxlKiBvbGRTdHlsZQogICAgICAgICBpZiAoc3RhY2tpbmdDb250ZXh0KCktPmhhc0NvbXBvc2l0 aW5nRGVzY2VuZGFudCgpKQogICAgICAgICAgICAgY29tcG9zaXRvcigpLT5zZXRDb21wb3NpdGlu Z0xheWVyc05lZWRSZWJ1aWxkKCk7CiAgICAgfQotICAgIAotICAgIGlmIChtX2JhY2tpbmcgJiYg ZGlmZiA+PSBTdHlsZURpZmZlcmVuY2VSZXBhaW50KQotICAgICAgICBtX2JhY2tpbmctPnNldENv bnRlbnRzTmVlZERpc3BsYXkoKTsKLSNlbHNlCi0gICAgVU5VU0VEX1BBUkFNKGRpZmYpOwogI2Vu ZGlmCiB9CiAK 113681 2011-11-04 11:00:25 -0700 2011-11-04 11:53:11 -0700 Patch for landing bug-71517-20111104110024.patch text/plain 2101 jchaffraix U3VidmVyc2lvbiBSZXZpc2lvbjogOTkyOTEKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA1YmFiNGRlOTAyYzFmOTVi ZGQyZGUwM2M4OTJiM2JhOWM5NTczYTU1Li41ODkyZjFhNjU0Y2I2NGZkMzhmMzljYzNjNWFjMjhh MTA5OWEwYzQ3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjAgQEAKKzIwMTEtMTEtMDQgIEp1bGll biBDaGFmZnJhaXggIDxqY2hhZmZyYWl4QHdlYmtpdC5vcmc+CisKKyAgICAgICAgUmVuZGVyTGF5 ZXI6OnN0eWxlQ2hhbmdlZCBpbnZhbGlkYXRlcyB0aGUgR3JhcGhpY3NMYXllciBuZWVkbGVzc2x5 CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03MTUxNwor CisgICAgICAgIFJldmlld2VkIGJ5IFNpbW9uIEZyYXNlci4KKworICAgICAgICBVbnVzZWQgY29k ZSByZW1vdmFsIGNvdmVyZWQgYnkgZXhpc3RpbmcgdGVzdHMuCisKKyAgICAgICAgVGhlIGN1cnJl bnQgY29kZSBpcyBpbnZhbGlkYXRpbmcgdGhlIHdob2xlIFJlbmRlckxheWVyQmFja2luZyBhcyBw YXJ0IG9mCisgICAgICAgIGEgc3R5bGUgY2hhbmdlLiBIb3dldmVyIHRoaXMgaXMgcmVkdW5kYW50 IHdpdGggdGhlIHJlcGFpbnRpbmcgbG9naWMgd2hpY2gKKyAgICAgICAgd291bGQgaW52YWxpZGF0 ZSBvbmx5IHRoZSBuZWNlc3NhcnkgYml0cy4KKworICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJM YXllci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJMYXllcjo6c3R5bGVDaGFuZ2VkKToK KyAgICAgICAgUmVtb3ZlIHRoZSBpbnZhbGlkYXRpb24gY29kZSBhcyBpdCBkdXBsaWNhdGVzIGFu ZCBkZWZlYXRzIHRoZSByZXBhaW50IGxvZ2ljLgorCiAyMDExLTExLTAzICBBbmRlcnMgQ2FybHNz b24gIDxhbmRlcnNjYUBhcHBsZS5jb20+CiAKICAgICAgICAgQWRkIE5ldHNjYXBlUGx1Z2luOjpj b252ZXJ0RnJvbVJvb3RWaWV3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcv UmVuZGVyTGF5ZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlckxheWVyLmNw cAppbmRleCA1YTQ4M2IyMWYwZTgyNmJiOGEwZWM5MjA2MTc0NTM5MTA1MWJhN2Q4Li45MGU0YWUw YmExOTRlMGI1NDVmNzMxNjA0ZTg1Mzk5OGU5ODg4YmU0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi Q29yZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXIuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRl cmluZy9SZW5kZXJMYXllci5jcHAKQEAgLTQxNzIsNyArNDE3Miw3IEBAIGJvb2wgUmVuZGVyTGF5 ZXI6OmlzU2VsZlBhaW50aW5nTGF5ZXIoKSBjb25zdAogICAgICAgICB8fCByZW5kZXJlcigpLT5p c1JlbmRlcklGcmFtZSgpOwogfQogCi12b2lkIFJlbmRlckxheWVyOjpzdHlsZUNoYW5nZWQoU3R5 bGVEaWZmZXJlbmNlIGRpZmYsIGNvbnN0IFJlbmRlclN0eWxlKiBvbGRTdHlsZSkKK3ZvaWQgUmVu ZGVyTGF5ZXI6OnN0eWxlQ2hhbmdlZChTdHlsZURpZmZlcmVuY2UsIGNvbnN0IFJlbmRlclN0eWxl KiBvbGRTdHlsZSkKIHsKICAgICBib29sIGlzTm9ybWFsRmxvd09ubHkgPSBzaG91bGRCZU5vcm1h bEZsb3dPbmx5KCk7CiAgICAgaWYgKGlzTm9ybWFsRmxvd09ubHkgIT0gbV9pc05vcm1hbEZsb3dP bmx5KSB7CkBAIC00MjM1LDExICs0MjM1LDYgQEAgdm9pZCBSZW5kZXJMYXllcjo6c3R5bGVDaGFu Z2VkKFN0eWxlRGlmZmVyZW5jZSBkaWZmLCBjb25zdCBSZW5kZXJTdHlsZSogb2xkU3R5bGUKICAg ICAgICAgaWYgKHN0YWNraW5nQ29udGV4dCgpLT5oYXNDb21wb3NpdGluZ0Rlc2NlbmRhbnQoKSkK ICAgICAgICAgICAgIGNvbXBvc2l0b3IoKS0+c2V0Q29tcG9zaXRpbmdMYXllcnNOZWVkUmVidWls ZCgpOwogICAgIH0KLSAgICAKLSAgICBpZiAobV9iYWNraW5nICYmIGRpZmYgPj0gU3R5bGVEaWZm ZXJlbmNlUmVwYWludCkKLSAgICAgICAgbV9iYWNraW5nLT5zZXRDb250ZW50c05lZWREaXNwbGF5 KCk7Ci0jZWxzZQotICAgIFVOVVNFRF9QQVJBTShkaWZmKTsKICNlbmRpZgogfQogCg==