8000 [css-animationworklet] Separate Stateful and Stateless animator interfaces #812 by majido · Pull Request #827 · w3c/css-houdini-drafts · GitHub
Skip to content

[css-animationworklet] Separate Stateful and Stateless animator interfaces #812#827

Merged
majido merged 2 commits into
w3c:masterfrom
majido:stateful
Oct 25, 2018
Merged

[css-animationworklet] Separate Stateful and Stateless animator interfaces #812#827
majido merged 2 commits into
w3c:masterfrom
majido:stateful

Conversation

@majido
Copy link
Copy Markdown
Contributor

@majido majido commented Oct 19, 2018

Fixes issue #812 .

  • Introduce two new interfaces StatefulAnimator and StatelessAnimator. Add sections to describe the difference.
  • Update Animator Definition to has stateful flag and initialize the flag based on the animator prototype object.
  • Update the algorithm for migration process to not use onDestroy callback but use state getter.
  • Update the examples to use these new interfaces and show how getter should be used.

@majido majido requested a review from stephenmcgruer October 19, 2018 21:15
Copy link
Copy Markdown
Contributor

@stephenmcgruer stephenmcgruer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me. I am assuming you have already considered the benefits of having this inheritance approach versus, say, a static member variable or an argument to the WorkletAnimation constructor. (I think baking it into the worklet is probably better, I can't think of a reason for the same code to be reused for stateful and stateless).

Comment thread css-animationworklet/Overview.bs Outdated

A user-defined stateful animator is expected to fulfill the required contract which is that its
state attribute is an object representing its state that can be serialized using structured
serialized algorithm and that it can also re-generatea its state given that same object passed to
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: re-generate, but would actually suggest "recreate".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Using recreate.

Comment thread css-animationworklet/Overview.bs Outdated
<pre class='lang-javascript'>
class BarAnimator extends StatefulAnimator {
constructor(options, state) {
// Called when a new animator is instantiated (either first time of after being respawned).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: 'first time or after'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Comment thread css-animationworklet/Overview.bs Outdated
Comment thread css-animationworklet/Overview.bs Outdated
Comment thread css-animationworklet/Overview.bs Outdated
An <dfn>Animator</dfn> represents an animation instance that is running on animation thread. Animators
are identified by a unique name and determine how the animation progresses its keyframe effects
given current input time. <a>Animator</a> instances live in {{AnimationWorkletGlobalScope}} and
each one is associated with a {{WorkletAnimation}} instance. An animator can only be instantiated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Just for my own clarification, I suppose a WorkletAnimation is the main thread instance?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is correct. I took a mental note to make this relationship more clear (perhaps even add a diagram).

Comment thread css-animationworklet/Overview.bs Outdated
Comment thread css-animationworklet/Overview.bs Outdated
Comment thread css-animationworklet/Overview.bs Outdated
Comment thread css-animationworklet/Overview.bs Outdated
Comment thread css-animationworklet/Overview.bs Outdated
Comment thread css-animationworklet/Overview.bs Outdated

</div>

Note: If an animator state getter throws the user agent will remove the animator but does not
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this normative text? If so, perhaps it shouldn't be a note?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was to make it clear that this is effectively the implication of the algorithm above. I remove the note to make it clear it is normative.

Comment thread css-animationworklet/Overview.bs Outdated
@majido
Copy link
Copy Markdown
Contributor Author

majido commented Oct 25, 2018

Thanks @birtles and @stephenmcgruer for feedback 👍 . I made corrections accordingly.

@majido majido merged commit 20de222 into w3c:master Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants