Bug 87503 – [Gtk] Add support for the Gamepad API

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Zan Dobersek]

This bug covers adding support for the Gamepad API[1] to the Gtk port. The implementation will rely on GUdev and GIO.

[Zan Dobersek]

Currently the implementation depends on two bugs to be resolved:
- bug #84929, the patch there will add the majority of support for compiling the gamepad module for Gtk
- bug #84863, this will add proper support for the T[] attributes in IDLs, such as 'axes' and 'buttons' attributes in Gamepad.idl

[Zan Dobersek]

Created attachment 144109 [details]
WIP patch

This is just a work-in-progress patch. It currently holds all the changes that are scheduled to be reviewed in bug #84929 and landed separately and bypasses the issue covered in bug #84863 by switching the 'axes' and 'buttons' attributes' types to sequence<float>.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebCore/platform/gtk/GamepadsGtk.cpp:124:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/gtk/GamepadsGtk.cpp:188:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 11 files


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

[Adam Barth]

Comment on attachment 144109 [details]
WIP patch

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

> Source/WebCore/Modules/gamepad/Gamepad.idl:35
> -        readonly attribute float[] axes;
> -        readonly attribute float[] buttons;
> +        readonly attribute sequence<float> axes;
> +        readonly attribute sequence<float> buttons;

Is this change correct?  We should check the spec.  Sadly, float[] and sequence<float> are subtly different.  I think you're right through that most attributes use sequence<float>.

[WebKit Review Bot]

Comment on attachment 144109 [details]
WIP patch

Attachment 144109 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12853137

[Zan Dobersek]

