Summary: | [Qt] Add support for the Gamepad API | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marcelo Lira <marcelo.lira> | ||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Marcelo Lira <marcelo.lira> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | cmarcelo, jesus, menard, rafael.lobo, vestbo, webkit.review.bot | ||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||
Bug Blocks: | 92873 | ||||||||||||||||||||||||||
Attachments: |
|
Description
Marcelo Lira
2012-07-05 14:58:22 PDT
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.
Created attachment 150991 [details]
WebView to test the patch.
To run:
qmlscene joystick.qml
Created attachment 150992 [details]
HTML to test the patch.
*** Bug 90636 has been marked as a duplicate of this bug. *** (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)? Created attachment 155818 [details]
Patch
(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. Created attachment 155823 [details]
Patch
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) (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? :) 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. Created attachment 156083 [details]
Patch
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.
Created attachment 156086 [details]
Patch
Created attachment 156371 [details]
Patch
Created attachment 156376 [details]
HTML to test the patch.
This is a nice test/demo for the gamepad API.
Just saying. :)
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() 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. Created attachment 156533 [details]
Patch
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 Created attachment 156951 [details]
Patch
Comment on attachment 156951 [details] Patch Clearing flags on attachment: 156951 Committed r124904: <http://trac.webkit.org/changeset/124904> All reviewed patches have been landed. Closing bug. 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? (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> |