[webkit-reviews] review denied: [Bug 93891] ASSERTION FAILED: !currBox->needsLayout() loading bing maps : [Attachment 164323] Patch

Sun Sep 16 21:35:08 PDT 2012

Abhishek Arya <inferno at chromium.org> has denied Zalan Bujtas
<zbujtas at gmail.com>'s request for review:
Bug 93891: ASSERTION FAILED: !currBox->needsLayout() loading bing maps
https://bugs.webkit.org/show_bug.cgi?id=93891

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

------- Additional Comments from Abhishek Arya <inferno at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=164323&action=review

> Source/WebCore/ChangeLog:13
> +	   RenderBlock::layoutPositionedObjects()'s logic relies on 'parent
first'

You need to add a line explaining why it is ''parent first' ordering and how
stuff is marked in RenderBlock::layoutInlineChildren.

> Source/WebCore/rendering/RenderBlock.cpp:3636
> +    bool didInsertDescendant = false;

Move this line after the comment below.

> Source/WebCore/rendering/RenderBlock.cpp:3639
> +    if (descendant->isRenderBlock()) {

I dont think you need to add this virtual call. We want to keep the
parent-child order in all cases.

> Source/WebCore/rendering/RenderBlock.cpp:3644
> +		   didInsertDescendant = descendantSet->insertBefore(box,
descendant).isNewEntry;

didInsertDescendant will always be true since you bail out on
'descendantSet->contains(descendant)' above. So, no need of the isNewEntry
check and assert below.

> LayoutTests/ChangeLog:3
> +	   ASSERTION FAILED: !currBox->needsLayout() loading bing maps

Please move this assertion failure to the description and keep the same title
as webcore/changelog here.

>
LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash
.html:1
> +<html>

Please declare a !DOCTYPE

>
LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash
.html:3
> +    <div id="toabsolute" style="position: fixed;">

instead of toabsolute, please use a good identifier like 'container'

>
LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash
.html:14
> +	    var m = document.getElementById("foo");

there is no correlation b/w m and foo, please use a better identifier.

>
LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash
.html:15
> +	    var c = document.getElementById("toabsolute");

instead of 'c', just use 'container'

>
LayoutTests/fast/dynamic/position-fixed-to-absolute-with-positioned-child-crash
.html:20
> +	    c.offsetHeight;

This layout call is not needed, it should be automatic.