ASSERTION FAILED: span >= 1 in WebCore::RenderTable::slowColElement

ASSERTION FAILED: span >= 1 in WebCore::RenderTable::slowColElement

Set the value of span to 1 if the given number is too big or zero.
Merged from WebKit (patch by Zsolt Borbely).
https://bugs.webkit.org/show_bug.cgi?id=129148

R=
BUG=351280
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169692

[kenneth.r.christiansen, 2014-03-11 13:59:15]

https://codereview.chromium.org/194593005/diff/1/Source/core/html/HTMLTableCo...
File Source/core/html/HTMLTableColElement.cpp (right):

https://codereview.chromium.org/194593005/diff/1/Source/core/html/HTMLTableCo...
Source/core/html/HTMLTableColElement.cpp:67: int newSpan = value.toInt();
I would add a comment here

// Use 1 if number is too big or zero.

[Julien - ping for review, 2014-03-13 18:29:23]

lgtm with improved testing

https://codereview.chromium.org/194593005/diff/20001/LayoutTests/fast/table/i...
File LayoutTests/fast/table/incorrect-colgroup-span-values.html (right):

https://codereview.chromium.org/194593005/diff/20001/LayoutTests/fast/table/i...
LayoutTests/fast/table/incorrect-colgroup-span-values.html:14: <p>This test
passes if it does not crash.</p>
Let's add a description to this test:

<p>This test checks that invalid 'span' values are rejected.</p>

https://codereview.chromium.org/194593005/diff/20001/LayoutTests/fast/table/i...
LayoutTests/fast/table/incorrect-colgroup-span-values.html:34: </table>
Missing from the testing:
-  colgroup span="cheese" (or whatever invalid value you fancy!)
- col span="..."
- Super important! Check that the returned value of getAttribute("span") is 1 as
expected (ie we rejected the value)

https://codereview.chromium.org/194593005/diff/20001/Source/core/html/HTMLTab...
File Source/core/html/HTMLTableColElement.cpp (right):

https://codereview.chromium.org/194593005/diff/20001/Source/core/html/HTMLTab...
Source/core/html/HTMLTableColElement.cpp:68: // Use 1 if number is too big or
zero.
This comment is ambiguous IMO: if |value| is not a number you would also get 0.

I would just put the quote from HTML5 (along with an explanation why it covers
also invalid integers):

// |span| must be a valid non-negative integer greater than zero.
// toInt() returns 0 if |value| is not a valid integer so we only check this
value.

[rtakacs, 2014-03-19 15:44:35]

On 2014/03/13 18:29:23, Julien Chaffraix - PST wrote:
> lgtm with improved testing
> 
>
https://codereview.chromium.org/194593005/diff/20001/LayoutTests/fast/table/i...
> File LayoutTests/fast/table/incorrect-colgroup-span-values.html (right):
> 
>
https://codereview.chromium.org/194593005/diff/20001/LayoutTests/fast/table/i...
> LayoutTests/fast/table/incorrect-colgroup-span-values.html:14: <p>This test
> passes if it does not crash.</p>
> Let's add a description to this test:
> 
> <p>This test checks that invalid 'span' values are rejected.</p>
> 
>
https://codereview.chromium.org/194593005/diff/20001/LayoutTests/fast/table/i...
> LayoutTests/fast/table/incorrect-colgroup-span-values.html:34: </table>
> Missing from the testing:
> -  colgroup span="cheese" (or whatever invalid value you fancy!)
> - col span="..."
> - Super important! Check that the returned value of getAttribute("span") is 1
as
> expected (ie we rejected the value)
> 
>
https://codereview.chromium.org/194593005/diff/20001/Source/core/html/HTMLTab...
> File Source/core/html/HTMLTableColElement.cpp (right):
> 
>
https://codereview.chromium.org/194593005/diff/20001/Source/core/html/HTMLTab...
> Source/core/html/HTMLTableColElement.cpp:68: // Use 1 if number is too big or
> zero.
> This comment is ambiguous IMO: if |value| is not a number you would also get
0.
> 
> I would just put the quote from HTML5 (along with an explanation why it covers
> also invalid integers):
> 
> // |span| must be a valid non-negative integer greater than zero.
> // toInt() returns 0 if |value| is not a valid integer so we only check this
> value.

All missing or ambigous things are fixed.

[Julien - ping for review, 2014-03-20 17:06:35]

lgtm (next time, no need to wait on me, jus find someone to cq the change!)

[Julien - ping for review, 2014-03-20 17:06:38]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-03-20 17:06:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtakacs.u-szeged@partner.samsung.com/1...

[commit-bot: I haz the power, 2014-03-20 17:09:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 17:09:18]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[Julien - ping for review, 2014-03-20 18:35:03]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-03-20 18:35:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtakacs.u-szeged@partner.samsung.com/1...

[commit-bot: I haz the power, 2014-03-20 19:52:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 19:52:09]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Julien - ping for review, 2014-03-20 20:04:10]

The CQ bit was checked by jchaffraix@chromium.org

[commit-bot: I haz the power, 2014-03-20 20:04:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtakacs.u-szeged@partner.samsung.com/1...

[commit-bot: I haz the power, 2014-03-20 21:05:55]

Change committed as 169692