Add a WebLayer:setScrollCompensationAdjustment between Blink and CC

Add a WebLayer::setScrollCompensationAdjustment between Blink and CC

Blink can only use the integer part of the scroll offset to position elements.
This CL makes Blink send the fractional part of the scroll offset to CC
separately to be clear that CC needs to take special care of it e.g. compensating
for fixed-position layer's position. Once Blink can fully position elements at
fractional boundary, we can get rid of this call.

BUG=414283
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189381

[Yufeng Shen (Slow to review), 2015-01-28 22:28:31]

miletus@chromium.org changed reviewers:
+ aelias@chromium.org, rbyers@chromium.org

[Yufeng Shen (Slow to review), 2015-01-28 22:28:50]

This is to be landed after 
https://codereview.chromium.org/877173002/

[aelias_OOO_until_Jul13, 2015-01-28 22:31:34]

https://codereview.chromium.org/889453002/diff/1/Source/core/page/scrolling/S...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/889453002/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollingCoordinator.cpp:405:
webLayer->setScrollPositionDouble(DoublePoint(flooredScrollPosition));
Let's send the full nonfloored position here.  The idea is that in CC we would
never need to reason about floored stuff in the normal scroll offset codepaths,
it can always assume scroll_offset_ contains the full offset from now on.

[Yufeng Shen (Slow to review), 2015-01-28 22:49:33]

https://codereview.chromium.org/889453002/diff/1/Source/core/page/scrolling/S...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/889453002/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollingCoordinator.cpp:405:
webLayer->setScrollPositionDouble(DoublePoint(flooredScrollPosition));
On 2015/01/28 22:31:34, aelias wrote:
> Let's send the full nonfloored position here.  The idea is that in CC we would
> never need to reason about floored stuff in the normal scroll offset
codepaths,
> it can always assume scroll_offset_ contains the full offset from now on.

Done.

[aelias_OOO_until_Jul13, 2015-01-28 22:54:42]

lgtm

[aelias_OOO_until_Jul13, 2015-01-28 22:54:42]

lgtm

[Rick Byers, 2015-01-29 20:53:13]

https://codereview.chromium.org/889453002/diff/20001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/889453002/diff/20001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:404:
webLayer->setScrollPositionDouble(scrollPosition);
You don't want flooredScrollPosition here?  If the API is to always pass the the
full double, then I guess I don't understand why CC doesn't just pull the
fractional part off itself rather than introducing a new API that's always
redundant?

https://codereview.chromium.org/889453002/diff/20001/public/platform/WebLayer.h
File public/platform/WebLayer.h (right):

https://codereview.chromium.org/889453002/diff/20001/public/platform/WebLayer...
public/platform/WebLayer.h:165: virtual void
setScrollPositionDouble(WebDoublePoint) = 0;
will we be changing this back to integer then?  If so, please add a FIXME to
make that clear.  If not, please add comments that describe the semantics of
"fractional part".  It's confusing now to think of fractions going across both
places.

[Rick Byers, 2015-01-29 20:55:07]

https://codereview.chromium.org/889453002/diff/1/Source/core/page/scrolling/S...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/889453002/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollingCoordinator.cpp:405:
webLayer->setScrollPositionDouble(DoublePoint(flooredScrollPosition));
On 2015/01/28 22:49:33, Yufeng Shen wrote:
> On 2015/01/28 22:31:34, aelias wrote:
> > Let's send the full nonfloored position here.  The idea is that in CC we
would
> > never need to reason about floored stuff in the normal scroll offset
> codepaths,
> > it can always assume scroll_offset_ contains the full offset from now on.
> 
> Done.

Just saw this - sorry.  Maybe I just need an concise API description in the
comments - clearly the semantics aren't as simple as "one API for the integer
portion, one API for the fractional portion".

[aelias_OOO_until_Jul13, 2015-01-29 21:25:36]

I guess the naming is poor.  In some sense these properties are orthogonal
(which is why they're separate), although the name makes it sound like they
aren't.  Maybe we could name this "scrollCompensationAdjustment" instead?

[Rick Byers, 2015-01-29 21:30:36]

On 2015/01/29 21:25:36, aelias wrote:
> I guess the naming is poor.  In some sense these properties are orthogonal
> (which is why they're separate), although the name makes it sound like they
> aren't.  Maybe we could name this "scrollCompensationAdjustment" instead?

That definitely sounds better to me.  Combine that with a decent comment
describing what it means and I'm happy.

[Yufeng Shen (Slow to review), 2015-01-29 22:34:38]

On 2015/01/29 21:30:36, Rick Byers wrote:
> On 2015/01/29 21:25:36, aelias wrote:
> > I guess the naming is poor.  In some sense these properties are orthogonal
> > (which is why they're separate), although the name makes it sound like they
> > aren't.  Maybe we could name this "scrollCompensationAdjustment" instead?
> 
> That definitely sounds better to me.  Combine that with a decent comment
> describing what it means and I'm happy.

done.

[Rick Byers, 2015-01-30 04:25:06]

Thanks LGTM

How will this code path eventually get tested?  Do we need a unit test in blink,
or can we reasonably test the blink+cc parts together once everything has
landed?

[aelias_OOO_until_Jul13, 2015-01-30 04:33:53]

Layout reftest is probably the best way to test this.  Scroll to a fractional
amount with a fixed-pos element, and compare that to a page with an absolutely
positioned element that should be in the same place.

[Yufeng Shen (Slow to review), 2015-01-30 17:59:45]

The CQ bit was checked by miletus@chromium.org

[Yufeng Shen (Slow to review), 2015-01-30 17:59:58]

The CQ bit was unchecked by miletus@chromium.org

[Yufeng Shen (Slow to review), 2015-01-30 19:47:19]

reftest added.

[aelias_OOO_until_Jul13, 2015-01-30 20:17:38]

lgtm

[Yufeng Shen (Slow to review), 2015-02-03 03:10:44]

The CQ bit was checked by miletus@chromium.org

[commit-bot: I haz the power, 2015-02-03 03:10:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/889453002/80001

[commit-bot: I haz the power, 2015-02-03 03:14:17]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189381