Bug 68559 – Split RenderLayer::convertToLayerCoords for clarity

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Julien Chaffraix]

The current code is doing several tasks at the same time: finding the appropriate ancestor, computing the appropriate offset if we have gone through our |ancestorLayer| or recurse.

The way it is written does not underline those steps and has some duplication that are unneeded. Some it are even confusing: like the fixed positioned case falling back to the absolutely positioned case.

[Julien Chaffraix]

Created attachment 108213 [details]
Proposed refactoring: Split the parent lookup out of convertToLayerCoords and simplify the code path.

[Simon Fraser (smfr)]

Comment on attachment 108213 [details]
Proposed refactoring: Split the parent lookup out of convertToLayerCoords and simplify the code path.

I don't really see the point of this refactor is we're not going to use those new methods elsewhere. The original code is pretty clear already.

[Julien Chaffraix]

(In reply to comment #2)
> (From update of attachment 108213 [details])
> I don't really see the point of this refactor is we're not going to use those new methods elsewhere.

I wonder if some could be reused in updateLayerPosition for example. My goal was not to reuse the extracted code - I wouldn't mind, just did not try.

Nice additions I see:
- remove a duplication of code: the logic for correcting the location if we passed through the ancestor layer was duplicated twice for no apparent reason.
- remove the weird fallback of going through the absolute path if we did not find our ancestor layer in the fixed path (walking thus twice the layer tree in this case).
- promote "finding our parent" as a real step which will make easier to implement checks for convertToLayerCoords abuse (see for example bug 34505) if we want to.

[WebKit Review Bot]

Comment on attachment 108213 [details]
Proposed refactoring: Split the parent lookup out of convertToLayerCoords and simplify the code path.

Attachment 108213 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9764952

New failing tests:
fast/reflections/abs-position-in-reflection.html

[WebKit Review Bot]

Comment on attachment 108213 [details]
Proposed refactoring: Split the parent lookup out of convertToLayerCoords and simplify the code path.

Attachment 108213 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9773875

[Julien Chaffraix]

Created attachment 108377 [details]
Proposed refactoring 2: Fixed an issue in the first patch.

[Julien Chaffraix]

Created attachment 109714 [details]
Simplified the code even more and increased the code sharing.

[Simon Fraser (smfr)]

Comment on attachment 109714 [details]
Simplified the code even more and increased the code sharing.

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

> Source/WebCore/ChangeLog:21
> +        Note: Some code added in r53742 that handled FixedPosition was removed as the
> +        test now passes without it.

Did you test this on Mac with compositing enabled? I'd be surprised if this code wasn't still required. Maybe we need more tests.

> Source/WebCore/rendering/RenderLayer.cpp:827
> +RenderLayer* RenderLayer::enclosingPositionedAncestor(const RenderLayer* boundAncestorLayer, bool& traversedBoundLayer) const

I don't know what 'bound' means in this context. It's really 'possiblyIntermediateLayer'.

Since boundAncestorLayer is an optional param, traversedBoundLayer should be too.

> Source/WebCore/rendering/RenderLayer.cpp:-1225
> -    if (position == FixedPosition) {
> -        // For a fixed layers, we need to walk up to the root to see if there's a fixed position container
> -        // (e.g. a transformed layer). It's an error to call convertToLayerCoords() across a layer with a transform,
> -        // so we should always find the ancestor at or before we find the fixed position container.
> -        RenderLayer* fixedPositionContainerLayer = 0;
> -        bool foundAncestor = false;
> -        for (RenderLayer* currLayer = parent(); currLayer; currLayer = currLayer->parent()) {
> -            if (currLayer == ancestorLayer)
> -                foundAncestor = true;
> -
> -            if (isFixedPositionedContainer(currLayer)) {
> -                fixedPositionContainerLayer = currLayer;
> -                ASSERT_UNUSED(foundAncestor, foundAncestor);
> -                break;
> -            }
> -        }
> -        
> -        ASSERT(fixedPositionContainerLayer); // We should have hit the RenderView's layer at least.
> -
> -        if (fixedPositionContainerLayer != ancestorLayer) {
> -            LayoutPoint fixedContainerCoords;
> -            convertToLayerCoords(fixedPositionContainerLayer, fixedContainerCoords);
> -
> -            LayoutPoint ancestorCoords;
> -            ancestorLayer->convertToLayerCoords(fixedPositionContainerLayer, ancestorCoords);
> -
> -            location += (fixedContainerCoords - ancestorCoords);
> -            return;
> -        }
> -    }
> -    
> -    RenderLayer* parentLayer;
> -    if (position == AbsolutePosition || position == FixedPosition) {
> -        // Do what enclosingPositionedAncestor() does, but check for ancestorLayer along the way.

I'm concerned about the code being removed. Also there's a subtlety here you may have missed. You can fall through the if (position == FixedPosition) clause into the if (position == AbsolutePosition || position == FixedPosition) clause. Does your new code handle that?

> Source/WebCore/rendering/RenderLayer.cpp:1223
> +void RenderLayer::convertToLayerCoords(const RenderLayer* coordinateOriginLayer, LayoutRect& rect) const

coordinateOriginLayer is a bit long-winded. Maybe targetLayer ?

[Julien Chaffraix]

Comment on attachment 109714 [details]
Simplified the code even more and increased the code sharing.

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

>> Source/WebCore/ChangeLog:21
>> +        test now passes without it.
> 
> Did you test this on Mac with compositing enabled? I'd be surprised if this code wasn't still required. Maybe we need more tests.

I tested on snow-leopard the test added as part of the change without problem. IIRC accelerated composition is enabled by default on Mac so it was covered.

>> Source/WebCore/rendering/RenderLayer.cpp:827
>> +RenderLayer* RenderLayer::enclosingPositionedAncestor(const RenderLayer* boundAncestorLayer, bool& traversedBoundLayer) const
> 
> I don't know what 'bound' means in this context. It's really 'possiblyIntermediateLayer'.
> 
> Since boundAncestorLayer is an optional param, traversedBoundLayer should be too.

Fine with both changes.

>> Source/WebCore/rendering/RenderLayer.cpp:-1225
>> -        // Do what enclosingPositionedAncestor() does, but check for ancestorLayer along the way.
> 
> I'm concerned about the code being removed. Also there's a subtlety here you may have missed. You can fall through the if (position == FixedPosition) clause into the if (position == AbsolutePosition || position == FixedPosition) clause. Does your new code handle that?

I mentioned this code in the ChangeLog as removing it means a subtle change in behavior that I wanted underlined. The new code does what was going on before the if (position == FixedPosition) branch was added and just use the fallback directly.

>> Source/WebCore/rendering/RenderLayer.cpp:1223
>> +void RenderLayer::convertToLayerCoords(const RenderLayer* coordinateOriginLayer, LayoutRect& rect) const
> 
> coordinateOriginLayer is a bit long-winded. Maybe targetLayer ?

Fine enough.

[Julien Chaffraix]

(In reply to comment #9)
> (From update of attachment 109714 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109714&action=review
> 
> >> Source/WebCore/ChangeLog:21
> >> +        test now passes without it.
> > 
> > Did you test this on Mac with compositing enabled? I'd be surprised if this code wasn't still required. Maybe we need more tests.
> 
> I tested on snow-leopard the test added as part of the change without problem. IIRC accelerated composition is enabled by default on Mac so it was covered.

For the record, it looks like the original test case does not cover the original bug (reverting the change and the test still passes on my machine). Also this was related to me.com which is not accepting any more people so I don't have a way to reproduce the bug and increase our coverage.

[Simon Fraser (smfr)]

(In reply to comment #10)
> For the record, it looks like the original test case does not cover the original bug (reverting the change and the test still passes on my machine). Also this was related to me.com which is not accepting any more people so I don't have a way to reproduce the bug and increase our coverage.

Did you just revert the change on TOT, or roll your whole tree back? You may have to do the latter.

[Julien Chaffraix]

(In reply to comment #11)
> (In reply to comment #10)
> > For the record, it looks like the original test case does not cover the original bug (reverting the change and the test still passes on my machine). Also this was related to me.com which is not accepting any more people so I don't have a way to reproduce the bug and increase our coverage.
> 
> Did you just revert the change on TOT, or roll your whole tree back? You may have to do the latter.

I definitely did the former. I think I also did the latter but let me double-check.

[Julien Chaffraix]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > For the record, it looks like the original test case does not cover the original bug (reverting the change and the test still passes on my machine). Also this was related to me.com which is not accepting any more people so I don't have a way to reproduce the bug and increase our coverage.
> > 
> > Did you just revert the change on TOT, or roll your whole tree back? You may have to do the latter.
> 
> I definitely did the former. I think I also did the latter but let me double-check.

Checked after rolling the tree and the test does not trigger in my environment. I have removed some Safari's internal settings I had set to prevent any interaction.

[Julien Chaffraix]

It looks like we are at a tie. Simon expressed some concerns about the change and I have no way to make sure that I can address them. Unless I hear some voice for the change, I will just drop it next week.

[Simon Fraser (smfr)]

If this is just refactoring, I don't think the risk is worth it.

[Julien Chaffraix]

(In reply to comment #15)
> If this is just refactoring, I don't think the risk is worth it.

It is.