[webkit-reviews] review denied: [Bug 100398] [CSS Exclusions] shape-outside on floats for rectangle shapes height/width : [Attachment 178027] Updated Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Bem Jones-Bey
<bjonesbe at adobe.com>'s request for review:
Bug 100398: [CSS Exclusions] shape-outside on floats for rectangle shapes
height/width
https://bugs.webkit.org/show_bug.cgi?id=100398

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

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


Thanks for investigating the check-layout.js possibility, I am fine with
ref-tests but have some questions / comments about the tests.

I am r- on the confusing behavior where the shape-outside is fully contained
into the float's content box, yet we allow another float to intrude into the
content box.

> Source/WebCore/ChangeLog:29
> +	   (WebCore): Associates the ExclusionShape object for shape outside
with a RenderBox. Analagous to

The (WebCore): entries should be removed. Our tools are confused unfortunately.


>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-re
ctangle-expected.html:8
> +	<style>
> +		.test {
> +			font: 10px/1 Ahem, sans-serif;
> +			border: 1px solid black;
> +		}

The indentation is wrong, it looks like you have some tabs in this file (which
means that patch will be rejected by the svn commit-hook).

>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-re
ctangle-horizontal-multiple.html:77
> +    <h2>These tests show that floats respect the shape outside on other
floats. Each test should have a red square centered in a larger rectangle.</h2>


Please don't use red as a way of meaning 'success'. Red should be reserved for
failure (see
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree and
more importantly http://wiki.csswg.org/test/format)

>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-re
ctangle-horizontal-multiple.html:80
> +	   <div class="float-right-exclusion"></div>
> +	   <div class="float-right"></div>

For float-right to be centered, it should intrude 10px into
float-right-exclusion. As discussed, this is confusing and I don't really see
why this can be right.

>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-re
ctangle-percentage.html:35
> +	   height: 66.66666666%;
> +	   width: 66.66666666%;

That's a pretty bad choice for a percent as it will be impacted by any rounding
errors. Using 50px and 50% is better (or whichever configuration you prefer as
long as it's a round number).