[webkit-reviews] review denied: [Bug 66142] [CSSRegions] RenderRegion should not reference a parent RenderFlowThread : [Attachment 104044] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 66142: [CSSRegions] RenderRegion should not reference a parent
RenderFlowThread
https://bugs.webkit.org/show_bug.cgi?id=66142

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=104044&action=review


> Source/WebCore/dom/NodeRenderingContext.cpp:373
> +    if (newRenderer->isRenderRegion())
> +	   toRenderRegion(newRenderer)->attachRegion();

Seems like this would be better placed in renderer methods than over here.
Maybe in RenderObjectChildList::append/insert/removeChildNode instead. Check
out updateListMarkerNumbers call sites in RenderObjectChildList to see where
you might attach and remove regions as they enter/leave the tree.

> Source/WebCore/rendering/RenderBlock.cpp:1535
> +    if (child->isRenderFlowThread())
> +	   return true;

You've turned this into a virtual function call hit by every child you check,
even non-positioned ones. This also only handles the insertion of positioned
objects from blocks with block-level children. Changing to:

if (child->isPositioned && !child->isRenderFlowThread())

would limit the virtual function call only to positioned children instead of
making it for all children.

However, you also didn't patch insertion from blocks with inline children that
happens in RenderBlockLineLayout.cpp near the top of layoutInlineChildren.

I think an easier way to handle this might be to just patch
insertPositionedObject to ignore RenderFlowThreads, and that way you only have
to patch one place.