RESOLVED FIXED 81546
[chromium] Use floating point scroll deltas for layers 
https://bugs.webkit.org/show_bug.cgi?id=81546
Summary [chromium] Use floating point scroll deltas for layers
Sami Kyostila
Reported 2012-03-19 13:11:12 PDT
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.
Attachments
Patch (10.92 KB, patch)
2012-03-19 13:22 PDT, Sami Kyostila 		no flags
DetailsFormatted DiffDiff
Patch (11.63 KB, patch)
2012-03-20 05:16 PDT, Sami Kyostila 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Sami Kyostila
Comment 1 2012-03-19 13:22:14 PDT
Created attachment 132645 [details] Patch
James Robinson
Comment 2 2012-03-19 13:44:33 PDT
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 3 2012-03-20 05:01:18 PDT
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
Comment 4 2012-03-20 05:16:08 PDT
Created attachment 132805 [details] Patch
James Robinson
Comment 5 2012-03-21 09:00:29 PDT
Comment on attachment 132805 [details] Patch r=me
WebKit Review Bot
Comment 6 2012-03-21 09:52:54 PDT
Comment on attachment 132805 [details] Patch Clearing flags on attachment: 132805 Committed r111555: <http://trac.webkit.org/changeset/111555>
WebKit Review Bot
Comment 7 2012-03-21 09:52:59 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.