Bug 50050 – [Qt] plugins/keyboard-events.html fails after r72717

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Martin Robinson]

Fixes to the Unix version of TestNetscapePlugin have uncovered a bug in PluginViewQt. This is related to a special-case for DumpRenderTree.

[Martin Robinson]

Committed r72719: <http://trac.webkit.org/changeset/72719>

[WebKit Review Bot]

http://trac.webkit.org/changeset/72719 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
editing/selection/extend-by-character-002.html

[Robert Hogan]

Created attachment 79103 [details]
Patch

[Andreas Kling]

Comment on attachment 79103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79103&action=review

> Source/WebCore/plugins/qt/PluginViewQt.cpp:427
> +        const char* chr = qKeyEvent->text().left(1).toUtf8().constData();
> +        xEvent->xkey.keycode = XKeysymToKeycode(QX11Info::display(), XStringToKeysym(chr));

'chr' points to deleted data on the second line since the QByteArray returned by toUtf8() is temporary.

[Robert Hogan]

Created attachment 79106 [details]
Patch

[Andreas Kling]

Comment on attachment 79106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=79106&action=review

LGTM, thanks for attending to this!

> Source/WebCore/plugins/qt/PluginViewQt.cpp:426
> +        QString chr = qKeyEvent->text().left(1);

Nit: We don't abbreviate variable names in WebKit.

[Robert Hogan]

Created attachment 79108 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 79108 [details]
Patch

Clearing flags on attachment: 79108

Committed r75894: <http://trac.webkit.org/changeset/75894>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Robert Hogan]

Committed r75895: <http://trac.webkit.org/changeset/75895>

[WebKit Review Bot]

http://trac.webkit.org/changeset/75897 might have broken Leopard Intel Release (Build)

[Robert Hogan]

I had to re-skip plugins/keyboard-events.html because it failed on the buildbot.

The change that broke this originally for Qt is http://trac.webkit.org/changeset/72717

I think the reason my patch failed is as follows, and am hoping Martin might be amenable to going back to using the keysym for this event.

In order to get the keycode of the keyboard event, we have to do:

xEvent->xkey.keycode = XKeysymToKeycode(QX11Info::display(), XStringToKeysym(keyText.toUtf8().constData()));

At first I thought it failed on the buildbot because it was headless, but the tests are run under xvfb so that was only half-right. The real reason, it seems to me, is because xvfb doesn't have a keyboard, so XKeysymToKeycode will always fail. (See http://lists.apple.com/archives/x11-users/2009/Apr/msg00016.html for an example of this elsewhere).

Since there's no keyboard mapping available to xvfb it can't determine the keycode. I recreated the problem in Ossy's 'QtWebKit Builder and Tester VM (Q-BAT VM)'.

I'm going to drop to a terminal session on this machine with no x server running so see if I can recreate it here too. I'll come back with my results but it looks as if using the keycodes won't work for us running our buildbot under xvfb.

Looking at the output from the other buildbots I think qt may be alone running the layout tests under xvfb. Who would be able to confirm that do you know?

[Robert Hogan]

So much for that theory - I can pass the test using xvfb-run in a session with no X server running on my own machine. The test always fails for me in the VM.

My version of Xvfb is from xorg-server 1.7.6, the version of Xvfb on the VM/buildbot is 1.4.2. Mmmm...

So that's my new copper-bottomed theory. I guess we wait for the buildbot to upgrade to debian squeeze and try again!

[Csaba Osztrogonác]

Bbandix, could you check Robert's idea on your hyper-up-to-date Arch Linux? :)

[Robert Hogan]

For ease of reference, this should pass:

xvfb-run --server-args="-screen 0 1024x768x24" Tools/Scripts/run-webkit-tests --
no-launch-safari --qt plugins/keyboard-events.html $@

[Robert Hogan]

Committed r86938: <http://trac.webkit.org/changeset/86938>