[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 #6 from Sami Kyostila <skyostil at google.com>  2011-12-02 11:21:49 PST ---
(From update of attachment 117426)
View in context: https://bugs.webkit.org/attachment.cgi?id=117426&action=review

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:302
>> +    layer->setMaxScrollPosition(m_maxScrollPosition);
> 
> How does this interact with CCLayerTreeHostImpl::updateMaxScrollPosition calculating and overwriting the max scroll independently on the impl thread, taking page scale into account? This patch probably needs a test where page scale delta != 1.

Agreed, the coverage for page scaling isn't nearly as good as I'd like it to be, mainly because of the difficulty in testing that feature interactively. I considered adding some rudimentary UI for page scaling for this purpose, but did not get that far yet.

I think I'll need to do another pass with a focus on page scaling interactions. I'll keep your comment in mind while doing that.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:313
>> +    }
> 
> Maybe I'm misreading this, but if a layer has valid bounds but the point is not contained, then all of its children are skipped? The layer tree is not a bounding box hierarchy, so I think you still need to check the children.  Can you add a test for this case?

Good catch. I thought that layers were bounded by their parents but clearly that isn't the case. I've changed this to always recurse through the children and check the content point against layerImpl->visibleLayerRect() since that also takes scrolling into account. I'll make sure this is tested as well.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:350
>> +        // the scroll step.
> 
> Does this cause any hitch when flinging?

I doesn't, because the fling range is limited so that the flung element comes to rest exactly at the edge of its scroll area. The fling should not affect the parent element -- unless the element was already flush against the scroll area edge when the gesture began, in which case we fling one of its parents instead.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:440
>> +    if (m_pinchGestureActive || !collectScrollDeltas(scrollInfo.get(), m_rootLayerImpl.get())) {
> 
> Can you explain why you dropped the m_pageScaleDelta check here? I think that's needed so that pinch zooms are sent back even when there are no scrolls.

Well spotted, that was my mistake. I'll make sure there's a test for this as well.

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