This is a meta ticket for enabling support so Qt could run WK2 API tests.
The first step is to implement a PlatformWebView (fortunately, both GTK and EFL could be used as a source of inspiration for it).
Created attachment 188374 [details] Patch First bit of work on PlatformWebView. It also enables some other api tests. Currently the build test fails with: Failed to start " QtWebProcess 11" And using a QQuickWebView may not be what we want in the long run, but it seemed simple and little code to copy from WebKitTestRunner.
Created attachment 188375 [details] Patch Uploaded from wrong folder.
That is great! > And using a QQuickWebView may not be what we want in the long run, but it seemed simple and little code to copy from WebKitTestRunner. What is the problem with QQuickWebView for API tests?
(In reply to comment #4) > That is great! > > > And using a QQuickWebView may not be what we want in the long run, but it seemed simple and little code to copy from WebKitTestRunner. > > What is the problem with QQuickWebView for API tests? Mostly that we are using experimental API extensions for it, so we might want something more stable, hopefully something we can share with more platforms.
Created attachment 188550 [details] Patch First working patch. Needs WebProcess in PATH, and TEST_WEBKIT_API_INJECTED_BUNDLE_PATH needs to point to WebKitBuild/$RELEASE$/lib. Several WebKit2 tests are skipped though, because they stall.
Comment on attachment 188550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188550&action=review > Tools/TestWebKitAPI/TestWebKitAPI.pri:17 > + $$PWD/qt/PlatformWebViewQt.cpp Could it be that this file is missing from the diff? :)
Created attachment 188554 [details] FullPatch Combined patch.
(In reply to comment #6) > First working patch. Needs WebProcess in PATH, and TEST_WEBKIT_API_INJECTED_BUNDLE_PATH needs to point to WebKitBuild/$RELEASE$/lib. WTR is targetted to [Release|Debug]/bin to fix this. Other API tests have a function called addQtWebProcessToPath() that does this automatically.
Created attachment 188567 [details] Patch Now with automatic detection of paths.
Comment on attachment 188567 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188567&action=review > Tools/TestWebKitAPI/InjectedBundle.pri:10 > +INCLUDEPATH += $$PWD $${ROOT_WEBKIT_DIR}/Source/ThirdParty/gtest/include Individual lines please. > Tools/TestWebKitAPI/TestWebKitAPI.pro:7 > +SUBDIRS += derived_sources injected_bundle Tests/WTF Tests/JavaScriptCore Tests/WebKit2 This kind of list spans usually on individual lines too. > Tools/TestWebKitAPI/qt/PlatformUtilitiesQt.cpp:33 > #include <unistd.h> That can be tossed away with usleep. > Tools/TestWebKitAPI/qt/PlatformUtilitiesQt.cpp:63 > + // FIXME: Get the current test source dir, not TestWebKitAPI source dir. > + path = QStringLiteral(APITEST_SOURCE_DIR "/Tests/WebKit2"); It wouldn't work by defining it in JavaScriptCore/WTF/WebKit2.pro? > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:40 > +#include <QtQuick/private/qquickwindow_p.h> Can you check if you really need this? I think this was for QQuickWindowPrivate::setRenderWithoutShowing. If so you can also remove the whole QT += *-private stuff from TestWebKitAPI.pri. > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:95 > + // resized to what the layout test expects. Not a big deal, but layout tests shouldn't apply here. > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:120 > + QKeyEvent event(QEvent::KeyPress, ' ', Qt::NoModifier); > + QCoreApplication::sendEvent(m_window, &event); > + QKeyEvent event2(QEvent::KeyRelease, ' ', Qt::NoModifier); > + QCoreApplication::sendEvent(m_window, &event2); ' ' -> Qt::Key_Space > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:128 > + QKeyEvent event(QEvent::KeyPress, 0, Qt::AltModifier); > + QCoreApplication::sendEvent(m_window, &event); > + QKeyEvent event2(QEvent::KeyRelease, 0, Qt::AltModifier); > + QCoreApplication::sendEvent(m_window, &event2); 0 -> Qt::Key_Alt And probably without the modifier. > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:133 > + QPoint mousePos(x, y); QPointF > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:134 > + QMouseEvent event(QEvent::MouseMove, mousePos, mousePos, Qt::NoButton, Qt::NoButton, Qt::NoModifier); Can you use the overload without screenPos? > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:140 > + QPoint mousePos(x, y); Ditto QPointF. > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:141 > + QMouseEvent event(QEvent::MouseMove, mousePos, mousePos, Qt::NoButton, Qt::NoButton, Qt::NoModifier); Why? > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:146 > + QCoreApplication::sendEvent(m_window, &event); > + QMouseEvent event2(QEvent::MouseButtonPress, mousePos, mousePos, Qt::RightButton, Qt::RightButton, Qt::NoModifier); > + QCoreApplication::sendEvent(m_window, &event2); > + QMouseEvent event3(QEvent::MouseButtonRelease, mousePos, mousePos, Qt::RightButton, Qt::NoButton, Qt::NoModifier); > + QCoreApplication::sendEvent(m_window, &event3); Ditto about the overload.
Created attachment 188584 [details] Patch Evaluate the rest of the tests, suppress debug output, cleanup according to feedback, and allow to run in desktop mode since that changes the outcome of some tests.
(In reply to comment #11) > > Tools/TestWebKitAPI/qt/PlatformUtilitiesQt.cpp:63 > > + // FIXME: Get the current test source dir, not TestWebKitAPI source dir. > > + path = QStringLiteral(APITEST_SOURCE_DIR "/Tests/WebKit2"); > > It wouldn't work by defining it in JavaScriptCore/WTF/WebKit2.pro? > That would make compilation fail in directories where it is not defined. > > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:40 > > +#include <QtQuick/private/qquickwindow_p.h> > > Can you check if you really need this? > I think this was for QQuickWindowPrivate::setRenderWithoutShowing. > If so you can also remove the whole QT += *-private stuff from TestWebKitAPI.pri. > I need that, especially now that I also set desktop mode. We may later drop all of that, but it has already been useful. > > Tools/TestWebKitAPI/qt/PlatformWebViewQt.cpp:141 > > + QMouseEvent event(QEvent::MouseMove, mousePos, mousePos, Qt::NoButton, Qt::NoButton, Qt::NoModifier); > > Why? > This is just how mouse clicks are usually simulated; move, mouse-down, mouse-up.
Created attachment 188587 [details] Patch
Created attachment 188591 [details] Patch
Comment on attachment 188591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188591&action=review > Tools/TestWebKitAPI/Tests/WebKit2/WebKit2.pro:56 > +FAILING_SOURCES += SpacebarScrolling.cpp # Only fails in flickable mode. Since you use flickable mode by default those could also run, they won't show up on the bot anyway. > Tools/TestWebKitAPI/qt/main.cpp:28 > +namespace TestWebKitAPI { > +bool s_useDesktopBehavior = true; > +} Ugly but I guess that's fine. Looks good to me otherwise, r=me
(In reply to comment #16) > Looks good to me otherwise, r=me Benjamin, what do you think?
Comment on attachment 188591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188591&action=review >> Tools/TestWebKitAPI/qt/main.cpp:28 >> +} > > Ugly but I guess that's fine. > > Looks good to me otherwise, r=me Strike that, I must be drunk. setFlickableViewportEnabled is already static, no need for this.
Created attachment 188812 [details] Patch
> (In reply to comment #16) > > Looks good to me otherwise, r=me > > Benjamin, what do you think? I did not get the time to check the patch, but don't wait for me to land if the patch is correct.
Comment on attachment 188812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188812&action=review > Tools/TestWebKitAPI/qt/PlatformUtilitiesQt.cpp:50 > + QString path = QString::fromLocal8Bit(qgetenv("TEST_WEBKIT_API_INJECTED_BUNDLE_PATH")); > + if (path.isEmpty()) One more thing that I just realized. I think that the reason GTK is using env vars here is because they didn't integrate it with their build system. No need to do both, we should either hardcode the path, either pass the path from the test runner script.
Comment on attachment 188812 [details] Patch (In reply to comment #21) > One more thing that I just realized. > I think that the reason GTK is using env vars here is because they didn't integrate it with their build system. > No need to do both, we should either hardcode the path, either pass the path from the test runner script. r=me if you remove the env part.
Committed r143197: <http://trac.webkit.org/changeset/143197>