CC Animation: Unify ElementId in Blink Compositor Worker.

CC Animation: Unify ElementId in Blink Compositor Worker.

1) Move cc::ElementId to separate file and expose it for blink as
an abstract CompositorElementId.

2) Use int (ElementId alias) instead of uint64_t in CompositorWorker so they will be unified. CompositorWorker
uses int DomNodeIds anyway.

BUG=510960
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[loyso (OOO), 2016-04-27 06:41:03]

Description was changed from

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

BUG=510960
==========

to

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

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

[loyso (OOO), 2016-04-27 06:45:20]

Description was changed from

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

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

to

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

Use int instead of uint64_t since:
- DOMNodeIds uses int
- uint64_t is slower than int on ARM32 arch.
- We have no StaticAtomicSequenceNumber for uint64_t on ARM32.

Next step: DOMNodeIds is very slow and not thread-safe.
Since Blink isn't the only compositor client, we will need
to generate our own ElementIds instead of using DOMNodeIds.

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

[loyso (OOO), 2016-04-27 06:45:20]

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

[ajuma, 2016-04-27 22:03:42]

Could you say a bit more about the overall direction you're heading with this?
Perhaps a short doc describing the plan would be helpful?

[loyso (OOO), 2016-04-27 23:22:59]

On 2016/04/27 22:03:42, ajuma wrote:
> Could you say a bit more about the overall direction you're heading with this?
> Perhaps a short doc describing the plan would be helpful?
I will sketch a couple of dependent CLs. Stay tuned :)

[loyso (OOO), 2016-04-28 02:02:11]

Description was changed from

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

Use int instead of uint64_t since:
- DOMNodeIds uses int
- uint64_t is slower than int on ARM32 arch.
- We have no StaticAtomicSequenceNumber for uint64_t on ARM32.

Next step: DOMNodeIds is very slow and not thread-safe.
Since Blink isn't the only compositor client, we will need
to generate our own ElementIds instead of using DOMNodeIds.

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

to

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

THIS IS A SKETCH. PREVIEW ONLY.

Use int instead of uint64_t since:
- DOMNodeIds uses int
- uint64_t is slower than int on ARM32 arch.
- We have no StaticAtomicSequenceNumber for uint64_t on ARM32.

Next step: DOMNodeIds is very slow and not thread-safe.
Since Blink isn't the only compositor client, we will need
to generate our own ElementIds instead of using DOMNodeIds.

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

[ajuma, 2016-04-28 17:59:56]

On 2016/04/27 23:22:59, loyso wrote:
> On 2016/04/27 22:03:42, ajuma wrote:
> > Could you say a bit more about the overall direction you're heading with
this?
> > Perhaps a short doc describing the plan would be helpful?
> I will sketch a couple of dependent CLs. Stay tuned :)

Thanks a lot for the investigation!

Using an int instead of an int64 sounds great.

Generating these ids in cc and storing them on each Element doesn't seem
necessary though (and would increase the size of every Element, which is
expensive since we have lots of Elements). We only need to generate ids on the
main thread, and we only need them for elements with composited animations. So
DOMNodeIds seem reasonable to continue to use for Blink. There's still the
question of what to do for UI. Since UI and Blink use different instances of cc,
we don't need to worry about the ids from Blink colliding with the ids from UI.
So one thing we could do would be to continue using layer ids as element ids for
UI.

[loyso (OOO), 2016-04-28 23:41:28]

On 2016/04/28 17:59:56, ajuma wrote:

> Using an int instead of an int64 sounds great.
> 
> Generating these ids in cc and storing them on each Element doesn't seem
> necessary though (and would increase the size of every Element, which is
> expensive since we have lots of Elements). We only need to generate ids on the
> main thread, and we only need them for elements with composited animations. So
> DOMNodeIds seem reasonable to continue to use for Blink. There's still the
> question of what to do for UI. Since UI and Blink use different instances of
cc,
> we don't need to worry about the ids from Blink colliding with the ids from
UI.
> So one thing we could do would be to continue using layer ids as element ids
for
> UI.
Thanks for commenting on this!
I had some thoughts about this.
Did you think about the impl-only animation case?
If impl thread wants to play animation, it needs some element_id association. If
we assign those ids 'lazily', then we should notify blink somehow.
As for storing ids intrusively in blink::Element, we can sacrifice this and
store 'pointer-to-id' mapping in CompositorIdToElementMap. This would be similar
to DomNodeIds except
that we can control the source of ids.

[loyso (OOO), 2016-04-28 23:42:07]

p.s. My 3rd and final CL is in progress. Let's see what details I can discover.

[loyso (OOO), 2016-04-28 23:44:48]

p.s. Moreover, think about the link highlights and programmatic scroll animator.
They have WebLayer/GraphicsLayer but no Element. They need element ids as well.

[Ian Vollick, 2016-04-29 02:14:49]

On 2016/04/28 23:44:48, loyso wrote:
> p.s. Moreover, think about the link highlights and programmatic scroll
animator.
> They have WebLayer/GraphicsLayer but no Element. They need element ids as
well.

LinkHighlights do have an associated node though we need to plumb the id
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The ProgrammaticScrollAnimator is associated with a ScrollableArea which could
provide its element id via its various subclasses, though special care will need
to be paid for the visual and root frame viewports

