Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2702143002:
Refactor IsScrolledBy and IsClosestScrollAncestor to use ScrollNode ids (Closed)
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... |
