Gamepad API support for chrome on android

content/public/android/java/src/org/chromium/content/browser/GamepadList.java

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.content.browser;
+
+import android.content.Context;
+import android.hardware.input.InputManager;
+import android.hardware.input.InputManager.InputDeviceListener;
+import android.view.InputDevice;
+import android.view.InputEvent;
+import android.view.KeyEvent;
+import android.view.MotionEvent;
+
+import org.chromium.base.CalledByNative;
+import org.chromium.base.JNINamespace;
+import org.chromium.base.ThreadUtils;
+
+/*
+ * Class to manage connected gamepad devices list.
+ */
+@JNINamespace("content")
+class GamepadList implements InputDeviceListener {
+    private static final int MAX_GAMEPADS = 4;
+    private GamepadDevice[] mGamepadDevices = new GamepadDevice[MAX_GAMEPADS];
+    protected boolean mHaveDevicesBeenInteractedWith = false;
+    private boolean mIsGamepadAccessed = false;
+    private final Object mLock = new Object();
+    private InputManager mInputManager;
+    private int mAttachedToWindowCounter;
+
+    private GamepadList() {}
+
+    private void initializeDevices() {
+        // Get list of all the attached input devices.
+        int[] deviceIds = mInputManager.getInputDeviceIds();
+        for (int i = 0; i < deviceIds.length; i++) {
+            InputDevice inputDevice = InputDevice.getDevice(deviceIds[i]);
+            // Check for gamepad device
+            if (isGamepadDevice(inputDevice)) {
+                // Register a new gamepad device.
+                registerGamepad(inputDevice);
+            }
+        }
+        // Register an input device listener.
+        mInputManager.registerInputDeviceListener(this, null);
+    }
+
+    private static GamepadList sGamepadList = null;
+
+    public static void onAttachedToWindow(Context context) {
+        assert ThreadUtils.runningOnUiThread();

[jdduke (slow), 2014/03/24 17:38:15]

Do we have any guarantees that this method will be called before the native
(C++) gamepad is accessed?

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:
Whenever a view becomes visible, I assumed that onAttachedToWindow is the first
callback that we get and everything happens after that.

+        createInstance();
+        sGamepadList.attachedToWindow(context);
+    }
+
+    private void attachedToWindow(Context context) {
+        if (mAttachedToWindowCounter++ == 0) {
+            synchronized (mLock) {
+                mInputManager =  (InputManager) context.getSystemService(Context.INPUT_SERVICE);

[jdduke (slow), 2014/03/24 17:38:15]

mInputManager is only accessed on the UI thread, so it can be pulled out of the
lock guard.

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Done.

+                initializeDevices();
+            }
+        }
+    }
+
+    public static void onDetachedFromWindow() {
+        assert ThreadUtils.runningOnUiThread();
+        sGamepadList.detachedFromWindow();
+    }
+
+    private void detachedFromWindow() {
+        if (--mAttachedToWindowCounter == 0) {
+            synchronized (mLock) {
+                for (int i = 0; i < MAX_GAMEPADS; ++i) {
+                    mGamepadDevices[i] = null;
+                }
+            }
+            mInputManager.unregisterInputDeviceListener(this);
+            mInputManager = null;
+        }
+    }
+
+    // Override InputDeviceListener methods
+    @Override
+    public void onInputDeviceChanged(int deviceId) {
+    }
+
+    @Override
+    public void onInputDeviceRemoved(int deviceId) {
+        synchronized (mLock) {
+            unregisterGamepad(deviceId);
+        }
+    }
+
+    @Override
+    public void onInputDeviceAdded(int deviceId) {
+        InputDevice inputDevice = InputDevice.getDevice(deviceId);
+        synchronized (mLock) {
+            if (isGamepadDevice(inputDevice))

[jdduke (slow), 2014/03/24 17:38:15]

Please pull this check outside the lock, and early return if false.

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Done.

+                registerGamepad(inputDevice);
+        }
+    }
+
+    public static void createInstance() {
+        if (sGamepadList == null) {
+            sGamepadList = new GamepadList();
+        }
+    }
+
+    private int getDeviceCount() {
+        int count = 0;
+        for (int i = 0; i < MAX_GAMEPADS; ++i) {
+            if (getDevice(i) != null) {
+                count++;
+            }
+        }
+        return count;
+    }
+
+    private String getMapping(int index) {
+        return getDevice(index).getMapping();
+    }
+
+    private String getDeviceName(int index) {
+        return getDevice(index).getName();
+    }
+
+    private long getDeviceTimestamp(int index) {
+        return getDevice(index).getTimestamp();
+    }
+
+    private float[] getDeviceAxes(int index) {
+        return getDevice(index).getAxes();
+    }
+
+    private float[] getDeviceButtons(int index) {
+        return getDevice(index).getButtons();
+    }
+
+    private boolean isDeviceConnected(int index) {
+        if (index < MAX_GAMEPADS && getDevice(index) != null) {
+            return true;
+        }
+        return false;
+    }
+
+    public GamepadDevice getDeviceById(int deviceId) {

[jdduke (slow), 2014/03/24 17:38:15]

Why is this public? Any public member that accesses private data will likely
need some kind of synchronization primitive.

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Changing it to private now.

+        for (int i = 0; i < MAX_GAMEPADS; ++i) {
+            GamepadDevice gamepad = mGamepadDevices[i];
+            if (gamepad != null && gamepad.getId() == deviceId) {
+                return gamepad;
+            }
+        }
+        return null;
+    }
+
+    public GamepadDevice getDevice(int index) {

[jdduke (slow), 2014/03/24 17:38:15]

Again, why public?

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Done.

+        // Maximum 4 Gamepads can be connected at a time starting at index zero.
+        assert index >= 0 && index <= MAX_GAMEPADS - 1;

[jdduke (slow), 2014/03/24 17:38:15]

index < MAX_GAMEPADS

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Done.

+        return mGamepadDevices[index];
+    }
+
+    public static boolean dispatchKeyEvent(KeyEvent event) {
+        if (sGamepadList.mIsGamepadAccessed && sGamepadList.isGamepadEvent(event))

[jdduke (slow), 2014/03/24 17:38:15]

These public static methods are accessing shared data (mIsGamepadAccessed), and
need a synchronization primitive to be thread safe. Here's how you might
structure this:

if (!isGamepadEvent(event)) return false;
return sGamepadList.updateForEvent(event);

Then, include the check for |mIsGamepadAccessed| in the synchronized portion of
|updateForEvent|.

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Done.

+            return sGamepadList.updateForEvent(event);
+        return false;
+    }
+
+    private boolean updateForEvent(KeyEvent event) {
+        synchronized (mLock) {
+            GamepadDevice gamepad = getGamepadForEvent(event);
+            if (gamepad != null) {
+                mHaveDevicesBeenInteractedWith = true;

[jdduke (slow), 2014/03/24 17:38:15]

Why not store this variable in each GamepadDevice?

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Now storing in GamepadDevice.

+                return gamepad.updateForEvent(event);
+            }
+            return false;
+        }
+    }
+
+    public static boolean onGenericMotionEvent(MotionEvent event) {
+        if (sGamepadList.mIsGamepadAccessed && sGamepadList.isGamepadEvent(event))
+            return sGamepadList.updateForEvent(event);
+        return false;
+    }
+
+    private boolean updateForEvent(MotionEvent event) {
+        synchronized (mLock) {
+            GamepadDevice gamepad = getGamepadForEvent(event);
+            if (gamepad != null) {
+                mHaveDevicesBeenInteractedWith = true;
+                return gamepad.updateForEvent(event);
+            }
+            return false;
+        }
+    }
+
+    protected int getNextAvailableIndex() {
+        // When multiple gamepads are connected to a user agent, indices must be assigned on a
+        // first-come first-serve basis, starting at zero. If a gamepad is disconnected, previously
+        // assigned indices must not be reassigned to gamepads that continue to be connected.
+        // However, if a gamepad is disconnected, and subsequently the same or a different
+        // gamepad is then connected, index entries must be reused.
+
+        for (int i = 0; i < MAX_GAMEPADS; ++i) {
+            if (getDevice(i) == null) {
+                return i;
+            }
+        }
+        // Reached maximum gamepads limit.
+        return -1;
+    }
+
+    protected boolean registerGamepad(InputDevice inputDevice) {
+        int index = getNextAvailableIndex();
+        if (index == -1) {
+            return false; // invalid index
+        }
+
+        GamepadDevice gamepad = new GamepadDevice(index, inputDevice);
+        mGamepadDevices[index] = gamepad;
+        return true;
+    }
+
+    protected void unregisterGamepad(int deviceId) {
+        GamepadDevice gamepadDevice = getDeviceById(deviceId);
+        if (gamepadDevice == null) {
+            return; // Not a registered device.
+        }
+        int index = gamepadDevice.getIndex();
+        mGamepadDevices[index] = null;
+    }
+
+    private boolean isGamepadDevice(InputDevice inputDevice) {

[jdduke (slow), 2014/03/24 17:38:15]

This method can be static.

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Done.

+        int sources = inputDevice.getSources();
+            if ((sources & InputDevice.SOURCE_JOYSTICK) == InputDevice.SOURCE_JOYSTICK ) {
+                //Found gamepad device
+                return true;
+            }
+        return false;
+    }
+
+    private GamepadDevice getGamepadForEvent(InputEvent event) {
+        int deviceId = event.getDeviceId();
+        GamepadDevice gamepad = getDeviceById(deviceId);
+        return gamepad;
+    }
+
+    private boolean isGamepadEvent(MotionEvent event) {

[jdduke (slow), 2014/03/24 17:38:15]

This method can be static.

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Done.

+        if ((event.getSource() & InputDevice.SOURCE_JOYSTICK) == InputDevice.SOURCE_JOYSTICK) {
+            return true;
+        }
+        return false;
+    }
+
+    // Returns true if event's keycode corresponds to a gamepad key.
+    // Specific handling for dpad keys is required because
+    // KeyEvent.isGamepadButton doesn't consider dpad keys.
+    public boolean isGamepadEvent(KeyEvent event) {

[jdduke (slow), 2014/03/26 16:37:58]

Hmm, is this accessed anywhere else? If not, please make private. Looks like it
can also be static.

[SaurabhK, 2014/03/27 13:32:31]

On 2014/03/26 16:37:58, jdduke wrote:
Made private static in next patch.

+        int keyCode = event.getKeyCode();
+        switch (keyCode) {
+            case KeyEvent.KEYCODE_DPAD_UP:
+            case KeyEvent.KEYCODE_DPAD_DOWN:
+            case KeyEvent.KEYCODE_DPAD_LEFT:
+            case KeyEvent.KEYCODE_DPAD_RIGHT:
+                return true;
+            default:
+                return KeyEvent.isGamepadButton(keyCode);
+        }
+    }
+
+    @CalledByNative
+    static void getGamepadData(long gamepads) {
+        assert sGamepadList != null;
+        sGamepadList.grabGamepadData(gamepads);
+    }
+
+    private void grabGamepadData(long gamepads) {
+        synchronized (mLock) {
+            for (int i = 0; i < MAX_GAMEPADS; i++) {
+                if (mHaveDevicesBeenInteractedWith && isDeviceConnected(i)) {
+                    getDevice(i).mapButtonsAndAxes();
+                    nativeSetGamepadData(gamepads, i, getMapping(i), true,

[jdduke (slow), 2014/03/24 17:38:15]

Why all these helper methods? Why not do something like:

final GamepadDevice device = getDevice(i);
if (device != null && device.hasBeenInteractedWith()) {
    nativeSetGamepadData(nativeGamepads, i, device.getMapping(), ...).
} else  {
    nativeSetGamepadData(...)
}

Then you can get rid of all the helper getters.

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Done.

+                                         getDeviceName(i), getDeviceTimestamp(i), getDeviceAxes(i),
+                                         getDeviceButtons(i));
+                }
+                else
+                    nativeSetGamepadData(gamepads, i, null, false, null, 0, null, null);
+            }
+        }
+    }
+
+    @CalledByNative
+    static void notifyForGamepadsAccess(boolean isaccesspaused) {

[jdduke (slow), 2014/03/26 16:37:58]

Nit: isAccessPaused.

[SaurabhK, 2014/03/27 13:32:31]

On 2014/03/26 16:37:58, jdduke wrote:

Done.

+        assert sGamepadList != null;
+        synchronized (sGamepadList.mLock) {
+            sGamepadList.mIsGamepadAccessed = !isaccesspaused;

[jdduke (slow), 2014/03/26 16:37:58]

It feels a little funky locking the static instance member in the static method.
 Please factor out this code into a (member) function, so you'll have:

assert sGamepadList != null;
sGamepadList.notifyForGamepadsAccess(isAccessPaused);

much as you've done with |getGamepadData()|.

[SaurabhK, 2014/03/27 13:32:31]

On 2014/03/26 16:37:58, jdduke wrote:

Done.

+            if (isaccesspaused) {
+                for (int i = 0; i < MAX_GAMEPADS; i++) {
+                    GamepadDevice gamepadDevice = sGamepadList.getDevice(i);
+                    if(gamepadDevice != null)
+                        gamepadDevice.clearData();
+                }
+            }
+        }
+    }
+
+    private native void nativeSetGamepadData(long gamepads,

[jdduke (slow), 2014/03/24 17:38:15]

Please rename all |gamepads| references to |nativeGamepads|.

[SaurabhK, 2014/03/26 16:10:07]

On 2014/03/24 17:38:15, jdduke wrote:

Is it needed?
because i was under the impression that when we prefix with 'native' we need to
provide the attached native class object. (?)
And 'gamepads' is a object of type webgamepad passed from native(C++).

[jdduke (slow), 2014/03/26 16:37:58]

Hmm, I think that's a convention, but I don't feel strongly about this either
way.  What you have is probably fine, maybe tedchoc@ has an opinion on this.

+                                             int index,
+                                             String mapping,
+                                             boolean connected,
+                                             String devicename,
+                                             long timestamp,
+                                             float[] axes,
+                                             float[] buttons);
+
+}