Use element id's for animations

Use element id's for animations.

The goal of this CL is to migrate cc's animation code
away from layer id's in anticipation of SPv2. The ids
chosen are a minor tweak on DOMNodeIds for blink.

Other clients of cc may essentially continue using
their respective layer id's as identifiers. cc itself
remains ignorant of the semantic meaning of these ids,
treating them as an opaque identifier.

This change is extremely similar to loyso@'s earlier
CL crrev.com/1944623002, the significant difference
being the scheme for choosing the ids.

Notable changes in this CL
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link highlight layers).
 * Removal of ElementLayers from cc

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/51659f1ad58fc058ebb95907734ce6b2c887898d
Committed: https://crrev.com/ef2ae922636be42ce063a534496f162c587d0e76
Cr-Original-Commit-Position: refs/heads/master@{#398366}
Cr-Commit-Position: refs/heads/master@{#402863}

[Ian Vollick, 2016-05-12 19:10:22]

Description was changed from

==========
Boring renames.

BUG=
==========

to

==========
Boring renames.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Ian Vollick, 2016-05-12 23:56:14]

Description was changed from

==========
Boring renames.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
NOT FOR REVIEW - sketch for id schemes

 * s/element id/animation target id/
 * for blink
   animation target id = (sub element << 32) | dom node id.
 * no need to connect animations and layers explicitly
   * we will instead push ids to cc via CLM
     * see updateAnimationTargetBlahBlah.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[loyso (OOO), 2016-05-13 05:44:25]

Simply speaking, this CL proposes to treat ElementId as:
struct AnimationTargetId {
  int layer_id;
  int user_data_bits;
};

which is similar to:
struct AnimationTargetId {
  Layer* layer;
  void* some_user_data;
};

How does it help in CompositorWorker reverse mapping?
I would propose to add unordered_map<ElementId, int /* DomNodeId */ > somewhere
in CompositorWorker root object to avoid influence on other subsystems.
I.e. that's how you associate an extra data member for a given object in
non-intrusive decoupled way.
Would be nice to treat our ids as a thread-safe handles (without interventions)
https://en.wikipedia.org/wiki/Handle_(computing)

[loyso (OOO), 2016-05-13 06:14:04]

Note that I want to rename ElementId into MutatorElementId. I would go with
AnimationTarget but it would require a renaming of cc::ElementAnimations into
TargetAnimations (?) 
cc::ElementAnimations is there for a long time.
Also, I want to make MutatorElementId a strongly typed.

[Ian Vollick, 2016-05-17 21:56:01]

Sorry for the slow reply, I was on vacation yesterday.

I should also mention that this is just an investigation into element ids. I
also want to look at associating animations more directly with property tree
nodes. The code is broken at the moment, so take it with a grain of salt.

On 2016/05/13 05:44:25, loyso wrote:
> Simply speaking, this CL proposes to treat ElementId as:
> struct AnimationTargetId {
>   int layer_id;
>   int user_data_bits;
> };
> 
> which is similar to:
> struct AnimationTargetId {
>   Layer* layer;
>   void* some_user_data;
> };
> 
> How does it help in CompositorWorker reverse mapping?
> I would propose to add unordered_map<ElementId, int /* DomNodeId */ >
somewhere
> in CompositorWorker root object to avoid influence on other subsystems.

How will you choose your ElementIds and how will this map be maintained? Will
you not have to vend these ids, store them somewhere, and plumb them to this
mapping?

My goal in this experiment is to try avoid creating a new id scheme. I wanted to
see how far I could get with DOMNodeIds. They almost work, except for the
special "sub elements" like link highlights. (other cc clients like ui can
continue to use layer ids).

My bit-twiddling is admittedly a little ugly, but I'd hoped it would minimize
impact on other subsystems (I'd considered adding new animatable properties for
stuff like link-highlight-opacity, but I didn't want to infect cc with such
blink-isms).

As far as cc is concerned, the number is an opaque id. In fact, by
disambiguating the main/scroll ids, we remove the need for
LayerTreeImpl::ElementLayers -- i.e., the influence on the cc subsystem is a bit
reduced, actually.

Blink, on the other hand, only needs to think about DOMNodeIds, so no impact
there.

The hope is that the only code that would need to know about sub elements is the
code that plugs in the ids (via GL::deprecatedSetElementId and
CAP::attachElement) and the code that gathers the cc entities for compositor
worker (CompositorMutableStateProvider). The "glue" code between blink/cc,
basically.

> I.e. that's how you associate an extra data member for a given object in
> non-intrusive decoupled way.
> Would be nice to treat our ids as a thread-safe handles (without
interventions)
> https://en.wikipedia.org/wiki/Handle_(computing)

[Ian Vollick, 2016-06-01 18:07:35]

Description was changed from

==========
NOT FOR REVIEW - sketch for id schemes

 * s/element id/animation target id/
 * for blink
   animation target id = (sub element << 32) | dom node id.
 * no need to connect animations and layers explicitly
   * we will instead push ids to cc via CLM
     * see updateAnimationTargetBlahBlah.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use element id's for animations.

Notable changes
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Ian Vollick, 2016-06-01 18:07:36]

vollick@chromium.org changed reviewers:
+ ajuma@chromium.org, loyso@chromium.org

[Ian Vollick, 2016-06-01 18:08:25]

