[webkit-reviews] review granted: [Bug 88888] RenderLayer subtrees without any self-painting layer shouldn't be walked during painting : [Attachment 147327] Change v4. Fixed the invalidation logic as it wasn't dirtying.

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 88888: RenderLayer subtrees without any self-painting layer shouldn't be
walked during painting
https://bugs.webkit.org/show_bug.cgi?id=88888

Attachment 147327: Change v4. Fixed the invalidation logic as it wasn't
dirtying.
https://bugs.webkit.org/attachment.cgi?id=147327&action=review

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


r+ but if you feel like doing another patch with the suggested improvements,
I'd like to see it.

> Source/WebCore/rendering/RenderLayer.cpp:450
> +void RenderLayer::dirtyHasSelfPaintingLayerDescendantStatus()
> +{
> +    for (RenderLayer* layer = this; layer; layer = layer->parent()) {
> +	   layer->m_hasSelfPaintingLayerDescendantDirty = true;

Would be nice to change dirtyVisibleDescendantStatus() and this to use the same
loop style.

Although dirtyVisibleDescendantStatus() doesn't, it would be nice of this
method had 'ancestor' in the name.

> Source/WebCore/rendering/RenderLayer.cpp:460
> +void RenderLayer::markAncestorHasSelfPaintingLayerDescendant()

The term 'mark' implies (to me) that its' a "mark for layout" type affair,
which is setting a dirty flag that gets cleared when something happens. This is
instead actually computing the final, known value of the flag, so 'set' or
'update' might be better terms.

> Source/WebCore/rendering/RenderLayer.cpp:462
> +    for (RenderLayer* layer = this; layer &&
(layer->m_hasSelfPaintingLayerDescendantDirty ||
!layer->hasSelfPaintingLayerDescendant()); layer = layer->parent()) {

I think it would be easier to read if the
(layer->m_hasSelfPaintingLayerDescendantDirty ||
!layer->hasSelfPaintingLayerDescendant() tests was done in the loop contents,
with a 'break'.

> Source/WebCore/rendering/RenderLayer.cpp:682
> +void RenderLayer::updateDescendantOrContentRelatedStatus()

Not a huge fan of this name. Maybe just updateDescendantDependentFlags or
something

> Source/WebCore/rendering/RenderLayer.cpp:728
> +	   for (RenderLayer* child = firstChild(); child; child =
child->nextSibling()) {
> +	       child->updateDescendantOrContentRelatedStatus();

It's a shame you add another tree walk here. Could you do one tree walk for
both flags, going as far as either flag needs?