Bug 235211 – Don't call invalidateRectsForAllMarkers() for every layer in the updateLayerPositions() traversal

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Don't call invalidateRectsForAllMarkers() for every layer in the updateLayerPositions() traversal

[Simon Fraser (smfr)]

Created attachment 449126 [details]
Patch

[Sam Weinig]

Comment on attachment 449126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449126&action=review

> Source/WebCore/rendering/RenderLayer.cpp:942
> +    renderer().document().markers().invalidateRectsForAllMarkers();

Why doesn't this one use willUpdateLayerPositions()?

[alan]

Comment on attachment 449126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449126&action=review

> Source/WebCore/rendering/RenderLayer.cpp:937
> +    willUpdateLayerPositions();
> +    recursiveUpdateLayerPositions(nullptr, flagsForUpdateLayerPositions(*this));

I don't think I could come up with better names, but these 2 lines will surely confuse me in the future.

[Simon Fraser (smfr)]

Created attachment 449130 [details]
Patch

[EWS]

Committed r288008 (246034@main): <https://commits.webkit.org/246034@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449130 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87587203>

[Antti Koivisto]

Comment on attachment 449126 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449126&action=review

> Source/WebCore/rendering/RenderLayer.cpp:1063
> +        child->recursiveUpdateLayerPositions(geometryMap, flags);

could this be something like updateLayerPositionsForSubtree?

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #7)
> Comment on attachment 449126 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449126&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1063
> > +        child->recursiveUpdateLayerPositions(geometryMap, flags);
> 
> could this be something like updateLayerPositionsForSubtree?

It could, but that doesn't make it obvious that it's call recursively.

[Antti Koivisto]

> It could, but that doesn't make it obvious that it's call recursively.

Why does that matter? If it was refactored to do the same thing iteratively would the callers care?

[Simon Fraser (smfr)]

If it were iterative, I would rename it of course, and the distinction between the "called-once" and the "called-one-per-layer" function wouldn't be necessary.