The CoordinatedGraphics rendering backend needs an extra step of synchronisation in order to generate reliable pixel snapshots. Currently pixel results are generated at the end of the test run synchronously so we should slightly change WTR in order to allow delaying the dump after WKPageForceRepaint call us back.
Created attachment 167549 [details] Patch
Comment on attachment 167549 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167549&action=review > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:117 > + if (!s_watchDog) > + s_watchDog = new ForceRepaintDoneWatchDog(this); > + s_watchDog->start(); It feels like you're basically implementing what runUntil already does. How about adding the extra async step to the chain of steps that we have in TestInvocation::invoke, keep textOutput, pixelResult and repaintRects in member variables, and use runUntil there to wait until the platform code obtained the dumped pixels, and then dump the results? That would also prevent other platforms that want to use pixel tests with coordinated graphics classes to have to re-implement all this in their TestController.
Created attachment 167777 [details] Patch
Created attachment 168028 [details] Patch
Comment on attachment 168028 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168028&action=review > Tools/WebKitTestRunner/TestInvocation.h:57 > + // Some platforms want to force a repaint before generating pixel result. > + static void forceRepaintDoneCallback(WKErrorRef, void* context) > + { > + reinterpret_cast<TestInvocation*>(context)->forceRepaintDone(); > + } This can go in the .cpp > Tools/WebKitTestRunner/TestInvocation.h:58 > + void forceRepaintDone(); Maybe put this under #if PLATFORM(QT)? > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:78 > + TestController::shared().runUntil(m_gotRepaint, TestController::ShortTimeout); This is using the same instance of TestController and are running a runUntil on top of another runUntil, including the full stack up to didReceiveMessageFromInjectedBundle. If it really is working well this might be fine since it's simpler for now, but if it starts smelling bad we could pick parts of your last patch to have this runUntil running after the previous one is finished. > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:81 > + if (m_gotRepaint) { In which cases is this false, can't we just assert?
> This is using the same instance of TestController and are running a runUntil on top of another runUntil, including the full stack up to didReceiveMessageFromInjectedBundle. > If it really is working well this might be fine since it's simpler for now, but if it starts smelling bad we could pick parts of your last patch to have this runUntil running after the previous one is finished. Ok, I'm going to check how it affects performance and whether a nested runLoop could work for all platforms. > > > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:81 > > + if (m_gotRepaint) { > > In which cases is this false, can't we just assert? It is false if smg is wrong with the synchronisation. Given that it is a bit fragile according to our experiences in connection this work, and that we want to run this on bots I believe it's better to handle this case gracefully.
Created attachment 168202 [details] Patch
Comment on attachment 168202 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=168202&action=review > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:70 > + reinterpret_cast<TestInvocation*>(context)->m_gotRepaint = true; static_cast > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:90 > + } else { > + // The test harness expects an image so we output an empty one. > + WKRect windowRect = TestController::shared().mainWebView()->windowFrame(); > + image = QImage(QSize(windowRect.size.width, windowRect.size.height), QImage::Format_ARGB32_Premultiplied); > + image.fill(Qt::black); > + } That case is pretty silent, we could try to make it obvious from the test results by printing it red. We could also print an error if you end up delaying the stderr #EOF like you proposed.
Created attachment 168235 [details] Patch
> > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:70 > > + reinterpret_cast<TestInvocation*>(context)->m_gotRepaint = true; > > static_cast Fixed. > > > Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:90 > > + } else { > > + // The test harness expects an image so we output an empty one. > > + WKRect windowRect = TestController::shared().mainWebView()->windowFrame(); > > + image = QImage(QSize(windowRect.size.width, windowRect.size.height), QImage::Format_ARGB32_Premultiplied); > > + image.fill(Qt::black); > > + } > > That case is pretty silent, we could try to make it obvious from the test results by printing it red. We could also print an error if you end up delaying the stderr #EOF like you proposed. Ok, changed it to red. Later we can add the error message.
Comment on attachment 168235 [details] Patch Clearing flags on attachment: 168235 Committed r131160: <http://trac.webkit.org/changeset/131160>
All reviewed patches have been landed. Closing bug.
(In reply to comment #11) > (From update of attachment 168235 [details]) > Clearing flags on attachment: 168235 > > Committed r131160: <http://trac.webkit.org/changeset/131160> It made layout testing extremely slow - http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/9671/steps/layout-test 2.5 hours - 9800 tests :((((
Re-opened since this is blocked by bug 99163
(In reply to comment #14) > Re-opened since this is blocked by bug 99163 Sorry, I realized that it should not be landed without the patch in bug 95992 because it makes pixel and ref tests time out.
Let's do one more iteration as I realized that I am leaking memory.
Created attachment 168433 [details] Patch Fixed memory leak by correctly adopting the snapshot image.
Landed in http://trac.webkit.org/changeset/131306
Re-opened since this is blocked by bug 99354
(In reply to comment #18) > Landed in http://trac.webkit.org/changeset/131306 Rolled out, because it broke layout testing again - http://trac.webkit.org/changeset/131339 I stopped the test after 3 hours 8 minutes (after an editing test). Normally all tests take only 25 minutes.
See http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Amazon%20EC2%29/builds/9742 for details.
Landed again in http://trac.webkit.org/changeset/131725