Replace layer id with Element id for tracking scrollbar animation controllers

cc/layers/layer.cc

Index: cc/layers/layer.cc
diff --git a/cc/layers/layer.cc b/cc/layers/layer.cc
index 72a530a3bf3f15434f20742c1e569175560f6963..64db1ecda23d7a5d9c67232e5af7448c1fc6d275 100644
--- a/cc/layers/layer.cc
+++ b/cc/layers/layer.cc
@@ -1129,6 +1129,7 @@ void Layer::PushPropertiesTo(LayerImpl* layer) {
   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
@@ -1169,7 +1170,6 @@ void Layer::PushPropertiesTo(LayerImpl* layer) {
   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