https://codereview.chromium.org/1973083002/diff/470001/cc/animation/element_id.h
File cc/animation/element_id.h (right):

https://codereview.chromium.org/1973083002/diff/470001/cc/animation/element_i...
cc/animation/element_id.h:33: struct CC_EXPORT ElementId {
Why a struct?
 1. Catch bugs. I started with a type alias, but I hit a large number of bugs
(eg, LayerById would accept an ElementId).
 2. Flexibility. The innards of this struct are the most interesting part of
this patch, and by encapsulating those details, it will be easier to change them
in future if we need to.

https://codereview.chromium.org/1973083002/diff/470001/cc/animation/element_i...
cc/animation/element_id.h:48: uint64_t value;
I went with a uint64_t here because I wanted cc to be wholly ignorant of the
meaning of this value. Although it is possible to hide the innards from cc code,
I wanted to avoid the concept from leaking into code. That said, there are other
options here.

 1. We could store two numbers here, an element id and a sub element id.
 2. We could teach DOMNodeIds to vend extra ids for non-DOM stuff. I don't
particularly like this approach because it doesn't allow us to link auxiliary
ids with an element. Specifically, it would preclude the removal of
ElementLayers, which I think is a nice feature of this patch.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:400:
compositorPlayer->attachElement(CompositorElementId(DOMNodeIds::idForNode(&element)));
By not pulling from cc, we are able to simplify this code considerably.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1639:
m_scrollingContentsLayer->setElementId(CompositorElementId(DOMNodeIds::idForNode(owningNode)
| kScrollSubElement));
NB: this is some of the glue code that knows about sub elements.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1052:
m_scrollLayer->setElementId(CompositorElementId(DOMNodeIds::idForNode(&m_layoutView.document())
| kScrollSubElement));
NB: here as well.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp
(right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp:29:
cc::LayerImpl* scrollLayer = m_state->LayerByElementId(cc::ElementId(element_id
| kScrollSubElement));
The mutable state provider knows about the sub elements in order to pull the
various compenents from cc.

[Ian Vollick, 2016-06-01 18:08:26]

https://codereview.chromium.org/1973083002/diff/470001/cc/animation/element_id.h
File cc/animation/element_id.h (right):

https://codereview.chromium.org/1973083002/diff/470001/cc/animation/element_i...
cc/animation/element_id.h:33: struct CC_EXPORT ElementId {
Why a struct?
 1. Catch bugs. I started with a type alias, but I hit a large number of bugs
(eg, LayerById would accept an ElementId).
 2. Flexibility. The innards of this struct are the most interesting part of
this patch, and by encapsulating those details, it will be easier to change them
in future if we need to.

https://codereview.chromium.org/1973083002/diff/470001/cc/animation/element_i...
cc/animation/element_id.h:48: uint64_t value;
I went with a uint64_t here because I wanted cc to be wholly ignorant of the
meaning of this value. Although it is possible to hide the innards from cc code,
I wanted to avoid the concept from leaking into code. That said, there are other
options here.

 1. We could store two numbers here, an element id and a sub element id.
 2. We could teach DOMNodeIds to vend extra ids for non-DOM stuff. I don't
particularly like this approach because it doesn't allow us to link auxiliary
ids with an element. Specifically, it would preclude the removal of
ElementLayers, which I think is a nice feature of this patch.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:400:
compositorPlayer->attachElement(CompositorElementId(DOMNodeIds::idForNode(&element)));
By not pulling from cc, we are able to simplify this code considerably.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1639:
m_scrollingContentsLayer->setElementId(CompositorElementId(DOMNodeIds::idForNode(owningNode)
| kScrollSubElement));
NB: this is some of the glue code that knows about sub elements.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
(right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:1052:
m_scrollLayer->setElementId(CompositorElementId(DOMNodeIds::idForNode(&m_layoutView.document())
| kScrollSubElement));
NB: here as well.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp
(right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateProvider.cpp:29:
cc::LayerImpl* scrollLayer = m_state->LayerByElementId(cc::ElementId(element_id
| kScrollSubElement));
The mutable state provider knows about the sub elements in order to pull the
various compenents from cc.

[ajuma, 2016-06-01 23:53:42]

I like this approach.

https://codereview.chromium.org/1973083002/diff/470001/cc/animation/element_id.h
File cc/animation/element_id.h (right):

https://codereview.chromium.org/1973083002/diff/470001/cc/animation/element_i...
cc/animation/element_id.h:48: uint64_t value;
On 2016/06/01 18:08:25, vollick wrote:
> I went with a uint64_t here because I wanted cc to be wholly ignorant of the
> meaning of this value. Although it is possible to hide the innards from cc
code,
> I wanted to avoid the concept from leaking into code. That said, there are
other
> options here.
> 
>  1. We could store two numbers here, an element id and a sub element id.
>  2. We could teach DOMNodeIds to vend extra ids for non-DOM stuff. I don't
> particularly like this approach because it doesn't allow us to link auxiliary
> ids with an element. Specifically, it would preclude the removal of
> ElementLayers, which I think is a nice feature of this patch.


(1) has the nice feature that you avoid bitwise OR-ing when calling setElementId
in blink, but maybe one way to accomplish the same goal is to add a helper
somewhere in blink (in CompositorElementId.h?) that takes the main id and
sub-ids and turns them into an actual id (and then the OR-ing is an
implementation detail of that helper rather than exposed everywhere)?

https://codereview.chromium.org/1973083002/diff/470001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1973083002/diff/470001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:969: }
The property trees pushed from the main thread will already have the new element
id, so I don't think we need to update property trees here.

https://codereview.chromium.org/1973083002/diff/470001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1973083002/diff/470001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:8091: SetElementIdsForTesting();
This adds ids to the the LTHI's active tree, right? This test only has a
main-thread tree and sets ids for it above, so is this call needed?

https://codereview.chromium.org/1973083002/diff/470001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:8634: SetElementIdsForTesting();
Is this needed? (this test only has Layers, not LayerImpls)

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1537:
mainMutableProperties = (CompositorMutableProperty::kOpacity |
CompositorMutableProperty::kTransform) & compositorMutableProperties;
Nit: maybe call this something like primaryMutableProperties or
cssMutableProperties, to avoid making this sound like "properties mutable on the
main thread".

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1639:
m_scrollingContentsLayer->setElementId(CompositorElementId(DOMNodeIds::idForNode(owningNode)
| kScrollSubElement));
On 2016/06/01 18:08:26, vollick wrote:
> NB: this is some of the glue code that knows about sub elements.

I'm probably missing something obvious, but shouldn't kScrollSubElement be
shifted before OR-ing with the DOMNodeId?

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:14: static
const int kLinkHighlightSubElement = 3;
I think these should go into a sub-namespace (e.g. CompositorElementSubId).

[Ian Vollick, 2016-06-02 19:03:12]

https://codereview.chromium.org/1973083002/diff/470001/cc/animation/element_id.h
File cc/animation/element_id.h (right):

https://codereview.chromium.org/1973083002/diff/470001/cc/animation/element_i...
cc/animation/element_id.h:48: uint64_t value;
On 2016/06/01 at 23:53:42, ajuma wrote:
> On 2016/06/01 18:08:25, vollick wrote:
> > I went with a uint64_t here because I wanted cc to be wholly ignorant of the
> > meaning of this value. Although it is possible to hide the innards from cc
code,
> > I wanted to avoid the concept from leaking into code. That said, there are
other
> > options here.
> > 
> >  1. We could store two numbers here, an element id and a sub element id.
> >  2. We could teach DOMNodeIds to vend extra ids for non-DOM stuff. I don't
> > particularly like this approach because it doesn't allow us to link
auxiliary
> > ids with an element. Specifically, it would preclude the removal of
> > ElementLayers, which I think is a nice feature of this patch.
> 
> 
> (1) has the nice feature that you avoid bitwise OR-ing when calling
setElementId in blink, but maybe one way to accomplish the same goal is to add a
helper somewhere in blink (in CompositorElementId.h?) that takes the main id and
sub-ids and turns them into an actual id (and then the OR-ing is an
implementation detail of that helper rather than exposed everywhere)?

I've tried out the helper function in the latest patch set.

https://codereview.chromium.org/1973083002/diff/470001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1973083002/diff/470001/cc/layers/layer_impl.c...
cc/layers/layer_impl.cc:969: }
On 2016/06/01 23:53:42, ajuma wrote:
> The property trees pushed from the main thread will already have the new
element
> id, so I don't think we need to update property trees here.

You're right. The problem was that I'd called SetElementIdsForTesting after
building the property trees in layer_tree_host_impl_unittest.cc and this was
certainly the wrong fix. I've since updated the ordering and removed this code.

https://codereview.chromium.org/1973083002/diff/470001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/1973083002/diff/470001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:8091: SetElementIdsForTesting();
On 2016/06/01 23:53:42, ajuma wrote:
> This adds ids to the the LTHI's active tree, right? This test only has a
> main-thread tree and sets ids for it above, so is this call needed?

Nope, removed.

https://codereview.chromium.org/1973083002/diff/470001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_common_unittest.cc:8634: SetElementIdsForTesting();
On 2016/06/01 23:53:42, ajuma wrote:
> Is this needed? (this test only has Layers, not LayerImpls)

Whoops, removed. I clearly added the host()->SetElementIdsForTesting above and
failed to remove this.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1537:
mainMutableProperties = (CompositorMutableProperty::kOpacity |
CompositorMutableProperty::kTransform) & compositorMutableProperties;
On 2016/06/01 23:53:42, ajuma wrote:
> Nit: maybe call this something like primaryMutableProperties or
> cssMutableProperties, to avoid making this sound like "properties mutable on
the
> main thread".

Done.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1639:
m_scrollingContentsLayer->setElementId(CompositorElementId(DOMNodeIds::idForNode(owningNode)
| kScrollSubElement));
On 2016/06/01 23:53:42, ajuma wrote:
> On 2016/06/01 18:08:26, vollick wrote:
> > NB: this is some of the glue code that knows about sub elements.
> 
> I'm probably missing something obvious, but shouldn't kScrollSubElement be
> shifted before OR-ing with the DOMNodeId?

It should indeed be shifted. I'd started implementing option 1 (storing multiple
values in ElementId) for a bit, but backtracked when I saw the shape of the code
in cc. When I did that, I failed to put the shifting back and I just happened to
get lucky with no collisions. I've fixed this (see CompositorElementId), and
added a test in CompositorMutableStateTest that checks for this sort of
collision in case I accidentally get rid of the shifting again.

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/1973083002/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:14: static
const int kLinkHighlightSubElement = 3;
On 2016/06/01 23:53:42, ajuma wrote:
> I think these should go into a sub-namespace (e.g. CompositorElementSubId).

Done.

[ajuma, 2016-06-02 19:37:27]

Thanks, lgtm

https://codereview.chromium.org/1973083002/diff/490001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1973083002/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1544:
m_graphicsLayer->setElementId(CompositorElementId(elementId));
Just for the sake of consistency, how about adding a
CompositorSubElementId::Value::Primary and calling createCompositorElementId
here rather than using elementId directly?

[Ian Vollick, 2016-06-02 19:46:22]

Description was changed from

==========
Use element id's for animations.

Notable changes
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use element id's for animations.

Notable changes
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Ian Vollick, 2016-06-02 19:46:22]

vollick@chromium.org changed reviewers:
+ jbroman@chromium.org

[Ian Vollick, 2016-06-02 19:48:05]

vollick@chromium.org changed reviewers:
+ haraken@chromium.org
- jbroman@chromium.org

[Ian Vollick, 2016-06-02 19:49:02]

vollick@chromium.org changed reviewers:
+ rbyers@chromium.org
- haraken@chromium.org

[Ian Vollick, 2016-06-02 19:51:19]

vollick@chromium.org changed reviewers:
+ bokan@chromium.org

[Ian Vollick, 2016-06-02 19:56:30]

loyso, can you PTAL at this approach?

+rbyers for public/platform/
+bokan for Source/web/

cc'd jbroman and esprehn in case they're interested from an architecture POV.

https://codereview.chromium.org/1973083002/diff/490001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1973083002/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1544:
m_graphicsLayer->setElementId(CompositorElementId(elementId));
On 2016/06/02 19:37:26, ajuma wrote:
> Just for the sake of consistency, how about adding a
> CompositorSubElementId::Value::Primary and calling createCompositorElementId
> here rather than using elementId directly?

sg. Done.

[jbroman, 2016-06-02 20:20:18]

jbroman@chromium.org changed reviewers:
+ jbroman@chromium.org

[jbroman, 2016-06-02 20:20:22]

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
File cc/animation/element_id.cc (right):

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
cc/animation/element_id.cc:16: bool CC_EXPORT operator==(uint64_t o, const
ElementId& e) {
What's going on here? CC_EXPORT is only meaningful if it can actually reach
another translation unit (which it can't -- this isn't a header). Do these have
any call sites, in any event?

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
cc/animation/element_id.cc:29: res->SetInteger("element_id",
base::saturated_cast<int>(value));
saturated_cast is wrong here: anything with a non-zero sub-element ID will
become INT_MAX (if, as is usual, int is int32_t).

You seem to be assuming here that int and int32_t are the same type; why not say
int32_t here if you actually want 32-bit parts? Also, a saturated_cast 

Also, does the compositor make any assumptions about the structure of this
integer, aside from this for tracing? Are all cc clients expected to obey this
32/32 split?

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_id.h
File cc/animation/element_id.h (right):

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
cc/animation/element_id.h:33: struct CC_EXPORT ElementId {
would you mind a comment describing what sort of element a cc::ElementId
identifies? How do they correspond to layers? Is cc responsible for issuing
them, or the client?

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
cc/animation/element_id.h:51: ElementId CC_EXPORT
LayerIdToElementIdForTesting(int layer_id);
style: please put CC_EXPORT before the return type in function declarations

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp
(right):

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:11: uint64_t
value = domNodeId;
I guess we're assuming here that domNodeId < (1 << 32UL)? Otherwise the failure
mode here seems pretty confusing.

If you can assume this statically (i.e. if we have no platforms where
sizeof(int) > sizeof(int32_t)), a static_assert here would be great in
explaining on what basis we assume this to be safe, i.e.

static_assert(sizeof(domNodeId) <= 4, "DOM node IDs are assumed not to be larger
than 32 bits");

If that's not the case, a comment and/or DCHECK would be appreciated.

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:13: struct
PLATFORM_EXPORT CompositorSubElementId {
PLATFORM_EXPORT does nothing here

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:14: enum class
Value {
What's the point of an enum class nested inside a struct? This means that a
value must always have two scope resolution operators (::), like
CompositorSubElementId::Value::Primary. It looks like this enum class should
just live in the blink namespace, and the surrounding struct can go away, no?

[jbroman, 2016-06-02 20:21:14]

That was a little more of a review than I intended, but since I had to read the
CL anyways...

At a high level, this seems reasonable, but I'm still not entirely clear on what
the term "element" means to cc.

[Rick Byers, 2016-06-02 20:31:15]

On 2016/06/02 19:56:30, vollick wrote:
> loyso, can you PTAL at this approach?
> 
> +rbyers for public/platform/

public/platform LGTM

> +bokan for Source/web/
> 
> cc'd jbroman and esprehn in case they're interested from an architecture POV.
> 
>
https://codereview.chromium.org/1973083002/diff/490001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> (right):
> 
>
https://codereview.chromium.org/1973083002/diff/490001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1544:
> m_graphicsLayer->setElementId(CompositorElementId(elementId));
> On 2016/06/02 19:37:26, ajuma wrote:
> > Just for the sake of consistency, how about adding a
> > CompositorSubElementId::Value::Primary and calling createCompositorElementId
> > here rather than using elementId directly?
> 
> sg. Done.

[esprehn, 2016-06-03 02:17:28]

Can you write a better change description? :) Neither this, or the bug, have
much detail at all about what you're doing here and this patch is huge.

[loyso (OOO), 2016-06-03 02:32:07]

Well, it resembles my previous approaches:
1)
crrev.com/1922393002 CC Animation: Unify ElementId in Blink Compositor Worker.
crrev.com/1921503008 Blink CompositorWorker: Use CompositorElementId and
CompositorIdToElementMap.
crrev.com/1944623002 CC Animation: Use ElementId to attach CC animation players.
2)
crrev.com/1968633003 CC Animation: Let animations use their own target ids.

except of that struct with two fields (a tagged element id). Semantically they
all the same, except some readability differences.

[loyso (OOO), 2016-06-03 02:45:34]

Overall approach looks good!

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_id.h
File cc/animation/element_id.h (right):

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
cc/animation/element_id.h:37: inline bool operator==(const ElementId& o) const {
return o.value == value; }
no need to write |inline| here.

[bokan, 2016-06-03 15:09:21]

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/LinkHighlightImpl.cpp (right):

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/LinkHighlightImpl.cpp:98:
m_compositorPlayer->attachElement(CompositorElementId(elementId));
Can't you just pass elementId here (and below) without constructing a new
CompositorElementId?

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (left):

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:180: EXPECT_EQ(0UL,
scrollLayer->elementId());
Has the logic here changed somehow? i.e. why aren't these now just EXPECT_FALSE?

[Ian Vollick, 2016-06-03 18:56:28]

Description was changed from

==========
Use element id's for animations.

Notable changes
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use element id's for animations.

The goal of this CL is to migrate cc's animation code
away from layer id's in anticipation of SPv2. The ids
chosen are a minor tweak on DOMNodeIds for blink.

Other clients of cc may essentially continue using
their respective layer id's as identifiers. cc itself
remains ignorant of the semantic meaning of these ids,
treating them as an opaque identifier.

This change is extremely similar to loyso@'s earlier
CL crrev.com/1944623002, the significant difference
being the scheme for choosing the ids.

Notable changes in this CL
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).
 * Removal of ElementLayers from cc

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Ian Vollick, 2016-06-03 19:31:21]

> Can you write a better change description? :) Neither this, or the bug, have
> much detail at all about what you're doing here and this patch is huge.

You're right -- it's mostly plumbing, but it is massive. I've updated the
description.

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
File cc/animation/element_id.cc (right):

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
cc/animation/element_id.cc:16: bool CC_EXPORT operator==(uint64_t o, const
ElementId& e) {
On 2016/06/02 20:20:21, jbroman wrote:
> What's going on here? CC_EXPORT is only meaningful if it can actually reach
> another translation unit (which it can't -- this isn't a header). Do these
have
> any call sites, in any event?

This copy and paste error was supposed to have been removed but got missed.
Removed.

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
cc/animation/element_id.cc:29: res->SetInteger("element_id",
base::saturated_cast<int>(value));
On 2016/06/02 20:20:21, jbroman wrote:
> saturated_cast is wrong here: anything with a non-zero sub-element ID will
> become INT_MAX (if, as is usual, int is int32_t).
> 
> You seem to be assuming here that int and int32_t are the same type; why not
say
> int32_t here if you actually want 32-bit parts? Also, a saturated_cast 
> 
> Also, does the compositor make any assumptions about the structure of this
> integer, aside from this for tracing? Are all cc clients expected to obey this
> 32/32 split?

Thank you for catching this. Since this approach is prone to bugs like these,
I've switched to storing two ints. I've named them primaryId and secondaryId to
keep them relatively opaque.

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_id.h
File cc/animation/element_id.h (right):

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
cc/animation/element_id.h:33: struct CC_EXPORT ElementId {
On 2016/06/02 20:20:21, jbroman wrote:
> would you mind a comment describing what sort of element a cc::ElementId
> identifies? How do they correspond to layers? Is cc responsible for issuing
> them, or the client?

Done.

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
cc/animation/element_id.h:37: inline bool operator==(const ElementId& o) const {
return o.value == value; }
On 2016/06/03 02:45:34, loyso wrote:
> no need to write |inline| here.

Done.

https://codereview.chromium.org/1973083002/diff/510001/cc/animation/element_i...
cc/animation/element_id.h:51: ElementId CC_EXPORT
LayerIdToElementIdForTesting(int layer_id);
On 2016/06/02 20:20:21, jbroman wrote:
> style: please put CC_EXPORT before the return type in function declarations

Done.

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp
(right):

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.cpp:11: uint64_t
value = domNodeId;
On 2016/06/02 20:20:21, jbroman wrote:
> I guess we're assuming here that domNodeId < (1 << 32UL)? Otherwise the
failure
> mode here seems pretty confusing.
> 
> If you can assume this statically (i.e. if we have no platforms where
> sizeof(int) > sizeof(int32_t)), a static_assert here would be great in
> explaining on what basis we assume this to be safe, i.e.
> 
> static_assert(sizeof(domNodeId) <= 4, "DOM node IDs are assumed not to be
larger
> than 32 bits");
> 
> If that's not the case, a comment and/or DCHECK would be appreciated.

This is indeed dodgy. I've changed the innards of ElementIds to be two ints to
avoid problems like this.

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:13: struct
PLATFORM_EXPORT CompositorSubElementId {
On 2016/06/02 20:20:21, jbroman wrote:
> PLATFORM_EXPORT does nothing here

Removed.

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:14: enum class
Value {
On 2016/06/02 20:20:21, jbroman wrote:
> What's the point of an enum class nested inside a struct? This means that a
> value must always have two scope resolution operators (::), like
> CompositorSubElementId::Value::Primary. It looks like this enum class should
> just live in the blink namespace, and the surrounding struct can go away, no?

Yes, this is much better. Done.

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/LinkHighlightImpl.cpp (right):

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/LinkHighlightImpl.cpp:98:
m_compositorPlayer->attachElement(CompositorElementId(elementId));
On 2016/06/03 15:09:21, bokan wrote:
> Can't you just pass elementId here (and below) without constructing a new
> CompositorElementId?

Indeed I can :) I think what happened here was that I used to be creating a
uint64_t which I then passed to the CompositorElementId ctor. This is silly now
and I've done what you suggested.

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp (left):

https://codereview.chromium.org/1973083002/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp:180: EXPECT_EQ(0UL,
scrollLayer->elementId());
On 2016/06/03 15:09:21, bokan wrote:
> Has the logic here changed somehow? i.e. why aren't these now just
EXPECT_FALSE?

Good question. The logic has changed, yes, and I've added a comment to this test
explaining why ElementId no longer implies has-a-compositor-proxy.

[bokan, 2016-06-03 19:34:32]

thanks. Source/web lgtm

[Ian Vollick, 2016-06-03 19:57:41]

On 2016/06/03 19:34:32, bokan wrote:
> thanks. Source/web lgtm

Unsurprisingly, big patch needs a big rebase. Working on it :)

[Ian Vollick, 2016-06-06 13:33:51]

On 2016/06/03 19:57:41, vollick wrote:
> On 2016/06/03 19:34:32, bokan wrote:
> > thanks. Source/web lgtm
> 
> Unsurprisingly, big patch needs a big rebase. Working on it :)

Rebase is done. PTAL, jbroman.

[jbroman, 2016-06-06 14:23:20]

Seems reasonable to me. (I haven't inspected all of the details, but much of it
seems fairly mechanical.)

https://codereview.chromium.org/1973083002/diff/570001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/1973083002/diff/570001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:14: Primary =
1,
nit: Any reason to assign these explicit values, instead of letting the compiler
assign them? Letting the compiler do it makes the diff smaller if one is ever
removed.

https://codereview.chromium.org/1973083002/diff/570001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp
(right):

https://codereview.chromium.org/1973083002/diff/570001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp:89:
// If we happen to do bit shifting in CompositorElementId, try to catch
Is this test still meaningful with the current pair-of-ints implementation?

[Ian Vollick, 2016-06-06 16:31:30]

https://codereview.chromium.org/1973083002/diff/570001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorElementId.h (right):

https://codereview.chromium.org/1973083002/diff/570001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorElementId.h:14: Primary =
1,
On 2016/06/06 14:23:20, jbroman wrote:
> nit: Any reason to assign these explicit values, instead of letting the
compiler
> assign them? Letting the compiler do it makes the diff smaller if one is ever
> removed.

No reason. Removed.

https://codereview.chromium.org/1973083002/diff/570001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp
(right):

https://codereview.chromium.org/1973083002/diff/570001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp:89:
// If we happen to do bit shifting in CompositorElementId, try to catch
On 2016/06/06 14:23:20, jbroman wrote:
> Is this test still meaningful with the current pair-of-ints implementation?

Not really. I'd put it in to catch a bug I'd caused earlier, but given that I've
switched to a pair of ints, it's so unlikely to repeat that I think this test is
just dead weight. Removed.

[Ian Vollick, 2016-06-07 00:51:33]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2016-06-07 00:51:34]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
rbyers@chromium.org, bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/1973083002/#ps590001
(title: "address reviewer feedback")

[commit-bot: I haz the power, 2016-06-07 00:52:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973083002/590001

[Ian Vollick, 2016-06-07 00:53:04]

The CQ bit was unchecked by vollick@chromium.org

[Ian Vollick, 2016-06-07 12:20:23]

On 2016/06/07 00:52:22, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1973083002/590001

ajuma, this has changed a fair bit since you looked at it last. Could you PTAL
when you have a chance?

[ajuma, 2016-06-07 13:44:01]

https://codereview.chromium.org/1973083002/diff/590001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1973083002/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1545:
m_graphicsLayer->setElementId(createCompositorElementId(elementId,
CompositorSubElementId::Primary));
This is implicitly relying on Primary having value 0, so layers that aren't
animated and don't have a compositor proxy get an element id of (0, 0) (and
don't end up getting added to the element-id-to-layer map in cc). I think it'd
be safer to check if elementId is still 0 here, and if so set element id to a
default-constructed CompositorElementId().

https://codereview.chromium.org/1973083002/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1551:
m_scrollingContentsLayer->setCompositorMutableProperties(scrollMutableProperties);
How does this relate to the logic added to ::updateScrollingLayers below? It
looks like scrollMutableProperties has value kNone here (unless the layer has a
compositor proxy). Should this really be kScrollLeft | kScrollTop? (Or can we
rely on the logic below to set up scroll layers correctly, and in that case do
we need the logic here at all?)

[Ian Vollick, 2016-06-07 14:30:08]

https://codereview.chromium.org/1973083002/diff/590001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/1973083002/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1545:
m_graphicsLayer->setElementId(createCompositorElementId(elementId,
CompositorSubElementId::Primary));
On 2016/06/07 13:44:01, ajuma wrote:
> This is implicitly relying on Primary having value 0, so layers that aren't
> animated and don't have a compositor proxy get an element id of (0, 0) (and
> don't end up getting added to the element-id-to-layer map in cc). I think it'd
> be safer to check if elementId is still 0 here, and if so set element id to a
> default-constructed CompositorElementId().

This is perhaps too subtle, but operator bool for ElementId's only depends on
the primaryId. As long as elementId is zero, I don't think we'll be adding it to
the element id to layer map. I think what you suggest is a great idea, though,
and I've updated the code, just to be safe.

https://codereview.chromium.org/1973083002/diff/590001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1551:
m_scrollingContentsLayer->setCompositorMutableProperties(scrollMutableProperties);
On 2016/06/07 13:44:01, ajuma wrote:
> How does this relate to the logic added to ::updateScrollingLayers below? It
> looks like scrollMutableProperties has value kNone here (unless the layer has
a
> compositor proxy). Should this really be kScrollLeft | kScrollTop? (Or can we
> rely on the logic below to set up scroll layers correctly, and in that case do
> we need the logic here at all?)

Good question.

 * element id: if you have a compositor proxy, an animation, or you're a
scroller (and might impl animate).
 * mutable properties: if you have a compositor proxy.

So I've set up the element id for the scroll layers when they're constructed,
but I conditionally set up the element id for the primary layer as well as the
mutable properties for all layers based on the rules above.

I've added more detailed comments about this.

[ajuma, 2016-06-07 14:39:33]

Thanks for the explanations! lgtm

[Ian Vollick, 2016-06-07 16:55:22]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2016-06-07 16:55:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
rbyers@chromium.org, ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/1973083002/#ps650001
(title: ".")

[commit-bot: I haz the power, 2016-06-07 16:56:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973083002/650001

[commit-bot: I haz the power, 2016-06-07 17:57:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-07 17:57:33]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ian Vollick, 2016-06-07 18:22:02]

The CQ bit was checked by vollick@chromium.org

[commit-bot: I haz the power, 2016-06-07 18:22:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1973083002/650001

[commit-bot: I haz the power, 2016-06-07 20:05:34]

Description was changed from

==========
Use element id's for animations.

The goal of this CL is to migrate cc's animation code
away from layer id's in anticipation of SPv2. The ids
chosen are a minor tweak on DOMNodeIds for blink.

Other clients of cc may essentially continue using
their respective layer id's as identifiers. cc itself
remains ignorant of the semantic meaning of these ids,
treating them as an opaque identifier.

This change is extremely similar to loyso@'s earlier
CL crrev.com/1944623002, the significant difference
being the scheme for choosing the ids.

Notable changes in this CL
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).
 * Removal of ElementLayers from cc

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use element id's for animations.

The goal of this CL is to migrate cc's animation code
away from layer id's in anticipation of SPv2. The ids
chosen are a minor tweak on DOMNodeIds for blink.

Other clients of cc may essentially continue using
their respective layer id's as identifiers. cc itself
remains ignorant of the semantic meaning of these ids,
treating them as an opaque identifier.

This change is extremely similar to loyso@'s earlier
CL crrev.com/1944623002, the significant difference
being the scheme for choosing the ids.

Notable changes in this CL
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).
 * Removal of ElementLayers from cc

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-07 20:05:37]

