Fix scroll position adjustment for fixed-position layers

Fix scroll position adjustment for fixed-position layers

blink is currently using ScrollCompensationAdjustment to store the
fractional bit of the scroll offset in cc. This CL incorporates that
value into property tree construction.

This adjustment only needs to be done on the main thread
since the fixed-pos layers will stay put by virtue of their
transform parent on the impl thread, so only the property tree
builder required updating.

BUG=470246
Committed: https://crrev.com/f6281c495acf0665126837bfead564b976cdefde
Cr-Commit-Position: refs/heads/master@{#322879}

[Ian Vollick, 2015-03-24 21:01:02]

vollick@chromium.org changed reviewers:
+ ajuma@chromium.org

[ajuma, 2015-03-26 18:03:25]

https://codereview.chromium.org/1021823006/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1021823006/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:8969:
scroller->SetScrollCompensationAdjustment(gfx::Vector2dF(0.0, -0.5f));
Does blink usually send negative values for scroll compensation adjustment? The
only other LayerTeeHostCommonTest for this uses the (positive) fractional
portion of scroll offset.

https://codereview.chromium.org/1021823006/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1021823006/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:169:
data_for_children->scroll_compensation_adjustment = gfx::Vector2d();
Does clearing here when is_fixed is true work when a fixed position layer has a
fixed position descendant (which has the same container)?

https://codereview.chromium.org/1021823006/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:171:
gfx::ToRoundedVector2d(layer->ScrollCompensationAdjustment());
It's not obvious that rounding is the right thing to do here. Does this work
when the compensation is smaller than 0.5? For example, suppose we have two
scrollers, each with scroll compensation adjustment 0.1. Rounding at each step
will mean data_for_children has an adjustment value of 2. Is this correct?

[Ian Vollick, 2015-03-30 17:51:25]

https://codereview.chromium.org/1021823006/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1021823006/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:8969:
scroller->SetScrollCompensationAdjustment(gfx::Vector2dF(0.0, -0.5f));
On 2015/03/26 18:03:25, ajuma wrote:
> Does blink usually send negative values for scroll compensation adjustment?
The
> only other LayerTeeHostCommonTest for this uses the (positive) fractional
> portion of scroll offset.

You're right, this is wrong. Blink sends the fractional portion of the scroll
offset (with the same sign) as the "scroll compensation adjustment". I've
updated the unit test.

https://codereview.chromium.org/1021823006/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1021823006/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:169:
data_for_children->scroll_compensation_adjustment = gfx::Vector2d();
On 2015/03/26 18:03:25, ajuma wrote:
> Does clearing here when is_fixed is true work when a fixed position layer has
a
> fixed position descendant (which has the same container)?

It does, yeah. The current GraphicsLayer tree is constructed in such a way that
fixed pos layers will descend from one another (as it's a stacking context) and
would therefore "descendant" fixed pos layers would get compensated by virtue of
their ancestor's compensation. This matches that behavior. For the CDP analog to
this, see the computation of
current_layer_resets_scroll_compensation_for_descendants in LTHC's
ComputeScrollCompensationMatrixForChildren.

https://codereview.chromium.org/1021823006/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:171:
gfx::ToRoundedVector2d(layer->ScrollCompensationAdjustment());
On 2015/03/26 18:03:25, ajuma wrote:
> It's not obvious that rounding is the right thing to do here. Does this work
> when the compensation is smaller than 0.5? For example, suppose we have two
> scrollers, each with scroll compensation adjustment 0.1. Rounding at each step
> will mean data_for_children has an adjustment value of 2. Is this correct?

You're right -- this isn't the right place for it. What's happening here was an
attempt to mimic the tweaking of the inherited scroll componsation adjustment to
account for scroll snapping. It turns out that this is much easier to do by
simply saving the scroll snap and applying it directly which I've done in the
latest patch.

[ajuma, 2015-03-30 19:08:52]

lgtm, thanks for the explanations!

https://codereview.chromium.org/1021823006/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1021823006/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:8969:
scroller->SetScrollCompensationAdjustment(gfx::Vector2dF(0.3, 0.7f));
0.3f

[Ian Vollick, 2015-03-30 19:38:50]

https://codereview.chromium.org/1021823006/diff/100001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1021823006/diff/100001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:8969:
scroller->SetScrollCompensationAdjustment(gfx::Vector2dF(0.3, 0.7f));
On 2015/03/30 19:08:52, ajuma wrote:
> 0.3f

Done.

[Ian Vollick, 2015-03-30 19:38:54]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2015-03-30 19:38:55]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1021823006/#ps120001
(title: ".")

[commit-bot: I haz the power, 2015-03-30 19:39:46]

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

[Ian Vollick, 2015-03-30 19:41:23]

The CQ bit was unchecked by vollick@chromium.org

[Ian Vollick, 2015-03-30 20:51:09]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2015-03-30 20:51:09]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1021823006/#ps140001
(title: "rebased")

[commit-bot: I haz the power, 2015-03-30 20:51:53]

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

[commit-bot: I haz the power, 2015-03-30 21:44:49]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-03-30 21:45:48]

Patchset 8 (id:??) landed as
https://crrev.com/f6281c495acf0665126837bfead564b976cdefde
Cr-Commit-Position: refs/heads/master@{#322879}