[webkit-reviews] review granted: [Bug 132164] Subpixel rendering: Inline text selection painting should not snap to integral CSS pixel position. : [Attachment 230131] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Zalan Bujtas <zalan at apple.com>'s
request for review:
Bug 132164: Subpixel rendering: Inline text selection painting should not snap
to integral CSS pixel position.
https://bugs.webkit.org/show_bug.cgi?id=132164

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=230131&action=review


I wish I understood the difference between “rounding” and “pixel snapping”.

> Source/WebCore/rendering/EllipsisBox.cpp:140
> +    FloatRect clipRect = pixelSnappedForPainting(x() + paintOffset.x(), top
+ paintOffset.y(), m_logicalWidth, h,
renderer().document().deviceScaleFactor());
> +
>      context->clip(clipRect);

Not sure we need a local variable for this.

> Source/WebCore/rendering/InlineTextBox.cpp:760
> +    LayoutUnit selHeight = std::max<LayoutUnit>(0, selectionBottom -
selectionTop);

Might be nice to use the word “selection” instead of the abbreviation “sel”
here.

> Source/WebCore/rendering/InlineTextBox.cpp:765
> +    FloatRect clipRect =
pixelSnappedForPainting(LayoutRect(LayoutPoint(localOrigin),
LayoutSize(m_logicalWidth, selHeight)), deviceScaleFactor);
>      context->clip(clipRect);

Not sure we need a local variable for this.

> Source/WebCore/rendering/InlineTextBox.h:199
>  INLINE_BOX_OBJECT_TYPE_CASTS(InlineTextBox, isInlineTextBox())
> -
> -void alignSelectionRectToDevicePixels(FloatRect&);
> -
>  } // namespace WebCore

Should leave a blank line here.