Gamepad: cleanup mappings and add support for Samsung and PS3 gamepads

Gamepad: cleanup mappings and add support for Samsung and PS3 gamepads

The way we map axes does not look quite correct to me. We rely on the ordering
in which InputDevice enumerates the motion ranges. There are several problems
with that. First, I don't find any evidence in the documentation that there is
a defined ordering. This means that InputDevice.getMotionRanges() does not
guarantee to enumerate the axes in the same order on different devices or
in different api versions. Second, this leads to using magic constants.
Third, ideally one should be able to add the code for a new device or at least
check if it is correct by looking at the Android specific documentation of the
device. But those documentations refer to api constants like
MotionEvent.AXIS_FOO instead of the index of the motion range.
This CL cleans it up by storing the axes values associatively indexed by api
constants, just like we do for buttons. Finally, it adds the device specific
code for the Samsung and the PS3 gamepads.

TEST=http://www.html5rocks.com/en/tutorials/doodles/gamepad/gamepad-tester/tester.html
BUG=330094
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275378

[kbalazs, 2014-06-03 21:07:01]

I believe I only made equivalent refactors for the XBox and Nvidia but I hope
@skhatri can actually test them. I tested the Samsung and PS3.

[jdduke (slow), 2014-06-03 22:10:46]

Thanks for the update, this looks pretty reasonable to me. skhatri@, PTAL to
make sure this looks good for XBox/Shield purposes, thanks.

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadDevice.java
(right):

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadDevice.java:56:
List<MotionRange> ranges = inputDevice.getMotionRanges();
Let's make this final.

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java
(right):

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:26:
} else if (deviceName.contains("Microsoft X-Box 360 pad")) {
We might as well move treat all constant strings here the same, and move them
out as with the other controllers.  Should we ignore case in the string check?

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:42:
float a = rawButtons[KeyEvent.KEYCODE_BUTTON_A];
Are the temporaries here just a stylistic preference?

[kbalazs, 2014-06-03 22:25:12]

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java
(right):

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:26:
} else if (deviceName.contains("Microsoft X-Box 360 pad")) {
On 2014/06/03 22:10:46, jdduke wrote:
> We might as well move treat all constant strings here the same, and move them
> out as with the other controllers.  Should we ignore case in the string check?


I don't think we need to be case insensitive if we know the exact string. I'm
assuming that this shouldn't change, i.e. the input system gets it from the
device itself.

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:42:
float a = rawButtons[KeyEvent.KEYCODE_BUTTON_A];
On 2014/06/03 22:10:46, jdduke wrote:
> Are the temporaries here just a stylistic preference?

Kind of, I prefer avoiding multiline statements when possible. But I guess I
forgot to update the ruler in my ide to 100 chars from 80, so this could
actually fit.

[SaurabhK, 2014-06-05 15:20:26]

Sorry for the delay in reply.

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java
(right):

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:140:
mapRXAndRYAxesToRightStick(mappedAxes, rawAxes);
SHield maps Z and RZ axes. Please use mapZAndRZAxesToRightStick for mapping.
Rest all is working perfectly.

[jdduke (slow), 2014-06-05 15:22:21]

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java
(right):

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:42:
float a = rawButtons[KeyEvent.KEYCODE_BUTTON_A];
On 2014/06/03 22:25:11, kbalazs wrote:
> On 2014/06/03 22:10:46, jdduke wrote:
> > Are the temporaries here just a stylistic preference?
> 
> Kind of, I prefer avoiding multiline statements when possible. But I guess I
> forgot to update the ruler in my ide to 100 chars from 80, so this could
> actually fit.

OK, but this actually makes inspection more difficult, as you have to scan to
find the original source token rather than just looking at the previous line.

[kbalazs, 2014-06-05 17:59:36]

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadDevice.java
(right):

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadDevice.java:56:
List<MotionRange> ranges = inputDevice.getMotionRanges();
On 2014/06/03 22:10:46, jdduke wrote:
> Let's make this final.

Done.

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
File
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java
(right):

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:26:
} else if (deviceName.contains("Microsoft X-Box 360 pad")) {
On 2014/06/03 22:25:11, kbalazs wrote:
> On 2014/06/03 22:10:46, jdduke wrote:
> > We might as well move treat all constant strings here the same, and move
them
> > out as with the other controllers.  Should we ignore case in the string
check?
> 
> 
> I don't think we need to be case insensitive if we know the exact string. I'm
> assuming that this shouldn't change, i.e. the input system gets it from the
> device itself.

I moved the constants but it seems like there is some uncertainty with the name
of the Nvidia device
(http://docs.nvidia.com/tegra/Content/AN_GC_Quickdoc_Advanced_Topics.html) so
for that I used startsWith with the known prefix.

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:42:
float a = rawButtons[KeyEvent.KEYCODE_BUTTON_A];
On 2014/06/05 15:22:21, jdduke wrote:
> On 2014/06/03 22:25:11, kbalazs wrote:
> > On 2014/06/03 22:10:46, jdduke wrote:
> > > Are the temporaries here just a stylistic preference?
> > 
> > Kind of, I prefer avoiding multiline statements when possible. But I guess I
> > forgot to update the ruler in my ide to 100 chars from 80, so this could
> > actually fit.
> 
> OK, but this actually makes inspection more difficult, as you have to scan to
> find the original source token rather than just looking at the previous line.

I know. I tried to make it a bit more review friendly but there are too many
changes so I couldn't make all hunks easily comparable.

https://codereview.chromium.org/307413004/diff/1/content/public/android/java/...
content/public/android/java/src/org/chromium/content/browser/input/GamepadMappings.java:140:
mapRXAndRYAxesToRightStick(mappedAxes, rawAxes);
On 2014/06/05 15:20:26, SaurabhK wrote:
> SHield maps Z and RZ axes. Please use mapZAndRZAxesToRightStick for mapping.
> Rest all is working perfectly.

Great, thanks for testing. I assumed that this is true for Xbox as well because
the original code for Xbox and Shield was the same.

[jdduke (slow), 2014-06-05 18:07:14]

lgtm, thanks!

[kbalazs, 2014-06-05 22:58:01]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-06-05 22:58:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/307413004/20001

[commit-bot: I haz the power, 2014-06-06 04:27:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-06 10:34:46]

Change committed as 275378