Avoid caret repaints if we're not showing carets anyway.
Created attachment 202368 [details] Patch
Committed r150396: <http://trac.webkit.org/changeset/150396>
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?
(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?
(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.
(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.