Don't update graphics layer positions during coordinated scrolling

Don't update graphics layer positions during coordinated scrolling

As the scroll offset for the Layer already entirely captures all of the
information for the scroll, don't update positions if the scroll offset
is going to be set.

This makes any scrollable layer be in one of two states:
(1) It is managed by the ScrollingCoordinator and can be scrolled on the
compositor thread.  The ScrollingCoordinator updates the scroll position
on the platform layer directly.
(2) It is not managed by the ScrollingCoordinator and can not be
scrolled on the compositor thread.  It just has the position on the
given GraphicsLayer moved around.

This is step 2 of a 3 step patch to disentangle Layer positions and
scroll offsets.  The #ifdef in this patch is turned on here:
https://codereview.chromium.org/18400003

R=jamesr@chromium.org
BUG=256381
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153910

[jamesr, 2013-07-03 22:11:38]

Can you help me understand this sentence:

> This makes any scrollable layer be in one of two states: either it is
scrollable by the compositor thread and so its scroll offset is adjusted
or it is just scrolled in Blink and its position is changed directly.

?  A scrollable layer can have its scroll position manipulated by both the main
thread and compositor thread, for instance if the user is touch-scrolling it
while javascript is setting scrollTop.  Those have to be combined somewhere.  Is
this state tied to a layer or a point in time (i.e. commit) or both?

[enne (OOO), 2013-07-03 22:18:29]

On 2013/07/03 22:11:38, jamesr wrote:
> Can you help me understand this sentence:
> 
> > This makes any scrollable layer be in one of two states: either it is
> scrollable by the compositor thread and so its scroll offset is adjusted
> or it is just scrolled in Blink and its position is changed directly.
> 
> ?  A scrollable layer can have its scroll position manipulated by both the
main
> thread and compositor thread, for instance if the user is touch-scrolling it
> while javascript is setting scrollTop.  Those have to be combined somewhere. 
Is
> this state tied to a layer or a point in time (i.e. commit) or both?

Yeah, I was trying to say that a layer that is only scrolled in Blink could just
have its position adjusted and a layer that was scrolled via the scrolling
coordinator would adjust its scroll offset.  In the latter case the scroll
offset would just be changed at two different times.

In retrospect, maintaining that distinction probably doesn't add anything. 
Maybe it'd be clearer to just not adjust position ever and always use scroll
offset.

[enne (OOO), 2013-07-03 23:45:09]

Ok, here's one reason for that distinction: GraphicsLayers don't have a scroll
offset, but WebLayers do.

It seemed to me like ScrollingCoordinator should be the only entity that's
updating scroll positions on WebLayers. 
ScrollingCoordinator::scrollableAreaScrollLayerDidChange already does some extra
work to synchronize properties from a ScrollableArea to a WebLayer that it seems
shouldn't be duplicated elsewhere.

If ScrollingCoordinator only manages scrolling layers that the compositor thread
can scroll, then there needs to be some way to manage scrolling layers that only
Blink can scroll.  So, those layers just get the position on their GraphicsLayer
synced to their scroll offset since that's the only handle available.

[jamesr, 2013-07-04 00:34:00]

lgtm

ok, that makes sense

[enne (OOO), 2013-07-08 20:29:21]

On 2013/07/04 00:34:00, jamesr wrote:
> lgtm

After some more local testing and a comment from vollick, I realized that I
should also get rid of an additional line in RenderLayerBacking that updates a
scroll layer position when --enable-accelerated-overflow-scroll is on.  There
aren't correctness issues with this missing, but it does make an additional
Layer::SetPosition call which will cause a commit for this case.

Can you take another look and make sure that sounds reasonable to you?

> ok, that makes sense

I've updated the comment in a way that hopefully is more clear.

[jamesr, 2013-07-08 20:32:15]

yup, makes sense! lgtm

[enne (OOO), 2013-07-09 23:22:58]

Ok, third time's a charm.

Issue #1: RTL layers still need position set to the scroll origin /
minimumScrollPosition, as scroll offset goes from 0 to scroll maximum on cc
layers, but scroll *position* can be negative.  I think what I did is a
reasonable way to handle this.

