Avoid caret repaints if we're not showing carets anyway.

[Andreas Kling]

Avoid caret repaints if we're not showing carets anyway.

[Andreas Kling]

Created attachment 202368 [details]
Patch

[Andreas Kling]

Committed r150396: <http://trac.webkit.org/changeset/150396>

[Darin Adler]

Comment on attachment 202368 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=202368&action=review

> Source/WebCore/editing/FrameSelection.cpp:1400
> +                repaintCaretForLocalRect(m_previousCaretNode.get(), oldRect);

What guarantees m_previousCaretNode is inside the same editable area? Can’t shouldRepaintCaret be incorrect for the previous caret node?

[Andreas Kling]

(In reply to comment #3)
> (From update of attachment 202368 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202368&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:1400
> > +                repaintCaretForLocalRect(m_previousCaretNode.get(), oldRect);
> 
> What guarantees m_previousCaretNode is inside the same editable area? Can’t shouldRepaintCaret be incorrect for the previous caret node?

Hmm. Is this a problem in practice?

If I understand correctly, that means m_previousCaretNode would have to be contenteditable, and moving the selection out of it should cause m_previousCaretNode to blur, and thus repaint anyway.

I guess it's not a bad idea to robustify the condition, e.g:

-if (shouldRepaintCaret(view, isContentEditable())) {
+if (shouldRepaintCaret(view, isContentEditable() || (m_previousCaretNode && m_previousCaretNode->isContentEditable()))) {

Maybe I'm misunderstanding how this is supposed to work?

[Darin Adler]

(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebCore/editing/FrameSelection.cpp:1400
> > > +                repaintCaretForLocalRect(m_previousCaretNode.get(), oldRect);
> > 
> > What guarantees m_previousCaretNode is inside the same editable area? Can’t shouldRepaintCaret be incorrect for the previous caret node?
> 
> Hmm. Is this a problem in practice?

I have no idea.

[Ryosuke Niwa]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 202368 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202368&action=review
> > 
> > > Source/WebCore/editing/FrameSelection.cpp:1400
> > > +                repaintCaretForLocalRect(m_previousCaretNode.get(), oldRect);
> > 
> > What guarantees m_previousCaretNode is inside the same editable area? Can’t shouldRepaintCaret be incorrect for the previous caret node?
> 
> Hmm. Is this a problem in practice?

Yes.

> If I understand correctly, that means m_previousCaretNode would have to be contenteditable, and moving the selection out of it should cause m_previousCaretNode to blur, and thus repaint anyway.

Not necessarily if the editability is changed by author script.

> I guess it's not a bad idea to robustify the condition, e.g:
> 
> -if (shouldRepaintCaret(view, isContentEditable())) {
> +if (shouldRepaintCaret(view, isContentEditable() || (m_previousCaretNode && m_previousCaretNode->isContentEditable()))) {
> 
> Maybe I'm misunderstanding how this is supposed to work?

Please do that.