[webkit-reviews] review denied: [Bug 100398] [CSS Exclusions] shape-outside on floats for rectangle shapes height/width : [Attachment 174489] 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 174489: Updated Patch
https://bugs.webkit.org/attachment.cgi?id=174489&action=review

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


> Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:52
> +    : m_box(box),
> +    m_logicalWidth(0), 
> +    m_logicalHeight(0)

Style violation, the comma should start a line, not end it.

> Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:67
> +    ExclusionShapeOutsideInfoMap& infoMap = exclusionShapeOutsideInfoMap();
> +    if (ExclusionShapeOutsideInfo* shapeInfo = infoMap.get(box))
> +	   return shapeInfo;
> +
> +    ExclusionShapeOutsideInfoMap::AddResult result = infoMap.add(box,
create(box));
> +    return result.iterator->value.get();

Note that this also involves 2 hash lookup if you don't have a cache miss.
There is a pattern to do only one but it would be an overkill.

> Source/WebCore/rendering/ExclusionShapeOutsideInfo.cpp:78
> +    // FIXME enable shape outside for non-rectangular shapes! (bug 98664)

The usual style is:

// FIXME: Enable shape outside for non-rectangular shapes! (bug 98664)

Note the ':' after FIXME.

> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:45
> +    WTF_MAKE_FAST_ALLOCATED;

I think it should be WTF_MAKE_NONCOPYABLE too.

> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:82
> +	   if (m_logicalWidth != logicalWidth || m_logicalHeight !=
logicalHeight) {

Early returns are preferred.

> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:83
> +	       dirtyShapeSize();

The pattern of basically free'ing and reallocating the computed shape seems
really bad here. I would rather if we had a way of just recomputing the cached
shape sizes.

> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:89
> +    void dirtyShapeSize() { m_computedShape.clear(); }

What does the dirtying and recomputing gives us?

> Source/WebCore/rendering/ExclusionShapeOutsideInfo.h:92
> +    ExclusionShapeOutsideInfo(RenderBox*);

explicit ExclusionShapeOutsideInfo(RenderBox*);?

> Source/WebCore/rendering/RenderBlock.cpp:3801
> +    LayoutUnit logicalWidth = logicalWidthForChild(o) +
marginStartForChild(o) + marginEndForChild(o);
> +#if ENABLE(CSS_EXCLUSIONS)
> +    ExclusionShapeOutsideInfo* shapeOutside =
o->exclusionShapeOutsideInfo();
> +    if (shapeOutside) {

If you have a shapeOutside, you basically compute the margins for nothing. You
could change this code to avoid that:

if (shapeOutside) {
    ....
    setLogicalWidthForFloat(newObj, shapeOutside->shapeLogicalWidth());
} else
#endif
   setLogicalWidthForFloat(newObj, logicalWidthForChild(o) +
marginStartForChild(o) + marginEndForChild(o));

> Source/WebCore/rendering/RenderBlock.cpp:3983
> +	   ExclusionShapeOutsideInfo* shapeOutside =
floatingObject->renderer()->exclusionShapeOutsideInfo();
> +	   hasShapeOutside = shapeOutside;

hasShapeOutside = floatingObject->renderer()->exclusionShapeOutsideInfo();

> LayoutTests/ChangeLog:9
> +	   Tests for changing the height and width of a float with a
rectangular
> +	   shape outside.

I don't understand what this means or what this adds to the whole.

>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is
-ignored-expected.html:26
> +    <p>

I would dump the bug number & title.

>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-margin-is
-ignored-expected.html:27
> +	   Requites Ahem font. Tests that the shape on a right float causes the
margin on the float to have no effect on layout.

Unneeded comment about Ahem: you are putting a 'font' declaration mentioning
it, that should be sufficient.

>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-re
ctangle-horizontal-larger.html:28
> +	   -webkit-shape-outside: rectangle(0px, -100px, 200px, 300px);

I am concerned about this: it means that x / y / width / height are physical
coordinates which seems very stupid and goes completely counter to the logical
coordinates / directions as specified in css 3 writing modes.

>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-simple-re
ctangle-horizontal-larger.html:58
> +	   X X X X X X X X X X X X X X X

I have tried understand this test but it's completely weird to me: using 31
characters in this test and allowing wrapping at every other characters (while
naming this pseudo dotted line a line above). I don't know how to make it
better as I can't see what it's trying to achieve: for that matter, it's never
*stated* what you are testing.