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

Tue Feb 12 21:06:17 PST 2013

vollick at chromium.org has asked	for review:
Bug 109591: RenderLayer::hasOutOfFlowPositionedDescendant should not be
affected by invisible children
https://bugs.webkit.org/show_bug.cgi?id=109591

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

------- Additional Comments from vollick at chromium.org
(In reply to comment #2)
> (From update of attachment 187880 [details])
> 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.
I've updated the code so we do much less work in updateDescendantDependentFlags
if the opt-in feature is disabled. This seemed cleaner than doing the check at
every spot where we muck with the dirty bit for this property. Is this better?
>
> > 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).
Done.
>
> >
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
Fixed.
>
> >
LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html
:4
> > +<html lang="en">
>
> No need for lang.
Removed.
>
> >
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.
Removed.
>
> >
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?
Yep. Removed.