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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:170
>> +    void scrollBy(const FloatSize& scroll);
> 
> it's very rarely a good idea to override a function like this with such similar types when the implementations do fairly different things. what's the story?
> 
> do we actually intend to support scrolling by non-integer amounts?

Based on offline discussion I'll change the layer scroll coordinates to be floating point instead.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:245
>> +    bool m_subtreeWasScrolled;
> 
> if i understand correctly, this is an optimization to avoid having to walk the full layer tree every commit to check for scrolls on sublayers, correct? would this be better done as a bit on the LTHI? this seems somewhat overdone for a small win

Yes, that's the idea. The problem with having the bit in the LTHI is that CCLayerImpl doesn't have a reference back to it (perhaps rightly so), so scrollBy() cannot go an flip that bit.

Perhaps this is a premature optimization, so I'll drop it for now and scan the whole layer tree instead.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:262
>> +    FloatSize m_scrollDeltaResidue;
> 
> what is the 'scroll delta residue'?

It is the fractional part of the scroll offset, but with floating point coordinates I can get rid of it.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:485
>> +            m_client->applyRootLayerScrollAndScale(info.scrolls[i].scrollDelta, info.pageScaleDelta);
> 
> this logic makes me wonder if trying to be generic about scroll on the root layer is a mistake and if we should instead keep the root scroll + page scale delta separate from scrolls on sublayers

I see what you mean. For the root layer the scroll offset and the page scale delta are tightly coupled, whereas the sublayers only care about scroll offset. I'll try to separate the two concepts.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:335
>> +CCInputHandlerClient::ScrollStatus CCLayerTreeHostImpl::beginScrollingInnermostLayerAtPoint(CCLayerImpl* layerImpl, const IntPoint& windowPoint)
> 
> what does "window" mean in "window point"? we don't have a defined notion of a window in the compositor today. viewport, perhaps? is this scaled or unscaled?

It refers to the target space of CCLAyerImpl::screenSpaceTransform(). View coordinate sounds good to me. The page scale does not affect view coordinates, but you have to apply the inverse of the page scale when going from view to content coordinates.

>> Source/WebKit/chromium/public/platform/WebLayerTreeViewClient.h:48
>> +    virtual void applyLayerScroll(int layerId, const WebSize& scrollDelta) = 0;
> 
> I don't think this makes sense on this interface - why would you talk about a sublayer through the WebLayerTreeViewClient instead of via that layer? How would you expect this API to be used?
> 
> We also do not (and should not) expose layer IDs via the public API.

Ah, I guess I picked the wrong path to feed back the scroll offsets to the main thread. I'll rework this use the individual layers instead.

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