Gamepad API: add support for gamepadconnected and gamepaddisconnected events

Source/modules/gamepad/NavigatorGamepad.cpp

Index: Source/modules/gamepad/NavigatorGamepad.cpp
diff --git a/Source/modules/gamepad/NavigatorGamepad.cpp b/Source/modules/gamepad/NavigatorGamepad.cpp
index 27c1e53322ceece8349fc24a3b5d8f530ab8e7e2..514fe4d600418e83cb066ee74baa339821053f7d 100644
--- a/Source/modules/gamepad/NavigatorGamepad.cpp
+++ b/Source/modules/gamepad/NavigatorGamepad.cpp
@@ -26,14 +26,67 @@
 #include "config.h"
 #include "modules/gamepad/NavigatorGamepad.h"
 
+#include "core/frame/DOMWindow.h"
+#include "core/frame/LocalFrame.h"
 #include "core/frame/Navigator.h"
+#include "modules/gamepad/GamepadEvent.h"
 #include "modules/gamepad/GamepadList.h"
 #include "modules/gamepad/WebKitGamepadList.h"
 #include "public/platform/Platform.h"
+#include "public/platform/WebGamepadListener.h"
+#include "wtf/HashSet.h"
 #include "wtf/PassOwnPtr.h"
 
 namespace WebCore {
 
+class GlobalGamepadListener : public blink::WebGamepadListener {

[abarth-chromium, 2014/03/19 18:00:33]

Can you put this class in its own file?  For consistency with other modules, we
might want to rename GlobalGamepadListener to GamepadDispatcher

+public:
+    GlobalGamepadListener();
+
+    void addClient(NavigatorGamepad* observer);
+    void removeClient(NavigatorGamepad* observer);
+
+private:
+    virtual void didConnectGamepad(unsigned index, const blink::WebGamepad&) OVERRIDE;
+    virtual void didDisconnectGamepad(unsigned index, const blink::WebGamepad&) OVERRIDE;
+
+    typedef WTF::HashSet<NavigatorGamepad*> ClientSet;
+    ClientSet m_clients;
+};
+
+GlobalGamepadListener::GlobalGamepadListener()
+{
+    blink::Platform::current()->setGamepadListener(this);

[abarth-chromium, 2014/03/19 18:00:33]

If you look at how DeviceOrientationDispatcher, you'll see that we usually use a
pattern that lets us unregister for this notifications when there aren't any
more listeners.  Hopefully that will save battery once no one cares about these
events.

[kbalazs, 2014/03/20 18:57:21]

Good point. After looking at orientation and motion I realized that the
requirements here are similar and it makes sense to reuse the same pattern. So
now NavigatorGamepad is a DeviceSensorEventController and GamepadDispatcher is a
DeviceSensorEventDispatcher. There is some difference in how I implemented the
virtual methods though because with gamepad a script can sample gamepads without
first adding the event listener.

+}
+
+void GlobalGamepadListener::addClient(NavigatorGamepad* observer)
+{
+    m_clients.add(observer);
+}
+
+void GlobalGamepadListener::removeClient(NavigatorGamepad* observer)
+{
+    m_clients.remove(observer);
+}
+
+void GlobalGamepadListener::didConnectGamepad(unsigned index, const blink::WebGamepad& gamepad)
+{
+    for (ClientSet::iterator it = m_clients.begin(), end = m_clients.end(); it != end; ++it)
+        (*it)->didConnectGamepad(index, gamepad);

[abarth-chromium, 2014/03/19 18:00:33]

What stops m_clients from being mutated during this iteration?  We're running
arbitrary script here, which means someone could add or remove a client.  Take a
look at how the DeviceOrientationDispatcher solves this problem.

[kbalazs, 2014/03/20 18:57:21]

Solved by using DeviceSensorDispatcher and DeviceSensorEventController.

+}
+
+void GlobalGamepadListener::didDisconnectGamepad(unsigned index, const blink::WebGamepad& gamepad)
+{
+    for (ClientSet::iterator it = m_clients.begin(), end = m_clients.end(); it != end; ++it)
+        (*it)->didDisconnectGamepad(index, gamepad);
+}
+
+static GlobalGamepadListener& globalListener()
+{
+    DEFINE_STATIC_LOCAL(GlobalGamepadListener, listener, ());
+    return listener;
+}
+
 template<typename T>
 static void sampleGamepad(unsigned index, T& gamepad, const blink::WebGamepad& webGamepad)
 {
@@ -67,12 +120,14 @@ static void sampleGamepads(ListType* into)
     }
 }
 
