1526850 - Refactoring for supporting multiple properties for compositor animations on individual transforms

Status: RESOLVED FIXED

(Core :: DOM: Animation, enhancement, P3)

Description

[Boris Chiou [:boris]]

We intend to support compositor animations for individual transforms, and motion-path (in the future). Those use different properties, so we have to extend the usage of eCSSProperty_transform (e.g. [1][2], ... and a lot of places) to display item which returns a property set or an array of transform-like properties [3].

[1] https://searchfox.org/mozilla-central/rev/5e3bffe964110b8d1885b4236b8abd5c94d8f609/layout/painting/nsDisplayList.cpp#8041-8042
[2] https://searchfox.org/mozilla-central/rev/5e3bffe964110b8d1885b4236b8abd5c94d8f609/dom/animation/KeyframeEffect.cpp#1695
[3] https://searchfox.org/mozilla-central/rev/5e3bffe964110b8d1885b4236b8abd5c94d8f609/layout/style/LayerAnimationInfo.h#30-31

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I am pretty sure this should fix bug 1505225.

Comment 2

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

I am pretty sure this should fix bug 1505225.

Thanks for this. This bug is split from Bug 1425837 because I have too many patches to do clean-up. :)

Comment 3

[Boris Chiou [:boris]]

Attachment: Bug 1526850 - Part 2: Let FindAnimationsForCompositor take nsCSSPropertyIDSet.

The caller of HasAnimationsForCompositor just wants to check if there is
any compositor animation for a display item, so we can replace it by the
display item, and get the properties from this display item.
Therefore, we can easily extend the property list for other
transform-like properties.

Comment 4

[Boris Chiou [:boris]]

Attachment: Bug 1526850 - Part 3: Use DisplayItemType as the input of AddTransitionsAndAnimationsToLayer.

Transform display item may have multiple properties, so it's better to
use display item type as the input.

Also, factor some code out of AddAnimationsForProperty, so we can easier
to extend this for multiple properties.

We will pass a list of layers::Animation to the compositor thread. In
this list, the animations belonging to the same property are grouped together,
so we can easily separate the animations by property and sample the animations
for each property on the compositor thread. (Will do thisin Bug 1425837.)

Depends on D19628

Comment 5

[Boris Chiou [:boris]]

Attachment: Bug 1526850 - Part 1: Add a function which sets RunningOnCompositor by DisplayItemType.

We need to set this flag by a property set (i.e. transform-like
properties), so add this function.

Depends on D19629

Comment 6

[Boris Chiou [:boris]]

Attachment: Bug 1526850 - Part 4: Check the existence of animations by a property set.

Sometimes we want to check if there is any transform animations. Here,
we replace the argument of nsCSSPropertyID with nsCSSPropertyIDSet.

Depends on D19630

Comment 7

[Boris Chiou [:boris]]

Attachment: Bug 1526850 - Part 5: Make sure the callers of ActivityTracker takes individual transforms into account.

Add comments and property check for the caller of ActivityTracker.

Depends on D19631

Comment 8

[Boris Chiou [:boris]]

Attachment: Bug 1526850 - Part 4: Add one more function to set performance warning by a property set.

Sometimes, we want to set the performance warning by a property set, so
add a new function for it.

Depends on D19632

Comment 9

[Boris Chiou [:boris]]

Attachment: Bug 1526850 - Part 5: Check all transform-like properties in CalculateCumulativeChangeHint and MaybeUpdateFrameForCompositor.

Depends on D19633

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1526850 - Part 8: Add comments in nsDOMCSSDeclaration::SetPropertyValue.

Depends on D19634

Comment 11

[Boris Chiou [:boris]]

Attachment: Bug 1526850 - Part 6: Check all transform-like properties in AnimationInfo::HasTransformAnimation.

LayerAnimationInfo knows the transform-like properties we support on
the compositor thread, so we should use it.

Depends on D19635

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I'd suggest to fix bug 1525805 first. I think part 4 is the fix for that bug, and it would be nice to have reftests for that.

Comment 13

[Boris Chiou [:boris]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)

I'd suggest to fix bug 1525805 first. I think part 4 is the fix for that bug, and it would be nice to have reftests for that.

I guess you mean Bug 1505225.

Sure, I will fix Bug 1505225 first. :)

Comment 14

[Hiroyuki Ikezoe (:hiro)]

lol. Of course I should fix bug 1525805 first. :)

Comment 15

[Phabricator Automation]

Comment on attachment 9043524 [details]
Bug 1526850 - Part 4: Check the existence of animations by a property set.

Revision D19631 was moved to bug 1529422. Setting attachment 9043524 [details] to obsolete.

Comment 16

[Phabricator Automation]

Comment on attachment 9043525 [details]
Bug 1526850 - Part 5: Make sure the callers of ActivityTracker takes individual transforms into account.

Revision D19632 was moved to bug 1529422. Setting attachment 9043525 [details] to obsolete.

Comment 17

[Phabricator Automation]

Comment on attachment 9043529 [details]
Bug 1526850 - Part 8: Add comments in nsDOMCSSDeclaration::SetPropertyValue.

Revision D19635 was moved to bug 1529422. Setting attachment 9043529 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9043530 [details]
Bug 1526850 - Part 6: Check all transform-like properties in AnimationInfo::HasTransformAnimation.

Revision D19636 was moved to bug 1425837. Setting attachment 9043530 [details] to obsolete.

Comment 19

[Phabricator Automation]

Comment on attachment 9043528 [details]
Bug 1526850 - Part 5: Check all transform-like properties in CalculateCumulativeChangeHint and MaybeUpdateFrameForCompositor.

Revision D19634 was moved to bug 1425837. Setting attachment 9043528 [details] to obsolete.

Comment 20

[Phabricator Automation]

Comment on attachment 9043526 [details]
Bug 1526850 - Part 4: Add one more function to set performance warning by a property set.

Revision D19633 was moved to bug 1425837. Setting attachment 9043526 [details] to obsolete.

Comment 21

[Boris Chiou [:boris]]

Attachment: Bug 1526850 - Part 4: Add nsCSSPropertyIDSet::IsSubsetOf() for checking the restrictions of some arguments.

We restrict the argument (i.e. nsCSSPropertyIDSet) of some functions is
the subset of other property set (e.g. transform-like properties), so
add this function for better readability.

Depends on D19629

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/159c31166c7a
Part 1: Add a function which sets RunningOnCompositor by DisplayItemType. r=hiro
https://hg.mozilla.org/integration/autoland/rev/3daab2871496
Part 2: Let FindAnimationsForCompositor take nsCSSPropertyIDSet. r=hiro
https://hg.mozilla.org/integration/autoland/rev/c05471f233f3
Part 3: Use DisplayItemType as the input of AddTransitionsAndAnimationsToLayer. r=hiro
https://hg.mozilla.org/integration/autoland/rev/653f907add4e
Part 4: Add nsCSSPropertyIDSet::IsSubsetOf() for checking the restrictions of some arguments. r=hiro

Comment 23

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/159c31166c7a
https://hg.mozilla.org/mozilla-central/rev/3daab2871496
https://hg.mozilla.org/mozilla-central/rev/c05471f233f3
https://hg.mozilla.org/mozilla-central/rev/653f907add4e