Bug 134386

Summary: Fire connected/disconnected events for Gamepads
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, syoichi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 134669, 134670    
Bug Blocks: 134076    
Attachments:
Description Flags
Patch v1
none
Patch v1a - Rebased, ready to EWS
dino: review+
Patch v2 none

Description Brady Eidson 2014-06-26 22:41:22 PDT
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.
Comment 1 Brady Eidson 2014-07-06 17:15:47 PDT
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.
Comment 2 Brady Eidson 2014-07-06 17:37:44 PDT
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)
Comment 3 Brady Eidson 2014-07-09 07:33:39 PDT
Created attachment 234638 [details]
Patch v1a - Rebased, ready to EWS
Comment 4 Dean Jackson 2014-07-09 14:36:28 PDT
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 .
Comment 5 Brady Eidson 2014-07-09 17:21:01 PDT
Created attachment 234672 [details]
Patch v2
Comment 6 WebKit Commit Bot 2014-07-09 23:09:22 PDT
Comment on attachment 234672 [details]
Patch v2

Clearing flags on attachment: 234672

Committed r170957: <http://trac.webkit.org/changeset/170957>
Comment 7 WebKit Commit Bot 2014-07-09 23:09:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Sam Weinig 2014-07-10 09:27:53 PDT
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?
Comment 9 Brady Eidson 2014-07-10 14:48:36 PDT
(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.