Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1586863002:
Devtools Animation: Deflake animation test (Closed)
|
Created:
4 years, 11 months ago by samli Modified:
4 years, 11 months ago Reviewers:
caseq CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-animation_chromium.org, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, Eric Willigers, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, rjwright, sergeyv+blink_chromium.org, shans Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevtools Animation: Deflake animation test
BUG=577246
Committed: https://crrev.com/df29988160d3c8e299dc5674a4165fdee2ce8902
Cr-Commit-Position: refs/heads/master@{#369339}
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : #Messages
Total messages: 18 (6 generated)
samli@chromium.org changed reviewers: + caseq@chromium.org
The CQ bit was checked by samli@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/1586863002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586863002/1
https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html (right): https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html:18: function rafWidth(resolve, reject) is this still relevant for this test? https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html:29: window.debugTest = true; drop this? https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html:40: InspectorTest.invokePageFunctionPromise("rafWidth", []).then(pause.bind(null, response.params.animation.id)); is pausing relevant for this test? https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html:45: InspectorTest.log("Box is animating: " + (width != 100).toString()); not sure if these are still meaningful.
https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html (right): https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html:18: function rafWidth(resolve, reject) On 2016/01/14 at 00:29:11, caseq wrote: > is this still relevant for this test? Yes. https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html:29: window.debugTest = true; On 2016/01/14 at 00:29:11, caseq wrote: > drop this? Done. https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html:40: InspectorTest.invokePageFunctionPromise("rafWidth", []).then(pause.bind(null, response.params.animation.id)); On 2016/01/14 at 00:29:11, caseq wrote: > is pausing relevant for this test? Yep. I'd like to isolate that release is working as intended since we generate a cloned animation which acts independently of the original. https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html:45: InspectorTest.log("Box is animating: " + (width != 100).toString()); On 2016/01/14 at 00:29:11, caseq wrote: > not sure if these are still meaningful. This is, yes.
https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html (right): https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html:18: function rafWidth(resolve, reject) On 2016/01/14 00:51:15, samli wrote: > On 2016/01/14 at 00:29:11, caseq wrote: > > is this still relevant for this test? > > Yes. My point was that with duration == 0 the animation takes effect synchronously and hence layoutAndPaintAsyncThen() is no longer necessary.
On 2016/01/14 at 00:59:10, caseq wrote: > https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html (right): > > https://codereview.chromium.org/1586863002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/inspector-protocol/animation/animation-release.html:18: function rafWidth(resolve, reject) > On 2016/01/14 00:51:15, samli wrote: > > On 2016/01/14 at 00:29:11, caseq wrote: > > > is this still relevant for this test? > > > > Yes. > > My point was that with duration == 0 the animation takes effect synchronously and hence layoutAndPaintAsyncThen() is no longer necessary. Ah ok. Yep, I've removed that now, thanks.
lgtm
The CQ bit was checked by samli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1586863002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586863002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by samli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1586863002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1586863002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Devtools Animation: Deflake animation test BUG=577246 ========== to ========== Devtools Animation: Deflake animation test BUG=577246 Committed: https://crrev.com/df29988160d3c8e299dc5674a4165fdee2ce8902 Cr-Commit-Position: refs/heads/master@{#369339} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/df29988160d3c8e299dc5674a4165fdee2ce8902 Cr-Commit-Position: refs/heads/master@{#369339} |
