Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2030843002:
Web Animations: Account for end delay in after phase active time (Closed)
|
Created:
4 years, 6 months ago by alancutter (OOO until 2018) Modified:
4 years, 4 months ago Reviewers:
suzyh_UTC10 (ex-contributor) CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, Eric Willigers, rjwright, shans Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWeb Animations: Account for end delay in after phase active time
This change implements a tweak to the Web Animations spec:
https://github.com/w3c/web-animations/commit/a9ba51338ed09170d16c47317f8e4e2eef122a82
Negative end delay now affects the progress used in the after phase.
Committed: https://crrev.com/473c692fac754376f0b4feb4ba9a2ae0e1472180
Cr-Commit-Position: refs/heads/master@{#407690}
Patch Set 1 #Patch Set 2 : WPT expectation #Patch Set 3 : Add additional behaviour and test #Patch Set 4 : Unit test #
Total comments: 19
Patch Set 5 : Review changes #Patch Set 6 : Add headers #
Messages
Total messages: 38 (20 generated)
The CQ bit was checked by alancutter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030843002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2030843002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alancutter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030843002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2030843002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by alancutter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030843002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2030843002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alancutter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alancutter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Web Animations: Account for end delay in after phase active time [speculative] This change is testing out a proposed change to the Web Animations spec. https://github.com/w3c/web-animations/issues/154 ========== to ========== Web Animations: Account for end delay in after phase active time This change implements a tweak to the Web Animations spec: https://github.com/w3c/web-animations/commit/a9ba51338ed09170d16c47317f8e4e2e... Negative end delay now affects the progress used in the after phase. ==========
alancutter@chromium.org changed reviewers: + suzyh@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alancutter@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt:7: FAIL change currentTime when fill forwards and endDelay is negative assert_equals: set currentTime same as endTime expected "0" but got "0.5" Is this an error in the upstream test? https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/web-animations-api/fill-forward-negative-end-delay.html (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/fill-forward-negative-end-delay.html:1: <script src="../resources/testharness.js"></script> Are we intending to upstream this one? https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/fill-forward-negative-end-delay.html:10: // TODO(alancutter): Implement animation.effect.getComputedTiming().progress to Don't we have this now? https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/TimingCalculations.h (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/TimingCalculations.h:128: // TODO(alancutter): Align this function with current Web Animations spec text. Let's start making sure all our TODOs have bugs attached to them. Please file a bug for this and add the bug number here. https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp:100: TEST(AnimationTimingCalculationsTest, IterationTime) This test is confusing and not easy to read, so let's leave it better than we found it. Perhaps split it up into several tests for the different conditions? Probably a good idea to add tests for different values for the phase argument as well, now that that's possible.
https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt:7: FAIL change currentTime when fill forwards and endDelay is negative assert_equals: set currentTime same as endTime expected "0" but got "0.5" On 2016/07/25 at 00:51:09, suzyh wrote: > Is this an error in the upstream test? Yes. https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/web-animations-api/fill-forward-negative-end-delay.html (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/fill-forward-negative-end-delay.html:1: <script src="../resources/testharness.js"></script> On 2016/07/25 at 00:51:09, suzyh wrote: > Are we intending to upstream this one? Yes. https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/fill-forward-negative-end-delay.html:10: // TODO(alancutter): Implement animation.effect.getComputedTiming().progress to On 2016/07/25 at 00:51:09, suzyh wrote: > Don't we have this now? Using it now. https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/TimingCalculations.h (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/TimingCalculations.h:128: // TODO(alancutter): Align this function with current Web Animations spec text. On 2016/07/25 at 00:51:09, suzyh wrote: > Let's start making sure all our TODOs have bugs attached to them. Please file a bug for this and add the bug number here. Done.
https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt:7: FAIL change currentTime when fill forwards and endDelay is negative assert_equals: set currentTime same as endTime expected "0" but got "0.5" On 2016/07/25 at 01:44:32, alancutter wrote: > On 2016/07/25 at 00:51:09, suzyh wrote: > > Is this an error in the upstream test? > > Yes. I recommend fixing the upstream test in the same pull request as adding the additional test below. https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/web-animations-api/fill-forward-negative-end-delay.html (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/fill-forward-negative-end-delay.html:1: <script src="../resources/testharness.js"></script> On 2016/07/25 at 01:44:32, alancutter wrote: > On 2016/07/25 at 00:51:09, suzyh wrote: > > Are we intending to upstream this one? > > Yes. In that case, please add the usual meta/header info to the start of the test (title, help link, etc). https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp:100: TEST(AnimationTimingCalculationsTest, IterationTime) On 2016/07/25 at 00:51:09, suzyh wrote: > This test is confusing and not easy to read, so let's leave it better than we found it. Perhaps split it up into several tests for the different conditions? Probably a good idea to add tests for different values for the phase argument as well, now that that's possible. Ping on this. I suspect this comment got lost because it is on a line that codereview is not automatically showing.
https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt:7: FAIL change currentTime when fill forwards and endDelay is negative assert_equals: set currentTime same as endTime expected "0" but got "0.5" On 2016/07/25 at 01:52:12, suzyh wrote: > On 2016/07/25 at 01:44:32, alancutter wrote: > > On 2016/07/25 at 00:51:09, suzyh wrote: > > > Is this an error in the upstream test? > > > > Yes. > > I recommend fixing the upstream test in the same pull request as adding the additional test below. Would the change get upstreamed? I'm not sure I see any reason to modify our copy of it if not. https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/web-animations-api/fill-forward-negative-end-delay.html (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/web-animations-api/fill-forward-negative-end-delay.html:1: <script src="../resources/testharness.js"></script> On 2016/07/25 at 01:52:12, suzyh wrote: > On 2016/07/25 at 01:44:32, alancutter wrote: > > On 2016/07/25 at 00:51:09, suzyh wrote: > > > Are we intending to upstream this one? > > > > Yes. > > In that case, please add the usual meta/header info to the start of the test (title, help link, etc). Done. https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp:100: TEST(AnimationTimingCalculationsTest, IterationTime) On 2016/07/25 at 01:52:12, suzyh wrote: > On 2016/07/25 at 00:51:09, suzyh wrote: > > This test is confusing and not easy to read, so let's leave it better than we found it. Perhaps split it up into several tests for the different conditions? Probably a good idea to add tests for different values for the phase argument as well, now that that's possible. > > Ping on this. I suspect this comment got lost because it is on a line that codereview is not automatically showing. I think it's not a good use of time to rewrite these unit tests when the code itself is in need of a rewrite to be more aligned with the spec. It's difficult to even know what's expected of code that doesn't have a clear specification.
lgtm https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt:7: FAIL change currentTime when fill forwards and endDelay is negative assert_equals: set currentTime same as endTime expected "0" but got "0.5" On 2016/07/25 at 04:46:48, alancutter wrote: > On 2016/07/25 at 01:52:12, suzyh wrote: > > On 2016/07/25 at 01:44:32, alancutter wrote: > > > On 2016/07/25 at 00:51:09, suzyh wrote: > > > > Is this an error in the upstream test? > > > > > > Yes. > > > > I recommend fixing the upstream test in the same pull request as adding the additional test below. > > Would the change get upstreamed? I'm not sure I see any reason to modify our copy of it if not. No, I meant to make the change upstream (since you're already intending to upstream fill-forward-negative-end-delay.html anyway), not to modify our copy. https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp:100: TEST(AnimationTimingCalculationsTest, IterationTime) On 2016/07/25 at 04:46:48, alancutter wrote: > On 2016/07/25 at 01:52:12, suzyh wrote: > > On 2016/07/25 at 00:51:09, suzyh wrote: > > > This test is confusing and not easy to read, so let's leave it better than we found it. Perhaps split it up into several tests for the different conditions? Probably a good idea to add tests for different values for the phase argument as well, now that that's possible. > > > > Ping on this. I suspect this comment got lost because it is on a line that codereview is not automatically showing. > > I think it's not a good use of time to rewrite these unit tests when the code itself is in need of a rewrite to be more aligned with the spec. > It's difficult to even know what's expected of code that doesn't have a clear specification. OK. Make a note in the rewrite bug you filed and/or add a TODO here.
https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/AnimationEffectTiming/getComputedStyle-expected.txt:7: FAIL change currentTime when fill forwards and endDelay is negative assert_equals: set currentTime same as endTime expected "0" but got "0.5" On 2016/07/25 at 04:55:23, suzyh wrote: > On 2016/07/25 at 04:46:48, alancutter wrote: > > On 2016/07/25 at 01:52:12, suzyh wrote: > > > On 2016/07/25 at 01:44:32, alancutter wrote: > > > > On 2016/07/25 at 00:51:09, suzyh wrote: > > > > > Is this an error in the upstream test? > > > > > > > > Yes. > > > > > > I recommend fixing the upstream test in the same pull request as adding the additional test below. > > > > Would the change get upstreamed? I'm not sure I see any reason to modify our copy of it if not. > > No, I meant to make the change upstream (since you're already intending to upstream fill-forward-negative-end-delay.html anyway), not to modify our copy. Whoops, confused PR for CL. https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp (right): https://codereview.chromium.org/2030843002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/animation/TimingCalculationsTest.cpp:100: TEST(AnimationTimingCalculationsTest, IterationTime) On 2016/07/25 at 04:55:23, suzyh wrote: > On 2016/07/25 at 04:46:48, alancutter wrote: > > On 2016/07/25 at 01:52:12, suzyh wrote: > > > On 2016/07/25 at 00:51:09, suzyh wrote: > > > > This test is confusing and not easy to read, so let's leave it better than we found it. Perhaps split it up into several tests for the different conditions? Probably a good idea to add tests for different values for the phase argument as well, now that that's possible. > > > > > > Ping on this. I suspect this comment got lost because it is on a line that codereview is not automatically showing. > > > > I think it's not a good use of time to rewrite these unit tests when the code itself is in need of a rewrite to be more aligned with the spec. > > It's difficult to even know what's expected of code that doesn't have a clear specification. > > OK. Make a note in the rewrite bug you filed and/or add a TODO here. The unit tests will be rewritten as part of rewriting the code, no need to add a TODO to do so.
The CQ bit was checked by alancutter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Web Animations: Account for end delay in after phase active time This change implements a tweak to the Web Animations spec: https://github.com/w3c/web-animations/commit/a9ba51338ed09170d16c47317f8e4e2e... Negative end delay now affects the progress used in the after phase. ========== to ========== Web Animations: Account for end delay in after phase active time This change implements a tweak to the Web Animations spec: https://github.com/w3c/web-animations/commit/a9ba51338ed09170d16c47317f8e4e2e... Negative end delay now affects the progress used in the after phase. Committed: https://crrev.com/473c692fac754376f0b4feb4ba9a2ae0e1472180 Cr-Commit-Position: refs/heads/master@{#407690} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/473c692fac754376f0b4feb4ba9a2ae0e1472180 Cr-Commit-Position: refs/heads/master@{#407690} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
