Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2799823002:
Gamepad: fix mappings for PS4 controller (bluetooth) on Android (Closed)
DescriptionGamepad: fix mappings for PS4 controller (bluetooth) on Android
It is a popular enough device to worth some extra care.
TEST=http://html5gamepad.com/
Review-Url: https://codereview.chromium.org/2799823002
Cr-Commit-Position: refs/heads/master@{#464215}
Committed: https://chromium.googlesource.com/chromium/src/+/ceee946b0e7737fb3e3b0b81ae6c6b7e80f3b86e
Patch Set 1 #Patch Set 2 : fix build and format #
Total comments: 4
Patch Set 3 : map slim as well #
Total comments: 1
Patch Set 4 : grammar #Patch Set 5 : fix test build #Patch Set 6 : Recheck for display:none in StyleAdjuster after calling AdjustStyleForHTMLElement #Messages
Total messages: 30 (13 generated)
The CQ bit was checked by b.kelemen@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
b.kelemen@samsung.com changed reviewers: + bajones@chromium.org, scottmg@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
scottmg@chromium.org changed reviewers: + aelias@chromium.org
https://codereview.chromium.org/2799823002/diff/20001/device/gamepad/android/... File device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java (right): https://codereview.chromium.org/2799823002/diff/20001/device/gamepad/android/... device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java:33: static final int PS4_CONTROLLER_PRODUCT_ID = 1476; Should this be applied to the other PS4 pads too? https://cs.chromium.org/chromium/src/device/gamepad/gamepad_standard_mappings... https://codereview.chromium.org/2799823002/diff/20001/device/gamepad/android/... device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java:51: // so we better go by the product and vendor id's. "vendor ids" https://codereview.chromium.org/2799823002/diff/20001/device/gamepad/android/... device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java:60: if (deviceName.startsWith(NVIDIA_SHIELD_DEVICE_NAME_PREFIX) It seems like it'd be nicer to use the vendor and product ID uniformly in the same way as the C++ ones. Is there a reason that's not done here?
https://codereview.chromium.org/2799823002/diff/20001/device/gamepad/android/... File device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java (right): https://codereview.chromium.org/2799823002/diff/20001/device/gamepad/android/... device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java:60: if (deviceName.startsWith(NVIDIA_SHIELD_DEVICE_NAME_PREFIX) On 2017/04/10 17:00:32, scottmg (slow until Apr 10) wrote: > It seems like it'd be nicer to use the vendor and product ID uniformly in the > same way as the C++ ones. Is there a reason that's not done here? That's because getProductId() and getVendorId() only available since Kitkat. Also, the device name (InputDevice.getName()) seems to be pretty much unique for all the other devices except the PS4.
On 2017/04/10 17:24:02, kbalazs wrote: > https://codereview.chromium.org/2799823002/diff/20001/device/gamepad/android/... > File > device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java > (right): > > https://codereview.chromium.org/2799823002/diff/20001/device/gamepad/android/... > device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java:60: > if (deviceName.startsWith(NVIDIA_SHIELD_DEVICE_NAME_PREFIX) > On 2017/04/10 17:00:32, scottmg (slow until Apr 10) wrote: > > It seems like it'd be nicer to use the vendor and product ID uniformly in the > > same way as the C++ ones. Is there a reason that's not done here? > > That's because getProductId() and getVendorId() only available since Kitkat. > Also, the device name (InputDevice.getName()) seems to be pretty much unique for > all the other devices except the PS4. Hi Scott. How do you like the patch in light of my comment? :)
On 2017/04/11 19:52:39, kbalazs wrote: > On 2017/04/10 17:24:02, kbalazs wrote: > > > https://codereview.chromium.org/2799823002/diff/20001/device/gamepad/android/... > > File > > > device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java > > (right): > > > > > https://codereview.chromium.org/2799823002/diff/20001/device/gamepad/android/... > > > device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java:60: > > if (deviceName.startsWith(NVIDIA_SHIELD_DEVICE_NAME_PREFIX) > > On 2017/04/10 17:00:32, scottmg (slow until Apr 10) wrote: > > > It seems like it'd be nicer to use the vendor and product ID uniformly in > the > > > same way as the C++ ones. Is there a reason that's not done here? > > > > That's because getProductId() and getVendorId() only available since Kitkat. > > Also, the device name (InputDevice.getName()) seems to be pretty much unique > for > > all the other devices except the PS4. > > Hi Scott. How do you like the patch in light of my comment? :) I think this mapping still ought to be applied for all PS4 product ids.
> I think this mapping still ought to be applied for all PS4 product ids. What do you mean by "all PS4 product ids"? This patch adds mapping for the one and only PS4 gamepad from Sony. Do you mean all Playstation devices? Other PS generations have different gamepads. We have support for PS3 but the way it's mapped by Android is quite different (thus we cannot use the same code).
On 2017/04/11 20:05:59, kbalazs wrote: > > I think this mapping still ought to be applied for all PS4 product ids. > > What do you mean by "all PS4 product ids"? This patch adds mapping for the one > and only PS4 gamepad from Sony. Do you mean all Playstation devices? Other PS > generations have different gamepads. We have support for PS3 but the way it's > mapped by Android is quite different (thus we cannot use the same code). As I linked above, on Linux we use the same mapper for both https://cs.chromium.org/chromium/src/device/gamepad/gamepad_standard_mappings... product id 0x05c4 which is what you have here (1476) and also product id 0x09cc. I strongly suspect that mapping both the dualshock and the dualshock for the Slim would make sense.
On 2017/04/11 20:09:51, scottmg wrote: > On 2017/04/11 20:05:59, kbalazs wrote: > > > I think this mapping still ought to be applied for all PS4 product ids. > > > > What do you mean by "all PS4 product ids"? This patch adds mapping for the one > > and only PS4 gamepad from Sony. Do you mean all Playstation devices? Other PS > > generations have different gamepads. We have support for PS3 but the way it's > > mapped by Android is quite different (thus we cannot use the same code). > > As I linked above, on Linux we use the same mapper for both > https://cs.chromium.org/chromium/src/device/gamepad/gamepad_standard_mappings... > product id 0x05c4 which is what you have here (1476) and also product id 0x09cc. > I strongly suspect that mapping both the dualshock and the dualshock for the > Slim would make sense. Done
lgtm https://codereview.chromium.org/2799823002/diff/40001/device/gamepad/android/... File device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java (right): https://codereview.chromium.org/2799823002/diff/40001/device/gamepad/android/... device/gamepad/android/java/src/org/chromium/device/gamepad/GamepadMappings.java:53: // so we better go by the product and vendor id's. "vendor ids" not "vendor id's".
grammar
The CQ bit was checked by b.kelemen@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2799823002/#ps60001 (title: "grammar")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
fix test build
On 2017/04/12 22:46:18, kbalazs wrote: > fix test build Minor changes. Could you rubber-stamp please? Would you set the autocommit flag if it looks good? Thanks.
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org Link to the patchset: https://codereview.chromium.org/2799823002/#ps80001 (title: "fix test build")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1492037428156400,
"parent_rev": "0948aaca335e4fa1debd8959f863eb5c2f0bd29c", "commit_rev":
"ceee946b0e7737fb3e3b0b81ae6c6b7e80f3b86e"}
Message was sent while issue was closed.
Description was changed from ========== Gamepad: fix mappings for PS4 controller (bluetooth) on Android It is a popular enough device to worth some extra care. TEST=http://html5gamepad.com/ ========== to ========== Gamepad: fix mappings for PS4 controller (bluetooth) on Android It is a popular enough device to worth some extra care. TEST=http://html5gamepad.com/ Review-Url: https://codereview.chromium.org/2799823002 Cr-Commit-Position: refs/heads/master@{#464215} Committed: https://chromium.googlesource.com/chromium/src/+/ceee946b0e7737fb3e3b0b81ae6c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ceee946b0e7737fb3e3b0b81ae6c... |
