[webkit-reviews] review granted: [Bug 106075] [CSS Regions][Mac] fast/regions/full-screen-video-from-region.html hits an assertion in RenderFlowThread::removeRenderBoxRegionInfo : [Attachment 185979] Patch with Zoltan's feedback

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Andrei Bucur <abucur at adobe.com>'s
request for review:
Bug 106075: [CSS Regions][Mac] fast/regions/full-screen-video-from-region.html
hits an assertion in RenderFlowThread::removeRenderBoxRegionInfo
https://bugs.webkit.org/show_bug.cgi?id=106075

Attachment 185979: Patch with Zoltan's feedback
https://bugs.webkit.org/attachment.cgi?id=185979&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=185979&action=review


> Source/WebCore/ChangeLog:11
> +	   When the region chain is invalidate the information is lost so we
need to return true, even for the

Grammar nit: "When the region chain is invalidated, the information is lost so
..."

> Source/WebCore/ChangeLog:18
> +	   The second problem is RenderMedia not inheriting for RenderBlock.
The logic of child relayout doesn't apply

Nit: for -> from

> Source/WebCore/rendering/RenderMedia.cpp:76
> +	       controlsRenderer->setNeedsLayout(true, MarkOnlyThis);

Rather than calling setNeedsLayout, I would probably do something like,
bool controlsNeedLayout = controlsRenderer->needsLayout();
if (inRenderFlowThread()) {
    ...
	controlsNeedLayout = true;
}

if (newSize == oldSize && !controlsNeedLayout)
    return;