67009 2011-08-25 18:12:40 -0700 Clicking causes dirty rects that may be unnecessary; this affects future optimizations. 2011-08-30 17:29:36 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified UNCONFIRMED P2 Normal --- 0 shawnsingh webkit-unassigned ap darin dglazkov fishd mitz vangelis webkit.review.bot oldest_to_newest 457318 0 shawnsingh 2011-08-25 18:12:40 -0700 To reproduce: use the --show-paint-rects option with software compositing in chromium. click several times in various places on somewhere near text, and small dirty rects will appear in other places. eventually the pattern will become clear the dirty rect appears near your previous click, where the invisible caret was probably located. My guess is that the problem is somewhere related to recomputeCaretRect() in WebCore/editing/FrameSelection.cpp. So far this problem was probably insignificant, but for upcoming chromium compositor optimizations these arbitrary rects can substantially inflate the area that needs to be redrawn. 458441 1 105520 shawnsingh 2011-08-29 14:01:19 -0700 Created attachment 105520 Patch 458443 2 shawnsingh 2011-08-29 14:04:40 -0700 This patch definitely needs a reviewer who is familiar with WebCore/editing/FrameSelection.cpp I am not confident this is correct, but the change does work testing manually, and all layout tests still pass (tested on on osx). This is sort of an internal change, so I don't think it makes sense to submit a layout test with it? Is there some other sort of test to include with it? 458461 3 105520 webkit.review.bot 2011-08-29 14:33:22 -0700 Comment on attachment 105520 Patch Attachment 105520 did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9566192 New failing tests: fast/repaint/japanese-rl-selection-repaint.html fast/repaint/selection-rl.html fast/repaint/4776765.html fast/repaint/4774354.html 458527 4 105520 darin 2011-08-29 16:17:59 -0700 Comment on attachment 105520 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=105520&action=review > Source/WebCore/ChangeLog:10 > + Tests N/A It’s not true that tests are not applicable to a change like this. We do want to come up with a way to test a change like this. > Source/WebCore/editing/FrameSelection.cpp:1222 > - m_absoluteCaretRepaintBounds = caretRepaintRect(m_selection.start().deprecatedNode()); > + m_absoluteCaretRepaintBounds = m_selection.isNone() ? caretRepaintRect(m_selection.start().deprecatedNode()) : LayoutRect(); This change means that when you are changing the selection to none, we won’t erase the old caret. That’s not correct. 458530 5 105520 darin 2011-08-29 16:19:38 -0700 Comment on attachment 105520 Patch A correct fix for this needs to understand the difference between an old caret that was visible and an old caret that was not visible. 458533 6 darin 2011-08-29 16:21:27 -0700 (In reply to comment #4) > (From update of attachment 105520 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105520&action=review > > > Source/WebCore/ChangeLog:10 > > + Tests N/A > > It’s not true that tests are not applicable to a change like this. We do want to come up with a way to test a change like this. We just need some way to detect the effects of repaint. I think others have done some work on this. 458539 7 darin 2011-08-29 16:25:02 -0700 I think a correct fix might be to invalidate the old rect only if m_caretPaint is true. 458545 8 shawnsingh 2011-08-29 16:32:21 -0700 (In reply to comment #7) > I think a correct fix might be to invalidate the old rect only if m_caretPaint is true. Thanks for your comments, I'll try to submit another patch in a few days. 459340 9 shawnsingh 2011-08-30 17:07:59 -0700 Darin, can you please suggest exactly how we should add an automated test for this? The problem is: a correct fix to this problem would not change anything in the final render trees and pixels... would there still exist a layout test for this, or is there some other set of automated tests I should be adding to? Thanks very much. 459354 10 darin 2011-08-30 17:29:36 -0700 There are tests called “repaint tests” that check to see whether we have proper repaint rectangles. Dan Bernstein knows more about those and might be able to tell you whether we could use one of those to test this. If not, we could add a DumpRenderTree hook specifically to check if there is repainting queued up, and use that hook to add a test. 105520 2011-08-29 14:01:19 -0700 2011-08-29 16:19:38 -0700 Patch bug-67009-20110829140118.patch text/plain 1723 shawnsingh U3VidmVyc2lvbiBSZXZpc2lvbjogOTM5NzcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCA3N2VlNTdlZTFiYTYyOTQ0 NDhlOGYwMWVkNWVhOTdiYjA3NDZlZWJlLi5iYTZjMmM4ZTU2ZjE2ZDdiN2JjMWI4MTRkZDE3ZDk4 YmJjYTFlMDQ3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDgtMjkgIFNoYXdu IFNpbmdoICA8c2hhd25zaW5naEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgV2hlbiBjbGlja2lu ZyBvbiB0ZXh0LCBhbiB1bm5lY2Vzc2FyeSBkaXJ0eSByZWN0IGlzIGNyZWF0ZWQKKyAgICAgICAg d2hlcmUgdGhlIGNhcmV0IHdhcyBsb2NhdGVkIHByZXZpb3VzbHkuICBUaGlzIHBhdGNoIHNob3Vs ZAorICAgICAgICBmaXggaXQuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD02NzAwOQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgIFRlc3RzIE4vQQorCisgICAgICAgICogZWRpdGluZy9GcmFtZVNlbGVjdGlvbi5j cHA6CisgICAgICAgIChXZWJDb3JlOjpGcmFtZVNlbGVjdGlvbjo6cmVjb21wdXRlQ2FyZXRSZWN0 KToKKwogMjAxMS0wOC0yNSAgUGF2ZWwgUG9kaXZpbG92ICA8cG9kaXZpbG92QGNocm9taXVtLm9y Zz4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiByZW5hbWUgc291cmNlRmlsZSB0byAocmF3fHVp KVNvdXJjZUNvZGUgaW4gRGVidWdnZXJQcmVzZW50YXRpb25Nb2RlbC4KZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL2VkaXRpbmcvRnJhbWVTZWxlY3Rpb24uY3BwIGIvU291cmNlL1dlYkNvcmUv ZWRpdGluZy9GcmFtZVNlbGVjdGlvbi5jcHAKaW5kZXggMThmZTllZjRiYTFiYzUwYmM0ODgzNWU1 MjkzMjVjZmMxOWZiYzBlNC4uMWQ3M2VjZmZjNzBmNTgyZWMyNjZlMzc2NzU0YTk3Yjg3YWU3NzUx ZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvZWRpdGluZy9GcmFtZVNlbGVjdGlvbi5jcHAK KysrIGIvU291cmNlL1dlYkNvcmUvZWRpdGluZy9GcmFtZVNlbGVjdGlvbi5jcHAKQEAgLTEyMTks NyArMTIxOSw3IEBAIGJvb2wgRnJhbWVTZWxlY3Rpb246OnJlY29tcHV0ZUNhcmV0UmVjdCgpCiAg ICAgICAgIAogICAgIExheW91dFJlY3Qgb2xkQWJzb2x1dGVDYXJldFJlcGFpbnRCb3VuZHMgPSBt X2Fic29sdXRlQ2FyZXRSZXBhaW50Qm91bmRzOwogICAgIC8vIFdlIGJlbGlldmUgdGhhdCB3ZSBu ZWVkIHRvIGluZmxhdGUgdGhlIGxvY2FsIHJlY3QgYmVmb3JlIHRyYW5zZm9ybWluZyBpdCB0byBv YnRhaW4gdGhlIHJlcGFpbnQgYm91bmRzLgotICAgIG1fYWJzb2x1dGVDYXJldFJlcGFpbnRCb3Vu ZHMgPSBjYXJldFJlcGFpbnRSZWN0KG1fc2VsZWN0aW9uLnN0YXJ0KCkuZGVwcmVjYXRlZE5vZGUo KSk7CisgICAgbV9hYnNvbHV0ZUNhcmV0UmVwYWludEJvdW5kcyA9IG1fc2VsZWN0aW9uLmlzTm9u ZSgpID8gY2FyZXRSZXBhaW50UmVjdChtX3NlbGVjdGlvbi5zdGFydCgpLmRlcHJlY2F0ZWROb2Rl KCkpIDogTGF5b3V0UmVjdCgpOwogCiAjaWYgRU5BQkxFKFRFWFRfQ0FSRVQpICAgIAogICAgIGlm IChSZW5kZXJWaWV3KiB2aWV3ID0gdG9SZW5kZXJWaWV3KG1fZnJhbWUtPmRvY3VtZW50KCktPnJl bmRlcmVyKCkpKSB7Cg==