Bug 133850 – [GTK] Implement button-press-event, button-release-event, and absolute-axis-event of GAMEPAD API.

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

[Michal Debski]

For information on spec, please see:
http://www.w3.org/TR/gamepad/

The gamepad API implementation consist of 3 main parts. Per DOMWindow EventController, global Gamepads Manager and port specific Provider.
GamepadEventController is created as supplement to DOMWindow, but does not trigger creation of GamepadsManager until any of Gamepad related functions are called. When the need for gamepad arise (through navigator.getGamepads, or window.addListner( gamepadevent )) GamepadEventController will request manager and register for all events. It will forward any events received from manager to DOMWindow for dispatching.
GamepadsManager is first created when any of GamepadEventControllers request it and will live as long as last GamepadEventController has reference to it(ownership of GamepadsManager is shared between controllers). This ensures that the Gamepad objects will live as long as page which might have reference to it. GamepadsManager creates GamepadsProvider on construction and allows it to update gamepads information manger's methods.
GamepadsProvder is port specific and communicates directly with devices. It informs manager about connected/disconnected and changed gamepads.

[Michal Debski]

Created attachment 233591 [details]
Patch

[Michal Debski]

Created attachment 233702 [details]
Patch

Rebase

[Michal Debski]

Created attachment 233703 [details]
Patch

Rebase

[WebKit Commit Bot]

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


