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

Issue 707217 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Code cleanup: simplify source_index in property_tree_builder.cc

Project Member Reported by smcgruer@chromium.org, Mar 31 2017

Issue description

Doing some code spelunging with weiliangc@ and vollick@, we believe we discovered that the code for source_index in property_tree_builder.cc is more complicated than is necessary.

The current calculation is as follows:

  LayerType* transform_parent = GetTransformParent(data_from_ancestor, layer);
  DCHECK(is_root || transform_parent);

  int parent_index = TransformTree::kRootNodeId;
  if (transform_parent)
    parent_index = transform_parent->transform_tree_index();

  int source_index = parent_index

  gfx::Vector2dF source_offset;
  if (transform_parent) {
    if (ScrollParent(layer)) {
      LayerType* source = Parent(layer);
      source_offset += source->offset_to_transform_parent();
      source_index = source->transform_tree_index();
    } else if (!is_fixed) {
      source_offset = transform_parent->offset_to_transform_parent();
    } else {
      source_offset = data_from_ancestor.transform_tree_parent
                          ->offset_to_transform_parent();
      source_index =
          data_from_ancestor.transform_tree_parent->transform_tree_index();
    }
  }

Where GetTransformParent expands to:

    return PositionConstraint(layer).is_fixed_position()
               ? data.transform_fixed_parent
               : data.transform_tree_parent;

So, there are four paths to take here (each assuming that the above are false).

1. transform_parent is nullptr. Here, source_index = parent_index = TransformTree::kRootNodeId.

2. The layer has a ScrollParent. Here, source_index = Parent(layer)->transform_tree_index();

3. The layer is not fixed. Here, source_index = parent_index = GetTransformParent(data_from_ancestor, layer)->transform_tree_index() = data_from_ancestor.transform_tree_parent->transform_tree_index();

4. The layer is fixed. Here, source_index = data_from_ancestor.transform_tree_parent->transform_tree_index();

Our belief is that cases #2-4 can be simplified to source_index = Parent(layer)->transform_tree_index(), and that this is really what the poorly documented 'source' represents (our 'source' layer in the layer tree).

To show this, let us look at what 'data_from_ancestor' is. It is part of the BuildPropertyTreesInternal recursion, and its name in the parent call is 'data_for_children'. And during the 'AddTransformNodeIfNeeded' for that parent call, whether or not we add a transform node for the particular layer, we have:

  data_for_children->transform_tree_parent = layer;

So, data_from_ancestor.transform_tree_parent is always the Layer that precedes us in the BuildPropertyTreesInternal. But is that always our Parent(layer)? Nope! However, it is always either our Parent(layer) or our ScrollParent(layer), and in the latter case we already override source_index to be Parent(layer)->transform_tree_index()!

Hopefully that makes sense.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/beeaae78ce679a18605c63591ec6532e304b3d9e

commit beeaae78ce679a18605c63591ec6532e304b3d9e
Author: smcgruer <smcgruer@chromium.org>
Date: Tue Apr 04 13:28:53 2017

Simplify source_index and source_offset calculation

The previous calculation is equivalent to looking up Parent(layer) for
each branch and variable.

BUG= 707217 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2787843004
Cr-Commit-Position: refs/heads/master@{#461694}

[modify] https://crrev.com/beeaae78ce679a18605c63591ec6532e304b3d9e/cc/trees/property_tree_builder.cc

Status: Fixed (was: Started)

Sign in to add a comment