[webkit-reviews] review granted: [Bug 123236] Don't round horizontal underline extents : [Attachment 215013] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 123236: Don't round horizontal underline extents
https://bugs.webkit.org/show_bug.cgi?id=123236

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

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


Looks like a good fix.

> Source/WebCore/ChangeLog:27
> +	   There are two pieces to this change. The first piece is in
> +	   InlineTextBox::paint(). When we were computing the bounding
> +	   box for the InlineTextBox, we were putting the floating-point
> +	   extents we received from the InlineBox into a LayoutSize. If
> +	   a LayoutSize were fixed-point with sufficient precision, this
> +	   would be okay, but right now a LayoutSize is simply an int,
> +	   meaning that we were clamping the underline boundaries to
> +	   integral values. Instead, we should be using the same unit
> +	   throughout all of this code, which means that the bounding
> +	   box should use floats instead.
> +
> +	   In addition, inside GraphicsContext::drawLineForText(), we are
> +	   rounding the underline to pixel boundaries so that it appears
> +	   very crisp on the screen. However, this does not play well with
> +	   these fractional underline extents. We should change the
> +	   rounding mode to perform floating point addition before rounding,
> +	   rather than rounding position and width before adding them
> +	   together. Note that this will not blend the pixel in question
> +	   between two colors; instead, whichever run contains the center
> +	   of the disputed pixel will win.

Next time, please write a much smaller comment, and do it per-function.

> Source/WebCore/ChangeLog:32
> +	   (WebCore::GraphicsContext::drawLineForText): Second part (See above)


I would say something like: “Round each end of the underline to a pixel
boundary.”

> Source/WebCore/ChangeLog:34
> +	   (WebCore::InlineTextBox::paint): First part (See above)

I would say something like: “Don’t convert to a LayoutSize just to convert to a
FloatSize.”

> LayoutTests/ChangeLog:14
> +	   This test contains two adjacent text runs. The first one has a
> +	   x-position of 21.3 with a width of 44.4. The second one has a
> +	   x-position of (21.3 + 44.4 =) 65.7. If we naively rounded these
> +	   to pixel coordinates, the left run would be rounded to a
> +	   x-position of 21 with a width of 44, so it would extend to pixel
> +	   65. However, the next run's x-position would be rounded to 66,
> +	   so there is a single pixel gap between the supposedly adjacent run.
> +	   This test makes sure that the above scenerio does not occur,
> +	   and that this "gap" pixel is filled in.

This comment is also too long, and more importantly, it would be much more
appropriate in the test rather than in the change log.

> LayoutTests/ChangeLog:17
> +	   *
platform/mac/fast/css3-text/css3-text-decoration/no-gap-between-two-rounded-tex
tboxes-expected.png: Added.

Could we make this a reference test somehow? Maybe use a canvas to draw the
underline?