Bug 88888

Created attachment 147117 [details] Proposed change v1.

Comment on attachment 147117 [details] Proposed change v1. View in context: https://bugs.webkit.org/attachment.cgi?id=147117&action=review > Source/WebCore/rendering/RenderLayer.cpp:446 > +void RenderLayer::setHasSelfPaintingLayerDescendant() This method name is confusing. It should be markAncestorsHaveSelfPaintingDescendant() or something. > Source/WebCore/rendering/RenderLayer.h:878 > + // If have no self-painting descendants, we don't have to walk our children during painting. > + // This can lead to significant savings, especially if the tree has lots of non-self-painting layers grouped together > + // (e.g. table cells). The current logic is simple and conservative which will lead to over-marking subtrees (FIXME). It would be nice to toggle this back to false when possible. Isn't there an existing tree walk you can piggy-back on to update this flag?

Comment on attachment 147117 [details] Proposed change v1. View in context: https://bugs.webkit.org/attachment.cgi?id=147117&action=review > Source/WebCore/rendering/RenderLayer.cpp:132 > + , m_hasSelfPaintingLayerDescendant(false) It would be better to give this a name that makes it clear it’s not a precise flag. Perhaps m_canAssumeNoSelfPaintingLayerDescendants would be a way to make a more precise flag? I’d only want this to have the simple name if it was kept accurate.

Comment on attachment 147117 [details] Proposed change v1. View in context: https://bugs.webkit.org/attachment.cgi?id=147117&action=review >> Source/WebCore/rendering/RenderLayer.h:878 >> + // (e.g. table cells). The current logic is simple and conservative which will lead to over-marking subtrees (FIXME). > > It would be nice to toggle this back to false when possible. Isn't there an existing tree walk you can piggy-back on to update this flag? We could potentially piggy-back on the visibility dirty & update mechanism. We would need to rename the whole logic to accommodate non-visibility related flags. I had something like that in mind as a follow-up patch due to 2 reasons: * any flaw in this logic would mean that we don't paint part of the page. * this means updateVisibilityStatus could do one more round of walking our children (not sure how hot this function is) I will just go ahead and do the proper invalidation and recomputing then.

Created attachment 147162 [details] Change v2: did markAncestorHasSelfPaintingLayerDescendant rename, added the proper invalidation / recomputation logic.

Comment on attachment 147162 [details] Change v2: did markAncestorHasSelfPaintingLayerDescendant rename, added the proper invalidation / recomputation logic. Attachment 147162 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12946628

Created attachment 147175 [details] Change v3: Same as previously but should compile on Mac.

Created attachment 147327 [details] Change v4. Fixed the invalidation logic as it wasn't dirtying.

Comment on attachment 147327 [details] Change v4. Fixed the invalidation logic as it wasn't dirtying. I believe the patch is good enough for a review now. The flaw in the logic made us not dirty anything (thus keeping m_hasSelfPaintingLayerDescendant set) and the test hit an ASSERT that I fixed so we should have some coverage.

Comment on attachment 147327 [details] Change v4. Fixed the invalidation logic as it wasn't dirtying. 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?

Comment on attachment 147327 [details] Change v4. Fixed the invalidation logic as it wasn't dirtying. 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. Another round of review, it will be. >> Source/WebCore/rendering/RenderLayer.cpp:450 >> + 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. dirtyVisibleDescendantStatus is using recursion which I would rather avoid as it's a very simple ancestor walking loop here. I would rather push aligning those 2 functions in another bug: the reason being that I would rather remove recursion from dirtyVisibleDescendantStatus() than aligning the other way around. This would also enable some name tweaking (like adding 'ancestor' to its 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. setAncestorHasSelfPaintingLayerDescendant() is better IMHO as 'update' would be the counterpart for 'mark' for me. >> 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'. Makes sense. >> Source/WebCore/rendering/RenderLayer.cpp:682 >> +void RenderLayer::updateDescendantOrContentRelatedStatus() > > Not a huge fan of this name. Maybe just updateDescendantDependentFlags or something Should work, I wanted to emphasize the fact that some flags are related to the render tree ("content") and not the layer tree ("descendants"). It's probably too long and will be shortened. >> Source/WebCore/rendering/RenderLayer.cpp:728 >> + 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? Sure.

Created attachment 147679 [details] Change v5. More bug fixes and reworking per Simon's review.

Comment on attachment 147679 [details] Change v5. More bug fixes and reworking per Simon's review. Nice.

Comment on attachment 147679 [details] Change v5. More bug fixes and reworking per Simon's review. Clearing flags on attachment: 147679 Committed r120395: <http://trac.webkit.org/changeset/120395>

All reviewed patches have been landed. Closing bug.