Bug 116489

Summary: Avoid caret repaints if we're not showing carets anyway.
Product: WebKit Reporter: Andreas Kling <kling>
Component: HTML EditingAssignee: 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 Flags
Patch simon.fraser: review+

Description Andreas Kling 2013-05-20 18:40:36 PDT
Avoid caret repaints if we're not showing carets anyway.
Comment 1 Andreas Kling 2013-05-20 18:42:45 PDT
Created attachment 202368 [details]
Patch
Comment 2 Andreas Kling 2013-05-20 18:52:58 PDT
Committed r150396: <http://trac.webkit.org/changeset/150396>
Comment 3 Darin Adler 2013-05-21 08:40:17 PDT
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?
Comment 4 Andreas Kling 2013-05-21 09:20:39 PDT
(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?
Comment 5 Darin Adler 2013-05-21 09:58:34 PDT
(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.
Comment 6 Ryosuke Niwa 2013-05-21 10:36:49 PDT
(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.