[webkit-reviews] review canceled: [Bug 89259] [CSS Exclusions] Enable shape-inside for simple rectangles : [Attachment 156593] Conditionally wrapping WrapShapeInfo

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled 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 156593: Conditionally wrapping WrapShapeInfo
https://bugs.webkit.org/attachment.cgi?id=156593&action=review

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


I haven't looked at the inline layout part in details.

> Source/WebCore/ChangeLog:71
> +	   (WebCore::RenderStyle::diff):

The function level entries should be filled in the final patch.

> Source/WebCore/rendering/RenderBlock.cpp:64
> +#if ENABLE(CSS_EXCLUSIONS)

As you are using runtime flags, you could drop the compile-time guard. It's a
pity as it would make the code more readable.

Unfortunately, the parsing is guarded in a similar fashion so we have to keep
the pattern.

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

This is potentially dangerous. As long as removeWrapShapeInfoForRenderBlock is
limited to removing the info in our map, it's fine. If it needs to call any
virtual method, it would be bad though.

> Source/WebCore/rendering/RenderBlock.cpp:329
> +	   WrapShapeInfo* wrapShapeInfo = this->wrapShapeInfo();
> +	   if (WrapShapeInfo::isWrapShapeInfoEnabledForBlock(this)) {

I still think this is an anti-pattern to return a wrapShapeInfo that disable. I
would just implement the filtering in wrapShapeInfo (I would do it in
RenderStyle) to avoid an extra step.

> Source/WebCore/rendering/RenderBlock.cpp:331
> +		   wrapShapeInfo->setNeedsUpdate(true);

Booleans should usually be avoided as it makes the call site less readable. For
example, can you tell me what repaint(true) means in WebKit without looking?

Here the boolean is unneeded as you only ever set it to true anyway.

> Source/WebCore/rendering/RenderBlock.cpp:336
> +    }

Maybe better to move that to an updateWrapeShapeInfoAfterStyleChange (not a
huge fan of update but at least it's consistent with some pattern we have).

> Source/WebCore/rendering/RenderBlock.cpp:1388
> +    setLogicalHeight(0);
> +    computeLogicalHeight();
> +    wrapShapeInfo->computeShapeSize(logicalWidth(), logicalHeight());
> +    setLogicalHeight(oldHeight);
> +    setLogicalTop(oldLogicalTop);

There is a pattern here where you do another computeLogicalHeight for each new
feature we support. It's annoying that we have to recompute the logical height
over and over. Couldn't we find a way to compute it once and for all *before*
we layout our children.

> Source/WebCore/rendering/RenderBlock.h:396
> +    bool hasShapeInside() const { return style() &&
style()->wrapShapeInside(); }

style() is never NULL except the first time styleWillChange is called. We don't
need to pay the price of the NULL-check for the common case. I would say the
need for this function is fairly small as it doesn't add much.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2188
> +    LineWidth width = LineWidth(m_block, lineInfo.isFirstLine(), segment);

Couldn't we let LineWidth constructor automatically handle this? That would
remove the need to the constructor's signature.

> Source/WebCore/rendering/WrapShapeInfo.cpp:37
> +#include "RuntimeEnabledFeatures.h"

Unneeded #include.

> Source/WebCore/rendering/WrapShapeInfo.cpp:60
> +WrapShapeInfo* WrapShapeInfo::createWrapShapeInfoForRenderBlock(RenderBlock*
block)

Nobody uses the returned value so it could be dropped. Not a huge fan of the
'create' in the method name but I didn't come up with something better.

> Source/WebCore/rendering/WrapShapeInfo.h:72
> +    void setNeedsUpdate(bool value) { m_needsUpdate = value; }

Generally the naming is poor. "update" means that the value is dirtied and
needs to be recomputed and it would be nicer to use a more precise naming. My
picks:
* dirtyCachedWrapShapeSize
* wrapShapeSizeNeedsRecomputing
* ...