Bug 109843 – [Qt][WK2] Support WK2 API tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Adenilson Cavalcanti Silva]

This is a meta ticket for enabling support so Qt could run WK2 API tests.

[Adenilson Cavalcanti Silva]

The first step is to implement a PlatformWebView (fortunately, both GTK and EFL could be used as a source of inspiration for it).

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 188375 [details]
Patch

Uploaded from wrong folder.

[Benjamin Poulain]

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?

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

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.

[Simon Hausmann]

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? :)

[Allan Sandfeld Jensen]

Created attachment 188554 [details]
FullPatch

Combined patch.

[Jocelyn Turcotte]

(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.

[Allan Sandfeld Jensen]

Created attachment 188567 [details]
Patch

Now with automatic detection of paths.

[Jocelyn Turcotte]

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.

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

Created attachment 188587 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 188591 [details]
Patch

[Jocelyn Turcotte]

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

[Jocelyn Turcotte]

(In reply to comment #16)
> Looks good to me otherwise, r=me

Benjamin, what do you think?

[Jocelyn Turcotte]

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.

[Allan Sandfeld Jensen]

Created attachment 188812 [details]
Patch

[Benjamin Poulain]

> (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.

[Jocelyn Turcotte]

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.

[Jocelyn Turcotte]

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.

[Allan Sandfeld Jensen]

Committed r143197: <http://trac.webkit.org/changeset/143197>