[webkit-reviews] review denied: [Bug 129148] ASSERTION FAILED: span >= 1 : [Attachment 225258] Proposed patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has denied Zsolt Borbely
<borbezs at inf.u-szeged.hu>'s request for review:
Bug 129148: ASSERTION FAILED: span >= 1
https://bugs.webkit.org/show_bug.cgi?id=129148

Attachment 225258: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=225258&action=review

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225258&action=review


r- because the isNull() case is not tested despite ChangeLog mentioning such a
test.

> LayoutTests/ChangeLog:8
> +	   Added test demonstrates the behavior of colgroup in case of large
negative, large positive  and null span values.

The added test is not hitting the null value case.
"value" will be null in parseAttribute() if the attribute is being
programmatically removed from the element; e.g
element.removeAttribute("colgroup")

> Source/WebCore/html/HTMLTableColElement.cpp:68
> -	   m_span = !value.isNull() ? value.toInt() : 1;
> +	   m_span = (value.isNull() || !value.toInt()) ? 1 : value.toInt();

This code looks a bit messy. In the common case, you are doing string-to-int
conversion twice.
I'd write it like this:

int newSpan = value.toInt();
m_span = newSpan ? newSpan : 1;