[webkit-reviews] review denied: [Bug 45561] NULL deref when SVG elements have table styles : [Attachment 92133] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Rob Buis <rwlbuis at gmail.com>'s
request for review:
Bug 45561: NULL deref when SVG elements have table styles
https://bugs.webkit.org/show_bug.cgi?id=45561

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92133&action=review

This doesn't seem like the correct fix. Consider the following test case:

data:text/html,<div style="display:table-caption"><svg
style="display:inherit"><text display="inherit">This test PASSED if we don't
crash.</text></svg><div>

We also need some test cases that use the CSS value "inherit".

> LayoutTests/svg/custom/display-table-caption-expected.txt:2
> +PASS
> +PASS

I suggest we add some more English to this such that a person can understand
what these PASSes mean; that is, neither sub test caused a crash.

> LayoutTests/svg/custom/display-table-caption.svg:6
> +  <text display="table-caption" y="10">PASS</text>

Can we put some text/comment in this test case to explain that this test PASSED
if it didn't crash? We need some English sentences in this test to explain its
purpose and how to interpret the results.

> LayoutTests/svg/custom/display-table-caption.svg:7
> +  <foreignObject display="table-caption"
y="10"><xhtml:div>PASS</xhtml:div></foreignObject>

I'm not very familiar with SVG and this looks like another test case for the
display property. If so, I suggest separating this into its own layout test
file since it seems more straightforward to diagnose a regression/crash from a
minimal test case as opposed to a test case that has many sub tests.