Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2621603002:
CSS Transitions: avoid flakes in cancel-transition.html (Closed)
|
Created:
3 years, 11 months ago by Eric Willigers Modified:
3 years, 11 months ago 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 Transitions: avoid flakes in cancel-transition.html
We avoid timing dependence by rAF looping until the animation is in
progress.
BUG=248938
Review-Url: https://codereview.chromium.org/2621603002
Cr-Commit-Position: refs/heads/master@{#442809}
Committed: https://chromium.googlesource.com/chromium/src/+/f6772d3dcc76f65a4b72cd81bd1ab25d3f295c6b
Patch Set 1 #
Total comments: 6
Patch Set 2 : retire waitSeveralFrames #Messages
Total messages: 17 (9 generated)
ericwilligers@chromium.org changed reviewers: + suzyh@chromium.org
This is because I was concerned about the possibility of flakes in the assert added last week, assert_greater_than(parseFloat(getComputedStyle(left).left), 50);
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: + alancutter@chromium.org
https://codereview.chromium.org/2621603002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/transitions/cancel-transition.html (right): https://codereview.chromium.org/2621603002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/transitions/cancel-transition.html:56: if (currentLeft === initialLeft) { Shouldn't we see a change in the value after a single RAF? https://codereview.chromium.org/2621603002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/transitions/cancel-transition.html:78: })).then(waitSeveralFrames).then(t.step_func_done(() => { I don't think this test should need to be wait more than a single frame here or above.
https://codereview.chromium.org/2621603002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/transitions/cancel-transition.html (right): https://codereview.chromium.org/2621603002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/transitions/cancel-transition.html:56: if (currentLeft === initialLeft) { On 2017/01/10 at 08:41:54, alancutter wrote: > Shouldn't we see a change in the value after a single RAF? I don't see the harm in continuing to call rAF in the event that it doesn't change after a single rAF. It's not clear to me what it would mean if it didn't change after a single rAF - whether that's within bounds of acceptable behaviour, or whether that would be a bug. But even in the latter case, I think this test would want to continue checking on each frame until timeout, and leave catching that bug for a separate test. https://codereview.chromium.org/2621603002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/transitions/cancel-transition.html:78: })).then(waitSeveralFrames).then(t.step_func_done(() => { On 2017/01/10 at 08:41:54, alancutter wrote: > I don't think this test should need to be wait more than a single frame here or above. Whichever is chosen, I'd suggest doing the same for both. I don't think we should have both waitForProgress and waitSeveralFrames in this test.
https://codereview.chromium.org/2621603002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/transitions/cancel-transition.html (right): https://codereview.chromium.org/2621603002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/transitions/cancel-transition.html:56: if (currentLeft === initialLeft) { On 2017/01/10 08:41:54, alancutter wrote: > Shouldn't we see a change in the value after a single RAF? We occasionally have more than 10 frames with the same value. https://codereview.chromium.org/2621603002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/transitions/cancel-transition.html:78: })).then(waitSeveralFrames).then(t.step_func_done(() => { On 2017/01/10 22:43:30, suzyh wrote: > On 2017/01/10 at 08:41:54, alancutter wrote: > > I don't think this test should need to be wait more than a single frame here > or above. > > Whichever is chosen, I'd suggest doing the same for both. I don't think we > should have both waitForProgress and waitSeveralFrames in this test. waitForProgress won't work here as no transitions are in progress. I have removed waitSeveralFrames as waiting isn't necessary.
lgtm
The CQ bit was checked by ericwilligers@chromium.org
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": 20001, "attempt_start_ts": 1484106461970920,
"parent_rev": "a22b359aaf415967f333e0a24adcf422e40a4024", "commit_rev":
"f6772d3dcc76f65a4b72cd81bd1ab25d3f295c6b"}
Message was sent while issue was closed.
Description was changed from ========== CSS Transitions: avoid flakes in cancel-transition.html We avoid timing dependence by rAF looping until the animation is in progress. BUG=248938 ========== to ========== CSS Transitions: avoid flakes in cancel-transition.html We avoid timing dependence by rAF looping until the animation is in progress. BUG=248938 Review-Url: https://codereview.chromium.org/2621603002 Cr-Commit-Position: refs/heads/master@{#442809} Committed: https://chromium.googlesource.com/chromium/src/+/f6772d3dcc76f65a4b72cd81bd1a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f6772d3dcc76f65a4b72cd81bd1a... |
