Summary: | Avoid caret repaints if we're not showing carets anyway. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||
Component: | HTML Editing | Assignee: | Andreas Kling <kling> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | darin, kling, rniwa | ||||
Priority: | P2 | Keywords: | Performance | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Andreas Kling
2013-05-20 18:40:36 PDT
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. |