Implement position: sticky updates on compositor

Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through TransformNode::UpdateLocalTransform.

BUG=512494
TEST=LayerTreeHostCommonTest.StickyPosition
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/2215b4e8cb1fb332ad6e7b495435deb3f89f41f8
Cr-Commit-Position: refs/heads/master@{#420144}

[flackr, 2016-08-18 15:06:07]

Description was changed from

==========
WIP - Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through
TransformNode::UpdateLocalTransform.

BUG=512494
TEST=Manual
==========

to

==========
WIP - Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through
TransformNode::UpdateLocalTransform.

BUG=512494
TEST=Manual
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[flackr, 2016-08-18 15:06:08]

Description was changed from

==========
WIP - Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through
TransformNode::UpdateLocalTransform.

BUG=512494
TEST=Manual
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
WIP - Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through
TransformNode::UpdateLocalTransform.

BUG=512494
TEST=Manual
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[flackr, 2016-09-07 14:12:54]

Description was changed from

==========
WIP - Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through
TransformNode::UpdateLocalTransform.

BUG=512494
TEST=Manual
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through
TransformNode::UpdateLocalTransform.

BUG=512494
TEST=Manual
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[flackr, 2016-09-07 14:12:55]

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

[flackr, 2016-09-08 17:47:46]

Description was changed from

==========
Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through
TransformNode::UpdateLocalTransform.

BUG=512494
TEST=Manual
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through
TransformNode::UpdateLocalTransform.

BUG=512494
TEST=LayerTreeHostCommonTest.StickyPosition
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[flackr, 2016-09-08 17:52:31]

Hey,

This patch plumbs the sticky constraints over to cc/ and implements the sticky
position update as part of UpdateLocalTransform. Can you take a look? I had a
couple questions about transform tree details which I've commented on inline.

Thanks!

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:863:
sticky_position_data_free_ids_.push_back(sticky_data_id);
I'm not sure if we always rebuild the tree when sticky status changes. If we
think we do, or if it's rare, we could remove the sticky_position_data_free_ids
and just always add new nodes at the end of the vector.

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:867: void
TransformTree::ToProtobuf(proto::PropertyTree* proto) const {
When is this used? Will the sticky position data need to be added to this?

[ajuma, 2016-09-08 22:08:54]

Seems reasonable overall.

https://codereview.chromium.org/2251303003/diff/40001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2251303003/diff/40001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:580: LayerStickyPositionConstraint
sticky_position_constraint_;
Does LayerImpl need to know this? Is it enough for the transform tree to have
it? If you just need this for tests, it can go on LayerImplTestProperties
instead.

https://codereview.chromium.org/2251303003/diff/40001/cc/layers/layer_sticky_...
File cc/layers/layer_sticky_position_constraint.h (right):

https://codereview.chromium.org/2251303003/diff/40001/cc/layers/layer_sticky_...
cc/layers/layer_sticky_position_constraint.h:28: gfx::Rect
absolute_containing_block_rect;
Please add a comment explaining what the offsets and rects represent.

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

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:6934:
sticky_position.absolute_containing_block_rect = gfx::Rect(0, 0, 50, 50);
Please also include tests that use the other sticky position constraint
settings, like is_anchored_bottom and left_offset.

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:407: TransformNode* ScrollingAncestor(TransformTree*
tree, TransformNode* node) {
This function seems to be unused?

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:431: if (constraint.is_anchored_right) {
Please explain (in a comment) what the math here is trying to do.

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:531: transform.Translate(StickyPositionOffset(this,
node));
I'd have expected this to happen a couple lines earlier, before we preconcat the
local transform (since this seems to be an adjustment to the layer's position,
and the position itself is part of the post_local transform). Try testing with
interesting values of node->local (e.g. consider the case where that's a scale;
then whether we translate before or after will change whether the translation is
affected by the scale).

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:863:
sticky_position_data_free_ids_.push_back(sticky_data_id);
On 2016/09/08 17:52:30, flackr wrote:
> I'm not sure if we always rebuild the tree when sticky status changes. If we
> think we do, or if it's rare, we could remove the
sticky_position_data_free_ids
> and just always add new nodes at the end of the vector.

Since Layer::SetStickyPositionConstraint calls SetNeedsCommit (rather than
SetNeedsCommitNoRebuild), we'll rebuild property trees whenever that gets called
with a changed sticky status. Rebuilding every time this changes seems fine
since I'd expect that to change rarely (if this turns out to be common, then it
would make sense to add logic for handling it without a rebuild).

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:867: void
TransformTree::ToProtobuf(proto::PropertyTree* proto) const {
On 2016/09/08 17:52:30, flackr wrote:
> When is this used? Will the sticky position data need to be added to this?

This was added for Blimp, though currently they don't actually use this code
(since their serialization point is now before cc builds property trees, so
property trees just get built on the client). But once property-tree-building
happens in Blink (as part of SPv2) I think they will need to serialize property
trees.

In any case, please do add the sticky position data to this.

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.h:129: gfx::Vector2dF main_thread_offset;
Please add a comment explaining what this represents. I think it's something
like the offset that Blink has applied to counteract the main thread scroll
offset of the scroll ancestor?

[flackr, 2016-09-20 17:08:13]

https://codereview.chromium.org/2251303003/diff/40001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2251303003/diff/40001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:580: LayerStickyPositionConstraint
sticky_position_constraint_;
On 2016/09/08 at 22:08:53, ajuma wrote:
> Does LayerImpl need to know this? Is it enough for the transform tree to have
it? If you just need this for tests, it can go on LayerImplTestProperties
instead.

Done.

https://codereview.chromium.org/2251303003/diff/40001/cc/layers/layer_sticky_...
File cc/layers/layer_sticky_position_constraint.h (right):

https://codereview.chromium.org/2251303003/diff/40001/cc/layers/layer_sticky_...
cc/layers/layer_sticky_position_constraint.h:28: gfx::Rect
absolute_containing_block_rect;
On 2016/09/08 at 22:08:53, ajuma wrote:
> Please add a comment explaining what the offsets and rects represent.

Done.

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:407: TransformNode* ScrollingAncestor(TransformTree*
tree, TransformNode* node) {
On 2016/09/08 at 22:08:54, ajuma wrote:
> This function seems to be unused?

Removed. Yes, this ended up not being needed as we set up the ancestor on the
constraint.

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:431: if (constraint.is_anchored_right) {
On 2016/09/08 at 22:08:54, ajuma wrote:
> Please explain (in a comment) what the math here is trying to do.

Done. I tried to summarize above.

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:531: transform.Translate(StickyPositionOffset(this,
node));
On 2016/09/08 at 22:08:54, ajuma wrote:
> I'd have expected this to happen a couple lines earlier, before we preconcat
the local transform (since this seems to be an adjustment to the layer's
position, and the position itself is part of the post_local transform). Try
testing with interesting values of node->local (e.g. consider the case where
that's a scale; then whether we translate before or after will change whether
the translation is affected by the scale).

Good catch, this is a position effect and shouldn't be affected by the local
transform. I've added a test case for this and for a scaled container.

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:863:
sticky_position_data_free_ids_.push_back(sticky_data_id);
On 2016/09/08 at 22:08:54, ajuma wrote:
> On 2016/09/08 17:52:30, flackr wrote:
> > I'm not sure if we always rebuild the tree when sticky status changes. If we
> > think we do, or if it's rare, we could remove the
sticky_position_data_free_ids
> > and just always add new nodes at the end of the vector.
> 
> Since Layer::SetStickyPositionConstraint calls SetNeedsCommit (rather than
SetNeedsCommitNoRebuild), we'll rebuild property trees whenever that gets called
with a changed sticky status. Rebuilding every time this changes seems fine
since I'd expect that to change rarely (if this turns out to be common, then it
would make sense to add logic for handling it without a rebuild).

Done. I agree, I expect sticky / not sticky changes to be rare.

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:867: void
TransformTree::ToProtobuf(proto::PropertyTree* proto) const {
On 2016/09/08 at 22:08:54, ajuma wrote:
> On 2016/09/08 17:52:30, flackr wrote:
> > When is this used? Will the sticky position data need to be added to this?
> 
> This was added for Blimp, though currently they don't actually use this code
(since their serialization point is now before cc builds property trees, so
property trees just get built on the client). But once property-tree-building
happens in Blink (as part of SPv2) I think they will need to serialize property
trees.
> 
> In any case, please do add the sticky position data to this.

Done.

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.h:129: gfx::Vector2dF main_thread_offset;
On 2016/09/08 at 22:08:54, ajuma wrote:
> Please add a comment explaining what this represents. I think it's something
like the offset that Blink has applied to counteract the main thread scroll
offset of the scroll ancestor?

Done.

[flackr, 2016-09-20 17:08:47]

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

https://codereview.chromium.org/2251303003/diff/40001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:6934:
sticky_position.absolute_containing_block_rect = gfx::Rect(0, 0, 50, 50);
On 2016/09/08 at 22:08:53, ajuma wrote:
> Please also include tests that use the other sticky position constraint
settings, like is_anchored_bottom and left_offset.

Done.

[ajuma, 2016-09-20 19:50:08]

cc lgtm. Thanks for all the tests and comments!

[flackr, 2016-09-20 19:57:32]

flackr@chromium.org changed reviewers:
+ rbyers@chromium.org, vollick@chromium.org

[flackr, 2016-09-20 19:57:32]

+vollick for third_party/WebKit/Source/platform
+rbyers for third_party/WebKit/public

PTAL, thanks!

[Ian Vollick, 2016-09-20 19:58:53]

On 2016/09/20 19:57:32, flackr wrote:
> +vollick for third_party/WebKit/Source/platform
> +rbyers for third_party/WebKit/public
> 
> PTAL, thanks!

platform/ lgtm.

[Rick Byers, 2016-09-21 13:31:39]

On 2016/09/20 19:57:32, flackr wrote:
> +vollick for third_party/WebKit/Source/platform
> +rbyers for third_party/WebKit/public
> 
> PTAL, thanks!

public/ LGTM

[Rick Byers, 2016-09-21 13:33:11]

On 2016/09/21 13:31:39, Rick Byers (slow until 9-26) wrote:
> On 2016/09/20 19:57:32, flackr wrote:
> > +vollick for third_party/WebKit/Source/platform
> > +rbyers for third_party/WebKit/public
> > 
> > PTAL, thanks!
> 
> public/ LGTM

With nit: we generally try to document the public API pretty well.  Can you add
some per -member comments to the new structure that defines the semantics /
constraints of each member? Thanks

[flackr, 2016-09-21 13:33:25]

The CQ bit was checked by flackr@chromium.org

[flackr, 2016-09-21 13:33:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2251303003/#ps80001
(title: "Remove obsolete test sticky-scrolls-on-main.html")

[flackr, 2016-09-21 13:33:32]

The CQ bit was unchecked by flackr@chromium.org

[flackr, 2016-09-21 15:51:56]

The CQ bit was checked by flackr@chromium.org

[flackr, 2016-09-21 15:51:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
vollick@chromium.org, ajuma@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2251303003/#ps100001
(title: "Merge with master and add comments to WebLayerStickyPositionConstraint
members.")

[commit-bot: I haz the power, 2016-09-21 15:52:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 17:54:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 17:54:33]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[flackr, 2016-09-21 18:36:17]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2016-09-21 18:36:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 20:16:41]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-21 20:18:48]

Description was changed from

==========
Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through
TransformNode::UpdateLocalTransform.

BUG=512494
TEST=LayerTreeHostCommonTest.StickyPosition
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Implement position: sticky updates on compositor

This plumbs the sticky position constraints to the compositor and updates the
position of sticky position elements through
TransformNode::UpdateLocalTransform.

BUG=512494
TEST=LayerTreeHostCommonTest.StickyPosition
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/2215b4e8cb1fb332ad6e7b495435deb3f89f41f8
Cr-Commit-Position: refs/heads/master@{#420144}
==========

[commit-bot: I haz the power, 2016-09-21 20:18:49]

Patchset 6 (id:??) landed as
https://crrev.com/2215b4e8cb1fb332ad6e7b495435deb3f89f41f8
Cr-Commit-Position: refs/heads/master@{#420144}