[webkit-reviews] review canceled: [Bug 89259] [CSS Exclusions] Enable shape-inside for simple rectangles : [Attachment 157809] updating patch

[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 157809: updating patch
https://bugs.webkit.org/attachment.cgi?id=157809&action=review

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


> Source/WebCore/rendering/RenderBlock.cpp:1376
> +    if (wrapShape != oldWrapShape) {

Early returns are preferred to nesting.

> Source/WebCore/rendering/RenderBlock.cpp:1382
> +	       WrapShapeInfo* wrapShapeInfo = this->wrapShapeInfo();
> +	       if (wrapShapeInfo)
> +		   wrapShapeInfo->wrapShapeSizeNeedsRecomputing();
> +	       else
> +		   WrapShapeInfo::createWrapShapeInfoForRenderBlock(this);

I would rather see the following as it would simplify the code path:
WrapShapeInfo* wrapShapeInfo = ensureWrapShapeInfo();
wrapShapeInfo->wrapShapeSizeNeedsRecomputing();

> Source/WebCore/rendering/RenderBlock.cpp:1485
> +    // FIXME: 93547 resolve logical height for percentage based vertical
lengths

// FIXME: Bug 93547: Resolve logical height for percentage based vertical
lengths

(not repeated for all comment missing the word 'bug')

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:165
> +	   computeAvailableWidthFromLeftAndRight();
> +	   return;

Those 2 lines are unneeded as you would do exactly the same operations below.
It's also better to avoid splitting our current code path.

> Source/WebCore/rendering/WrapShapeInfo.cpp:42
> +WrapShapeInfoMap* WrapShapeInfo::s_wrapShapeInfoMap = 0;

I completely agree with your proposal to move this to use DEFINE_STATIC_LOCAL.
You wouldn't have to expose the internal getter and that would remove all the
initialization / destruction / NULL checks below.

> Source/WebCore/rendering/WrapShapeInfo.cpp:61
> +    if (!isWrapShapeInfoEnabledForBlock(block)) {

Note that this doesn't work properly if you transition from a supported to an
unsupported shape. This is why I was hinting at doing this check in
RenderBlock::wrapShapeInfo and make all style changes use this method. This
would ensure that we never use an unsupported shape.

Even if this code is temporary, it's important that it's somewhat bullet-proof.


> Source/WebCore/rendering/WrapShapeInfo.cpp:62
> +	   removeWrapShapeInfoForRenderBlock(block);

This is unneeded AFAICT as you will never insert the shape if it's disabled.

> Source/WebCore/rendering/WrapShapeInfo.cpp:70
> +    WrapShapeInfo* wrapShapeInfo = new WrapShapeInfo(block);
> +    s_wrapShapeInfoMap->set(block, adoptPtr(wrapShapeInfo));

We usually have a pattern for creating an OwnPtr:

static PassOwnPtr<WrapShapeInfo> create(RenderBlock* block)
{
    return adoptPtr(new WrapShapeInfo(block));
}

This ensures that you never forget to call adoptPtr.

> Source/WebCore/rendering/WrapShapeInfo.cpp:97
> +    s_wrapShapeInfoMap->take(block);

remove is better as you don't need the return value, take is more if you want
to do something with the value before it's destroyed.

> Source/WebCore/rendering/WrapShapeInfo.h:48
> +    LayoutUnit left;
> +    LayoutUnit right;

I would renamed those 2 to be logicalLeft and logicalRight to underline that
they are not physical but logical coordinates.

> Source/WebCore/rendering/WrapShapeInfo.h:78
> +    static bool isWrapShapeInfoEnabledForBlock(const RenderBlock*);
> +    static WrapShapeInfoMap* s_wrapShapeInfoMap;

Anything that doesn't need to be known outside the implementation file doesn't
need to be exposed here. Those 2, for example, could have a static binding
inside WrapShapeInfo.cpp so that you never expose them.

> Source/WebCore/rendering/WrapShapeInfo.h:93
> +    // OwnPtr needs access to our destructor
> +    friend void WTF::deleteOwnedPtr<WrapShapeInfo>(WrapShapeInfo*);

Better to just expose the destructor then.