New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 702777 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 4
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , All
Pri: 2
Type: Feature

Blocked on:
issue 702774

Blocking:
issue 709137



Sign in to add a comment

cc: Refactor element id to layer id map on both main and impl thread to be for scrolling only

Project Member Reported by weiliangc@chromium.org, Mar 17 2017

Issue description

Remove element_layers_map_ from LayerTreeHost and LayerTreeImpl, since animation from element id should tick property tree nodes directly.
 
Blocking: 709137
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

Comment 3 by ajuma@chromium.org, 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.)
Cc: chrishtr@chromium.org pdr@chromium.org
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.
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!
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Cc: -pdr@chromium.org
Owner: pdr@chromium.org
Status: Assigned (was: Available)
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

Comment 8 by pdr@chromium.org, Aug 22 2017

Cc: pdr@chromium.org
Owner: ----
Status: Available (was: Assigned)
Summary: cc: Refactor element id to layer id map on both main and impl thread to be for scrolling only (was: cc: Remove element id to layer id map on both main and impl thread)
I think the core issue is still worth a bug: remove all non-scrolling uses of the map.
Project Member

Comment 9 by sheriffbot@chromium.org, Aug 23

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
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
Owner: pdr@chromium.org
Status: Assigned (was: Untriaged)
Assigning back to pdr to own or close.
Owner: petermayo@google.com
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.
Labels: -Pri-3 Pri-2
Owner: petermayo@chromium.org
(oops, adding the correct petermayo@chromium.org).
Owner: flackr@chromium.org
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 :-).
Project Member

Comment 15 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)

Sign in to add a comment