[webkit-reviews] review denied: [Bug 25228] SelectionController::absoluteCaretBounds returns an inflated caret (the caret repaint rect) : [Attachment 29527] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Justin Garcia
<justin.garcia at apple.com>'s request for review:
Bug 25228: SelectionController::absoluteCaretBounds returns an inflated caret
(the caret repaint rect)
https://bugs.webkit.org/show_bug.cgi?id=25228

Attachment 29527: patch
https://bugs.webkit.org/attachment.cgi?id=29527&action=review

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

> -    IntRect oldAbsRepaintRect = m_absCaretBounds;
> -    m_absCaretBounds = caretRepaintRect();
> +    IntRect oldAbsCaretBounds = m_absCaretBounds;
> +    m_absCaretBounds = m_caretRect;
> +    // m_caretRect is the local rect, we need the bounds of (possibly
transformed) caret in absolute coordinates.
> +    RenderObject* caretPainter = caretRenderer();
> +    if (caretPainter)
> +	   m_absCaretBounds =
caretPainter->localToAbsoluteQuad(FloatRect(m_caretRect)).enclosingBoundingBox(
);
>      m_absCaretBoundsDirty = false;
>      
> -    if (oldAbsRepaintRect == m_absCaretBounds)
> +    if (oldAbsCaretBounds == m_absCaretBounds)
>	   return false;
>      
>      if (RenderView* view =
static_cast<RenderView*>(m_frame->document()->renderer())) {
>	   // FIXME: make caret repainting container-aware.
> -	   view->repaintRectangleInViewAndCompositedLayers(oldAbsRepaintRect,
false);
> -	   view->repaintRectangleInViewAndCompositedLayers(m_absCaretBounds,
false);
> +	  
view->repaintRectangleInViewAndCompositedLayers(repaintRectForCaret(oldAbsCaret
Bounds), false);
> +	  
view->repaintRectangleInViewAndCompositedLayers(repaintRectForCaret(m_absCaretB
ounds), false);

I don't think this is right. This does the inflateX(1) after the transforms,
whereas it needs to be before.

I think the correct solution is to make absoluteCaretBounds() look like
caretRepaintRect(), without
doing the inflateX(1) via repaintRectForCaret.

Also, maybe we can get rid of the inflation entirely after bug 24527 is fixed?
Bug 19086 needs re-testing.