[webkit-reviews] review granted: [Bug 113255] [CSS Exclusions] The radius of a circle should be computed based on the shorter available dimension : [Attachment 194961] proposed patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 113255: [CSS Exclusions] The radius of a circle should be computed based on
the shorter available dimension
https://bugs.webkit.org/show_bug.cgi?id=113255

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=194961&action=review


>
LayoutTests/fast/exclusions/shape-inside/shape-inside-on-nested-container-with-
unresolved-height.html:6
> +    font: 14px Ahem, sans-serif;

Not really needed as it's a ref test and I would expect the font resolution to
give similar results on both tests. It also makes the output unreadable by
maintainers.

(and btw using 14px as the font size makes little sense, it's a lot easier to
use a round number like 10px or 15px so that it's easier to know where the line
layout algorithm breaks).

>
LayoutTests/fast/exclusions/shape-inside/shape-inside-on-nested-container-with-
unresolved-height.html:9
> +    height: 300px;

This is unneeded and should be removed.

>
LayoutTests/fast/exclusions/shape-inside/shape-inside-on-nested-container-with-
unresolved-height.html:28
> +<p>Requires Ahem font. Since the nested container's height is not
resolvable, we shouldn't apply the circle shape-inside on the content.</p>

No need for Ahem.

>
LayoutTests/fast/exclusions/shape-inside/shape-inside-on-nested-container-with-
unresolved-height.html:29
> +<p>Bug <a href="http://webkit.org/b/113255 ">113255</a>: The radius of a
circle should be computed based on the shorter available dimensions</p>

Please add a description (ie what is expected in the output) before landing.