[webkit-reviews] review granted: [Bug 99861] RenderTableCol::computePreferredLogicalWidths and RenderTableCol::layout should never be called : [Attachment 170678] Updated change: removed ASSERT_NOT_REACHED() from layout() as simplified normal flow layout hit it.

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 99861: RenderTableCol::computePreferredLogicalWidths and
RenderTableCol::layout should never be called
https://bugs.webkit.org/show_bug.cgi?id=99861

Attachment 170678: Updated change: removed ASSERT_NOT_REACHED() from layout()
as simplified normal flow layout hit it.
https://bugs.webkit.org/attachment.cgi?id=170678&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=170678&action=review


Would be nice if you could add a targetted test for the simplified layout case
as well. I realize the inspector tests hit that code accidentally, but it'd be
nice to cover it explicitly. Not a big deal.

> LayoutTests/fast/table/col-span-change-relayout.html:41
> +	       {
> +		   var col = document.getElementById('column');
> +		   var oldCellWidth =
getComputedStyle(document.getElementById('table')).width;
> +		   col.setAttribute("span", "1");
> +		   var newCellWidth =
getComputedStyle(document.getElementById('table')).width;
> +
> +		   log("Table width was " + oldCellWidth);
> +		   log("Table width is " + newCellWidth);
> +		   if (oldCellWidth != newCellWidth)
> +		       log("PASSED: Table changed width");
> +		   else
> +		       log("FAILED: Table did not change width");
> +	       }

FWIW, you could make this a check-layout.js change.

function changeColSpan()
{
    document.getElementById('table').setAttribute('data-expected-width', 300);
    checkLayout('table');
    document.getElementById('table').setAttribute('data-expected-width', 200);
    checkLayout('table');
}

I find that easier to read, also, I prefer for it to say the exact width's
expected. These shouldn't be platform dependent in anyway, so we should be able
to make this test stricter.