Bug 90637 – [Qt] Add support for the Gamepad API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Marcelo Lira]

[Qt] Add support for the Gamepad API

[Marcelo Lira]

Created attachment 150988 [details]
[Qt] Add support for the Gamepad API

The Qt port doesn't have support for the Gamepad API. The attached patch is an initial and unpolished implementation of the API. It depends on libudev for watching the gamepads. As the other ports it works only on Linux.

[Marcelo Lira]

Created attachment 150991 [details]
WebView to test the patch.

To run:
qmlscene joystick.qml

[Marcelo Lira]

Created attachment 150992 [details]
HTML to test the patch.

[Marcelo Lira]

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

[Alexis Menard (darktears)]

(In reply to comment #1)
> Created an attachment (id=150988) [details]
> [Qt] Add support for the Gamepad API
> 
> The Qt port doesn't have support for the Gamepad API. The attached patch is an initial and unpolished implementation of the API. It depends on libudev for watching the gamepads. As the other ports it works only on Linux.

Then you need to at least protect the new code with ifdef otherwise as it is today it will break Windows.

Does it unskip layout test (if you can even make one)?

[Marcelo Lira]

Created attachment 155818 [details]
Patch

[Marcelo Lira]

(In reply to comment #5)
> (In reply to comment #1)
> > Created an attachment (id=150988) [details] [details]
> > [Qt] Add support for the Gamepad API
> > 
> > The Qt port doesn't have support for the Gamepad API. The attached patch is an initial and unpolished implementation of the API. It depends on libudev for watching the gamepads. As the other ports it works only on Linux.
> 
> Then you need to at least protect the new code with ifdef otherwise as it is today it will break Windows.
Done.

> Does it unskip layout test (if you can even make one)?
Now it unskips the simpler layout test, that only checks if the Gamepade API is there.
The more complex one that requires including a gamepad controller to be manipulated via javascript is still skipped. I thought it would be best follow the EFL approach and make it a separated issue, so I created bug 92873.

[Marcelo Lira]

Created attachment 155823 [details]
Patch

[Jesus Sanchez-Palencia]

Comment on attachment 155823 [details]
Patch

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

Cool! And there is a lot of "raw" udev code around, huh?! :)

> Source/WebCore/platform/qt/GamepadsQt.cpp:33
> +#include "GamepadDeviceLinux.h"

Does it make sense to include a Linux specific header for all QtWebKit supported platorms? I got that GamepadDevice is only available for GNU/Linux, but shouldn't we then add this only for this specific platform? Or am I missing something here?

> Source/WebCore/platform/qt/GamepadsQt.cpp:50
> +class GamepadDeviceQt : public QObject, public GamepadDeviceLinux {

Ditto. (inheritance from the mentioned class)

[Marcelo Lira]

(In reply to comment #9)
> (From update of attachment 155823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155823&action=review
> 
> Cool! And there is a lot of "raw" udev code around, huh?! :)
> 
> > Source/WebCore/platform/qt/GamepadsQt.cpp:33
> > +#include "GamepadDeviceLinux.h"
> 
> Does it make sense to include a Linux specific header for all QtWebKit supported platorms? I got that GamepadDevice is only available for GNU/Linux, but shouldn't we then add this only for this specific platform? Or am I missing something here?
> 
> > Source/WebCore/platform/qt/GamepadsQt.cpp:50
> > +class GamepadDeviceQt : public QObject, public GamepadDeviceLinux {
> 
> Ditto. (inheritance from the mentioned class)

Until now the working (as in "with real gamepads") implementations (first Gtk and then Qt) were done only for linux. Since the Gamepad API is still in development, and behind a flag it is acceptable to go this way for the time being, to have something to test and work on, and later on we can try in platforms other than linux.

On the other hand, I would like to see how the compilation goes with the --gamepad in a windows machine, i.e. if the flag is properly ignored. Any volunteers? :)

[Alexis Menard (darktears)]

Comment on attachment 155823 [details]
Patch

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

> Source/WebCore/Target.pri:3100
> +contains(DEFINES, ENABLE_GAMEPAD=1) {

Make this conditional on linux based paltforms. qmake handle that easily.

[Marcelo Lira]

Created attachment 156083 [details]
Patch

[WebKit Review Bot]

Attachment 156083 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/platform/qt/GamepadsQt.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/qt/GamepadsQt.cpp:135:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marcelo Lira]

Created attachment 156086 [details]
Patch

[Marcelo Lira]

Created attachment 156371 [details]
Patch

[Marcelo Lira]

Created attachment 156376 [details]
HTML to test the patch.

This is a nice test/demo for the gamepad API.
Just saying. :)

[Noam Rosenthal]

Comment on attachment 156371 [details]
Patch

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

It's unclear from the filenames and some of the other naming choices that this is linux specific.

> Source/WebCore/platform/qt/GamepadsQt.cpp:31
> +// We do not use ENABLE or USE because moc does not expand these macros.
> +#if defined(ENABLE_GAMEPAD)

So, if we pass ENABLE_GAMEPAD=0 this would still be compiled in?
I prefer something like #if (ENABLE(GAMEPAD) || (MOC && ...))  or something like that.

> Source/WebCore/platform/qt/GamepadsQt.cpp:50
> +class GamepadDeviceQt : public QObject, public GamepadDeviceLinux {

Maybe GamepadDeviceLinuxQt?

> Source/WebCore/platform/qt/GamepadsQt.cpp:80
> +    delete m_notifier;

QSocketNotifier is a QObject, why not just make it a child of GamepadDeviceQt?

> Source/WebCore/platform/qt/GamepadsQt.cpp:201
> +        if (m_slots[i].get() && m_slots[i]->connected()) {

you don't need .get()

[Marcelo Lira]

Comment on attachment 156371 [details]
Patch

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

>> Source/WebCore/platform/qt/GamepadsQt.cpp:31
>> +#if defined(ENABLE_GAMEPAD)
> 
> So, if we pass ENABLE_GAMEPAD=0 this would still be compiled in?
> I prefer something like #if (ENABLE(GAMEPAD) || (MOC && ...))  or something like that.

It would, except that with ENABLE_GAMEPAD=0 the file will not even be included in the build by qmake.
Considering this the define should not even be there, but it was kinda informative so I made as the others do for features as this.

Your suggestion looks good, but moc can't handle "ENABLE()" in any way.
I will remove this #if completely, since the conditional exclusion of this file is already done in the "*.pri" files.

[Marcelo Lira]

Created attachment 156533 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156533 [details]
Patch

Rejecting attachment 156533 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
D at 169.
Hunk #2 succeeded at 216 with fuzz 2 (offset -29 lines).
1 out of 2 hunks FAILED -- saving rejects to file Tools/qmake/mkspecs/features/features.prf.rej
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/platform/qt/TestExpectations
Hunk #1 succeeded at 117 (offset -1 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Alexis Men..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13446744

[Marcelo Lira]

Created attachment 156951 [details]
Patch

[WebKit Review Bot]

Comment on attachment 156951 [details]
Patch

Clearing flags on attachment: 156951

Committed r124904: <http://trac.webkit.org/changeset/124904>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rafael Brandao]

I'm getting this when I build:
"Source/WebCore/platform/qt/GamepadsQt.cpp:83:62: error: 'read' was not declared in this scope", but I'm not sure of how to fix though. I didn't find this read being defined anywhere, so this kind of makes sense. Can you double check what is missing?

[Alexis Menard (darktears)]

(In reply to comment #24)
> I'm getting this when I build:
> "Source/WebCore/platform/qt/GamepadsQt.cpp:83:62: error: 'read' was not declared in this scope", but I'm not sure of how to fix though. I didn't find this read being defined anywhere, so this kind of makes sense. Can you double check what is missing?

Build fix landed : Committed r125027: <http://trac.webkit.org/changeset/125027>