Bug 217226 – [GTK] Build broken with ENABLE_GAMEPAD enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Adrian Perez]

Trying to build with -DENABLE_GAMEPAD=ON results in compilation errors.
The main change that causes this was introduced in r264004

[Adrian Perez]

Created attachment 410326 [details]
Tentative WIP patch

[Adrian Perez]

CC'ing Changseok, who initially wrote the gamepad support using
libmanette. Feedback on the proposed patch is welcome: things seem
to work with my limited testing, but I am not super sure this is
the best way to do things. Should I keep around a list of invisible
gamepads like the Cocoa implementation does and wait that a gamepad
has some input before exposing them? (I *think* that's what the
Cocoa implementation is doing, but hadn't have the time to properly
understand how it all works).

[Adrian Perez]

Also I have been thinking that it would be positive to
make the value of ENABLE_GAMEPAD depend on the value of
the ENABLE_EXPERIMENTAL_FEATURES: this way we would let
bots (including EWS ones) build the code, thus preventing
accidental breakage.

[ChangSeok Oh]

Comment on attachment 410326 [details]
Tentative WIP patch

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

In overall, it looks good to me.

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:137
> +        value.setValue(0.0);

Simply, m_axisValues.resize(static_cast<size_t>(StandardGamepadAxis::Count), SharedGamepadValue()) not working?
Or else, what about m_axisValues.assign(SharedGamepadValue(), static_cast<size_t>(StandardGamepadAxis::Count)) ?

> Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:141
> +        value.setValue(0.0);

Ditto.

[ChangSeok Oh]

(In reply to Adrian Perez from comment #3)
> Also I have been thinking that it would be positive to
> make the value of ENABLE_GAMEPAD depend on the value of
> the ENABLE_EXPERIMENTAL_FEATURES: this way we would let
> bots (including EWS ones) build the code, thus preventing
> accidental breakage.

I am glad to hear that =)

[ChangSeok Oh]

(In reply to Adrian Perez from comment #2)
> CC'ing Changseok, who initially wrote the gamepad support using
> libmanette. Feedback on the proposed patch is welcome: things seem
> to work with my limited testing, but I am not super sure this is
> the best way to do things. Should I keep around a list of invisible
> gamepads like the Cocoa implementation does and wait that a gamepad
> has some input before exposing them? (I *think* that's what the
> Cocoa implementation is doing, but hadn't have the time to properly
> understand how it all works).

This change would not affect the existing behavior of gamepad for webkitgtk since gamepads were inactive as default before users input something to gamepads. Initially, I considered automatic activation for connected gamepads though, I eventually forgot it. Reconsidering it now, there would not be many benefits, but security risks. I like the idea, "activating a gamepad on input".

[Adrian Perez]

(In reply to ChangSeok Oh from comment #4)
> Comment on attachment 410326 [details]
> Tentative WIP patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410326&action=review
> 
> In overall, it looks good to me.

Thanks for taking a look \o/

> > Source/WebCore/platform/gamepad/manette/ManetteGamepad.cpp:137
> > +        value.setValue(0.0);
> 
> Simply, m_axisValues.resize(static_cast<size_t>(StandardGamepadAxis::Count),
> SharedGamepadValue()) not working?
> Or else, what about m_axisValues.assign(SharedGamepadValue(),
> static_cast<size_t>(StandardGamepadAxis::Count)) ?

If I understood the SharedGamepadValue implementation, using
“.resize(N, SharedGamepadValue())” creates only one
SharedGamepadValue::Data element object, and then each SharedGamepadValue
element in the array will keep a reference to the one ::Data object.
I will double check.

[Adrian Perez]

(In reply to ChangSeok Oh from comment #6)
> (In reply to Adrian Perez from comment #2)
> > CC'ing Changseok, who initially wrote the gamepad support using
> > libmanette. Feedback on the proposed patch is welcome: things seem
> > to work with my limited testing, but I am not super sure this is
> > the best way to do things. Should I keep around a list of invisible
> > gamepads like the Cocoa implementation does and wait that a gamepad
> > has some input before exposing them? (I *think* that's what the
> > Cocoa implementation is doing, but hadn't have the time to properly
> > understand how it all works).
> 
> This change would not affect the existing behavior of gamepad for webkitgtk
> since gamepads were inactive as default before users input something to
> gamepads. Initially, I considered automatic activation for connected
> gamepads though, I eventually forgot it. Reconsidering it now, there would
> not be many benefits, but security risks. I like the idea, "activating a
> gamepad on input".

Then I think that I will go ahead with my proposed patch, adding the
ChangeLog entries and double-checking the SharedGamepadValue detail
we discussed in the other comment. I will leave the “activate gamepad
on input” for a separate follow-up patch/bug.

Thanks again!

[Adrian Perez]

Created attachment 410887 [details]
Patch

[Adrian Perez]

Comment on attachment 410887 [details]
Patch

Oh, I understood now what the m_connectionDelayTimer does, and
how to adapt that part to the changes in GamepadProvider. I'll
bring the code back and reupload the patch.

[Adrian Perez]

Created attachment 410889 [details]
Patch

[Adrian Perez]

Created attachment 410967 [details]
Patch

[EWS]

Committed r268312: <https://trac.webkit.org/changeset/268312>

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