[webkit-reviews] review granted: [Bug 198091] Layer flashing and poor perf during scrolling of message list on gmail.com and hotmail.com - overlap testing needs to constrained to clipping scopes : [Attachment 370354] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 198091: Layer flashing and poor perf during scrolling of message list on
gmail.com and hotmail.com - overlap testing needs to constrained to clipping
scopes
https://bugs.webkit.org/show_bug.cgi?id=198091

Attachment 370354: Patch

https://bugs.webkit.org/attachment.cgi?id=370354&action=review




--- Comment #3 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 370354
  --> https://bugs.webkit.org/attachment.cgi?id=370354
Patch

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

> Source/WebCore/rendering/LayerOverlapMap.cpp:180
> +    auto* clippingScope =
findClippingScopeForLayers(enclosingClippingLayers);
> +    if (!clippingScope)
> +	   return false;

Does this happen and what does it mean if it does? Could we assert here
instead?

> Source/WebCore/rendering/LayerOverlapMap.cpp:246
> +OverlapMapContainer::ClippingScope*
OverlapMapContainer::findClippingScopeForLayers(const
Vector<LayerOverlapMap::LayerAndBounds>& enclosingClippingLayers) const
> +{
> +    ASSERT(enclosingClippingLayers.size());
> +    ASSERT(enclosingClippingLayers[0].layer.isRenderViewLayer());
> +
> +    const auto* currScope = &m_rootScope;
> +    for (unsigned i = 1; i < enclosingClippingLayers.size(); ++i) {
> +	   auto& scopeLayerAndBounds = enclosingClippingLayers[i];
> +	   auto* childScope =
currScope->childWithLayer(scopeLayerAndBounds.layer);
> +	   if (!childScope)
> +	       return nullptr;
> +
> +	   currScope = childScope;
> +    }
> +
> +    return const_cast<ClippingScope*>(currScope);

As discussed, maybe this can be replaced with a side hashmap? That might make
the code easier to follow and avoid passing the full enclosingClippingLayers
stack.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1811
> +    for (const auto* currLayer = layer.parent(); currLayer; currLayer =
currLayer->parent()) {

Not a fan of the 'currFoo' naming.