Bug 67009 – Clicking causes dirty rects that may be unnecessary; this affects future optimizations.

• Status: UNCONFIRMED
• Component: WebCore Misc.

[Shawn Singh]

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.

[Shawn Singh]

Created attachment 105520 [details]
Patch

[Shawn Singh]

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?

[WebKit Review Bot]

Comment on attachment 105520 [details]
Patch

Attachment 105520 [details] 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

[Darin Adler]

Comment on attachment 105520 [details]
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.

[Darin Adler]

Comment on attachment 105520 [details]
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.

[Darin Adler]

(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.

[Darin Adler]

I think a correct fix might be to invalidate the old rect only if m_caretPaint is true.

[Shawn Singh]

(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.

[Shawn Singh]

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.

[Darin Adler]

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.