Trying to build with -DENABLE_GAMEPAD=ON results in compilation errors. The main change that causes this was introduced in r264004
Created attachment 410326 [details] Tentative WIP patch
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).
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.
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.
(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 =)
(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".
(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.
(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!
Created attachment 410887 [details] Patch
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.
Created attachment 410889 [details] Patch
Created attachment 410967 [details] Patch
Committed r268312: <https://trac.webkit.org/changeset/268312> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410967 [details].