[webkit-reviews] review granted: [Bug 35632] htmlediting.cpp : isEmptyTableCell() is incomplete : [Attachment 50872] patch - change isEmptyTableCell() and layout tests

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Roland Steiner
<rolandsteiner at chromium.org>'s request for review:
Bug 35632: htmlediting.cpp : isEmptyTableCell() is incomplete
https://bugs.webkit.org/show_bug.cgi?id=35632

Attachment 50872: patch - change isEmptyTableCell() and layout tests
https://bugs.webkit.org/attachment.cgi?id=50872&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
This test shouldn't be platform specific. r=me if you remove adding the test to
the skipped lists. Please consider doing the other comments below as well.

WebCore/editing/htmlediting.cpp:870
 +	//   .) the BR child of such a table cell
Side note: This seems wrong to me. Seems like it will lead to confusing calling
code. In either case, your patch isn't adding this. Might deserve a FIXME
though if you agree that this is string. I'd be happier if this were called
isEmptyTableCellOrInsideOf...or something like that. Not asking you to change
this for this patch though. Just a cleanup to consider for a followup patch.

LayoutTests/editing/deleting/delete-br-in-last-table-cell.html:41
 +  \ No newline at end of file
please add a newline.

LayoutTests/editing/deleting/delete-br-in-last-table-cell.html:35
 +	layoutTestController.dumpAsText();
dump_as_markup didn't exist when you wrote this patch, but it would be
appropriate for this test. tony can show you how to write one if you have
trouble (you just include the dump_as_markup.js file as a script element).