I'd also hope that we could plumb element ids naturally to impl-only animations.
For example, the scroll nodes should eventually have an element id that could be
plumbed to the impl-only scroll animation here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

In general, I'm reluctant to have cc vend ids for blink or maintaining a
parallel id scheme apart from DOMNodeIds or layer ids if there's a way to make
them work.

[loyso (OOO), 2016-05-04 02:38:47]

On 2016/04/29 02:14:49, vollick wrote:
> LinkHighlights do have an associated node though we need to plumb the id
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
There is an implicit contract that each blink:Element may have 1 main graphics
layer (transform/filter/opacity properties) and 1 scroll layer (scrolloffset
property). See cc::LayerImpl::ElementLayers struct.
Note that Node* passed to LinkHighlightImpl can contain compositor animations
running on its main layer.
So, LinkHighlightImpl violates this contract by creating a second main layer to
run opacity animation on it.
That's why I need to create a unique ElementId for LinkHighlightImpl content
layer in this CL: https://codereview.chromium.org/1944623002/

[loyso (OOO), 2016-05-04 06:47:16]

On 2016/04/29 02:14:49, vollick wrote:
> I'd also hope that we could plumb element ids naturally to impl-only
animations.
> For example, the scroll nodes should eventually have an element id that could
be
> plumbed to the impl-only scroll animation here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
As for impl-side only scroll offset animations: impl thread may choose ANY
LayerImpl to start that animation on it (according
LayerTreeHostImpl::ScrollBegin).
And you need some element_id to start it. In theory, blink core:: could read
back if any element_id is already assigned to WebLayer before assigning a new
one.
But that's dependent on layers again.

Overall, that's why lazily assigning element_id-to/from-layer mapping seems too
sophisticated for me. Since our data flow is mainly uni-directional (blink -> MT
cc > CT cc), it makes sense to know all the stable element ids and mappings
ahead of time at the beginning of the chain.

[loyso (OOO), 2016-05-04 07:07:19]

Unrelated, general notes:
We need two maps:
1) idToPtr (element_id -> Element*)
2) ptrToId (Element* -> element_id)
The first mapping is very rare. That's why I suggest to keep it optional (see
blink::CompositorElementId::RegisterElement in dependent CLs).
The second mapping is very useful and should be extremely fast.
Element::compositorElementId() consumes just four bytes and that's a must have
idiom,  I think.
(see how people use LayerById and layer ids on MT since I introduced it). 
In an ideal world I would like to name CompositorElementId as blink::ElementId
and make it more universal across all systems.

[loyso (OOO), 2016-05-04 07:09:13]

Typo: Read as 'CompositorIdToElementMap::registerElement'

[loyso (OOO), 2016-05-10 05:25:39]

Description was changed from

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

THIS IS A SKETCH. PREVIEW ONLY.

Use int instead of uint64_t since:
- DOMNodeIds uses int
- uint64_t is slower than int on ARM32 arch.
- We have no StaticAtomicSequenceNumber for uint64_t on ARM32.

Next step: DOMNodeIds is very slow and not thread-safe.
Since Blink isn't the only compositor client, we will need
to generate our own ElementIds instead of using DOMNodeIds.

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

to

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

1) Move ElementId is separate file and expose it for blink as
an abstract CompositorElementId.

2) Use int instead of uint64_t in CompositorWorker so they
will be unified.

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

[loyso (OOO), 2016-05-10 05:26:00]

Description was changed from

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

1) Move ElementId is separate file and expose it for blink as
an abstract CompositorElementId.

2) Use int instead of uint64_t in CompositorWorker so they
will be unified.

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

to

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

1) Move cc::ElementId to separate file and expose it for blink as
an abstract CompositorElementId.

2) Use int instead of uint64_t in CompositorWorker so they
will be unified.

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

[loyso (OOO), 2016-05-10 05:26:34]

Description was changed from

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

1) Move cc::ElementId to separate file and expose it for blink as
an abstract CompositorElementId.

2) Use int instead of uint64_t in CompositorWorker so they
will be unified.

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

to

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

1) Move cc::ElementId to separate file and expose it for blink as
an abstract CompositorElementId.

2) Use int (ElementId alias) instead of uint64_t in CompositorWorker so they
will be unified.

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

[loyso (OOO), 2016-05-10 05:35:38]

Description was changed from

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

1) Move cc::ElementId to separate file and expose it for blink as
an abstract CompositorElementId.

2) Use int (ElementId alias) instead of uint64_t in CompositorWorker so they
will be unified.

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

to

==========
CC Animation: Unify ElementId in Blink Compositor Worker.

1) Move cc::ElementId to separate file and expose it for blink as
an abstract CompositorElementId.

2) Use int (ElementId alias) instead of uint64_t in CompositorWorker so they
will be unified. CompositorWorker
uses int DomNodeIds anyway.

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

[loyso (OOO), 2016-05-10 06:02:07]

ajuma@, vollick@ - can we land this particular CL? CC ElementAnimations,
Attach/Detach Element and ElementId are already there.
And CompositorWorker uses int DomNodeIds anyway.