Bug 92490

I skipped these tests until the proper fix in http://trac.webkit.org/changeset/123868

There are some fails on Qt Wk2 bot also. fast/animation/request-animation-frame-iframe.html: --- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-iframe-expected.txt +++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-iframe-actual.txt @@ -3,7 +3,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS callbackInvoked is true +FAIL callbackInvoked should be true. Was false. PASS successfullyParsed is true TEST COMPLETE fast/animation/request-animation-frame-timestamps-advance.html: --- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-timestamps-advance-expected.txt +++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-timestamps-advance-actual.txt @@ -1,12 +1,2 @@ -Tests the timestamps provided to requestAnimationFrame callbacks advance +FAIL: Timed out waiting for notifyDone to be called -On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". - - -PASS firstTimestamp is defined. -PASS secondTimestamp is defined. -PASS secondTimestamp > firstTimestamp is true -PASS successfullyParsed is true - -TEST COMPLETE - fast/animation/request-animation-frame-timestamps.html: --- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-timestamps-expected.txt +++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-timestamps-actual.txt @@ -3,10 +3,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS firstTimestamp is defined. -PASS secondTimestamp is defined. -PASS firstTimestamp is secondTimestamp -PASS firstTimestamp is defined. +FAIL firstTimestamp should be defined. Was undefined PASS successfullyParsed is true TEST COMPLETE fast/animation/request-animation-frame-within-callback.html: --- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-within-callback-expected.txt +++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-within-callback-actual.txt @@ -3,7 +3,6 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS sameFrame is false PASS successfullyParsed is true TEST COMPLETE fast/animation/request-animation-frame.html: --- /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-expected.txt +++ /ramdisk/qt-linux-32-release-webkit2/build/layout-test-results/fast/animation/request-animation-frame-actual.txt @@ -3,7 +3,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". -PASS callbackInvoked is true +FAIL callbackInvoked should be true. Was false. PASS successfullyParsed is true TEST COMPLETE

Created attachment 155167 [details] patch for review.

Comment on attachment 155167 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155167&action=review > Source/WebKit2/ChangeLog:8 > + Make sure that an actual repaint happens, when flushPendingLayerChanges > + is exeuted from within forceRepaint. In this case we do not want to wait > + for the UIProcess. This looks like a good fix, but is it related to the animation bug? > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.h:107 > + bool flushPendingLayerChanges(bool forceRepaint = false); Boolean trap. Please use an enum { ForceRepaint, DontForceRepaint }.

Comment on attachment 155167 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155167&action=review > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:254 > - m_waitingForUIProcess = true; > + m_waitingForUIProcess = !forceRepaint; Humm I know I hinted you this change, but I was wrong. I thought that the problem was that there was a m_waitingForUIProcess check when calling forceRepaint and it's not the case. The problem seems to be that since forceRepaint calls flushPendingLayerChanges, it doesn't end up calling serviceScriptedAnimations. So I think a more deterministic fix would be to move serviceScriptedAnimations deeper inside flushPendingLayerChange instead of performScheduledLayerFlush to make sure that it gets called when forceRepaint is called as well.

Created attachment 155255 [details] patch for review.

Comment on attachment 155255 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155255&action=review > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:181 > + m_webPage->layoutIfNeeded(); I thought flushPendingLayerChanges already calls layoutIfNeeded. isn't this superfluous?

Comment on attachment 155255 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155255&action=review >> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:181 >> + m_webPage->layoutIfNeeded(); > > I thought flushPendingLayerChanges already calls layoutIfNeeded. isn't this superfluous? No, flushPendingLayerChanges does not. performScheduledLayerFlush does. But this one returns early due to m_waitingForUIProcess in case of running layout tests.

Comment on attachment 155255 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155255&action=review > Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:180 > + m_webPage->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime())); I prefer it if this moved to a different function, e.g. LayerTreeCoordinator::serviceScriptedAnimations() >>> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:181 >>> + m_webPage->layoutIfNeeded(); >> >> I thought flushPendingLayerChanges already calls layoutIfNeeded. isn't this superfluous? > > No, flushPendingLayerChanges does not. performScheduledLayerFlush does. But this one returns early due to m_waitingForUIProcess in case of running layout tests. OK, then shouldn't we move layoutIfNeeded into flushPendingLayerChanges? I don't see how this behavior should be specific to rAF, it might break other tests as well.

Comment on attachment 155255 [details] patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=155255&action=review >>>> Source/WebKit2/WebProcess/WebPage/LayerTreeCoordinator/LayerTreeCoordinator.cpp:181 >>>> + m_webPage->layoutIfNeeded(); >>> >>> I thought flushPendingLayerChanges already calls layoutIfNeeded. isn't this superfluous? >> >> No, flushPendingLayerChanges does not. performScheduledLayerFlush does. But this one returns early due to m_waitingForUIProcess in case of running layout tests. > > OK, then shouldn't we move layoutIfNeeded into flushPendingLayerChanges? > I don't see how this behavior should be specific to rAF, it might break other tests as well. Or, even better, have something like a syncDisplayState() function that calls serviceAnimations, layout and flushLayerChanges, and call that function both from forceRepaint and from performFlush.

Created attachment 155284 [details] patch

Created attachment 155289 [details] patch

Comment on attachment 155289 [details] patch Attachment 155289 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13381836 New failing tests: platform/chromium/compositing/accelerated-drawing/svg-filters.html platform/chromium/compositing/accelerated-drawing/alpha.html

Created attachment 155310 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 155478 [details] patch for testing with cr-linux.

Comment on attachment 155289 [details] patch Cleared Noam Rosenthal's review+ from obsolete attachment 155289 [details] so that this bug does not appear in http://webkit.org/pending-commit.

And it made one more test fail in debug mode: expected.txt +++ /home/webkitbuildbot/slaves/debug64bit/buildslave/qt-linux-64-debug/build/layout-test-results/fast/animation/request-animation-frame-timestamps-actual.txt @@ -5,7 +5,7 @@ PASS firstTimestamp is defined. PASS secondTimestamp is defined. -PASS firstTimestamp is secondTimestamp +FAIL firstTimestamp should be 1343744605297. Was 1343744605207. PASS firstTimestamp is defined. PASS successfullyParsed is true

*** Bug 92759 has been marked as a duplicate of this bug. ***

Comment on attachment 155478 [details] patch for testing with cr-linux. Please remove the tests from the skipped list before landing.

And one more test on WK2: --- /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/dom/Window/post-message-crash-expected.txt +++ /home/webkitbuildbot/slaves/release64bitWebKit2_EC2/buildslave/qt-linux-64-release-webkit2/build/layout-test-results/fast/dom/Window/post-message-crash-actual.txt @@ -1,3 +1,3 @@ This ensures that postMessage called in a callback does not crash. -PASS +FAIL

Created attachment 155806 [details] patch for landing. - Unskip affected tests as well.

(In reply to comment #21) > Created an attachment (id=155806) [details] > patch for landing. - Unskip affected tests as well. If this is for landing - why the r? I thought we reviewed this patch already...

(In reply to comment #22) > If this is for landing - why the r? I thought we reviewed this patch already... I had to modify it slightly. The cr-linux bot was not happy with the reviewed one. Also i unskipped the tests.

Comment on attachment 155806 [details] patch for landing. - Unskip affected tests as well. Clearing flags on attachment: 155806 Committed r124336: <http://trac.webkit.org/changeset/124336>

All reviewed patches have been landed. Closing bug.