Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2614683002:
CSS Animations: Fix flaky change-keyframes-name.html (Closed)
|
Created:
3 years, 11 months ago by Eric Willigers Modified:
3 years, 11 months ago Reviewers:
suzyh_UTC10 (ex-contributor) CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, chromium-reviews, Eric Willigers, rjwright, shans Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCSS Animations: Fix flaky change-keyframes-name.html
We no longer require precise timing during execution of
change-keyframes-name.html
BUG=248938
Review-Url: https://codereview.chromium.org/2614683002
Cr-Commit-Position: refs/heads/master@{#441824}
Committed: https://chromium.googlesource.com/chromium/src/+/efa56265d8881a61f53d1a5c4f46f54db6c03077
Patch Set 1 #
Total comments: 4
Patch Set 2 : rewrite #
Total comments: 6
Patch Set 3 : no setTimeout #
Total comments: 4
Patch Set 4 : assert keyframes #
Messages
Total messages: 26 (15 generated)
The CQ bit was checked by ericwilligers@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.
ericwilligers@chromium.org changed reviewers: + suzyh@chromium.org
We could also change the animation duration. The comments in the test are also slightly wrong.
On 2017/01/04 at 20:30:47, ericwilligers wrote: > We could also change the animation duration. I suggest changing the animation duration, but for clarity in reading the test rather than specifically for the flakiness. I think making this change is also worthwhile, even if you change the duration. Do we know why the test expectation was TIMEOUT as well as PASS and FAILURE? > The comments in the test are also slightly wrong. In that case, please update the comments too. https://codereview.chromium.org/2614683002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/animations/change-keyframes-name.html (right): https://codereview.chromium.org/2614683002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:16: -webkit-animation-duration: 0.5s; Update to not use the prefixed properties? https://codereview.chromium.org/2614683002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:32: [0.25, "box", "left", 200, 0], To make this "0.25" time easier to understand, let's make the animation duration a nice round 1s and change "0.25" to "0.5". https://codereview.chromium.org/2614683002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:72: This test changes the name of the @keyframes rule so that it matches For clarity of reading can you please indent everything inside the body? https://codereview.chromium.org/2614683002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:76: <div id="pre-result"> Unused?
Description was changed from ========== CSS Animations: Reduce flakiness of change-keyframes-name.html We no longer require precise timing during execution of change-keyframes-name.html BUG=248938 ========== to ========== CSS Animations: Fix flaky change-keyframes-name.html We no longer require precise timing during execution of change-keyframes-name.html BUG=248938 ==========
Patchset #2 (id:20001) has been deleted
The cleanest way to fix the various issues was to rewrite the test.
The CQ bit was checked by ericwilligers@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...
On 2017/01/05 at 05:36:13, ericwilligers wrote: > The cleanest way to fix the various issues was to rewrite the test. Curious. I didn't realise my suggestions were so significant as to trigger a rewrite. What in particular triggered this? That said, the rewrite is sufficiently easier to read that I've noticed more things to say. Yay! https://codereview.chromium.org/2614683002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/animations/change-keyframes-name.html (right): https://codereview.chromium.org/2614683002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:18: animation-name: bar; Is this animation-name property actually needed? https://codereview.chromium.org/2614683002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:50: keyframes.name = 'anim'; Why does this get set here? Isn't 'foo' the existing keyframes name, which could be used as the new value for target.style.animationName? https://codereview.chromium.org/2614683002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:55: requestAnimationFrame(t.step_func_done(() => { I thought the combination of setTimeout and requestAnimationFrame was a warning sign for flakiness in other tests. Is the use of requestAnimationFrame needed here? What does the combination gain us?
Adding comments to the test is easier when the test is direct. https://codereview.chromium.org/2614683002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/animations/change-keyframes-name.html (right): https://codereview.chromium.org/2614683002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:18: animation-name: bar; On 2017/01/05 05:53:43, suzyh wrote: > Is this animation-name property actually needed? The different name makes clear that we are later changing the name. https://codereview.chromium.org/2614683002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:50: keyframes.name = 'anim'; On 2017/01/05 05:53:43, suzyh wrote: > Why does this get set here? Isn't 'foo' the existing keyframes name, which could > be used as the new value for target.style.animationName? I think much of the purpose of this test is updating the name of the keyframes. https://codereview.chromium.org/2614683002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:55: requestAnimationFrame(t.step_func_done(() => { On 2017/01/05 05:53:43, suzyh wrote: > I thought the combination of setTimeout and requestAnimationFrame was a warning > sign for flakiness in other tests. Is the use of requestAnimationFrame needed > here? What does the combination gain us? setTimeout alone often leads to flakiness. Without requestAnimationFrame, styles might not have been recomputed. I'm using setTimeout to ensure we have made progress in the animation, and are not at the initial frame.
Cool, thanks for the explanations. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've updated the test to avoid setTimeout.
lgtm with optional nits https://codereview.chromium.org/2614683002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/animations/change-keyframes-name.html (right): https://codereview.chromium.org/2614683002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:48: document.addEventListener('animationend', t.step_func_done(() => { Of course if the animation doesn't run, this will never be fired and the test will timeout. Given that under that circumstance, the assertion below (left is 100px) should fail first, so I'm fine with this. Consider adding a cautionary comment. https://codereview.chromium.org/2614683002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:53: var keyframes = findKeyframesRule('foo'); Maybe assert keyframes is not null
https://codereview.chromium.org/2614683002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/animations/change-keyframes-name.html (right): https://codereview.chromium.org/2614683002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:48: document.addEventListener('animationend', t.step_func_done(() => { On 2017/01/06 00:01:56, suzyh wrote: > Of course if the animation doesn't run, this will never be fired and the test > will timeout. Given that under that circumstance, the assertion below (left is > 100px) should fail first, so I'm fine with this. Consider adding a cautionary > comment. Added description to assert. https://codereview.chromium.org/2614683002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/animations/change-keyframes-name.html:53: var keyframes = findKeyframesRule('foo'); On 2017/01/06 00:01:56, suzyh wrote: > Maybe assert keyframes is not null Done.
The CQ bit was checked by ericwilligers@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from suzyh@chromium.org Link to the patchset: https://codereview.chromium.org/2614683002/#ps80001 (title: "assert keyframes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483661805539140,
"parent_rev": "e8e7e02ebf0123bead605c0f974badf6c8260d1e", "commit_rev":
"efa56265d8881a61f53d1a5c4f46f54db6c03077"}
Message was sent while issue was closed.
Description was changed from ========== CSS Animations: Fix flaky change-keyframes-name.html We no longer require precise timing during execution of change-keyframes-name.html BUG=248938 ========== to ========== CSS Animations: Fix flaky change-keyframes-name.html We no longer require precise timing during execution of change-keyframes-name.html BUG=248938 Review-Url: https://codereview.chromium.org/2614683002 Cr-Commit-Position: refs/heads/master@{#441824} Committed: https://chromium.googlesource.com/chromium/src/+/efa56265d8881a61f53d1a5c4f46... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/efa56265d8881a61f53d1a5c4f46... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