(In reply to comment #4)
> (From update of attachment 144109 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144109&action=review
> 
> > Source/WebCore/Modules/gamepad/Gamepad.idl:35
> > -        readonly attribute float[] axes;
> > -        readonly attribute float[] buttons;
> > +        readonly attribute sequence<float> axes;
> > +        readonly attribute sequence<float> buttons;
> 
> Is this change correct?  We should check the spec.  Sadly, float[] and sequence<float> are subtly different.  I think you're right through that most attributes use sequence<float>.

As said in comment #2, this is just a workaround as JSC doesn't support attributes with array type.

Bug #84863 is working towards proper support for T[] and sequence<T> types as it was outlined back in https://bugs.webkit.org/show_bug.cgi?id=80696#c67 so this bug depends on it.

[Adam Barth]

Ah, thanks.

[Carlos Garcia Campos]

Comment on attachment 144109 [details]
WIP patch

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

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:35
> +#include <linux/joystick.h>

Maybe we should check this header is available in configure and add an #ifded here or simply disable gamepad if not present.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:45
> +class GUdevGamepadDevice {

This class has nothing to do with GUdev in the end, it's a linux specific implementation. Maybe we could even move this to its own file that could be shared by other ports running on linux like Qt or efl.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:55
> +    GUdevGamepadDevice(char* deviceFile);

This should be const char*

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:57
> +    GInputStream* m_inputStream;

You could use GRefPtr for the input stream

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:70
> +PassOwnPtr<GUdevGamepadDevice> GUdevGamepadDevice::create(char* deviceFile)

const char* here too

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:92
> +    ioctl(fd, JSIOCGAXES, &m_numberOfAxes);
> +    ioctl(fd, JSIOCGBUTTONS, &m_numberOfButtons);

We should probably check the return value and not fill the arrays in case of error

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:97
> +    m_source = g_pollable_input_stream_create_source(G_POLLABLE_INPUT_STREAM(m_inputStream), NULL);

Use 0 instead of NULL.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:98
> +    g_source_set_callback(m_source, (GSourceFunc)readCallback, this, NULL);

Use a C++ style cast and 0 instead of NULL.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:104
> +    g_source_unref(m_source);

This is not enough to remove the source from the context. In g_source_attach() the context takes a reference of the source, and you have another one, so here the source is not destroyed. so, you should either use g_source_destroy() or return FALSE from the read callback when done.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:108
> +PassRefPtr<Gamepad> GUdevGamepadDevice::createGamepad(int index)

Use unsigned instead of int, since it's what Gamepad::index() expects.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:113
> +    gamepad->timestamp(monotonicallyIncreasingTime());

js_event contains a timestamp, shouldn't we use that instead?

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:120
> +void GUdevGamepadDevice::updateFromInputStream()

I would call this readFromInputStream()

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:127
> +    if (len < 0)
> +        return;

We should check whether the error is G_IO_ERROR_WOULD_BLOCK to destroy the source otherwise (or report the error somehow using g_printerr or g_warning)

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:134
> +    if (len == sizeof(event)) {
> +        if (event.type & JS_EVENT_AXIS)
> +            m_axes[event.number] = event.value / 32767.0f;
> +        else if (event.type & JS_EVENT_BUTTON)
> +            m_buttons[event.number] = event.value / 1.0f;
> +    }

This could be an early return. If we move the linux impl to its one class, this would notify such class using something like gamepadDevice->handleEvent(event);

Those numbers look like magic numbers, we could add a constant or something to give them a name. We could also add a comment to explain why that is needed or even helper functions like normalizeAxisValue() normalizeButtonValue(), for example

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:163
> +    DEFINE_STATIC_LOCAL(GRegex*, sysfsPathRegex, (g_regex_new("/input[0-9]+/js[0-9]+$", (GRegexCompileFlags)0, (GRegexMatchFlags)0, 0)));
> +    gboolean match = g_regex_match(sysfsPathRegex, sysfsPath, (GRegexMatchFlags)0, 0);
> +    if (!match)
> +        return FALSE;
> +
> +    return TRUE;

Instead of using a regexp with the syspath we could just check whether the device file starts with /dev/input/js, this could be just

return g_str_has_prefix(deviceFile, "/dev/input/js");

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:171
> +    char* deviceFile = const_cast<char*>(g_udev_device_get_device_file(device));

Why do you need this to be char instead of const char*?

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:184
> +    static GUdevClient* gudevClient = 0;

You could use GRefPtr to make sure the client is freed

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:190
> +    g_signal_connect(gudevClient, "uevent", G_CALLBACK(onUEventCallback), NULL);

Use 0 instead of NULL

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:197
> +            gamepadDeviceMap.set(deviceFile, GUdevGamepadDevice::create(deviceFile));

deviceFile is owned by GUdevDevice, does HashMap copy the string? Maybe we could use String or CString as the key of the HashMap

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:200
> +    g_list_free_full(devicesList, g_object_unref);

This iterates the list again, you could call g_object_unref() for every item in the previous loop and here just call g_list_free()

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:211
> +        RefPtr<Gamepad> gamepad = device->createGamepad(index);

Looking at chromium implementation, it seems that GamepadList might not be empty, they check first whether there's a Gamepad already in the array to updated it instead of creating a new one

> configure.ac:1264
> +   PKG_CHECK_MODULES([GIO_UNIX], [gio-unix-2.0])
> +   PKG_CHECK_MODULES([GUDEV], [gudev-1.0])

We could merge these and use GAMEPAD_CFLAGS and GAMEPAD_LIBS. We should also disable gamepad if these can't be found.

[Zan Dobersek]

Comment on attachment 144109 [details]
WIP patch

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

Thanks for the review, all other comments are noted as well.

>> Source/WebCore/platform/gtk/GamepadsGtk.cpp:35
>> +#include <linux/joystick.h>
> 
> Maybe we should check this header is available in configure and add an #ifded here or simply disable gamepad if not present.

The latter would be best - I can't provide support for any other OS than Linux.

>> Source/WebCore/platform/gtk/GamepadsGtk.cpp:45
>> +class GUdevGamepadDevice {
> 
> This class has nothing to do with GUdev in the end, it's a linux specific implementation. Maybe we could even move this to its own file that could be shared by other ports running on linux like Qt or efl.

Current implementation is still GIO-specific though, so at least Qt wouldn't have any benefit from it, don't know about EFL. It still deserves a separate file.

>> Source/WebCore/platform/gtk/GamepadsGtk.cpp:113
>> +    gamepad->timestamp(monotonicallyIncreasingTime());
> 
> js_event contains a timestamp, shouldn't we use that instead?

True, js_event's timestamp should suffice.

>> Source/WebCore/platform/gtk/GamepadsGtk.cpp:171
>> +    char* deviceFile = const_cast<char*>(g_udev_device_get_device_file(device));
> 
> Why do you need this to be char instead of const char*?

The const casting is a leftover of another approach at handling GUdev devices that I forgot to remove. const char* will work.

>> Source/WebCore/platform/gtk/GamepadsGtk.cpp:211
>> +        RefPtr<Gamepad> gamepad = device->createGamepad(index);
> 
> Looking at chromium implementation, it seems that GamepadList might not be empty, they check first whether there's a Gamepad already in the array to updated it instead of creating a new one

That's a better approach indeed. Along with that the GUdevGamepadDevice (to be renamed) should also support handling the connected state properly to conform to the specification (more directly to paragraph 5.1: https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#idl-def-Navigator)

>> configure.ac:1264
>> +   PKG_CHECK_MODULES([GUDEV], [gudev-1.0])
> 
> We could merge these and use GAMEPAD_CFLAGS and GAMEPAD_LIBS. We should also disable gamepad if these can't be found.

This would work for now.

[Carlos Garcia Campos]

(In reply to comment #9)
> (From update of attachment 144109 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144109&action=review
> 
> Thanks for the review, all other comments are noted as well.
> 
> >> Source/WebCore/platform/gtk/GamepadsGtk.cpp:45
> >> +class GUdevGamepadDevice {
> > 
> > This class has nothing to do with GUdev in the end, it's a linux specific implementation. Maybe we could even move this to its own file that could be shared by other ports running on linux like Qt or efl.
> 
> Current implementation is still GIO-specific though, so at least Qt wouldn't have any benefit from it, don't know about EFL. It still deserves a separate file.

I mean creating a new calls with only linux specific things. In GamepadsGtk we would create the stream to notify the generic class when an event happens. That way the generic class doesn't need to use anything from glib/gio

[Philippe Normand]

Comment on attachment 144109 [details]
WIP patch

r- because it breaks EWS build (and all the next ones until I do a clean build)

  CC     Source/WebKit/gtk/tests/Programs_unittests_testdomdomwindow-testdomdomwindow.o
In file included from ./DerivedSources/webkit/webkitdom.h:131:0,
                 from ../../Source/WebKit/gtk/webkit/webkit.h:27,
                 from ../../Source/WebKit/gtk/tests/testapplicationcache.c:23:
./DerivedSources/webkit/WebKitDOMNavigator.h:215:1: error: unknown type name 'WebKitDOMGamepadList'
make[1]: *** [Source/WebKit/gtk/tests/Programs_unittests_testapplicationcache-testapplicationcache.o] Error 1
make[1]: *** Waiting for unfinished jobs....
In file included from ./DerivedSources/webkit/webkitdom.h:131:0,
                 from ../../Source/WebKit/gtk/webkit/webkit.h:27,
                 from ../../Source/WebKit/gtk/tests/testcontextmenu.c:20:
./DerivedSources/webkit/WebKitDOMNavigator.h:215:1: error: unknown type name 'WebKitDOMGamepadList'
make[1]: *** [Source/WebKit/gtk/tests/Programs_unittests_testcontextmenu-testcontextmenu.o] Error 1
In file included from ./DerivedSources/webkit/webkitdom.h:131:0,
                 from ../../Source/WebKit/gtk/webkit/webkit.h:27,
                 from ../../Source/WebKit/gtk/tests/testdomdocument.c:25:
./DerivedSources/webkit/WebKitDOMNavigator.h:215:1: error: unknown type name 'WebKitDOMGamepadList'
make[1]: *** [Source/WebKit/gtk/tests/Programs_unittests_testdomdocument-testdomdocument.o] Error 1
In file included from ./DerivedSources/webkit/webkitdom.h:131:0,
                 from ../../Source/WebKit/gtk/webkit/webkit.h:27,
                 from ../../Source/WebKit/gtk/tests/testdomdomwindow.c:25:
./DerivedSources/webkit/WebKitDOMNavigator.h:215:1: error: unknown type name 'WebKitDOMGamepadList'
make[1]: *** [Source/WebKit/gtk/tests/Programs_unittests_testdomdomwindow-testdomdomwindow.o] Error 1
make[1]: Leaving directory `/home/phil/gst/jhbuild/makes/WebKit/WebKitBuild/Release'

[Philippe Normand]

Link to the build log: http://queues.webkit.org/results/12839872

[Philippe Normand]

I know GTK's build system sucks but OTOH it doesn't make much sense to mark this patch r? and have EWS try it without first landing at least bug 84929.

EWS should apply patch dependencies before trying to build, I guess.

[Carlos Garcia Campos]

(In reply to comment #13)
> I know GTK's build system sucks but OTOH it doesn't make much sense to mark this patch r? and have EWS try it without first landing at least bug 84929.

Zan said this was a wip patch, and he didn't mark this patch r?, see the log

https://bugs.webkit.org/show_activity.cgi?id=87503

[Philippe Normand]

(In reply to comment #14)
> (In reply to comment #13)
> > I know GTK's build system sucks but OTOH it doesn't make much sense to mark this patch r? and have EWS try it without first landing at least bug 84929.
> 
> Zan said this was a wip patch, and he didn't mark this patch r?, see the log
> 
> https://bugs.webkit.org/show_activity.cgi?id=87503

Maybe someone clicked on the fancy "Submit for EWS analysis" button then :) Anyway, no big deal...

[Zan Dobersek]

(In reply to comment #11)
> (From update of attachment 144109 [details])
> r- because it breaks EWS build (and all the next ones until I do a clean build)
> 
>   CC     Source/WebKit/gtk/tests/Programs_unittests_testdomdomwindow-testdomdomwindow.o
> In file included from ./DerivedSources/webkit/webkitdom.h:131:0,
>                  from ../../Source/WebKit/gtk/webkit/webkit.h:27,
>                  from ../../Source/WebKit/gtk/tests/testapplicationcache.c:23:
> ./DerivedSources/webkit/WebKitDOMNavigator.h:215:1: error: unknown type name 'WebKitDOMGamepadList'
> make[1]: *** [Source/WebKit/gtk/tests/Programs_unittests_testapplicationcache-testapplicationcache.o] Error 1
> make[1]: *** Waiting for unfinished jobs....
> In file included from ./DerivedSources/webkit/webkitdom.h:131:0,
>                  from ../../Source/WebKit/gtk/webkit/webkit.h:27,
>                  from ../../Source/WebKit/gtk/tests/testcontextmenu.c:20:
> ./DerivedSources/webkit/WebKitDOMNavigator.h:215:1: error: unknown type name 'WebKitDOMGamepadList'
> make[1]: *** [Source/WebKit/gtk/tests/Programs_unittests_testcontextmenu-testcontextmenu.o] Error 1
> In file included from ./DerivedSources/webkit/webkitdom.h:131:0,
>                  from ../../Source/WebKit/gtk/webkit/webkit.h:27,
>                  from ../../Source/WebKit/gtk/tests/testdomdocument.c:25:
> ./DerivedSources/webkit/WebKitDOMNavigator.h:215:1: error: unknown type name 'WebKitDOMGamepadList'
> make[1]: *** [Source/WebKit/gtk/tests/Programs_unittests_testdomdocument-testdomdocument.o] Error 1
> In file included from ./DerivedSources/webkit/webkitdom.h:131:0,
>                  from ../../Source/WebKit/gtk/webkit/webkit.h:27,
>                  from ../../Source/WebKit/gtk/tests/testdomdomwindow.c:25:
> ./DerivedSources/webkit/WebKitDOMNavigator.h:215:1: error: unknown type name 'WebKitDOMGamepadList'
> make[1]: *** [Source/WebKit/gtk/tests/Programs_unittests_testdomdomwindow-testdomdomwindow.o] Error 1
> make[1]: Leaving directory `/home/phil/gst/jhbuild/makes/WebKit/WebKitBuild/Release'

This failure occurred because of improperly handled GObject bindings for Gamepad-related stuff when building with the gamepad feature disabled. Will be fixed in next patch.

[Zan Dobersek]

Comment on attachment 144109 [details]
WIP patch

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

>>> Source/WebCore/platform/gtk/GamepadsGtk.cpp:113
>>> +    gamepad->timestamp(monotonicallyIncreasingTime());
>> 
>> js_event contains a timestamp, shouldn't we use that instead?
> 
> True, js_event's timestamp should suffice.

Turns out event's timestamp is based on jiffies - it's useless as it is not indicating a time since epoch as it is required for the gamepad timestamp.

[Zan Dobersek]

Created attachment 145446 [details]
WIP patch

New version.

[Zan Dobersek]

(In reply to comment #18)
> Created an attachment (id=145446) [details]
> WIP patch
> 
> New version.

The Gamepad feature is now only enabled on Linux, with bindings properly generating (apart from the lacking support for sequence<T>/T[] types).

The implementation now relies on a GamepadDeviceLinuxInputProvider to provide file descriptor of the device and properly read from it, and a GamepadDeviceLinuxInputProviderClient that can be updated for a joystick event. The names are long and hard to read, if we're to stick with this infrastructure they should probably be renamed to something better.

GamepadDeviceLinux class is now present as a GamepadDeviceLinuxInputProviderClient. It operates with the file descriptor of the device to get all the data when setting the device up (device name, number of axes and buttons). It can be updated for a js_event, with axis and button values now being normalized in a more plain and obvious way.

GamepadDeviceInputProviderGUdev (just realized, it again has little connection to gudev - perhaps GamepadDeviceInputProviderGIO would go better) is a GamepadDeviceLinuxInputProvider, its task being opening the device file and establishing the polling when required. It reads from the descriptor and updates the client with joystick events.

GamepadDeviceMapper is a class that assigns devices to appropriate slots. Its task is to gather all the devices at the start and handle newly-added and removed ones. Its slot count is the same as the GamepadList's length when created, filling the slots with GamepadDeviceLinux objects. Their initial state is 'null', meaning physical devices are not connected yet to those slots. But when such device is registered, it is given a proper slot and provided an input provider to update it. When the physical device is removed, the corresponding GamepadDeviceLinux is cleaned up and returned into the 'null' state. This way the implementation properly handles the 'not-connected devices'[1].

As for comment #17, the spec doesn't strictly request timestamp to be a value of (milli)seconds since epoch, so the js_event timestamp is used until that changes (if ever).

This patch is still missing some proper logging (LOG(Gamepad, ...) should be used) and handling of the possible G_IO_ERROR_WOULD_BLOCK error when reading from input stream.

[1] - https://dvcs.w3.org/hg/gamepad/raw-file/default/gamepad.html#widl-Navigator-gamepads

[Carlos Garcia Campos]

I think this could be simplified a bit. For example, a GamepadsGtk class that uses udev to register/unregister gamepads, similar to your current DeviceMapper class. The GamepadDeviceLinux class could be created with a deviceFile, so that it contains the file descriptor that would be initiliazed in the constructor. And then a class GamepadDeviceGtk that inherits from GamepadDeviceLinux that creates a GInputStream for the descriptor in the constructor. When an event is read from the stream it simply calls handleEvent() implemented in the parent class GamepadDeviceLinux. What do you think?

[Zan Dobersek]

Created attachment 145856 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 15 files


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

[Zan Dobersek]

(In reply to comment #21)
> Created an attachment (id=145856) [details]
> Patch

The client and provider classes are now removed. GamepadDeviceGtk inherits from GamepadDeviceLinux. The constructor of the latter takes in a String that represents the device file. If it is null we don't try to open the file but rather consider the device to be in null-state - it's just filling the slot in the device mapping class, giving room to a proper device when it is connected. If the device file is valid it is open, acquiring the file descriptor, device name and buttons and axes count.

GamepadDeviceGtk only goes on to open the input stream and attach the source to the main context when the file descriptor is valid (it is not valid in the case of an error or a null device). Perhaps this class could be moved into a file of its own.

GamepadsGtk is the renamed device-mapping class, initially filling the slots with null-state GamepadDeviceGtk objects. On initialization it also goes through the current devices and registers any joystick devices present. On registering devices, the slot with a device in null-state is filled in with a proper GamepadDeviceGtk object with according device file. On unregistering devices, the slot with the device being unregistered is filled in with a new device in null-state.

Still missing is better error-handling and logging. The code would also benefit from some comments on what is being done in specific places, just for clarity really.

[WebKit Review Bot]

Comment on attachment 145856 [details]
Patch

Attachment 145856 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12896835

[Carlos Garcia Campos]

Comment on attachment 145856 [details]
Patch

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

It looks much better. Setting r- because of the memory leaks, coding style issues and other minor issues.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:56
> +    GSource* m_source;

You should use GRefPtr for the source too, g_source_destroy() removed the source from the context and marks it as destroyed, but the memory is not freed.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:62
> +    , m_inputStream(0)

This is not needd GRefPtr already initializes its internal pointer to 0

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:68
> +    m_inputStream = g_unix_input_stream_new(m_fileDescriptor, TRUE);

You should adopt the reference with adoptGRef(). It's a bit weird that this clases closes the descriptor that was open by the parent class. Even if it's conveneint, I guess it would be better to pass FALSE here, close the descriptor in the parent destructor.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:69
> +    m_source = g_pollable_input_stream_create_source(G_POLLABLE_INPUT_STREAM(m_inputStream.get()), 0);

Use adoptGRef here too when m_source is changed to GRefPtr

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:71
> +    g_source_attach(m_source, NULL);

Use 0 instead of NULL.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:80
> +void GamepadDeviceGtk::readFromInputStream()

Make this return a bool and return false if an error happens, that way we can cancel the source in case of errors

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:101
> +    gamepadDevice->readFromInputStream();
> +
> +    return TRUE;

And this would be return gamepadDevice->readFromInputStream(); Or maybe we can read the stream here, since we have a pointer to the stream, and simply call gamepadDevice->updateForEvent(event);

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:119
> +    Vector<OwnPtr<GamepadDeviceGtk> > m_slots;
> +    HashMap<String, GamepadDeviceGtk*> m_deviceMap;

Why do we need both the Vector and the HashMap, isn't it enough with the HashMap

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:131
> +    for (unsigned i = 0; i < m_length; i++)
> +        m_slots.append(GamepadDeviceGtk::create());

Why not creating the devices on demand?

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:134
> +    m_gudevClient = g_udev_client_new(subsystems);

You should use adoptGRef() here too

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:137
> +    GList* devicesList = g_udev_client_query_by_subsystem(m_gudevClient.get(), subsystems[0]);

You can use GOwnPtr for the list, since the items are already freed in the loop

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:139
> +        GUdevDevice* device = reinterpret_cast<GUdevDevice*>(listItem->data);

For GObjects it's better to use the GObject cast macros, that also check the glib type. G_UDEV_DEVICE(listIem->data) in this case

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:167
> +    m_slots[index] = GamepadDeviceGtk::create(deviceFile);

m_slots[index] already contains a GamepadDeviceGtk

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:192
> +            m_slots[i]->updateGamepad(gamepad.get());

I think it would be clearer to add getters to GamepadDevice class and fill the Gaempad object here using those getters, similar to what chromium does.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:205
> +    String deviceFile = String(g_udev_device_get_device_file(device));

I guess g_udev_device_get_device_file() returns an UTF8 string, so you should use String::fromUTF8(). But I think it would be better to pass the returned char * to the registerDevice and unregisterDevice methods, and convert to String when accesing the HashMap.

> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:39
> +    : m_deviceFile(deviceFile)

Do we need to keep the deviceFile? Maybe we could use CString instead of String mor simply pass the char * and use it directly in open().

> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:47
> +    if (isNull())
> +        return;

I still don't get why we want null devices.

> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:73
> +    gamepad->id(m_deviceName);
> +    gamepad->timestamp(m_lastTimestamp);
> +    gamepad->axes(m_numberOfAxes, m_axes.data());
> +    gamepad->buttons(m_numberOfButtons, m_buttons.data());

As I said I thnk this would be clearer in the caller, adding getters for m_deviceName, m_lastTimestamp, axes and buttons.

[Zan Dobersek]

Comment on attachment 145856 [details]
Patch

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

>> Source/WebCore/platform/gtk/GamepadsGtk.cpp:119
>> +    HashMap<String, GamepadDeviceGtk*> m_deviceMap;
> 
> Why do we need both the Vector and the HashMap, isn't it enough with the HashMap

Vector is the same size as the GamepadsList, storing OwnPtrs to gamepad devices in a specific slot. This ensures that when two devices are connected in the first two slots and the device in the first slot is removed, the second device doesn't move into the now-empty slot.

HashMap maps gamepad devices to device file names. That way it's easy to control currently-present devices.

>> Source/WebCore/platform/gtk/GamepadsGtk.cpp:205
>> +    String deviceFile = String(g_udev_device_get_device_file(device));
> 
> I guess g_udev_device_get_device_file() returns an UTF8 string, so you should use String::fromUTF8(). But I think it would be better to pass the returned char * to the registerDevice and unregisterDevice methods, and convert to String when accesing the HashMap.

To me it seems to better to use String::fromUTF8 here and pass String around, that way the passed argument in registerDevice and unregisterDevice can be immediately checked for it's (non)presence in the HashMap.

>> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:47
>> +        return;
> 
> I still don't get why we want null devices.

Indeed, that's an overkill.

[Zan Dobersek]

Created attachment 146326 [details]
WIP patch

Yet another WIP patch! Still missing logging and G_IO_ERROR_WOULD_BLOCK handling.

[Carlos Garcia Campos]

(In reply to comment #26)
> (From update of attachment 145856 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145856&action=review
> 
> >> Source/WebCore/platform/gtk/GamepadsGtk.cpp:119
> >> +    HashMap<String, GamepadDeviceGtk*> m_deviceMap;
> > 
> > Why do we need both the Vector and the HashMap, isn't it enough with the HashMap
> 
> Vector is the same size as the GamepadsList, storing OwnPtrs to gamepad devices in a specific slot. This ensures that when two devices are connected in the first two slots and the device in the first slot is removed, the second device doesn't move into the now-empty slot.

Ah, ok, I see.

> HashMap maps gamepad devices to device file names. That way it's easy to control currently-present devices.

Yes, it's the Vector what I didn't know its purpose.

> >> Source/WebCore/platform/gtk/GamepadsGtk.cpp:205
> >> +    String deviceFile = String(g_udev_device_get_device_file(device));
> > 
> > I guess g_udev_device_get_device_file() returns an UTF8 string, so you should use String::fromUTF8(). But I think it would be better to pass the returned char * to the registerDevice and unregisterDevice methods, and convert to String when accesing the HashMap.
> 
> To me it seems to better to use String::fromUTF8 here and pass String around, that way the passed argument in registerDevice and unregisterDevice can be immediately checked for it's (non)presence in the HashMap.

Ok.

> >> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:47
> >> +        return;
> > 
> > I still don't get why we want null devices.
> 
> Indeed, that's an overkill.

:-)

[Carlos Garcia Campos]

Comment on attachment 146326 [details]
WIP patch

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

This looks really good! I have just few more minor comments.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:55
> +    GamepadDeviceGtk(String deviceFile);
> +
> +    GRefPtr<GInputStream> m_inputStream;
> +    GRefPtr<GSource> m_source;
> +    static gboolean readCallback(GObject* pollableStream, gpointer data);

Nit: typically variables are below the methods in the class declaration.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:62
> +    if (m_fileDescriptor < 0)
> +        return;

I think this should be if (!m_fileDescriptor) to avoid the zero comparison. But the file descriptor is initialized to -1, so you can use if (m_fileDescriptor != -1). Open will return -1 in case of error too, so I think it's safe to check if m_fileDescriptor != -1

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:112
> +    static void onUEventCallback(GUdevClient*, gchar* action, GUdevDevice*, gpointer data);
> +    static gboolean isGamepadDevice(GUdevDevice*);

Same comment here, I would move these right after the destructor.

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:193
> +    GamepadsGtk* deviceMapper = reinterpret_cast<GamepadsGtk*>(data);

Nit: deviceMapper -> gamepadsGtk or gamepads

> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:46
> +    if (m_fileDescriptor < 0)
> +        return;

open return s-1 in case of error, so you could check if m_fileDescriptor == -1

> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:63
> +    if (m_fileDescriptor >= 0)

if (m_fileDescriptor != -1)

> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:94
> +    // Normalize from range [0, 1] into range [0.0, 1.0]
> +    return value / 1.0f;

Since value can only be 1 or 0, maybe it's more obvious to use someting like return value ? 1 : 0; instead of using a division.

> Source/WebCore/platform/linux/GamepadDeviceLinux.h:68
> +    Vector<float> m_axes;
> +    float normalizeAxisValue(short value);
> +
> +    Vector<float> m_buttons;
> +    float normalizeButtonValue(short value);

I would move the functions at the beginning of the private section.

[Zan Dobersek]

Comment on attachment 146326 [details]
WIP patch

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

>> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:94
>> +    return value / 1.0f;
> 
> Since value can only be 1 or 0, maybe it's more obvious to use someting like return value ? 1 : 0; instead of using a division.

PS3 controllers actually have pressure-sensitive buttons, meaning values could actually vary in between 0 and 1, so this actually seems wrong altogether. I wonder if it's more of a driver problem though. Got to get that controller to try it out ...
See https://bugzilla.mozilla.org/show_bug.cgi?id=604039#c26

[Zan Dobersek]

Created attachment 147369 [details]
WIP patch

Another WIP patch after bug #84863 has been resolved

[Carlos Garcia Campos]

Comment on attachment 147369 [details]
WIP patch

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

This looks pretty good to me!

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:150
> +    unsigned index;
> +    for (index = 0; index < m_slots.size(); index++) {
> +        if (!m_slots[index])
> +            break;
> +    }
> +
> +    if (index >= m_slots.size())
> +        return;
> +
> +    m_slots[index] = GamepadDeviceGtk::create(deviceFile);
> +    m_deviceMap.add(deviceFile, m_slots[index].get());

Since registering the device once you have the free slot is just two lines, this could be simplified a bit registering the device inside the loop, so that you don't need to check whether the loop finished because of the break.

for (unsigned index = 0; index < m_slots.size(); index++) {
    if (!m_slots[index]) {
        m_slots[index] = GamepadDeviceGtk::create(deviceFile);
        m_deviceMap.add(deviceFile, m_slots[index].get());
        break;
    }
}

[Zan Dobersek]

Created attachment 148341 [details]
Patch for review

[Gustavo Noronha (kov)]

Comment on attachment 148341 [details]
Patch for review

Attachment 148341 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12990021

[Zan Dobersek]

(In reply to comment #34)
> (From update of attachment 148341 [details])
> Attachment 148341 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/12990021

The EWS is missing gudev development packages. Similar issues possible on the GTK builders.

[Carlos Garcia Campos]

(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 148341 [details] [details])
> > Attachment 148341 [details] [details] did not pass gtk-ews (gtk):
> > Output: http://queues.webkit.org/results/12990021
> 
> The EWS is missing gudev development packages. Similar issues possible on the GTK builders.

I guess we should update the jhbuild moduleset then.

[Carlos Garcia Campos]

Comment on attachment 148341 [details]
Patch for review

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

Patch looks good to me, setting r- only because of the build issues. I guess we should update the jhbuild moduleset to add gudev as a dependency or make gamepad optional and only fail in configure if --enable-gamepad has been explicitly passed and gudev is not found

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:89
> +    if (error)
> +        return error->code == G_IO_ERROR_WOULD_BLOCK;

It's very unlikely but this could be true for an error in a different domain with the same code than would block. To be extra sure you could use:

g_error_matches(error.get(), G_IO_ERROR, G_IO_ERROR_WOULD_BLOCK)

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:119
> +    static const gchar* subsystems[] = { "input", 0 };

Use char instead of gchar here.

> Source/WebCore/platform/linux/GamepadDeviceLinux.h:32
> +#include <wtf/PassOwnPtr.h>

Do you need PassOwnPtr.h here? I think it could be moved to GamepadsGtk.cpp

[Carlos Garcia Campos]

Does this bug still depend on #84929?

[Zan Dobersek]

Created attachment 148550 [details]
Patch for review

[Zan Dobersek]

(In reply to comment #39)
> Created an attachment (id=148550) [details]
> Patch for review

EWS will complain again as I've decided to update jhbuild.modules in a different bug.

[Zan Dobersek]

Created attachment 149306 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 149306 [details]
Patch

Attachment 149306 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13097155

[Zan Dobersek]

Created attachment 149310 [details]
Patch

Previous patch somehow included modified jhbuild.modules file. This one doesn't.

[Carlos Garcia Campos]

Comment on attachment 149310 [details]
Patch

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

Great!

> Source/WebCore/platform/gtk/GamepadsGtk.cpp:86
> +    // FIXME: Properly log the error.

Are you going to fix this in a follow up patch?

> Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:43
> +    // FIXME: Log errors when returning early.

Ditto.

> Tools/Scripts/webkitperl/FeatureList.pm:217
> -      define => "ENABLE_GAMEPAD", default => 0, value => \$gamepadSupport },
> +      define => "ENABLE_GAMEPAD", default => isGtk(), value => \$gamepadSupport },

Should we change the default in configure as well?

[Zan Dobersek]

(In reply to comment #44)
> (From update of attachment 149310 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149310&action=review
> 
> Great!
> 
> > Source/WebCore/platform/gtk/GamepadsGtk.cpp:86
> > +    // FIXME: Properly log the error.
> 
> Are you going to fix this in a follow up patch?
> 
> > Source/WebCore/platform/linux/GamepadDeviceLinux.cpp:43
> > +    // FIXME: Log errors when returning early.
> 
> Ditto.
> 

A new logging channel should be created and these FIXMEs populated with logging calls. I'll do that in a separate bug.

> > Tools/Scripts/webkitperl/FeatureList.pm:217
> > -      define => "ENABLE_GAMEPAD", default => 0, value => \$gamepadSupport },
> > +      define => "ENABLE_GAMEPAD", default => isGtk(), value => \$gamepadSupport },
> 
> Should we change the default in configure as well?

I talked with Xan about that a while back and we came to conclusion that the specification is too far from being stable enough for the feature to be enabled by default in release versions. There is a runtime configuration flag for this feature though, so perhaps the feature could be enabled at build-time but then disabled by default at runtime, with the possibility to enable it.

[Zan Dobersek]

Comment on attachment 149310 [details]
Patch

Clearing flags on attachment: 149310

Committed r121332: <http://trac.webkit.org/changeset/121332>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.