[webkit-reviews] review denied: [Bug 57148] Delete table in contentEditable/designMode produces odd contents : [Attachment 95025] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Annie Sullivan
<sullivan at chromium.org>'s request for review:
Bug 57148: Delete table in contentEditable/designMode produces odd contents
https://bugs.webkit.org/show_bug.cgi?id=57148

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95025&action=review

> LayoutTests/editing/deleting/delete-last-char-in-table-expected.txt:6
> +When deleteing the last character in a table deletes the table, no styled
spans should be left behind. To test manually, place cursor after "X" and do a
backward delete. No styled span should be created.
> +
> +
> +execDeleteCommand: <br>

Maybe it's clear when this test passes from this output.  Can you add a some
check?	e.g. by checking document.getElementsByTagName('span').length == 0.

> LayoutTests/editing/deleting/delete-last-char-in-table.html:5
> +<p>See bug 57148.</p>
> +<p>When deleteing the last character in a table deletes the table, no styled
spans should be left behind. To test manually, place cursor after "X" and do a
backward delete. No styled span should be created.</p>

I would combine these two paragraphs.

> Source/WebCore/editing/EditingStyle.cpp:-54
> -    CSSPropertyBorderCollapse,

We need a test for this.  r- due to the lack of test for border-collapse.

> Source/WebCore/editing/EditingStyle.h:62
> +    enum PropertiesToInclude { AllProperties,
OnlyEditingInheritableProperties,
InheritablePropertiesAndBackgroundColorInEffect };

You should also rename InheritablePropertiesAndBackgroundColorInEffect.