Replace layer id with Element id for tracking scrollbar animation controllers

cc/layers/layer.cc

 // Copyright 2010 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.
 
 #include "cc/layers/layer.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 1111 matching lines @@
 
 void Layer::PushPropertiesTo(LayerImpl* layer) {
   TRACE_EVENT0("cc", "Layer::PushPropertiesTo");
   DCHECK(layer_tree_host_);
 
   // If we did not SavePaintProperties() for the layer this frame, then push the
   // real property values, not the paint property values.
   bool use_paint_properties = paint_properties_.source_frame_number ==
                               layer_tree_host_->SourceFrameNumber();
 
+  layer->SetElementId(inputs_.element_id);

[pdr., 2017/04/15 06:02:00]

LayerTreeHostImplTest.ScrollbarVisibilityChangeCausesRedrawAndCommit failed
because of this. The issue was that layer->SetScrollClipLayer depends on the
element id being set, but SetElementId was after SetScrollClipLayer. This was
flaky but would pass on retry because of sync timing, but luckily
linux_chromium_tsan_rel_ng failed reliably.

[enne (OOO), 2017/04/18 00:47:57]

Good catch! That sounds like a terrible thing to have to track down!


Maybe I'm just unfamiliar with this code but "element id" doesn't mean very much
to me.  I'm honestly surprised a layer could have a different element id over
time.  Is there some documentation for what this means?  Sadly,
Layer::SetElementId has no comments either.

If ElementId is related to scrolling, should SetScrollClipLayer become
SetScrollInfo(element id, clip layer id?) so they can't be called in the
incorrect order?  Or should SetElementId do more work to fix up whatever
SetScrollClipLayer did so that they can be called in either order?

[pdr., 2017/04/18 04:34:11]

We do have a big comment in element_id.h that is... very out of date. In the
latest patch I've updated it to describe the two usecases for element ids
(stable ids for scrolling and animations) and a little more about the overall
design. Walter, could you please take a look at the new comment too?
Good catch.

I looked around and I don't think element ids need to change (now, or in the
future). Refactoring layers to take an element id parameter does not look
trivial though. This patch does introduce a new ordering dependency, so we
should enforce that better. I need to think of a better way to handle this..

   layer->SetBackgroundColor(inputs_.background_color);
   layer->SetSafeOpaqueBackgroundColor(safe_opaque_background_color_);
   layer->SetBounds(use_paint_properties ? paint_properties_.bounds
                                         : inputs_.bounds);
 
 #if defined(NDEBUG)
   if (frame_viewer_instrumentation::IsTracingLayerTreeSnapshots())
     layer->SetDebugInfo(TakeDebugInfo());
 #else
   layer->SetDebugInfo(TakeDebugInfo());
@@ skipping 20 matching lines @@
   layer->set_should_flatten_transform_from_property_tree(
       should_flatten_transform_from_property_tree_);
   layer->SetUseParentBackfaceVisibility(inputs_.use_parent_backface_visibility);
   layer->SetUseLocalTransformForBackfaceVisibility(
       use_local_transform_for_backface_visibility_);
   layer->SetShouldCheckBackfaceVisibility(should_check_backface_visibility_);
 
   layer->SetScrollClipLayer(inputs_.scroll_clip_layer_id);
   layer->set_user_scrollable_horizontal(inputs_.user_scrollable_horizontal);
   layer->set_user_scrollable_vertical(inputs_.user_scrollable_vertical);
-  layer->SetElementId(inputs_.element_id);
   layer->SetMutableProperties(inputs_.mutable_properties);
 
   // When a scroll offset animation is interrupted the new scroll position on
   // the pending tree will clobber any impl-side scrolling occuring on the
   // active tree. To do so, avoid scrolling the pending tree along with it
   // instead of trying to undo that scrolling later.
   if (ScrollOffsetAnimationWasInterrupted())
     layer->layer_tree_impl()
         ->property_trees()
         ->scroll_tree.SetScrollOffsetClobberActiveValue(layer->id());
@@ skipping 259 matching lines @@
       ->num_copy_requests_in_subtree;
 }
 
 gfx::Transform Layer::ScreenSpaceTransform() const {
   DCHECK_NE(transform_tree_index_, TransformTree::kInvalidNodeId);
   return draw_property_utils::ScreenSpaceTransform(
       this, layer_tree_host_->property_trees()->transform_tree);
 }
 
 }  // namespace cc