[webkit-reviews] review granted: [Bug 52655] Hidden composited iframes cause infinite loop : [Attachment 111878] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 52655: Hidden composited iframes cause infinite loop
https://bugs.webkit.org/show_bug.cgi?id=52655

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=111878&action=review


> Source/WebCore/rendering/RenderLayerCompositor.cpp:251
> +    int nonRootCompositedLayerCount = m_compositedLayerCount;
> +    if (rootLayer->isComposited())
> +	   --nonRootCompositedLayerCount;
> +
> +    return nonRootCompositedLayerCount;

I would write this as:

    // We want to return true if only the root layer is composited.
    return m_compositedLayerCount == rootLayer->isComposited();

Maybe that’s not as clear to you, but it seems pretty clear to me.

>>> Source/WebCore/rendering/RenderLayerCompositor.h:155
>>> +	 void layerBecameNonComposited(const RenderLayer*)
>> 
>> why are you passing a layer here if you just keep a count? Is the idea to
expand this to keeping a Set<> sometimes?
> 
> No, I just thought it made sense for a delegate-type method. It might be
useful in future, but I had no specific plans for it.

In debug builds you should keep a HashSet.