[webkit-reviews] review canceled: [Bug 126601] [CSS Shapes] First line gets incorrectly adjusted in shape-inside due to rounding in some polygons : [Attachment 220552] proposed patch

[bugzilla-daemon at webkit.org]

Zoltan Horvath <zoltan at webkit.org> has canceled Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 126601: [CSS Shapes] First line gets incorrectly adjusted in shape-inside
due to rounding in some polygons
https://bugs.webkit.org/show_bug.cgi?id=126601

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

------- Additional Comments from Zoltan Horvath <zoltan at webkit.org>
(In reply to comment #3)
> (From update of attachment 220552 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=220552&action=review
> 
> > Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:467
> > +		 return ceilf(wordMeasurements[i].width);
> 
> Have you tested this with subpixel rendering both on and off?

I haven't tested. As far as I know subpixel layout is enabled only on ELF and
GTK. We don't have any tests which would fail due to this rounding (most of the
tests uses fixed font width, or shapes without the requirement of first line
adjusting, thus the new rounding won't change anything), otherwise it would
have failed already.

(In reply to comment #4)
> (From update of attachment 220552 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=220552&action=review
> 
> >
LayoutTests/fast/shapes/shape-inside/shape-inside-polygon-rounded-first-fit.htm
l:14
> > +<div class="shape-inside">The</div>
> 
> I am worried that this test will be flaky on different platforms with
different fonts. At the very least, I think you should specify a font size and
font face if you can't put this together with something like Ahem.

Unfortunately, we can't use Ahem, since it has fixed width, thus it won't
result a floating width. I specified the font in the test to make it clear.

I also added a short explanation about the problem to the changelog.