Summary: | REGRESSION(r233325): [GTK] Broke 40 animations tests | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||
Component: | Animations | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bugs-noreply, calvaris, cgarcia, dino, ews-watchlist, graouts, mcatanzaro, webkit-bug-importer, zan | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | PC | ||||||||
OS: | Linux | ||||||||
Attachments: |
|
Description
Michael Catanzaro
2018-07-02 14:02:41 PDT
Affected tests: webkit.org/b/187264 animations/transition-and-animation-2.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/animation-model/keyframe-effects/effect-value-context.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/cancel.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/pause.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/pending.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/play.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/playbackRate.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/ready.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/Animation/startTime.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/interfaces/AnimationEffectTiming/duration.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/canceling-an-animation.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/pausing-an-animation.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/playing-an-animation.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/reversing-an-animation.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-target-effect-of-an-animation.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/animations/set-the-timeline-of-an-animation.html [ Failure ] webkit.org/b/187264 imported/w3c/web-platform-tests/web-animations/timing-model/timelines/document-timelines.html [ Failure ] webkit.org/b/187264 compositing/animation/layer-for-filling-animation.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-animations/test_animation-cancel.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-animations/test_animation-computed-timing.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-animations/test_animation-currenttime.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-animations/test_animation-finish.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-animations/test_animation-pausing.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-animations/test_animation-ready.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-animations/test_animation-starttime.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-animations/test_setting-effect.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-cancel.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-computed-timing.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-currenttime.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-pausing.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-ready.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-transitions/test_animation-starttime.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-transitions/test_event-dispatch.html [ Timeout ] webkit.org/b/187264 imported/mozilla/css-transitions/test_setting-effect.html [ Timeout ] webkit.org/b/187264 transitions/move-after-transition.html [ Timeout ] webkit.org/b/187264 webanimations/opacity-animation-yields-compositing-span.html [ Timeout ] webkit.org/b/187264 webanimations/opacity-animation-yields-compositing.html [ Timeout ] Of these, all but two are timeouts (the other failures are timeouts not registered with the test runner). animations/transition-and-animation-2.html has a weird text diff: --- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/animations/transition-and-animation-2-expected.txt +++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/animations/transition-and-animation-2-actual.txt @@ -1,3 +1,3 @@ This test has a transition and animation on the same property (-webkit-transform). The animation starts and then the transition is triggered. The transition should start at the position before the animation started (the unanimated position), which is (0,0). If it starts from the start point of the animation (0,100) then there is an error -PASS - "webkitTransform.5" property for "box" element at 0.4s saw something close to: 0 +FAIL - "webkitTransform.5" property for "box" element at 0.4s expected: 0 but saw: matrix(1, 0, 0, 1, 0, 100) It almost looks unrelated, but the only animations-related commit within the range where it started failing is r233325, so I'll mark it against this bug. Then imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html started to fail with this diff: --- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context-expected.txt +++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context-actual.txt @@ -1,6 +1,6 @@ PASS Element.animate() creates an animation with the correct timeline when called on an element in a document without a browsing context PASS The timeline associated with an animation trigger on an element in a document without a browsing context is inactive -FAIL Replacing the timeline of an animation targetting an element in a document without a browsing context leaves it in the pending state assert_true: The animation should still be pending after replacing the document timeline expected true got false +PASS Replacing the timeline of an animation targetting an element in a document without a browsing context leaves it in the pending state PASS Replacing the timeline of an animation targetting an element in a document without a browsing context and then adopting that element causes it to start updating style I'll update our expectation for that test, since the PASS looks nicer than a FAIL to me. This is because DocumentAnimationScheduler uses DisplayRefreshMonitor, that is only used by GTK port when accelerated compositing is enabled. Tests work if AC is forced, and fail if AC is disabled or even on demand. In the case of being on demand it fails because it expects a monitor refresh before entering AC mode. I'm not sure how to solve this, though in the case of on demand AC. I've added a DisplayRefreshMonitorGtk to be used when not in AC mode and it works, but I think it conflicts with the threaded refresh display monitor created by the threaded compositor when entering AC. It seems to work, but it's probably inefficient. So, maybe we could try to enter AC whenever DocumentAnimationScheduler tries to create a display refresh monitor. Another option could be to destroy the GTK refresh monitor when entering AC. Created attachment 344275 [details]
Patch
Attachment 344275 [details] did not pass style-queue:
ERROR: Source/WebKit/Shared/CoordinatedGraphics/threadedcompositor/ThreadedCompositor.cpp:59: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 12 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 344275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344275&action=review LGTM but I think it needs to be run past Miguel or Zan. Do we now have two display refresh monitors running while in AC mode? > Source/WebCore/platform/graphics/gtk/DisplayRefreshMonitorGtk.cpp:62 > + m_window = gtk_offscreen_window_new(); I assume you've checked to ensure that you don't need to sink the ref. (In reply to Michael Catanzaro from comment #5) > Comment on attachment 344275 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344275&action=review > > LGTM but I think it needs to be run past Miguel or Zan. > > Do we now have two display refresh monitors running while in AC mode? No, the GTK one is destroyed when entering AC. > > Source/WebCore/platform/graphics/gtk/DisplayRefreshMonitorGtk.cpp:62 > > + m_window = gtk_offscreen_window_new(); > > I assume you've checked to ensure that you don't need to sink the ref. It's a toplevel. Comment on attachment 344275 [details] Patch Attachment 344275 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/8435876 New failing tests: animations/needs-layout.html Created attachment 344282 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Committed r233512: <https://trac.webkit.org/changeset/233512> |