[webkit-reviews] review granted: [Bug 95613] fast/table/empty-row-crash.html and fast/table/inline-form-assert.html should be dumpAsText tests : [Attachment 161886] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Pravin D
<pravind.2k4 at gmail.com>'s request for review:
Bug 95613: fast/table/empty-row-crash.html and
fast/table/inline-form-assert.html should be dumpAsText tests
https://bugs.webkit.org/show_bug.cgi?id=95613

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

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


> LayoutTests/ChangeLog:8
> +	    Modified the test cases empty-row-crash.html and
inline-form-assert.html to dumpAsText tests. 

It's always nice to explain the 'why' behind a change. Here it should outline
why we don't lose coverage by doing this conversion.

> LayoutTests/ChangeLog:9
> +	    Removed obsolete baselines.

You could put that below the removed baselines.

> LayoutTests/fast/table/empty-row-crash.html:15
> +In order to insert a row into a table atleast one column must exist.
Otherwise it will result in a crash.
> +As an empty table will have a minimum of one column, inserting a row into
such a table should not result in a crash.<br><br>

Please don't insert this description as I am not convinced it is a fine
assumption to make. Always having a column means that a table without any cells
takes some width due to border-spacing but it may not take any height (f.e.
<table><tbody></tbody></table>). Browsers are not consistent on that and CSS
2.1 is not super clear on what we should do so I would rather not document our
behavior as we may decide to change that.

Something without making those assumptions would do:

This test checks that inserting a row in an empty table doesn't crash.

> LayoutTests/fast/table/empty-row-crash.html:16
> +No crash means test PASS.

Shouldn't it be:

No crash means that the test PASSED ?

(I am not a native speaker but this sentence sounds better to me)

> LayoutTests/fast/table/inline-form-assert.html:14
> +    Test for <i><a
href="http://webkit.org/b/12373">http://webkit.org/b/12373</a>

Nit: This is an unneeded change as both URL are equivalent. I don't mind the
shorter version though.