RenderGeometryMap currently gathers mappings by climbing the rendering tree. This is slow and can produce large number of mapping steps. In the common case we already have the child layer coordinates available in the layer tree and we can just use that.
<rdar://problem/10559009>
Created attachment 149194 [details] patch
Attachment 149194 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/RenderLayer.h:483: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 149194 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=149194&action=review > Source/WebCore/rendering/RenderGeometryMap.cpp:182 > + if (ancestorLayer && layer->canUseConvertToLayerCoords() && renderer->style()->position() != FixedPosition) { Doesn't this need to check canUseConvertToLayerCoords() on all layers between layer and ancestorLayer? There's no guarantee that ancestorLayer is an immediate ancestor. > Source/WebCore/rendering/RenderGeometryMap.h:52 > + // Called by code walking the layer tree. > + void pushMappingsToAncestor(const RenderLayer*, const RenderLayer* ancestorLayer); > + void popMappingsToAncestor(const RenderLayer*); I'd like to keep versions that are renderer-based, so we can call them from absoluteRects()/absoluteQuads() at some point (we already know of cases where these are a bottleneck).
Also, I think we should try to wean ourselves off of convertToLayerCoords in the long term, since it would be better to have one way to do coordinate math that always "just works", rather than checking whether it's OK to call it.
(In reply to comment #4) > Doesn't this need to check canUseConvertToLayerCoords() on all layers between layer and ancestorLayer? There's no guarantee that ancestorLayer is an immediate ancestor. Sounds likely. Can you think of a way to test that case? The patch is logic pretty well tested due to ASSERT(enclosingIntRect(rendererMappedResult) == enclosingIntRect(FloatQuad(result).boundingBox())); in RenderGeometryMap::absoluteRect() and we don't hit the case in layout tests. > I'd like to keep versions that are renderer-based, so we can call them from absoluteRects()/absoluteQuads() at some point (we already know of cases where these are a bottleneck). We don't usually leave around dead code. It is trivial to add back later. Are you planning to use it soon?
(In reply to comment #5) > Also, I think we should try to wean ourselves off of convertToLayerCoords in the long term, since it would be better to have one way to do coordinate math that always "just works", rather than checking whether it's OK to call it. Sounds very sensible, this is not nice at all. The new scheme will need some sort of caching though, just walking up the rendering tree is pretty slow.
Created attachment 149206 [details] updated patch - check that all layers between the current and the ancestor have canUseConvertToLayerCoords(). - put the renderer version of pushMappingsToAncestor back for future use
Attachment 149206 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/rendering/RenderLayer.h:483: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 149206 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=149206&action=review > Source/WebCore/rendering/RenderGeometryMap.cpp:190 > + canConvertInLayerTree = current->canUseConvertToLayerCoords(); Shouldn't this be &= ?
(In reply to comment #10) > Shouldn't this be &= ? No need, we bail out from the loop if the variable turns false.
http://trac.webkit.org/changeset/121124
http://trac.webkit.org/changeset/121130 fixes an assert with fast/block/inline-children-root-linebox-crash.html
This change caused RenderGeometryMap::pushView() to no longer get called. This might have broken compositing overlap testing inside of position:fixed, I'm not sure.
push(renderer, toLayoutSize(layerOffset), /*accumulatingTransform*/ true, /*isNonUniform*/ false, /*isFixedPosition*/ false, /*hasTransform*/ false); accumulatingTransform should also be false; that refers to preserve-3d.
...and I think we need to check for position:fixed on intermediate layers.
The assert in absoluteRect should verify that the results are identical to the old code across all our tests. I wonder if those cases don't occur for some reason? Or is the test coverage just weak?
We'd hae to have a test that has compositing layers inside position:fixed, with a scrolled page. We might not. Or maybe the breakage is benign so far (but I'm making some changes that hit it).
The top level view will use pushView() due to the ancestorLayer test. I think the problems would be with scrolled subframes. Not sure if canUseConvertToLayerCoords test catches them.
pushView was never hit when using the convertToLayerCoords path.
(In reply to comment #20) > pushView was never hit when using the convertToLayerCoords path. Correct. The idea is to not take convertToLayerCoords path for the RenderViews.
Yeah. Push the RenderView alone, then push layer offsets from it.
The patch in bug 90342 has some relevant RenderGeometryMap changes (assertions and fixes), but not a fix for the pushView issue.
This caused bug 90919.