|
|
DescriptionRefactor IsScrolledBy and IsClosestScrollAncestor to use ScrollNode ids
This patch switches IsScrolledBy and IsClosestScrollAncestor to use
ScrollNode indexes instead of ScrollNode's owning_layer_id. There should
be no change in behavior and this gets us a little closer to removing
owning_layer_id.
BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2702143002
Cr-Commit-Position: refs/heads/master@{#452589}
Committed: https://chromium.googlesource.com/chromium/src/+/87f502185cad0c3cbdd7019790a7cff3d37bbf5b
Patch Set 1 #
Total comments: 7
Patch Set 2 : Switch to using layer_id_to_scroll_node_index #
Total comments: 2
Messages
Total messages: 27 (15 generated)
Description was changed from ========== Refactor IsScrolledBy and IsClosestScrollAncestor to use ScrollNode ids This patch switches IsScrolledBy and IsClosestScrollAncestor to use ScrollNode indexes instead of ScrollNode's owning_layer_id. There should be no change in behavior and this gets us a little closer to removing owning_layer_id. BUG=693740 ========== to ========== Refactor IsScrolledBy and IsClosestScrollAncestor to use ScrollNode ids This patch switches IsScrolledBy and IsClosestScrollAncestor to use ScrollNode indexes instead of ScrollNode's owning_layer_id. There should be no change in behavior and this gets us a little closer to removing owning_layer_id. BUG=693740 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by pdr@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: This issue passed the CQ dry run.
pdr@chromium.org changed reviewers: + ajuma@chromium.org, bokan@chromium.org
Description was changed from ========== Refactor IsScrolledBy and IsClosestScrollAncestor to use ScrollNode ids This patch switches IsScrolledBy and IsClosestScrollAncestor to use ScrollNode indexes instead of ScrollNode's owning_layer_id. There should be no change in behavior and this gets us a little closer to removing owning_layer_id. BUG=693740 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Refactor IsScrolledBy and IsClosestScrollAncestor to use ScrollNode ids This patch switches IsScrolledBy and IsClosestScrollAncestor to use ScrollNode indexes instead of ScrollNode's owning_layer_id. There should be no change in behavior and this gets us a little closer to removing owning_layer_id. BUG=693740 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
ajuma@chromium.org changed reviewers: + sunxd@chromium.org
+sunxd https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:168: if (scroll_node->id == ancestor->scroll_tree_index()) Just wanted to point out that this could change behavior if IsScrolledBy was called with |ancestor| not owning a scroll node (since previously, that would always return false, but now it could return true). But it looks like this only gets called when |ancestor| owns a scroll node. https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:2629: return scroll_node->id == scroll_ancestor->scroll_tree_index(); Same comment as above, but this time it looks like this can indeed get called (from ScrollBegin) with |scroll_ancestor| not owning a scroll node (it can be a scrollbar layer that isn't itself scrollable). For background on why scrollbar layers are a special case, see https://codereview.chromium.org/1878323002 and https://codereview.chromium.org/238803005. I wonder if we just need to make that special-casing more explicit somehow? sunxd@, any ideas?
https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:168: if (scroll_node->id == ancestor->scroll_tree_index()) On 2017/02/21 16:05:25, ajuma wrote: > Just wanted to point out that this could change behavior if IsScrolledBy was > called with |ancestor| not owning a scroll node (since previously, that would > always return false, but now it could return true). But it looks like this only > gets called when |ancestor| owns a scroll node. Ali, could you help me understand how that could happen? If ancestor doesn't own a ScrollNode, won't scroll_tree_index be invalid and this check always be false?
https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:168: if (scroll_node->id == ancestor->scroll_tree_index()) On 2017/02/21 19:45:25, bokan wrote: > On 2017/02/21 16:05:25, ajuma wrote: > > Just wanted to point out that this could change behavior if IsScrolledBy was > > called with |ancestor| not owning a scroll node (since previously, that would > > always return false, but now it could return true). But it looks like this > only > > gets called when |ancestor| owns a scroll node. > > Ali, could you help me understand how that could happen? If ancestor doesn't own > a ScrollNode, won't scroll_tree_index be invalid and this check always be false? Every layer has a scroll_tree_index. If a layer doesn't own a ScrollNode, it's scroll_tree_index will be that of the closest ancestor that owns one (where 'ancestor' is either real ancestor in the main-thread Layer tree or Layer::scroll_parent ancestor if there is one). Non-test LayerImpls should never have invalid indices for any of the property trees.
https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:168: if (scroll_node->id == ancestor->scroll_tree_index()) On 2017/02/21 20:07:56, ajuma wrote: > On 2017/02/21 19:45:25, bokan wrote: > > On 2017/02/21 16:05:25, ajuma wrote: > > > Just wanted to point out that this could change behavior if IsScrolledBy was > > > called with |ancestor| not owning a scroll node (since previously, that > would > > > always return false, but now it could return true). But it looks like this > > only > > > gets called when |ancestor| owns a scroll node. > > > > Ali, could you help me understand how that could happen? If ancestor doesn't > own > > a ScrollNode, won't scroll_tree_index be invalid and this check always be > false? > > Every layer has a scroll_tree_index. If a layer doesn't own a ScrollNode, it's > scroll_tree_index will be that of the closest ancestor that owns one (where > 'ancestor' is either real ancestor in the main-thread Layer tree or > Layer::scroll_parent ancestor if there is one). Non-test LayerImpls should never > have invalid indices for any of the property trees. Ah, got it - I was misunderstanding how that worked. Thanks.
https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:2629: return scroll_node->id == scroll_ancestor->scroll_tree_index(); On 2017/02/21 16:05:25, ajuma wrote: > Same comment as above, but this time it looks like this can indeed get called > (from ScrollBegin) with |scroll_ancestor| not owning a scroll node (it can be a > scrollbar layer that isn't itself scrollable). For background on why scrollbar > layers are a special case, see https://codereview.chromium.org/1878323002 and > https://codereview.chromium.org/238803005. I wonder if we just need to make that > special-casing more explicit somehow? sunxd@, any ideas? In some cases we have MainThreadScrollingReason::kScrollbarScrolling for scrollbars, but at least for SolidColorScrollbarLayer, we do not have such a reason, so probably we do not have a scroll node for this layer.
The CQ bit was checked by pdr@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...
Nice find Ali. This is actually pretty easy to fix using layer_id_to_scroll_node_index. PTAL https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2702143002/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:168: if (scroll_node->id == ancestor->scroll_tree_index()) On 2017/02/21 at 20:25:22, bokan wrote: > On 2017/02/21 20:07:56, ajuma wrote: > > On 2017/02/21 19:45:25, bokan wrote: > > > On 2017/02/21 16:05:25, ajuma wrote: > > > > Just wanted to point out that this could change behavior if IsScrolledBy was > > > > called with |ancestor| not owning a scroll node (since previously, that > > would > > > > always return false, but now it could return true). But it looks like this > > > only > > > > gets called when |ancestor| owns a scroll node. > > > > > > Ali, could you help me understand how that could happen? If ancestor doesn't > > own > > > a ScrollNode, won't scroll_tree_index be invalid and this check always be > > false? > > > > Every layer has a scroll_tree_index. If a layer doesn't own a ScrollNode, it's > > scroll_tree_index will be that of the closest ancestor that owns one (where > > 'ancestor' is either real ancestor in the main-thread Layer tree or > > Layer::scroll_parent ancestor if there is one). Non-test LayerImpls should never > > have invalid indices for any of the property trees. > > Ah, got it - I was misunderstanding how that worked. Thanks. +1, I had the same misunderstanding.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, just have a question about how this is going to work in SPv2 when nodes don't have a unique owning layer: https://codereview.chromium.org/2702143002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2702143002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:166: property_trees->layer_id_to_scroll_node_index.find(ancestor->id()); Is the idea that for SPv2, we'd potentially have multiple layer ids mapped to the same scroll node index?
https://codereview.chromium.org/2702143002/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2702143002/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:166: property_trees->layer_id_to_scroll_node_index.find(ancestor->id()); On 2017/02/23 at 14:19:00, ajuma wrote: > Is the idea that for SPv2, we'd potentially have multiple layer ids mapped to the same scroll node index? Yeah, here's my mental model of a usecase and how it would work: Imagine we have a scrollable area that paints 3 horizontal sections of content with a total area of 6*3: +------+-+ |AAAAAA|^| |BBBBBB| | |AAAAAA|v| +------+-+ In SPV1, we would always create a layer for the AAAAAA content of at least size 6*3, and this layer would own the scroll node. If the user specifies will-change on the BBBBBB, a child layer may be created of size 6*1. In SPV2, we want to be able to create 3 non-overlapping layers for the will-change:B case: A1, B, and A2, each of size 6*1. Each of these layers would be in the layer_id_to_scroll_node_index map, sort of as if they collectively "own" the scroll node.
On 2017/02/23 19:19:19, pdr. wrote: > https://codereview.chromium.org/2702143002/diff/20001/cc/trees/layer_tree_hos... > File cc/trees/layer_tree_host_impl.cc (right): > > https://codereview.chromium.org/2702143002/diff/20001/cc/trees/layer_tree_hos... > cc/trees/layer_tree_host_impl.cc:166: > property_trees->layer_id_to_scroll_node_index.find(ancestor->id()); > On 2017/02/23 at 14:19:00, ajuma wrote: > > Is the idea that for SPv2, we'd potentially have multiple layer ids mapped to > the same scroll node index? > > Yeah, here's my mental model of a usecase and how it would work: > > Imagine we have a scrollable area that paints 3 horizontal sections of content > with a total area of 6*3: > +------+-+ > |AAAAAA|^| > |BBBBBB| | > |AAAAAA|v| > +------+-+ > > In SPV1, we would always create a layer for the AAAAAA content of at least size > 6*3, and this layer would own the scroll node. If the user specifies will-change > on the BBBBBB, a child layer may be created of size 6*1. > > In SPV2, we want to be able to create 3 non-overlapping layers for the > will-change:B case: A1, B, and A2, each of size 6*1. Each of these layers would > be in the layer_id_to_scroll_node_index map, sort of as if they collectively > "own" the scroll node. Thanks for the example! lgtm
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487878041559410, "parent_rev": "4c25edcf92c0d4acccdc02f6f3a4a5264115e05c", "commit_rev": "87f502185cad0c3cbdd7019790a7cff3d37bbf5b"}
Message was sent while issue was closed.
Description was changed from ========== Refactor IsScrolledBy and IsClosestScrollAncestor to use ScrollNode ids This patch switches IsScrolledBy and IsClosestScrollAncestor to use ScrollNode indexes instead of ScrollNode's owning_layer_id. There should be no change in behavior and this gets us a little closer to removing owning_layer_id. BUG=693740 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Refactor IsScrolledBy and IsClosestScrollAncestor to use ScrollNode ids This patch switches IsScrolledBy and IsClosestScrollAncestor to use ScrollNode indexes instead of ScrollNode's owning_layer_id. There should be no change in behavior and this gets us a little closer to removing owning_layer_id. BUG=693740 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2702143002 Cr-Commit-Position: refs/heads/master@{#452589} Committed: https://chromium.googlesource.com/chromium/src/+/87f502185cad0c3cbdd7019790a7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/87f502185cad0c3cbdd7019790a7... |