WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
217226
[GTK] Build broken with ENABLE_GAMEPAD enabled
https://bugs.webkit.org/show_bug.cgi?id=217226
Summary
[GTK] Build broken with ENABLE_GAMEPAD enabled
Adrian Perez
Reported
2020-10-02 08:36:52 PDT
Trying to build with -DENABLE_GAMEPAD=ON results in compilation errors. The main change that causes this was introduced in
r264004
Attachments
Tentative WIP patch
(7.76 KB, patch)
2020-10-02 08:49 PDT
,
Adrian Perez
no flags
Details
Formatted Diff
Diff
Patch
(13.37 KB, patch)
2020-10-08 14:38 PDT
,
Adrian Perez
no flags
Details
Formatted Diff
Diff
Patch
(14.81 KB, patch)
2020-10-08 15:10 PDT
,
Adrian Perez
no flags
Details
Formatted Diff
Diff
Patch
(16.54 KB, patch)
2020-10-09 13:17 PDT
,
Adrian Perez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Adrian Perez
Comment 1
2020-10-02 08:49:17 PDT
Created
attachment 410326
[details]
Tentative WIP patch
Adrian Perez
Comment 2
2020-10-02 08:53:30 PDT
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
Comment 3
2020-10-02 13:42:01 PDT
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 4
2020-10-07 00:13:18 PDT
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
Comment 5
2020-10-07 00:14:54 PDT
(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
Comment 6
2020-10-07 00:23:06 PDT
(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
Comment 7
2020-10-07 16:59:57 PDT
(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
Comment 8
2020-10-07 17:02:15 PDT
(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
Comment 9
2020-10-08 14:38:24 PDT
Created
attachment 410887
[details]
Patch
Adrian Perez
Comment 10
2020-10-08 14:47:54 PDT
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
Comment 11
2020-10-08 15:10:36 PDT
Created
attachment 410889
[details]
Patch
Adrian Perez
Comment 12
2020-10-09 13:17:41 PDT
Created
attachment 410967
[details]
Patch
EWS
Comment 13
2020-10-10 12:46:38 PDT
Committed
r268312
: <
https://trac.webkit.org/changeset/268312
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 410967
[details]
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug