[webkit-reviews] review denied: [Bug 26342] Absolutely positioned HTML element within foreignObject of absolutely positioned SVG crashes Safari : [Attachment 39535] patch that fixes the crash problem with SVG foreignobject with absolute positioning

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Charles Wei
<charles.wei at torchmobile.com.cn>'s request for review:
Bug 26342: Absolutely positioned HTML element within foreignObject of
absolutely positioned SVG crashes Safari
https://bugs.webkit.org/show_bug.cgi?id=26342

Attachment 39535: patch that fixes the crash problem with SVG foreignobject
with absolute positioning
https://bugs.webkit.org/attachment.cgi?id=39535&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for tackling this!

> +	   This is to fix the crash reported with
> +	   https://bugs.webkit.org/show_bug.cgi?id=26342
> +
> +	   Test case at : http://www.barhams.info/webkit/crash.xml

In the WebKit project we require regression tests for any bug fix. So this fix
needs to include a test for the LayoutTests directory that demonstrates the
crash and that it is fixed.

>      if (!o || !o->isRenderBlock())
> -	   return 0; // Probably doesn't happen any more, but leave just in
case. -dwh
> +	   return document()->renderView(); // return the render view of the
document, which is the top level containner

This comment doesn't indicate why it is correct and acceptable to return
container.

Typo here: "containner"

WebKit style uses sentence style comments, with capital letters and periods.

> -    ASSERT(useTransforms); // mapping a point through SVG w/o respecting
trasnforms is useless.
> +//	 ASSERT(useTransforms); // mapping a point through SVG w/o respecting
trasnforms is useless.

We don't include commented-out code in WebKit. If the assertion is wrong it
should be removed, not commented out. Further, you don't explain why you are
removing this assertion.

> -    ASSERT(useTransforms); // mapping a point through SVG w/o respecting
trasnforms is useless.
> +//	 ASSERT(useTransforms); // mapping a point through SVG w/o respecting
trasnforms is useless.

Ditto.