ERROR: Source/WebCore/Modules/gamepad/Gamepad.h:65:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.h:80:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.h:98:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/GamepadsProvider.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/efl/GamepadsProviderEfl.cpp:32:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:30:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:32:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:35:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:48:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:49:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:50:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:110:  Missing space before ( in if(  [whitespace/parens] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.h:44:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.h:61:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.h:64:  The parameter name "gamepad" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.h:65:  The parameter name "gamepad" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/gtk/GamepadsProviderGtk.cpp:32:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/platform/gtk/GamepadsProviderGtk.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/gtk/GamepadsProviderGtk.cpp:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/gamepad/GamepadsManager.cpp:30:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadsManager.cpp:31:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadsManager.cpp:33:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:31:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:31:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:60:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:38:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:44:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:54:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:59:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:60:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/NavigatorGamepad.cpp:31:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.cpp:30:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.cpp:30:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.cpp:69:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.cpp:68:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.cpp:69:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadButton.h:51:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadButton.h:56:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadButton.h:73:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadButton.h:77:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadButton.h:81:  Missing space before {  [whitespace/braces] [5]
Total errors found: 44 in 31 files


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

[Michal Debski]

Created attachment 233710 [details]
Patch

Fixed some code-styles (rest is unsupported uniform initalizations) and EFL build.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/Modules/gamepad/Gamepad.h:65:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.h:80:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.h:98:  The parameter name "data" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/platform/GamepadsProvider.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:48:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:49:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:50:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/gamepad/GamepadEventController.cpp:110:  Missing space before ( in if(  [whitespace/parens] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.h:44:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.h:61:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/gtk/GamepadsProviderGtk.cpp:32:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/platform/gtk/GamepadsProviderGtk.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/platform/gtk/GamepadsProviderGtk.cpp:87:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:60:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:38:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:44:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:54:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:59:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadEvent.cpp:60:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.cpp:69:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.cpp:68:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/Gamepad.cpp:69:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadButton.h:51:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadButton.h:56:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadButton.h:73:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadButton.h:77:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/Modules/gamepad/GamepadButton.h:81:  Missing space before {  [whitespace/braces] [5]
Total errors found: 30 in 29 files


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

[ChangSeok Oh]

Created attachment 400046 [details]
Patch

[ChangSeok Oh]

Would anyone review this? Although there is an issue for flatpak SDK, that is orthogonal to this change since we can build and test it with jhbuild.

[Carlos Garcia Campos]

Comment on attachment 400046 [details]
Patch

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

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:58
> +    if (!gamepad)
> +        return;

I don't think this is possible, we will always get a non-null pointer here. To ensure this is not called after the ManetteGamepad is destroyed, we need to disconnect all the signals in destructor.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:112
> +    if (!gamepad)
> +        return;

Ditto.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:124
> +    if (!gamepad)
> +        return;

Ditto.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:141
> +    m_id = String(manette_device_get_name(m_device.get()));

String::fromUTF8

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:145
> +    m_axisValues.resize(static_cast<size_t>(StandardGamepadAxis::Count));
> +    m_axisValues.fill(0);

You can pass a size to fill and it will be resized too.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:149
> +    listenTo(device);

Do you plan to call this from more places? Because I would just connect the signals here, making it clear they are connected on construction and disconnected on destruction.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:156
> +    g_signal_connect(G_OBJECT(device), "button-press-event", (GCallback)onButtonPressEvent, this);

g_signal_connect expects a gpointer so tou don't need the GObject cast. Use G_CALLBACK() instead of the C-style cast.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:158
> +    g_signal_connect(G_OBJECT(device), "absolute-axis-event", (GCallback)onAbsoluteAxisEvent, this);

And disconnect the signals in the destructor.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:163
> +    if (button == StandardGamepadButton::Unknown || m_device.get() != device)

How can we get a signal for a different device?

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:54
> +    void gamepadHadInput(ManetteGamepad&, bool hadButtonPresses);

Use an enum instead of a boolean parameter to improve the readability. Something like enum class ShouldMakeGamepadsVisible { No, Yes };

[ChangSeok Oh]

Created attachment 401508 [details]
Patch

[ChangSeok Oh]

Comment on attachment 400046 [details]
Patch

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

Thanks for your review!

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:58
>> +        return;
> 
> I don't think this is possible, we will always get a non-null pointer here. To ensure this is not called after the ManetteGamepad is destroyed, we need to disconnect all the signals in destructor.

The if statement is removed and g_signal_handler_disconnect is added in the destructor.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:112
>> +        return;
> 
> Ditto.

Yup.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:124
>> +        return;
> 
> Ditto.

Done.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:141
>> +    m_id = String(manette_device_get_name(m_device.get()));
> 
> String::fromUTF8

Done.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:145
>> +    m_axisValues.fill(0);
> 
> You can pass a size to fill and it will be resized too.

O.K. Good to know.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:149
>> +    listenTo(device);
> 
> Do you plan to call this from more places? Because I would just connect the signals here, making it clear they are connected on construction and disconnected on destruction.

The listenTo() is a dangling method after cleanup. Removed.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:156
>> +    g_signal_connect(G_OBJECT(device), "button-press-event", (GCallback)onButtonPressEvent, this);
> 
> g_signal_connect expects a gpointer so tou don't need the GObject cast. Use G_CALLBACK() instead of the C-style cast.

Done.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:158
>> +    g_signal_connect(G_OBJECT(device), "absolute-axis-event", (GCallback)onAbsoluteAxisEvent, this);
> 
> And disconnect the signals in the destructor.

Done.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:163
>> +    if (button == StandardGamepadButton::Unknown || m_device.get() != device)
> 
> How can we get a signal for a different device?

The comparison is meaningless. It was for avoiding unused variable warnings. Removed.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:54
>> +    void gamepadHadInput(ManetteGamepad&, bool hadButtonPresses);
> 
> Use an enum instead of a boolean parameter to improve the readability. Something like enum class ShouldMakeGamepadsVisible { No, Yes };

O.K.

[Carlos Garcia Campos]

Comment on attachment 401508 [details]
Patch

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

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:32
> +#include <linux/input-event-codes.h>

I didn't notice this in the previous revision. Is gamepad only enabled for linux?

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:150
> +    if (m_buttonPressEventHandlerID)
> +        g_signal_handler_disconnect(m_device.get(), m_buttonPressEventHandlerID);
> +    if (m_buttonReleaseEventHandlerID)
> +        g_signal_handler_disconnect(m_device.get(), m_buttonReleaseEventHandlerID);
> +    if (m_absoluteAxisEventHandlerID)
> +        g_signal_handler_disconnect(m_device.get(), m_absoluteAxisEventHandlerID);

You can do g_signal_handlers_disconnect_by_data(m_device.get(), this); and then you don't need to keep the ids.

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:76
> +    g_signal_handlers_disconnect_by_func(m_monitor.get(), reinterpret_cast<gpointer>(onDeviceConnected), this);
> +    g_signal_handlers_disconnect_by_func(m_monitor.get(), reinterpret_cast<gpointer>(onDeviceDisconnected), this);

Same here, better use by_data().

[ChangSeok Oh]

Comment on attachment 401508 [details]
Patch

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

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:32
>> +#include <linux/input-event-codes.h>
> 
> I didn't notice this in the previous revision. Is gamepad only enabled for linux?

Yes, it should. It seems manette to care linux key codes only. I set PLATFORM(GTK) in UIGamepadProviderManette.cpp where a ManetteGameProvider singleton instance is created. But I didn't guard for non-linux systems. Do we need it?

>> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:150
>> +        g_signal_handler_disconnect(m_device.get(), m_absoluteAxisEventHandlerID);
> 
> You can do g_signal_handlers_disconnect_by_data(m_device.get(), this); and then you don't need to keep the ids.

Good to know. Thanks.

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.cpp:76
>> +    g_signal_handlers_disconnect_by_func(m_monitor.get(), reinterpret_cast<gpointer>(onDeviceDisconnected), this);
> 
> Same here, better use by_data().

Yup.

[ChangSeok Oh]

Created attachment 401588 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 401588 [details]
Patch

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

> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:72
>      GUniquePtr<ManetteMonitor> m_monitor;

Isn't ManetteMonitor a GObject? this should be a GRefPtr, not a GUniquePtr. File a new bug report if that's the case, please.

[EWS]

Committed r262899: <https://trac.webkit.org/changeset/262899>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401588 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64246503>

[Radar WebKit Bug Importer]

<rdar://problem/64246502>

[ChangSeok Oh]

Comment on attachment 401588 [details]
Patch

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

>> Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:72
>>      GUniquePtr<ManetteMonitor> m_monitor;
> 
> Isn't ManetteMonitor a GObject? this should be a GRefPtr, not a GUniquePtr. File a new bug report if that's the case, please.

Oops. The original patch used GRefPtr. Perhaps, I mistakenly changed it to GUniquePtr. https://github.com/shivamidow/webkit/commit/93c63373158213248e31e61771166afff04ba8e3#diff-e640f599c0078f17008404c71a3c05b6R73
I've opened https://bugs.webkit.org/show_bug.cgi?id=213094 to address it. Thanks.