-NavigatorGamepad::NavigatorGamepad()
+NavigatorGamepad::NavigatorGamepad(LocalFrame* frame)
+    : DOMWindowLifecycleObserver(frame ? frame->domWindow() : 0)
 {
 }
 
 NavigatorGamepad::~NavigatorGamepad()
 {
+    globalListener().removeClient(this);

[abarth-chromium, 2014/03/19 18:00:33]

The problem with only calling removeClient in the destructor is that we'll leak
information to the web page about when the destructor is called.  That's
dependent on garbage collection, which means we'll leak information about when
GC occurs to the web site.  Instead, we should unregister for these
notifications at a defined point in the lifecycle.  Take a look at how device
orientation solves this problem.  My guess is that we're subclassing
DOMWindowProperty and stop listening when the DOMWindow disconnects from the
frame.

[kbalazs, 2014/03/20 18:57:21]

Orientation and motion unregisters when the event listener is removed. But this
doesn't work here because the gamepads can be sampled without event listeners so
I again inherit from DOMWindowProperty as well and unregister in
willDestroyGlobalObjectInFrame and willDetachGlobalObjectInFrame.

 }
 
 const char* NavigatorGamepad::supplementName()
@@ -84,7 +139,7 @@ NavigatorGamepad& NavigatorGamepad::from(Navigator& navigator)
 {
     NavigatorGamepad* supplement = static_cast<NavigatorGamepad*>(Supplement<Navigator>::from(navigator, supplementName()));
     if (!supplement) {
-        supplement = new NavigatorGamepad();
+        supplement = new NavigatorGamepad(navigator.frame());
         provideTo(navigator, supplementName(), adoptPtr(supplement));
     }
     return *supplement;
@@ -116,4 +171,49 @@ GamepadList* NavigatorGamepad::gamepads()
     return m_gamepads.get();
 }
 
+void NavigatorGamepad::didConnectGamepad(unsigned index, const blink::WebGamepad& webGamepad)
+{
+    if (index >= blink::WebGamepads::itemsLengthCap || !webGamepad.connected)
+        return;

[abarth-chromium, 2014/03/19 18:00:33]

How could index >= blink::WebGamepads::itemsLengthCap?  That seems like it ought
to be an assert...

[kbalazs, 2014/03/20 18:57:21]

Done.

+
+    if (!m_gamepads)
+        m_gamepads = GamepadList::create();
+
+    RefPtrWillBeRawPtr<Gamepad> gamepad = m_gamepads->item(index);
+    if (!gamepad)
+        gamepad = Gamepad::create();
+    sampleGamepad(index, *gamepad, webGamepad);
+    m_gamepads->set(index, gamepad);
+
+    if (window()) {

[abarth-chromium, 2014/03/19 18:00:33]

Do we want to do the work above if we don't have a window?  It seems like once
we lose the connection to the window, we should stop sampling the gamepad.

[kbalazs, 2014/03/20 18:57:21]

Done.

+        RefPtr<GamepadEvent> event = GamepadEvent::create(EventTypeNames::gamepadconnected, false, true, gamepad.get());
+        window()->dispatchEvent(event);
+    }
+}
+
+void NavigatorGamepad::didDisconnectGamepad(unsigned index, const blink::WebGamepad& webGamepad)
+{
+    if (index >= blink::WebGamepads::itemsLengthCap)
+        return;
+
+    if (!m_gamepads)
+        m_gamepads = GamepadList::create();
+
+    RefPtrWillBeRawPtr<Gamepad> gamepad = m_gamepads->item(index);
+    if (!gamepad)
+        gamepad = Gamepad::create();
+    sampleGamepad(index, *gamepad, webGamepad);
+
+    if (window()) {
+        RefPtr<GamepadEvent> event = GamepadEvent::create(EventTypeNames::gamepaddisconnected, false, true, gamepad.get());
+        window()->dispatchEvent(event);
+    }
+}
+
+void NavigatorGamepad::didAddEventListener(DOMWindow*, const AtomicString& eventType)
+{
+    if (eventType == EventTypeNames::gamepadconnected || eventType == EventTypeNames::gamepaddisconnected)
+        globalListener()
+}
+
 } // namespace WebCore