stylo: Update CSS animations during StyleDocument() instead of RecreateStyleContexts().

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Blocks: 1 bug)

45 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

a year ago
So that we can incorporate initial animation styles in the first place.

To do this we need to:
1) Make KeyframeEffectReadOnly::SetKeyframes() independent from nsStyleContext.
2) Make all relevant stuff thread-safe

From what I understand, computing StyleAnimationValue for the compositor is the last barrier of this issue.
(Assignee)

Updated

a year ago
Depends on: 1335942
(Assignee)

Updated

a year ago
Depends on: 1338927
(Assignee)

Comment 1

a year ago
Initially I thought we can create update_animations() in rust which is equivalent of nsAnimationManager::UpdateAnimations(), but today I realized that we can't do it because of nsAutoAnimationMutationBatch.  We are using nsAutoAnimationMutationBatch, which is a stack base class, to notify animation state changes to devtools, we need to call another variant of nsAnimationManager::UpdateAnimations() to use the stack class from update_aniamtions on servo side.  The another UpdateAnimations requires servo's computed values instead of nsStyleContext* just like what I am doing in bug 1338927.
(Assignee)

Comment 2

a year ago
It's a tough work to rewrite UpdateAnimations(). It's not only using servo's computed values instead of nsStyleContext but also dropping StyleAnimation.  We need to go back and forth between gecko and servo.  I am trying to bring minimum maintenance cost of both of gecko and stylo, but I am not really sure.  Anyway once we did this here, we can take a similar way for nsTransitionManager.
(Assignee)

Comment 3

a year ago
I missed that nsAutoAnimationMutationBatch is not thread safe now.  If we notified animation change's state on some threads simultaneously, what happens?  We should wait for servo's parallel styling (I mean the finish of StyleDocument()) and the notify all of the changes at once?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> I missed that nsAutoAnimationMutationBatch is not thread safe now.  If we
> notified animation change's state on some threads simultaneously, what
> happens?  We should wait for servo's parallel styling (I mean the finish of
> StyleDocument()) and the notify all of the changes at once?

Oh, it is not thread-safe, so I think the answer is yes. I also have a similar problem, so need to maintain a list during parallel styling, but the problem is maintaining the list should be thread-safe, so I'm not sure what kind of list could be used efficiently and is there any overhead of those read-write locks.
(Assignee)

Updated

a year ago
Depends on: 1340322
(Assignee)

Comment 5

a year ago
Bug 1340340 noticed me that CSSAnimation has the same problem.
Summary: stylo: Update CSS animations during StyleDocument() instead of RecreateStyleContexts(). → stylo: Update CSS animations during StyleDocument() (i.e. during the parallel traversal) instead of RecreateStyleContexts().
(Assignee)

Updated

a year ago
Depends on: 1340938
(Assignee)

Updated

a year ago
Summary: stylo: Update CSS animations during StyleDocument() (i.e. during the parallel traversal) instead of RecreateStyleContexts(). → stylo: Update CSS animations during StyleDocument() instead of RecreateStyleContexts().
Assigning to Hiro just to have an owner. Feel free to reassign or reprioritize.
Assignee: nobody → hikezoe
Priority: -- → P2
(Assignee)

Comment 7

a year ago
This has been also fixed by bug 1341985.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.