[webkit-reviews] review denied: [Bug 89259] [CSS Exclusions] Enable shape-inside for simple rectangles : [Attachment 155369] Fix windows project

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Bear Travis
<betravis at adobe.com>'s request for review:
Bug 89259: [CSS Exclusions] Enable shape-inside for simple rectangles
https://bugs.webkit.org/show_bug.cgi?id=89259

Attachment 155369: Fix windows project
https://bugs.webkit.org/attachment.cgi?id=155369&action=review

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


First high level comment, the spec is pretty rough and should be updated before
we implement it. shape-inside seems to be on the safe side so this patch is OK.


> Source/WebCore/rendering/RenderBlock.cpp:211
> +    WrapShapeInfo::removeWrapShapeInfoForRenderBlock(this);

It's annoying that we need a static function. Those tends to be bad and cause
lots of pain (see for example counters).

> Source/WebCore/rendering/RenderObject.h:1044
> +	   ADD_BOOLEAN_BITFIELD(hasShapeInside, HasShapeInside);

The bar for adding a new boolean to RenderObject is high and I don't think this
meets the requirements. Your logic could be completely based on
RenderStyle::shapeInside() with some modification to the architecture.

> Source/WebCore/rendering/WrapShapeInfo.cpp:75
> +static inline bool needsWrapShapeInfo(RenderBlock* block)

This function is badly named. It doesn't underline what you are trying to
achieve (which is basically filtering the cases you can handle). Here is a
better list of names:
* isWrapShapeInfoDisabledForBlock()
* isWrapShapeInfoEnabledForBlock()

As a whole I think this should be done in RenderStyle::wrapShapeInside(). This
would simplify the code and remove a lot of the complexity.

> Source/WebCore/rendering/WrapShapeInfo.cpp:78
> +    if (!RuntimeEnabledFeatures::cssExclusionsEnabled())
> +	   return false;

I don't think this is right: normally you *should* disable CSS parsing to avoid
breaking feature detection in CSS. This means that this code cannot be executed
if exclusion is disabled.

> Source/WebCore/rendering/WrapShapeInfo.cpp:89
> +void WrapShapeInfo::updateWrapShapeInfoShapeForRenderBlock(RenderBlock*
block)

I am really not supportive of this naming.
updateWrapShapeInfoShapeForRenderBlock doesn't really say anything about what
it does and the function is doing too much stuff. It would be better if we had
a create and destroy function and that's it. If you need an update then it
should only dirty your function.

> Source/WebCore/rendering/WrapShapeInfo.cpp:102
> +    ASSERT(!s_wrapShapeInfoMap || !block->hasShapeInside());

I don't understand this ASSERT. This is the only place where you reset the
|hasShapeInside| flag which means you will trigger this ASSERT.

> Source/WebCore/rendering/WrapShapeInfo.cpp:149
> +    default:

Better to avoid |default|, it defeats the compile time check that we handle all
types.

> Source/WebCore/rendering/WrapShapeInfo.cpp:150
> +	   ASSERT_NOT_REACHED();

AFAICT we can reach other shape here as the parsing is implemented. It's better
to use notImplemented() in this case.

> Source/WebCore/rendering/WrapShapeInfo.h:48
> +typedef HashMap<const RenderBlock*, WrapShapeInfo*> WrapShapeInfoMap;

Better to use HashMap<const RenderBlock*, OwnPtr<WrapShareInfo> > here.

> Source/WebCore/rendering/WrapShapeInfo.h:61
> +    float shapeTop() { return m_shapeTop; }

You are not coherent here in terms of units. Most of the layout code uses
LayoutUnit, yet you return a float here. As you are not touching line layout,
AFAICT this should be only LayoutUnit.

> Source/WebCore/rendering/WrapShapeInfo.h:62
> +    float shapeBottom() { return m_shapeTop + m_shapeHeight; }

Unused method.

> Source/WebCore/rendering/WrapShapeInfo.h:64
> +    bool hasSegments();
> +    SegmentList& segments() { ASSERT(hasSegments()); return m_segments; }

Those getters should be |const|.

> Source/WebCore/rendering/WrapShapeInfo.h:77
> +    LayoutUnit m_shapeLeft, m_shapeTop, m_shapeWidth, m_shapeHeight;

Please one member per line.