[webkit-reviews] review denied: [Bug 34819] Extraneous repaint of caret rectangle when selection is not in content editable element : [Attachment 49109] Patch with test case

Thu Mar 4 13:10:34 PST 2010

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 34819: Extraneous repaint of caret rectangle when selection is not in
content editable element
https://bugs.webkit.org/show_bug.cgi?id=34819

Attachment 49109: Patch with test case
https://bugs.webkit.org/attachment.cgi?id=49109&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/editing/SelectionController.cpp
> ===================================================================

>	  
view->repaintRectangleInViewAndCompositedLayers(oldAbsoluteCaretRepaintBounds,
false);
> -	  
view->repaintRectangleInViewAndCompositedLayers(m_absoluteCaretRepaintBounds,
false);
> +	   repaintCaretInViewIfNeeded(view);

Since m_absoluteCaretRepaintBounds is no longer being passed in, you're forced
to recompute it. You should pass it in.

> +void SelectionController::repaintCaretInViewIfNeeded(RenderView* view) const

Passing in the view here seems silly; it really isn't saving any work.

> +    bool caretBrowsing = m_frame && m_frame->settings() &&
m_frame->settings()->caretBrowsingEnabled();
> +    if (!caretBrowsing && !isContentEditable())
> +	   return;

What causes the caret to get repainted when the state of caretBrowsingEnabled()
changes?

> ---
LayoutTests/fast/repaint/no-caret-repaint-in-non-content-editable-element.html 
  (revision 0)
> +++
LayoutTests/fast/repaint/no-caret-repaint-in-non-content-editable-element.html 
  (revision 0)
> @@ -0,0 +1,22 @@
> +<html>
> +<head>
> +<script src="resources/repaint.js"></script>
> +<script>
> +function repaintTest()
> +{
> +    if (!window.eventSender)
> +	   return;
> +
> +    var target = document.getElementById("target");
> +    eventSender.mouseMoveTo(target.offsetLeft, target.offsetTop);
> +    eventSender.mouseDown();
> +    eventSender.mouseUp();
> +}
> +window.onload = runRepaintTest;
> +</script>
> +</head>
> +<body>
> +    <p>This tests that clicking on a non-content editable element does not
cause a repaint of it in whole or in part. In particular, the <a
href="http://trac.webkit.org/browser/trunk/WebCore/editing/SelectionController.
cpp?rev=53218#L953">computed caret rectangle</a> should not be repainted when
the caret will not be shown anyway (because the rectangle is part of an element
that cannot be edited).</p>
> +    <p id="target">This element is not content editable. When clicked,
neither a text insertion caret should appear nor any part of this line be
repainted.</p>
> +</body>
> +</html>

Try to keep the amount of text down to avoid spurious test failures due to
antialiasing differences.

r- for the recomputation of the caret rect.