Bug 160871 – Cleanup WK2 platform gamepad handling

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Cleanup WK2 platform gamepad handling

[Brady Eidson]

Created attachment 286103 [details]
Patch

[Alex Christensen]

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

[Alex Christensen]

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).

:(

[Brady Eidson]

(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"? :)

[Brady Eidson]

(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.

[WebKit Commit Bot]

Comment on attachment 286103 [details]
Patch

Clearing flags on attachment: 286103

Committed r204506: <http://trac.webkit.org/changeset/204506>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

/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]

[Chris Dumez]

Reverted r204506 for reason:

Broke the build

Committed r204509: <http://trac.webkit.org/changeset/204509>

[Alex Christensen]

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 :(

[Brady Eidson]

(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."

[Brady Eidson]

(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?

[Chris Dumez]

(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

[Brady Eidson]

(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.

[Brady Eidson]

(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*

[Brady Eidson]

Created attachment 286202 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286202 [details]
Patch

Clearing flags on attachment: 286202

Committed r204524: <http://trac.webkit.org/changeset/204524>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.