Bug 134386 – Fire connected/disconnected events for Gamepads

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Fire connected/disconnected events for Gamepads

GamepadManager is already a great chokepoint for this.

We'll likely DOMWindow register itself for the events whenever the appropriate event listeners are added/removed from the window.

[Brady Eidson]

Filed https://bugs.webkit.org/show_bug.cgi?id=134670 to move the integer index into PlatformGamepad

Filed https://bugs.webkit.org/show_bug.cgi?id=134669 to have DOMWindows register themselves with the GamepadManager whenever they are actually interested in gamepad events.

[Brady Eidson]

Created attachment 234472 [details]
Patch v1

This patch relies on the ones in 134699 and 134670, so it probably won't apply on the bots.  I'm just attaching it for posterity (for now)

[Brady Eidson]

Created attachment 234638 [details]
Patch v1a - Rebased, ready to EWS

[Dean Jackson]

Comment on attachment 234638 [details]
Patch v1a - Rebased, ready to EWS

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

> Source/WebCore/ChangeLog:20
> +        * Modules/gamepad/GamepadManager.cpp:
> +        (WebCore::navigatorGamepadFromDOMWindow): Handling converting a possibly-null Navigator into
> +          a possibly null NavigatorGamepad.
> +        (WebCore::GamepadManager::platformGamepadConnected): Notify blind Navigator/DOMWindows of all
> +          previously attached Gamepads, then notify everybody of this new gamepad.
> +        (WebCore::GamepadManager::platformGamepadDisconnected): Handle dispatching the disconnected
> +          event to all registered DOMWindows.
> +        (WebCore::GamepadManager::platformGamepadInputActivity): Notify blind Navigator/DOMWindows of all
> +          attached Gamepads.
> +        (WebCore::GamepadManager::makeGamepadVisible): Handles notifying setting up a new gamepads
> +          with all NavigatorGamepads as well as dispatching the connected even to DOMWindows.

Nit on your indentation there.

> Source/WebCore/Modules/gamepad/GamepadManager.cpp:74
> +    // Notify everyone of this new gamepad

Nit: missing .

[Brady Eidson]

Created attachment 234672 [details]
Patch v2

[WebKit Commit Bot]

Comment on attachment 234672 [details]
Patch v2

Clearing flags on attachment: 234672

Committed r170957: <http://trac.webkit.org/changeset/170957>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sam Weinig]

Comment on attachment 234672 [details]
Patch v2

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

> Source/WebCore/Modules/gamepad/GamepadManager.cpp:65
> +        if (!gamepad || gamepad == &platformGamepad)

When will gamepad be null here?

> Source/WebCore/Modules/gamepad/GamepadManager.cpp:81
> +    Vector<DOMWindow*> domWindowVector;
> +    copyToVector(m_domWindows, domWindowVector);

It seems like this would be much more clear if we used WeakPtrs here.  There lifetimes are really hard to follow.

> Source/WebCore/Modules/gamepad/GamepadManager.cpp:94
> +        NavigatorGamepad* navigator = navigatorGamepadFromDOMWindow(window);
> +        if (!navigator)
> +            continue;
> +

I think you should have a comment explaining when the navigator can be null, and what the consequences are.

> Source/WebCore/Modules/gamepad/GamepadManager.cpp:99
> +        RefPtr<Gamepad> gamepad = navigator->gamepadAtIndex(platformGamepad.index());

This should pass the platformGamepad itself, in which case, it could return a Ref<Gamepad>

> Source/WebCore/Modules/gamepad/NavigatorGamepad.cpp:75
> +Gamepad* NavigatorGamepad::gamepadAtIndex(unsigned index)

Can this take a PlatformGamepad& instead?

[Brady Eidson]

(In reply to comment #8)
> (From update of attachment 234672 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234672&action=review
> 
> > Source/WebCore/Modules/gamepad/GamepadManager.cpp:65
> > +        if (!gamepad || gamepad == &platformGamepad)
> 
> When will gamepad be null here?

Gamepads have the unfortunate characteristic of existing in a sparse array.

The scenario being the user connects a gamepad which becomes index 0, then the user connects a gamepad which becomes index 1, then the user disconnected that first gamepad.

Now the array is of size 2 and contains { nullptr, gamepad(1) }

> 
> > Source/WebCore/Modules/gamepad/GamepadManager.cpp:81
> > +    Vector<DOMWindow*> domWindowVector;
> > +    copyToVector(m_domWindows, domWindowVector);
> 
> It seems like this would be much more clear if we used WeakPtrs here.  There lifetimes are really hard to follow.

I think this is a great idea - Until we looked at this this morning I wasn't aware that we'd finally gotten an established WeakPtr pattern in WebCore.

> > Source/WebCore/Modules/gamepad/GamepadManager.cpp:94
> > +        NavigatorGamepad* navigator = navigatorGamepadFromDOMWindow(window);
> > +        if (!navigator)
> > +            continue;
> > +
> 
> I think you should have a comment explaining when the navigator can be null, and what the consequences are.

Sure.

> > Source/WebCore/Modules/gamepad/GamepadManager.cpp:99
> > +        RefPtr<Gamepad> gamepad = navigator->gamepadAtIndex(platformGamepad.index());
> 
> This should pass the platformGamepad itself, in which case, it could return a Ref<Gamepad>
> 
> > Source/WebCore/Modules/gamepad/NavigatorGamepad.cpp:75
> > +Gamepad* NavigatorGamepad::gamepadAtIndex(unsigned index)
> 
> Can this take a PlatformGamepad& instead?

These seem like fine ideas.