Issue #2: ScrollingCoordinator needs to return a boolean about whether it
handled the layer or not.

https://codereview.chromium.org/18187004/diff/16001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/18187004/diff/16001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:1832: Bug(enne)
compositing/fixed-body-background-positioned.html [ Pass Failure ]
All of these tests change only be the removal of a position line in the layer
tree as text output for a scrolled layer.  I will land a change that turns these
into NeedsRebaseline once the #ifdefs get removed.

[jamesr, 2013-07-09 23:39:38]

Where are the gyp changes? As far as I know, you can't directly set a define
from the chromium side of the world.

https://codereview.chromium.org/18187004/diff/16001/Source/core/page/scrollin...
File Source/core/page/scrolling/ScrollingCoordinator.h (right):

https://codereview.chromium.org/18187004/diff/16001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.h:93: // Returns true iff the
coordinator handled this change.
while iff is more precise than if, i don't think it's used very widely in blink
compared to 'if' so we should probably just use that. just don't tell my
geometry teacher!

https://codereview.chromium.org/18187004/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/18187004/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:696: #ifndef
BLINK_SCROLLING_POSITION_NO_OFFSET
i think you want to add something to core.gyp to set this #define sometimes,
right?

[enne (OOO), 2013-07-09 23:43:47]

https://codereview.chromium.org/18187004/diff/16001/Source/core/page/scrollin...
File Source/core/page/scrolling/ScrollingCoordinator.h (right):

https://codereview.chromium.org/18187004/diff/16001/Source/core/page/scrollin...
Source/core/page/scrolling/ScrollingCoordinator.h:93: // Returns true iff the
coordinator handled this change.
On 2013/07/09 23:39:38, jamesr wrote:
> while iff is more precise than if, i don't think it's used very widely in
blink
> compared to 'if' so we should probably just use that. just don't tell my
> geometry teacher!

Iff you say so.  Done.

https://codereview.chromium.org/18187004/diff/16001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.cpp (right):

https://codereview.chromium.org/18187004/diff/16001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.cpp:696: #ifndef
BLINK_SCROLLING_POSITION_NO_OFFSET
On 2013/07/09 23:39:38, jamesr wrote:
> i think you want to add something to core.gyp to set this #define sometimes,
> right?

I added a define here: https://codereview.chromium.org/18400003/

[jamesr, 2013-07-09 23:47:29]

On 2013/07/09 23:43:47, enne wrote:
>
https://codereview.chromium.org/18187004/diff/16001/Source/core/page/scrollin...
> File Source/core/page/scrolling/ScrollingCoordinator.h (right):
> 
>
https://codereview.chromium.org/18187004/diff/16001/Source/core/page/scrollin...
> Source/core/page/scrolling/ScrollingCoordinator.h:93: // Returns true iff the
> coordinator handled this change.
> On 2013/07/09 23:39:38, jamesr wrote:
> > while iff is more precise than if, i don't think it's used very widely in
> blink
> > compared to 'if' so we should probably just use that. just don't tell my
> > geometry teacher!
> 
> Iff you say so.  Done.
> 
>
https://codereview.chromium.org/18187004/diff/16001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderLayerBacking.cpp (right):
> 
>
https://codereview.chromium.org/18187004/diff/16001/Source/core/rendering/Ren...
> Source/core/rendering/RenderLayerBacking.cpp:696: #ifndef
> BLINK_SCROLLING_POSITION_NO_OFFSET
> On 2013/07/09 23:39:38, jamesr wrote:
> > i think you want to add something to core.gyp to set this #define sometimes,
> > right?
> 
> I added a define here: https://codereview.chromium.org/18400003/

That's one way to do it, but the buildbots/goma cache/etc will be much happier
with you if you just set the #define for the two targets that need it.

[enne (OOO), 2013-07-10 00:41:23]

Added a define in core.gyp for the webcore_rendering target.  PTAL.

[jamesr, 2013-07-10 04:09:21]

lgtm

[commit-bot: I haz the power, 2013-07-10 16:03:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/18187004/32001

[commit-bot: I haz the power, 2013-07-10 17:32:54]

Change committed as 153910