[chromium] Use floating point scroll deltas for layers

[Sami Kyostila]

The Chromium compositor should use floating point scroll deltas to keep track of layer scroll offsets relative the main thread. This is because due to page scaling it may be necessary to scroll layers in sub-CSS-pixel steps to avoid visible jumps. When the floating point scroll offset is committed to the main thread, it is truncated to integer, but the fractional part is kept on the CC side to make sure fractional scroll offsets are accumulated correctly over multiple commits.

[Sami Kyostila]

Created attachment 132645 [details]
Patch

[James Robinson]

Comment on attachment 132645 [details]
Patch

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

Looks good, just one quibble about size/point distinction

> Source/WebCore/platform/graphics/FloatPoint.h:244
> +inline FloatSize toSize(const FloatPoint& a)

i don't think you need this - looks like flooredIntSize() will give you what you want

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:156
> +    IntSize scrollTotal = toSize(flooredIntPoint(m_scrollLayerImpl->scrollPosition() + m_scrollLayerImpl->scrollDelta()));

flooredIntSize() ?

Alternately, should scrollTotal be an IntPoint ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:735
> +    scroll.scrollDelta = toSize(flooredIntPoint(m_scrollLayerImpl->scrollDelta()));

There's also a flooredIntSize(), should we just be using that instead?

I think that scrollDelta should be a IntSize, since it's representing an offset and not an absolute coordinate

[Sami Kyostila]

Comment on attachment 132645 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:156
>> +    IntSize scrollTotal = toSize(flooredIntPoint(m_scrollLayerImpl->scrollPosition() + m_scrollLayerImpl->scrollDelta()));
> 
> flooredIntSize() ?
> 
> Alternately, should scrollTotal be an IntPoint ?

Thanks, I managed to miss flooredIntSize(). It was missing the obvious FloatSize -> IntSize overload so I also added that.

I think it would make more sense for scrollTotal to be an IntPoint, but that makes interfacing with CCPageScaleAnimation a bit awkward since the latter uses IntSizes for its coordinates. I'm not very familiar with that code so I won't go and change it now.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:735
>> +    scroll.scrollDelta = toSize(flooredIntPoint(m_scrollLayerImpl->scrollDelta()));
> 
> There's also a flooredIntSize(), should we just be using that instead?
> 
> I think that scrollDelta should be a IntSize, since it's representing an offset and not an absolute coordinate

Ditto with flooredIntSize().

scroll.scrollDelta is already an IntSize, and with flooredIntSize() there's no intermediate IntPoint any longer so this should be fixed now.

[Sami Kyostila]

Created attachment 132805 [details]
Patch

[James Robinson]

Comment on attachment 132805 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 132805 [details]
Patch

Clearing flags on attachment: 132805

Committed r111555: <http://trac.webkit.org/changeset/111555>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.