[webkit-reviews] review denied: [Bug 136087] [CSS Shapes] Negative raster shape height leads to crash : [Attachment 236842] Patch

[bugzilla-daemon at webkit.org]

Bem Jones-Bey <bjonesbe at adobe.com> has denied Zoltan Horvath
<zoltan at webkit.org>'s request for review:
Bug 136087: [CSS Shapes] Negative raster shape height leads to crash
https://bugs.webkit.org/show_bug.cgi?id=136087

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

------- Additional Comments from Bem Jones-Bey <bjonesbe at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236842&action=review


Given the complexity of the test case, I'm not convinced this is the proper
fix.

> Source/WebCore/ChangeLog:9
> +	   The original test was provided by the u-szeged fuzzer. I reduced the
test
> +	   case and handled the negative margin-IntRect case in the shapes
code.

Can you give a better description of what this is fixing?

>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-raster-shape-negativ
e-height.html:3
> +* {

This test seems way too complex, especially since you're matching everything
here. Are you sure you couldn't get something simpler? If you can't reduce it
to simply a single float with shape-outside and a negative height margin box, I
wonder if there is something more complex here than just having the height be
negative. Is there a condition before it even gets to the shapes code that is
failing?

>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-raster-shape-negativ
e-height.html:17
> +<button>a</button>

Does this have to be a button for this to fail, or would a <span> work? (Or do
you even need it at all?)

>
LayoutTests/fast/shapes/shape-outside-floats/shape-outside-raster-shape-negativ
e-height.html:20
> +<p style="position: absolute; top: 100px;">

I wouldn't muck around with absolute positioning. Just have this paragraph in
here, and dumpAsText() the entire test. You're not gaining anything from it
being a ref test except complexity.