cc: Refactor element id to layer id map on both main and impl thread to be for scrolling only |
||||||||||
Issue descriptionRemove element_layers_map_ from LayerTreeHost and LayerTreeImpl, since animation from element id should tick property tree nodes directly.
,
Apr 7 2017
Brief survey scoping work that looks like it would be involved:
LayerTreeHost: element_layers_map_ is used by
+ LayerByElementId
o IsElementInList
o SetElementFilterMutated
o SetElementOpacityMutated
o SetElementTransformMutated
o SetElementScrollOffsetMutated
o GetScrollOffsetForAnimation
+ RegisterElement
o SetLayerTreeHost
o SetElementId
LayerTreeImpl: element_layers_map_ is used by
+ LayerIdByElementId
o LayerByElementId
o SetCurrentlyScrollingNode
+ AddToElementMap
o LayerImpl ctor
o SetElementId
o SetMutableProperties
+ RemoveFromElementMap
o LayerImpl dtor
o SetElementId
Focusing for sake of example on LayerTreeHost::SetElementFilterMutated, to remove element_layers_map_ indirect usage through call to LayerByElementId:
- today the method looks up layer by element id and then calls Layer::OnFilterAnimated passing the new filters
+ Layer::OnFilterAnimated stores the filters on Layer.inputs_.filters field
+ Layer.inputs_.filters is then read in four places in layer.cc
- instead, we'd do something like:
+ use element_id_to_effect_node_index to get the affected effect node
+ update the filters on that effect node
+ change refs to Layer.inputs_filters in layer.cc to retrieve and check the effect node filter field for the layer instead
+ can then delete Layer.inputs_.filters field
,
Apr 7 2017
Deleting something like Layer.inputs_.filters seems hard as long as cc::PropertyTreeBuilder exists, since Blink (in SPv1 mode) or ui might want to set a filter on a layer before we've built property trees, so we need somewhere to put that filter. (And same for transform or opacity.)
,
Apr 10 2017
Hmm, good point. Our uncertainty re: other solutions noted in http://crbug.com/702350 is what is leading us to consider prioritizing http://crbug.com/709137 (see comments #8 - 11) for SPv2, of which this sub-task seemed like a core starting point. And would mean making this a blocking task for shipping SPv2. Will consider further.
,
May 22 2017
https://codereview.chromium.org/2895793002/ is in review to explore moving the was_ever_ready_since_last_transform_animation flag from LayerImpl to TransformNode. Now with 19 chars (ready_since_animate) vs 45 chars for ~58% (potential) shorter naming!
,
May 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3808a61e679c88a041a6bca1abb392a452d151ef commit 3808a61e679c88a041a6bca1abb392a452d151ef Author: Walter Korman <wkorman@chromium.org> Date: Tue May 23 21:31:41 2017 Remove transform animation checkerboard prevention logic Logic was originally added to track when a layer with a transform animation has no missing tiles in http://crrev.com/1461243003 for http://crbug.com/554924 . Many changes have since been made to image, animation, and paint related logic. In the absence of a real world test case that is still failing in the manner originally noted in 554294, prefer removing this special-case logic. Removes one LayerByElementId call site in LayerTreeHostImpl. This patch is a step toward removing animation system dependency on layers. Animation subsystem logic should involve only element ids and their property tree nodes. See also discussion in http://crrev.com/2895793002. Bug: 702777 , 709137 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I91c8f0afc1727a096a03901810455a52633e51a0 Reviewed-on: https://chromium-review.googlesource.com/512987 Commit-Queue: Walter Korman <wkorman@chromium.org> Reviewed-by: Vladimir Levin <vmpstr@chromium.org> Cr-Commit-Position: refs/heads/master@{#474063} [modify] https://crrev.com/3808a61e679c88a041a6bca1abb392a452d151ef/cc/layers/layer_impl.cc [modify] https://crrev.com/3808a61e679c88a041a6bca1abb392a452d151ef/cc/layers/layer_impl.h [modify] https://crrev.com/3808a61e679c88a041a6bca1abb392a452d151ef/cc/trees/layer_tree_host_impl.cc [modify] https://crrev.com/3808a61e679c88a041a6bca1abb392a452d151ef/cc/trees/layer_tree_host_impl_unittest.cc [modify] https://crrev.com/3808a61e679c88a041a6bca1abb392a452d151ef/cc/trees/layer_tree_impl.cc
,
Aug 22 2017
It is my understanding that pdr@ found, through work on scrolling, that we will still want this map for scrolling specific layers only. Thus the updated plan is to keep it, rename its involved member data, methods and such to reflect that it is scroll layer specific, and consider if we can add DCHECK or otherwise enforce scroll only usage. Which is basically a cleanup task. Given that, perhaps we should close this bug as WontFix. To pdr@ for input. I am tracking cleanup work in burndown list: https://docs.google.com/spreadsheets/d/17RQvn9ZsfHcf6OUOwwp7Ru1RTtLIs3aFCyueZ9wqQog/edit#gid=0
,
Aug 22 2017
I think the core issue is still worth a bug: remove all non-scrolling uses of the map.
,
Aug 23
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 24
Assigning back to pdr to own or close.
,
Aug 24
The only remaining non-scroll non-layer-list use of this is ElementAnimations::InitAffectedElementTypes. Peter, could you own removing this? We should be able to check for an effect or transform node with the element id, instead of checking for the layer.
,
Sep 12
(oops, adding the correct petermayo@chromium.org).
,
Nov 29
The use by InitAffectedElementTypes has been removed by my patch https://chromium-review.googlesource.com/c/chromium/src/+/1347784. I'll put a patch to no longer put elements in the element_layers_map_ if they're not scrolling related and then I think we can close this bug :-).
,
Nov 29
,
Dec 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/31cab084885ef072ff89cfb83c1973a18b4d3495 commit 31cab084885ef072ff89cfb83c1973a18b4d3495 Author: Robert Flack <flackr@chromium.org> Date: Tue Dec 04 17:53:35 2018 [BlinkGenPropertyTrees] Only register element ids of scrollable layers. With BGPT enabled, we now only need to look up layers by element ids for scrollable layers. This patch makes sure to only register scrollable layers to verify that we aren't relying on looking up non scrollable layers. Cq-Include-Trybots: luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2 Bug: 702777 Change-Id: I019ab8cb1d09726274a051dd9a935b72e5df6cda Reviewed-on: https://chromium-review.googlesource.com/c/1355326 Reviewed-by: Philip Rogers <pdr@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#613591} [modify] https://crrev.com/31cab084885ef072ff89cfb83c1973a18b4d3495/cc/layers/layer.cc [modify] https://crrev.com/31cab084885ef072ff89cfb83c1973a18b4d3495/cc/layers/layer_unittest.cc [modify] https://crrev.com/31cab084885ef072ff89cfb83c1973a18b4d3495/cc/trees/layer_tree_host.cc [modify] https://crrev.com/31cab084885ef072ff89cfb83c1973a18b4d3495/cc/trees/layer_tree_host.h
,
Dec 4
|
||||||||||
►
Sign in to add a comment |
||||||||||
Comment 1 by wkorman@chromium.org
, Apr 7 2017