Bug 116489 - Avoid caret repaints if we're not showing carets anyway.
Summary: Avoid caret repaints if we're not showing carets anyway.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance
Depends on:
Blocks:
 
Reported: 2013-05-20 18:40 PDT by Andreas Kling
Modified: 2013-05-21 10:36 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.75 KB, patch)
2013-05-20 18:42 PDT, Andreas Kling
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.