Details

Reviewers

Commits

Restricted Diffusion Commit
rMOZILLACENTRALa2ab78c846cf: Bug 1676791 - Part 5: Define the finite timeline and use it in Play() and Pause…

Bugzilla Bug ID

1676791

Summary

Based on https://github.com/w3c/csswg-drafts/pull/4842, we define
"has finite timeline", which is a timeline that's not monotonically increasing.
We need this to update start time and hold time for scroll-timeline, so
we play scroll-linked animations as we expected, e.g. GetLocalTime() returns
the correct time value from GetCurrentTimeAsDuration().

Known issue: we still have bugs when setting "animation-play-state:paused".
Will do that in Bug 1741255.

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Build Status

Buildable 379384
Build 474221: arc lint + arc unit

Event Timeline

Address comments

Herald added a project: secure-revision. · View Herald TranscriptNov 15 2021, 8:32 PM

boris added a child revision: D129102: Bug 1676791 - Part 6: Prevent getAnimations() from returning scroll-linked animations..Nov 15 2021, 8:32 PM

Harbormaster completed remote builds in B371933: Diff 508052.Nov 15 2021, 8:32 PM

boris added a reviewer: Restricted Project.Nov 15 2021, 8:32 PM

phab-bot published this revision for review.Nov 15 2021, 8:34 PM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

Code analysis found 3 defects in the diff 508052:

3 defects found by clang-tidy

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Looks great! (Though I wasn't aware of the spec change, it exactly matches my mental model of scroll linked animation)

To be honest, I don't know there's any tests specific for this change, but I suppose most of scroll-linked animation test cases won't work without this change.

dom/animation/Animation.cpp
1377	Can we factor out this `mTimeline && !mTimeline->IsMonotonicallyIncreasing()` as HasFiniteTimeline() function? And if the implementation is defined in Animation.h, I'd expect it'd be inlined.
dom/animation/AnimationTimeline.h
106	suggestion: we should make this function virtual, and return true in DocumentTimeline, return false in ScrollTimeline.

This revision is now accepted and ready to land.Nov 16 2021, 2:48 AM

In D131168#4265210, @hiro wrote:

To be honest, I don't know there's any tests specific for this change, but I suppose most of scroll-linked animation test cases won't work without this change.

Yes. This PR is for scroll animations. Without this, we cannot pass the tests added in the patch series.

Address comments

Harbormaster completed remote builds in B372910: Diff 509248.Nov 17 2021, 10:26 PM

Code analysis found 3 defects in the diff 509248:

3 defects found by clang-tidy

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Rearrange

boris retitled this revision from Bug 1676791 - Part 4: Define the finite timeline and use it in Play() and Pause(). to Bug 1676791 - Part 5: Define the finite timeline and use it in Play() and Pause()..Nov 18 2021, 12:06 AM

boris edited parent revisions, added: D131449: Bug 1676791 - Part 4: Make TimingParams be a LiteralType.; removed: D129101: Bug 1676791 - Part 3: Implement the computation of timing..

Harbormaster completed remote builds in B372937: Diff 509286.Nov 18 2021, 12:06 AM

boris removed a reviewer: Restricted Project.Nov 18 2021, 11:13 PM

Address final comments and rebased

Harbormaster completed remote builds in B376869: Diff 513973.Nov 30 2021, 7:46 PM

Code analysis found 3 defects in the diff 513973:

3 defects found by clang-tidy

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Rebased

Harbormaster completed remote builds in B379045: Diff 516529.Dec 7 2021, 9:04 PM

Code analysis found 3 defects in the diff 516529:

3 defects found by clang-tidy

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Move test and ready to land

Harbormaster completed remote builds in B379157: Diff 516649.Dec 8 2021, 1:14 AM

Code analysis found 3 defects in the diff 516649:

3 defects found by clang-tidy

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Closed by commit rMOZILLACENTRALa2ab78c846cf: Bug 1676791 - Part 5: Define the finite timeline and use it in Play() and Pause… (authored by boris). · Explain WhyDec 8 2021, 2:33 AM

This revision was automatically updated to reflect the committed changes.

boris added a commit: rMOZILLACENTRALa2ab78c846cf: Bug 1676791 - Part 5: Define the finite timeline and use it in Play() and Pause….

boris added a commit: Restricted Diffusion Commit.Feb 18 2022, 7:45 PM

Harbormaster completed remote builds in B379384: Diff 516681.Feb 18 2022, 7:45 PM

Bug 1676791 - Part 5: Define the finite timeline and use it in Play() and Pause().
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 516681

dom/animation/Animation.h

dom/animation/Animation.cpp

dom/animation/AnimationTimeline.h

dom/animation/DocumentTimeline.h

dom/animation/ScrollTimeline.h

Bug 1676791 - Part 5: Define the finite timeline and use it in Play() and Pause().ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 516681

dom/animation/Animation.h

dom/animation/Animation.cpp

dom/animation/AnimationTimeline.h

dom/animation/DocumentTimeline.h

dom/animation/ScrollTimeline.h

Bug 1676791 - Part 5: Define the finite timeline and use it in Play() and Pause().
ClosedPublic
Actions

Revision Contents
Changeset List