Summary: | Layer flashing and poor perf during scrolling of message list on gmail.com and hotmail.com - overlap testing needs to constrained to clipping scopes | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||
Component: | New Bugs | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | koivisto, simon.fraser, webkit-bug-importer, zalan | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2019-05-21 15:50:06 PDT
Created attachment 370354 [details]
Patch
Comment on attachment 370354 [details] 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. |