Closed Bug 1526850 Opened 8 months ago Closed 8 months ago

Refactoring for supporting multiple properties for compositor animations on individual transforms

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(4 files, 6 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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

I am pretty sure this should fix bug 1505225.

Blocks: 1505225

(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. :)

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.

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

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

Depends on D19629

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

Depends on D19630

Add comments and property check for the caller of ActivityTracker.

Depends on D19631

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

Depends on D19632

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

Depends on D19635

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.

(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. :)

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

Depends on: 1529422

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.

Attachment #9043524 - Attachment is obsolete: true

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.

Attachment #9043525 - Attachment is obsolete: true

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.

Attachment #9043529 - Attachment is obsolete: true
No longer depends on: 1529422
Attachment #9043526 - Attachment description: Bug 1526850 - Part 6: Add one more function to set performance warning by a property set. → Bug 1526850 - Part 4: Add one more function to set performance warning by a property set.
Attachment #9043528 - Attachment description: Bug 1526850 - Part 7: Clean up the rest of eCSSProperty_transform in KeyframeEffect. → Bug 1526850 - Part 5: Check all transform-like properties in CalculateCumulativeChangeHint and MaybeUpdateFrameForCompositor.
Attachment #9043530 - Attachment description: Bug 1526850 - Part 9: Use LayerAnimationInfo to check the transform-like properties in AnimationInfo::HasTransformAnimation. → Bug 1526850 - Part 6: Check all transform-like properties in AnimationInfo::HasTransformAnimation.

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.

Attachment #9043530 - Attachment is obsolete: true

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.

Attachment #9043528 - Attachment is obsolete: true
Summary: Clean up the usage of eCSSProperty_transform for layer animations → Refactoring for supporting multiple properties for compositor animations on individual transforms

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.

Attachment #9043526 - Attachment is obsolete: true
Attachment #9043523 - Attachment description: Bug 1526850 - Part 3: Add a function which sets RunningOnCompositor by a property set. → Bug 1526850 - Part 1: Add a function which sets RunningOnCompositor by DisplayItemType.
Attachment #9043521 - Attachment description: Bug 1526850 - Part 1: Use DisplayItemType as the input of HasAnimationsForCompositor. → Bug 1526850 - Part 2: Let FindAnimationsForCompositor take nsCSSPropertyIDSet.
Attachment #9043522 - Attachment description: Bug 1526850 - Part 2: Use DisplayItemType as the input of AddTransitionsAndAnimationsToLayer. → Bug 1526850 - Part 3: Use DisplayItemType as the input of AddTransitionsAndAnimationsToLayer.

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

Pushed by boris.chiou@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.