Document some transform node animation fields.

cc/trees/transform_node.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CC_TREES_TRANSFORM_NODE_H_
 #define CC_TREES_TRANSFORM_NODE_H_
 
 #include "cc/cc_export.h"
 #include "ui/gfx/geometry/point_f.h"
 #include "ui/gfx/geometry/scroll_offset.h"
@@ skipping 51 matching lines @@
   int source_node_id;
 
   // This id determines which 3d rendering context the node is in. 0 is a
   // special value and indicates that the node is not in any 3d rendering
   // context.
   int sorting_context_id;
 
   // TODO(vollick): will be moved when accelerated effects are implemented.
   bool needs_local_transform_update : 1;
 
+  // Whether this node or any ancestor has a potentially running
+  // (i.e., irrespective of exact timeline) transform animation or an
+  // invertible transform. Defaults to true. See also

[chrishtr, 2017/05/13 01:07:18]

Defaults to true is I think a bit misleading here, and below.
It's defaulted to true in the constructor, but
PropertyTreeBuilder will only leave it true under some conditions.

I think all of these should be defaulted to false, or else they
are inconsistent with each other. I recommend doing it in this
patch.

[wkorman, 2017/05/15 19:48:27]

Brief update -- working on this since this morning. Changing these defaults not
too surprisingly breaks a batch of tests including failure mode in form of
infinite hang. Exploring currently.

I had assumed these defaults (what's in ctor vs what builder or transform node
helper does later) were sort of "the initial value that one would expect to be
most common, or intuitive, for the member data semantic meaning in question."
However per feedback on this change it's obviously not really the case, since
for example has_only_translation_animations is initialized to true but in common
case we have no animations.

So I think this grew organically to have whatever values were convenient perhaps
even for unit tests.

I'm following your instruction re: making them all default to false, but I think
a case could be made that having them default to the "common/expected value" may
make as much or more sense. Avoid setting again when unneeded, etc.

Next bit was written before enne@ feedback but leaving for posterity: Also on
reflection, am not sure stating the default in comment is useful, so removed
that bit but open to thought. If we want to say something perhaps we're better
off summarizing what logic property tree or builder applies to compute, and
perhaps when in overview of frame lifecycle.

+  // TransformTree::UpdateNodeAndAncestorsAreAnimatedOrInvertible.
   bool node_and_ancestors_are_animated_or_invertible : 1;

[pdr., 2017/05/13 02:25:33]

This might be cleaner if we just tracked ancestor_has_potential_animation and
ancestors_are_invertible separately. Anyway, not for this patch.

[enne (OOO), 2017/05/15 17:14:51]

Agreed on both counts.

[wkorman, 2017/05/15 19:48:27]

Acknowledged.

[wkorman, 2017/05/15 23:53:17]

Current logic in TransformTree::UpdateNodeAndAncestorsAreAnimatedOrInvertible
relies on node_and_ancestors_are_animated_or_invertible being initialized to
true. I spent time but am not yet sure what changes are needed to fix.
Potentially revising TransformTree::SetRootTransformsAndScales but I made a
naive initial effort there.

At this point I've more time into this than I think warranted currently,
especially since from above thread it sounds like we'd prefer to track these
separately. But then on that front, don't we already have cached values for
is_invertible and has_potential_animation, is_currently_animating, etc.?
Something in logic must be different re: computing this value.

Tests involved in the parent node node_and_ancestors_are_animated_or_invertible
check are for the record:

  LayerTreeHostCommonTest.LayerSkippingInSubtreeOfSingularTransform
  LayerTreeHostCommonTest.SkippingLayerImpl

I consulted with Jayadev on chat re: the above.

Suggest pursuing this in person together at some point. It's not clear to me
that just having them all default to false, but then carefully having logic to
identify when we're constructing/initializing for first time in property tree or
elsewhere, is better than having the mixed false/true values we have here today.

[jaydasika, 2017/05/15 23:53:54]

