[Webkit-unassigned] [Bug 73350] [chromium] Allow scrolling non-root layers in the compositor thread

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73350





--- Comment #34 from Sami Kyostila <skyostil at google.com>  2011-12-20 13:05:56 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=119898&action=review

Thanks for taking a look. The layer walking order should indeed be corrected since the user may get some unexpected results. The latest version of the patch fixes this.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:477
>> +CCInputHandlerClient::ScrollStatus CCLayerTreeHostImpl::beginScrollingInnermostLayerAtPoint(CCLayerImpl* layerImpl, const IntPoint& viewportPoint)
> 
> sorry to reverse myself earlier here but now that I think about this more I don't think this is right. just finding the first most-nested layer is not going to find what the user really wants, especially if the tree order doesn't match the z-order very well
> 
> instead of iterating through the layer tree in tree order, you really need to walk in z-order here by going through the CCRenderSurface list

Good catch. I'll rework this to walk the layers in reverse z-order so we scroll what the user actually sees.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:488
>> +    IntPoint contentPoint(layerImpl->screenSpaceTransform().inverse().mapPoint(viewportPoint));
> 
> if the layer's screenSpaceTransform is not invertible, inverse() will return an identity transform which will probably _not_ do what you expect here. if the screenSpaceTransform is not invertible we probably do not want to let it scroll, since that means that some dimension got mapped to zero and the layer isn't actually visible
> 
> also do you need to consider layer backfaces here?

I've added a check against non-invertible transforms. The latest revision of the patch is walking the list of render surface layers which does not include single sided backfacing layers. Double sided backfacing layers I think we should just scroll normally.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:626
>> +    bool didMove = m_scrollLayerImpl && (didScrollSubtree(m_scrollLayerImpl.get()) || m_pageScaleDelta != 1.0f);
> 
> webkit style nit: the comparison should be written as "m_pageScaleDelta != 1", without the ".0f" qualifier.

Done.

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:68
>> +#include "RenderLayer.h"
> 
> pretty sure you don't need this #include any more now either (it isn't wrong in terms of code layering, just unnecessary)

Done.

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:172
>> +    RefPtr<CCLayerImpl> child = CCLayerImpl::create(0);
> 
> nit: i think it's generally a bad idea to construct CCLayerImpls with overlapping IDs in tests since we maintain the invariant that we never reuse IDs in the normal code.

Looks like I confused the ID with LayerChromium's delegate pointer. I'll replace these with unique numbers.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.