[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


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #118602|review?                     |review-
               Flag|                            |




--- Comment #10 from James Robinson <jamesr at chromium.org>  2011-12-09 14:00:11 PST ---
(From update of attachment 118602)
View in context: https://bugs.webkit.org/attachment.cgi?id=118602&action=review

I haven't looked through everything in detail, but have found some issues.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:170
>      void scrollBy(const IntSize& scroll);
> +    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?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:245
> +    // Tracks if this layer or a descendant was scrolled since the last sync.
> +    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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:262
> +    FloatSize m_scrollDeltaResidue;

what is the 'scroll delta residue'?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:485
> +    for (size_t i = 0; i < info.scrolls.size(); ++i) {
> +        if (m_rootScrollLayer && info.scrolls[i].layerId == m_rootScrollLayer->id())
> +            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

> 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?

> Source/WebKit/chromium/public/platform/WebLayerTreeViewClient.h:48
> +    // Applies a scroll delta to a non-root layer. This is triggered by events
> +    // sent to the compositor thread through the WebCompositor interface.
> +    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.

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