[webkit-reviews] review denied: [Bug 109591] RenderLayer::hasOutOfFlowPositionedDescendant should not be affected by invisible children : [Attachment 187880] Patch

Tue Feb 12 09:14:24 PST 2013

Simon Fraser (smfr) <simon.fraser at apple.com> has denied vollick at chromium.org's
request for review:
Bug 109591: RenderLayer::hasOutOfFlowPositionedDescendant should not be
affected by invisible children
https://bugs.webkit.org/show_bug.cgi?id=109591

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=187880&action=review

> Source/WebCore/ChangeLog:10
> +	   Currently, we do not check visibility when setting the
> +	   m_hasOutOfFlowPositionedDescendant flag, but we should. With this
flag
> +	   we do. We also dirty the flag when visibility changes.

I'm a little concerned that we're spending time maintaining the
m_hasOutOfFlowPositionedDescendant when most of the time it's unused.

> Source/WebCore/rendering/RenderLayer.cpp:1010
> +	       bool childIsOutOfFlowPositioned = child->renderer() &&
child->renderer()->isOutOfFlowPositioned() && child->isVisuallyNonEmpty();

isVisuallyNonEmpty() is a bit expensive to call here. I think you should just
check the visibility flags (hasVisibleContent/hasVisibleDescendant).

>
LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html
:2
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
> +   "http://www.w3.org/TR/html4/loose.dtd">

Just <!DOCTYPE html> please

>
LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html
:4
> +<html lang="en">

No need for lang.

>
LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html
:6
> +  <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>

No need for this.

>
LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html
:64
> +	   var layerTree = window.internals.layerTreeAsText(document);

Doesn't layerTreeAsText() update layout already?