Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2183923002:
Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. (Closed)
DescriptionUndo scroll snaps in to_parent when NeedsSourceToParentUpdate.
When we scroll on impl thread, we do scroll snapping in transform trees
if the transform node has a non-integer screen space transform. However,
the main thread logic also do snapping. When updating source to parent
transforms on main thread, it is possible that the scroll snaps are used
to compute the to_parent transform.
It becomes a problem if the node is created due to a fixed pos layer,
the snapping will add a small translation to the to_parent transform and
result in the unstable position. This CL undoes the snapping in this
case. It is notable that scroll_parent might also be influenced by this
change.
BUG=584598
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/8a9a60982e6cf62b6fbe45326141fda8263d2dfe
Cr-Commit-Position: refs/heads/master@{#408699}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Unit tests #Patch Set 3 : Remove scroll snap in BuildPropertyTrees #Patch Set 4 : Remove code no longer in use #
Total comments: 4
Patch Set 5 : Nit change #
Messages
Total messages: 37 (27 generated)
Description was changed from ========== Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. When we scroll on impl thread, we do scroll snapping in transform trees if the transform node has a non-integer screen space transform. However, the main thread logic also do snapping. When updating source to parent transforms on main thread, it is possible that the scroll snaps are used to compute the to_parent transform. It becomes a problem if the node is created due to a fixed pos layer, the snapping will add a small translation to the to_parent transform and result in the unstable position. This CL undoes the snapping in this case. It is notable that scroll_parent might also be influenced by this change. BUG=584598 ========== to ========== Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. When we scroll on impl thread, we do scroll snapping in transform trees if the transform node has a non-integer screen space transform. However, the main thread logic also do snapping. When updating source to parent transforms on main thread, it is possible that the scroll snaps are used to compute the to_parent transform. It becomes a problem if the node is created due to a fixed pos layer, the snapping will add a small translation to the to_parent transform and result in the unstable position. This CL undoes the snapping in this case. It is notable that scroll_parent might also be influenced by this change. BUG=584598 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. When we scroll on impl thread, we do scroll snapping in transform trees if the transform node has a non-integer screen space transform. However, the main thread logic also do snapping. When updating source to parent transforms on main thread, it is possible that the scroll snaps are used to compute the to_parent transform. It becomes a problem if the node is created due to a fixed pos layer, the snapping will add a small translation to the to_parent transform and result in the unstable position. This CL undoes the snapping in this case. It is notable that scroll_parent might also be influenced by this change. BUG=584598 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. When we scroll on impl thread, we do scroll snapping in transform trees if the transform node has a non-integer screen space transform. However, the main thread logic also do snapping. When updating source to parent transforms on main thread, it is possible that the scroll snaps are used to compute the to_parent transform. It becomes a problem if the node is created due to a fixed pos layer, the snapping will add a small translation to the to_parent transform and result in the unstable position. This CL undoes the snapping in this case. It is notable that scroll_parent might also be influenced by this change. BUG=584598 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
sunxd@chromium.org changed reviewers: + ajuma@chromium.org
Hi Ali, I want to make sure this is the fix we want (like the influence this CL has on scroll_parent stuff). Thanks!
Hi Ali, I want to make sure this is the fix we want (like the influence this CL has on scroll_parent stuff). Thanks!
https://codereview.chromium.org/2183923002/diff/1/cc/trees/property_tree.cc File cc/trees/property_tree.cc (right): https://codereview.chromium.org/2183923002/diff/1/cc/trees/property_tree.cc#n... cc/trees/property_tree.cc:338: unsnapping.Subtract(parent_node->scroll_snap); This is the right general approach, but I think there are some more details to handle in order for this to work for the scroll parent case. 1) I'm not sure it's true that the lowest ancestor of the source node whose id is <= the parent node will also be an ancestor of the parent node. We used to have code to find the lowest common ancestor of a pair of nodes but it was removed in https://codereview.chromium.org/1070133002. I think we'll need something like that. 2) When traversing the path from the lowest common ancestor to the parent, I think you need to add rather than subtract? Adding DCHECKs for the assumptions you're using might help to find tests with examples where these assumptions don't hold. Please also add LayerTreeHostCommonTests (for both the fixed-pos and scroll parent cases) where the computed screen space transform changes with this patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * master.tryserver.blink builder linux_blink_rel was renamed to linux_precise_blink_rel (http://crbug.com/590036)
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL was about to rely on recently removed CQ feature(s): * master.tryserver.blink builder linux_blink_rel was renamed to linux_precise_blink_rel (http://crbug.com/590036)
Description was changed from ========== Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. When we scroll on impl thread, we do scroll snapping in transform trees if the transform node has a non-integer screen space transform. However, the main thread logic also do snapping. When updating source to parent transforms on main thread, it is possible that the scroll snaps are used to compute the to_parent transform. It becomes a problem if the node is created due to a fixed pos layer, the snapping will add a small translation to the to_parent transform and result in the unstable position. This CL undoes the snapping in this case. It is notable that scroll_parent might also be influenced by this change. BUG=584598 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. When we scroll on impl thread, we do scroll snapping in transform trees if the transform node has a non-integer screen space transform. However, the main thread logic also do snapping. When updating source to parent transforms on main thread, it is possible that the scroll snaps are used to compute the to_parent transform. It becomes a problem if the node is created due to a fixed pos layer, the snapping will add a small translation to the to_parent transform and result in the unstable position. This CL undoes the snapping in this case. It is notable that scroll_parent might also be influenced by this change. BUG=584598 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sunxd@chromium.org changed reviewers: + aelias@chromium.org, chrishtr@chromium.org
Sorry I forgot to remove unnecessary code.
The CQ bit was checked by sunxd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2183923002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2183923002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:6894: EXPECT_EQ(expected_scroll_child_screen_space_transform, Nit: Use EXPECT_TRANSFORMATION_MATRIX_EQ to compare these transforms. https://codereview.chromium.org/2183923002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:6905: // + fixed_pos nit: fixed_pos is a child of scroller, so needs to be indented more
https://codereview.chromium.org/2183923002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (right): https://codereview.chromium.org/2183923002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:6894: EXPECT_EQ(expected_scroll_child_screen_space_transform, On 2016/07/29 17:33:33, ajuma wrote: > Nit: Use EXPECT_TRANSFORMATION_MATRIX_EQ to compare these transforms. Done. https://codereview.chromium.org/2183923002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:6905: // + fixed_pos On 2016/07/29 17:33:33, ajuma wrote: > nit: fixed_pos is a child of scroller, so needs to be indented more Done.
The CQ bit was checked by sunxd@chromium.org
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/2183923002/#ps80001 (title: "Nit change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. When we scroll on impl thread, we do scroll snapping in transform trees if the transform node has a non-integer screen space transform. However, the main thread logic also do snapping. When updating source to parent transforms on main thread, it is possible that the scroll snaps are used to compute the to_parent transform. It becomes a problem if the node is created due to a fixed pos layer, the snapping will add a small translation to the to_parent transform and result in the unstable position. This CL undoes the snapping in this case. It is notable that scroll_parent might also be influenced by this change. BUG=584598 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. When we scroll on impl thread, we do scroll snapping in transform trees if the transform node has a non-integer screen space transform. However, the main thread logic also do snapping. When updating source to parent transforms on main thread, it is possible that the scroll snaps are used to compute the to_parent transform. It becomes a problem if the node is created due to a fixed pos layer, the snapping will add a small translation to the to_parent transform and result in the unstable position. This CL undoes the snapping in this case. It is notable that scroll_parent might also be influenced by this change. BUG=584598 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. When we scroll on impl thread, we do scroll snapping in transform trees if the transform node has a non-integer screen space transform. However, the main thread logic also do snapping. When updating source to parent transforms on main thread, it is possible that the scroll snaps are used to compute the to_parent transform. It becomes a problem if the node is created due to a fixed pos layer, the snapping will add a small translation to the to_parent transform and result in the unstable position. This CL undoes the snapping in this case. It is notable that scroll_parent might also be influenced by this change. BUG=584598 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Undo scroll snaps in to_parent when NeedsSourceToParentUpdate. When we scroll on impl thread, we do scroll snapping in transform trees if the transform node has a non-integer screen space transform. However, the main thread logic also do snapping. When updating source to parent transforms on main thread, it is possible that the scroll snaps are used to compute the to_parent transform. It becomes a problem if the node is created due to a fixed pos layer, the snapping will add a small translation to the to_parent transform and result in the unstable position. This CL undoes the snapping in this case. It is notable that scroll_parent might also be influenced by this change. BUG=584598 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/8a9a60982e6cf62b6fbe45326141fda8263d2dfe Cr-Commit-Position: refs/heads/master@{#408699} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8a9a60982e6cf62b6fbe45326141fda8263d2dfe Cr-Commit-Position: refs/heads/master@{#408699} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
