Cleanup WK2 platform gamepad handling
Created attachment 286103 [details] Patch
Comment on attachment 286103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286103&action=review > Source/WebCore/platform/gamepad/GamepadProviderClient.h:40 > + virtual void setInitialConnectedGamepads(const Vector<PlatformGamepad*>&) { } I don't understand the lifetime of these PlatformGamepads. Where do they come from? How do we know they will still be alive as long as we have pointers to them? > Source/WebKit2/UIProcess/Gamepad/UIGamepadProvider.h:87 > + bool m_hasInitialGamepads { false }; This is only used in an assertion. Could be compiled out. Probably nbd > Source/WebKit2/WebProcess/WebProcess.cpp:1036 > +void WebProcess::setInitialGamepads(const Vector<WebKit::GamepadData>& gamepadDatas) data is already plural
Comment on attachment 286103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286103&action=review > Source/WebCore/ChangeLog:8 > + No new tests (No currently testable change in behavior). :(
(In reply to comment #2) > Comment on attachment 286103 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286103&action=review > > > Source/WebCore/platform/gamepad/GamepadProviderClient.h:40 > > + virtual void setInitialConnectedGamepads(const Vector<PlatformGamepad*>&) { } > > I don't understand the lifetime of these PlatformGamepads. Where do they > come from? How do we know they will still be alive as long as we have > pointers to them? The platform GamepadProvider owns them. They are valid until you get a "platformGamepadDisconnected" message. > > > Source/WebKit2/UIProcess/Gamepad/UIGamepadProvider.h:87 > > + bool m_hasInitialGamepads { false }; > > This is only used in an assertion. Could be compiled out. Probably nbd I'd like to leave it for now, as there are grander plans for it going forward - Though I will remain cognizant of whether or not it is assertion-only. > > > Source/WebKit2/WebProcess/WebProcess.cpp:1036 > > +void WebProcess::setInitialGamepads(const Vector<WebKit::GamepadData>& gamepadDatas) > > data is already plural Do you have a naming suggestion for a "vector of GamepadData" that is not "gamepadDatas" but also not "gamepadDataVector"? :)
(In reply to comment #3) > Comment on attachment 286103 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286103&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests (No currently testable change in behavior). > > :( As you know, I have grand plans for next week.
Comment on attachment 286103 [details] Patch Clearing flags on attachment: 286103 Committed r204506: <http://trac.webkit.org/changeset/204506>
All reviewed patches have been landed. Closing bug.
/Volumes/Data/cdumez/WebKit/OpenSource/Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.cpp:105:77: error: null passed to a callee that requires a non-null argument [-Werror,-Wnonnull]
Reverted r204506 for reason: Broke the build Committed r204509: <http://trac.webkit.org/changeset/204509>
Comment on attachment 286103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286103&action=review > Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.cpp:105 > + IOHIDManagerRegisterInputValueCallback(m_manager.get(), nullptr, nullptr); It looks like the second parameter of this cannot be null :(
(In reply to comment #10) > Comment on attachment 286103 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286103&action=review > > > Source/WebCore/platform/gamepad/mac/HIDGamepadProvider.cpp:105 > > + IOHIDManagerRegisterInputValueCallback(m_manager.get(), nullptr, nullptr); > > It looks like the second parameter of this cannot be null :( This is absolutely 100% incorrect. From https://developer.apple.com/library/mac/documentation/DeviceDrivers/Conceptual/HID/new_api_10_5/tn2187.html "Note: To unregister pass NULL for the callback."
(In reply to comment #8) > /Volumes/Data/cdumez/WebKit/OpenSource/Source/WebCore/platform/gamepad/mac/ > HIDGamepadProvider.cpp:105:77: error: null passed to a callee that requires > a non-null argument [-Werror,-Wnonnull] Where did you see this?
(In reply to comment #12) > (In reply to comment #8) > > /Volumes/Data/cdumez/WebKit/OpenSource/Source/WebCore/platform/gamepad/mac/ > > HIDGamepadProvider.cpp:105:77: error: null passed to a callee that requires > > a non-null argument [-Werror,-Wnonnull] > > Where did you see this? Locally but it also showed on internal bots and I provided a link on IRC
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #8) > > > /Volumes/Data/cdumez/WebKit/OpenSource/Source/WebCore/platform/gamepad/mac/ > > > HIDGamepadProvider.cpp:105:77: error: null passed to a callee that requires > > > a non-null argument [-Werror,-Wnonnull] > > > > Where did you see this? > > Locally but it also showed on internal bots and I provided a link on IRC I was not on IRC at the time, and internal bot links are currently broken. Yay! Anyways, a macOS regression in this API. Working around and filing a bug on the relevant team.
(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > (In reply to comment #8) > > > > /Volumes/Data/cdumez/WebKit/OpenSource/Source/WebCore/platform/gamepad/mac/ > > > > HIDGamepadProvider.cpp:105:77: error: null passed to a callee that requires > > > > a non-null argument [-Werror,-Wnonnull] > > > > > > Where did you see this? > > > > Locally but it also showed on internal bots and I provided a link on IRC > > I was not on IRC at the time, and internal bot links are currently broken. > Yay! > > Anyways, a macOS regression in this API. macOS Sierra*
Created attachment 286202 [details] Patch
Comment on attachment 286202 [details] Patch Clearing flags on attachment: 286202 Committed r204524: <http://trac.webkit.org/changeset/204524>