Committed patchset #34 (id:650001)

[commit-bot: I haz the power, 2016-06-07 20:07:06]

Description was changed from

==========
Use element id's for animations.

The goal of this CL is to migrate cc's animation code
away from layer id's in anticipation of SPv2. The ids
chosen are a minor tweak on DOMNodeIds for blink.

Other clients of cc may essentially continue using
their respective layer id's as identifiers. cc itself
remains ignorant of the semantic meaning of these ids,
treating them as an opaque identifier.

This change is extremely similar to loyso@'s earlier
CL crrev.com/1944623002, the significant difference
being the scheme for choosing the ids.

Notable changes in this CL
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).
 * Removal of ElementLayers from cc

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use element id's for animations.

The goal of this CL is to migrate cc's animation code
away from layer id's in anticipation of SPv2. The ids
chosen are a minor tweak on DOMNodeIds for blink.

Other clients of cc may essentially continue using
their respective layer id's as identifiers. cc itself
remains ignorant of the semantic meaning of these ids,
treating them as an opaque identifier.

This change is extremely similar to loyso@'s earlier
CL crrev.com/1944623002, the significant difference
being the scheme for choosing the ids.

Notable changes in this CL
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).
 * Removal of ElementLayers from cc

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/51659f1ad58fc058ebb95907734ce6b2c887898d
Cr-Commit-Position: refs/heads/master@{#398366}
==========

[commit-bot: I haz the power, 2016-06-07 20:07:10]

Patchset 34 (id:??) landed as
https://crrev.com/51659f1ad58fc058ebb95907734ce6b2c887898d
Cr-Commit-Position: refs/heads/master@{#398366}

[Ian Vollick, 2016-06-08 13:40:46]

A revert of this CL (patchset #34 id:650001) has been created in
https://codereview.chromium.org/2049063002/ by vollick@chromium.org.

The reason for reverting is: This caused:
https://bugs.chromium.org/p/chromium/issues/detail?id=618286.

[Ian Vollick, 2016-06-29 15:22:12]

I'm elated to say that I've now got a test for my fix. (See
virtual/threaded/animations/ changes). PTAL!

https://codereview.chromium.org/1973083002/diff/690001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1973083002/diff/690001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:849: for (; it !=
LayerListIterator<LayerImpl>(nullptr); ++it) {
This is another noteworthy change. Now that "call function for every layer"
won't work without a layer list, I can't depend on it for setting all the
element id's in unit tests prior to building the layer list.

What I've done instead is to iterate over the layers as we used to rather than
using the convenience function.

https://codereview.chromium.org/1973083002/diff/690001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1973083002/diff/690001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:380: void
CompositorAnimations::attachCompositedLayers(Element& element, const Animation&
animation)
This is the noteworthy behavioral change in this patch. I've replaced the
nullity checks that I removed when I'd originally landed this patch.

[Ian Vollick, 2016-06-29 15:43:01]

On 2016/06/29 at 15:22:12, vollick wrote:
> I'm elated to say that I've now got a test for my fix. (See
virtual/threaded/animations/ changes). PTAL!
> 
>
https://codereview.chromium.org/1973083002/diff/690001/cc/trees/layer_tree_im...
> File cc/trees/layer_tree_impl.cc (right):
> 
>
https://codereview.chromium.org/1973083002/diff/690001/cc/trees/layer_tree_im...
> cc/trees/layer_tree_impl.cc:849: for (; it !=
LayerListIterator<LayerImpl>(nullptr); ++it) {
> This is another noteworthy change. Now that "call function for every layer"
won't work without a layer list, I can't depend on it for setting all the
element id's in unit tests prior to building the layer list.
> 
> What I've done instead is to iterate over the layers as we used to rather than
using the convenience function.
> 
>
https://codereview.chromium.org/1973083002/diff/690001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/animation/CompositorAnimations.cpp
(right):
> 
>
https://codereview.chromium.org/1973083002/diff/690001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/animation/CompositorAnimations.cpp:380: void
CompositorAnimations::attachCompositedLayers(Element& element, const Animation&
animation)
> This is the noteworthy behavioral change in this patch. I've replaced the
nullity checks that I removed when I'd originally landed this patch.

..and rebased. There were some minor conflicts in cc due to root_layer becoming
test-only.

[ajuma, 2016-06-29 15:53:02]

On 2016/06/29 15:22:12, vollick wrote:
> I'm elated to say that I've now got a test for my fix. (See
> virtual/threaded/animations/ changes). PTAL!

lgtm!

[Ian Vollick, 2016-06-29 17:05:18]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2016-06-29 17:05:19]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1973083002/#ps710001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-29 17:06:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 17:54:44]

Committed patchset #37 (id:710001)

[commit-bot: I haz the power, 2016-06-29 17:54:52]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 17:56:48]

Description was changed from

==========
Use element id's for animations.

The goal of this CL is to migrate cc's animation code
away from layer id's in anticipation of SPv2. The ids
chosen are a minor tweak on DOMNodeIds for blink.

Other clients of cc may essentially continue using
their respective layer id's as identifiers. cc itself
remains ignorant of the semantic meaning of these ids,
treating them as an opaque identifier.

This change is extremely similar to loyso@'s earlier
CL crrev.com/1944623002, the significant difference
being the scheme for choosing the ids.

Notable changes in this CL
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).
 * Removal of ElementLayers from cc

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/51659f1ad58fc058ebb95907734ce6b2c887898d
Cr-Commit-Position: refs/heads/master@{#398366}
==========

to

==========
Use element id's for animations.

The goal of this CL is to migrate cc's animation code
away from layer id's in anticipation of SPv2. The ids
chosen are a minor tweak on DOMNodeIds for blink.

Other clients of cc may essentially continue using
their respective layer id's as identifiers. cc itself
remains ignorant of the semantic meaning of these ids,
treating them as an opaque identifier.

This change is extremely similar to loyso@'s earlier
CL crrev.com/1944623002, the significant difference
being the scheme for choosing the ids.

Notable changes in this CL
 * Introduces an ElementId structure.
 * Augments DOMNodeIds in order to handle sub elements (eg, for scroll and link
highlight layers).
 * Removal of ElementLayers from cc

BUG=616542
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/51659f1ad58fc058ebb95907734ce6b2c887898d
Committed: https://crrev.com/ef2ae922636be42ce063a534496f162c587d0e76
Cr-Original-Commit-Position: refs/heads/master@{#398366}
Cr-Commit-Position: refs/heads/master@{#402863}
==========

[commit-bot: I haz the power, 2016-06-29 17:56:51]

Patchset 37 (id:??) landed as
https://crrev.com/ef2ae922636be42ce063a534496f162c587d0e76
Cr-Commit-Position: refs/heads/master@{#402863}