[webkit-reviews] review canceled: [Bug 100428] Teach RenderTable how to use Vector::insert and Vector::append instead of its own custom memmove code : [Attachment 170752] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Eric Seidel
<eric at webkit.org>'s request for review:
Bug 100428: Teach RenderTable how to use Vector::insert and Vector::append
instead of its own custom memmove code
https://bugs.webkit.org/show_bug.cgi?id=100428

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

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


The direction is fine, not r+'ing due to the lost underflow ASSERT.

> Source/WebCore/rendering/RenderTable.cpp:-702
> -    ASSERT(oldSpan > firstSpan);

I am not happy about losing this ASSERT. You are doing unsigned arithmetic and
this would catch underflow in a way the new logic doesn't. Your new ASSERT was
covered by the old one but is only a subset of what we were ASSERTing.

> Source/WebCore/rendering/RenderTable.cpp:701
> +    ASSERT(m_columns[position + 1].span > 0); // Every column must span at
least 1!

The comment doesn't add much here.

> Source/WebCore/rendering/RenderTable.h:132
> +	   explicit ColumnStruct(unsigned initialSpan = 1)

I don't see the interest of the default initialSpan as all call sites pass
something.