We already have the separate bools you have mentioned
(to_screen_is_potentially_animated and ancestors_are_invertible). The reason you
still need this bool also is to distinguish the two cases:
1. T1(singular)->T2(animating) 
2. T1(singular and animating)->T2.

Notice that in both these cases, for T2 : ancestors_are_invertible = false and
ancestors_are_animating = true.
But we want ancestors_are_invertible_or_animating to be false for case 1 and
true for case 2.
So, you can't infer this bool from the other 2 bools.

[wkorman, 2017/05/16 00:28:01]

Is the below equivalent?

  T2.node_and_ancestors_are_animated_or_invertible =
    (T1.to_screen_is_potentially_animated &&
     T2.ancestors_are_invertible)

If so, do we not rely on above just because we want to avoid having to reference
T1 when we are looking for the value for T2?

[jaydasika, 2017/05/16 01:48:02]

It's not equivalent. For case 2 in my example, if T0(not animating, invertible)
is parent of T1:

  T1.node_and_ancestors_are_animated_or_invertible should be true, but
  (T0.to_screen_is_potentially_animated &&
     T1.ancestors_are_invertible) = false 

 
   bool is_invertible : 1;
+  // Whether the transform from this node to the screen is
+  // invertible. Defaults to true. See also
+  // TransformTree::UpdateScreenSpaceTransform.
   bool ancestors_are_invertible : 1;
 
+  // Whether this node has a potentially running (i.e., irrespective
+  // of exact timeline) transform animation. Defaults to false.

[chrishtr, 2017/05/13 01:07:18]

Defaults to false, vs true above for
node_and_ancestors_are_animated_or_invertible, is pretty weird..

[wkorman, 2017/05/15 19:48:27]

Acknowledged.

   bool has_potential_animation : 1;
+  // Whether this node has a currently running transform
+  // animation. Defaults to false.

[enne (OOO), 2017/05/15 17:14:51]

I'm not sure I really like all this "defaults to false".  Can we initialize
these members in the header and let the code be the documentation?

[wkorman, 2017/05/15 19:48:27]

Removed all notes about defaults in these comments.

It doesn't look like a bit-field member can have an in-class initializer:

In file included from ../../cc/trees/transform_node.cc:9:
../../cc/trees/transform_node.h:75:58: error: bit-field member cannot have an
in-class initializer
  bool node_and_ancestors_are_animated_or_invertible : 1 = false;

More here:

http://stackoverflow.com/questions/16520701/bit-fields-in-class-initializatio...

   bool is_currently_animating : 1;
+  // Whether this node *or an ancestor* has a potentially running
+  // (i.e., irrespective of exact timeline) transform
+  // animation. Defaults to false. See also
+  // TransformTree::UpdateAnimationProperties.
   bool to_screen_is_potentially_animated : 1;
+  // Whether all animations on this transform node are simple
+  // translations. Defaults to true. Used when computing animation

[enne (OOO), 2017/05/15 17:14:51]

I'm not sure that explaining where this is used adds a ton here.  I think
knowing that a node has only translations allows for simplified decomposition of
the transform matrix, but that's also kind of implicit.

[wkorman, 2017/05/15 19:48:27]

Removed this and similar above.

+  // scale factor for caching. See also
+  // PropertyTrees::GetAnimationScales.
   bool has_only_translation_animations : 1;
 
   // Flattening, when needed, is only applied to a node's inherited transform,
   // never to its local transform.
   bool flattens_inherited_transform : 1;
 
   // This is true if the to_parent transform at every node on the path to the
   // root is flat.
   bool node_and_ancestors_are_flat : 1;
 
@@ skipping 60 matching lines @@
   gfx::Transform to_screen;
 
   bool is_showing_backface : 1;
 
   bool operator==(const TransformCachedNodeData& other) const;
 };
 
 }  // namespace cc
 
 #endif  // CC_TREES_TRANSFORM_NODE_H_