Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2900773003:
Allow gaming_seat to use ozone gamepad as back-end (Closed)
DescriptionAllow gaming_seat to use ozone gamepad as back-end
This patch enable gaming_seat to use ozone gamepad as back-end. It is
protected by build args. If enable_exo_ozone_gamepad is turned off,
gaming_seat will fallback use old joydev implementation.
BUG=717246
Review-Url: https://codereview.chromium.org/2900773003
Cr-Commit-Position: refs/heads/master@{#478834}
Committed: https://chromium.googlesource.com/chromium/src/+/f72a28a67a141af1e5c29f608e30453c7cc94a5a
Patch Set 1 #Patch Set 2 : Add gaming_seat_ozone to exo #
Total comments: 5
Patch Set 3 : Add gaming_seat_ozone to exo #Patch Set 4 : Add gaming_seat_ozone to exo #
Total comments: 43
Patch Set 5 : Add gaming_seat_ozone to exo #
Total comments: 21
Patch Set 6 : Add gaming_seat_ozone to exo #
Total comments: 10
Patch Set 7 : Add gaming_seat_ozone to exo #
Total comments: 6
Patch Set 8 : Allow gaming_seat to use ozone gamepad as backend #Patch Set 9 : Allow gaming_seat to use ozone gamepad as backend #Patch Set 10 : Allow gaming_seat to use ozone gamepad as backend #
Messages
Total messages: 36 (13 generated)
Description was changed from ========== Add gaming_seat_ozone to exo This patch adds gaming_seat_ozone. It will use ozone/gamepad as backend of gamepad. The old gaming_seat is renamed to gaming_seat_joydev. The choice of gaming_seat_ozone or gaming_seat_joydev (using /device/gamepad) is controlled by a boolean in gaming_seat.cc. BUG=717246 ========== to ========== Add gaming_seat_ozone to exo This patch adds gaming_seat_ozone. It will use ozone/gamepad as backend of gamepad. The old gaming_seat is renamed to gaming_seat_joydev. The choice of gaming_seat_ozone or gaming_seat_joydev (using /device/gamepad) is controlled by a boolean in gaming_seat.cc. BUG=717246 ==========
jkwang@google.com changed reviewers: + reveman@chromium.org
@reveman, please take a look when you get some time.
why do we need run time configuration support? is build time not enough?
On 2017/05/30 21:33:32, reveman wrote: > why do we need run time configuration support? is build time not enough? I have made it compile time decision.
https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... File components/exo/gaming_seat.h (right): https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... components/exo/gaming_seat.h:23: static GamingSeat* CreateGamingSeat(GamingSeatDelegate* gaming_seat_delegate, do we need to use a factory pattern if this is now compile time? why not just put a few ifdefs in gaming_seat.h/cc and be done? https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... File components/exo/gaming_seat_joydev_unittest.cc (right): https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... components/exo/gaming_seat_joydev_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. why would we just be testing the joydev version? doesn't the same test apply to the ozone impl?
https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... File components/exo/gaming_seat.h (right): https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... components/exo/gaming_seat.h:23: static GamingSeat* CreateGamingSeat(GamingSeatDelegate* gaming_seat_delegate, On 2017/05/31 22:42:23, reveman wrote: > do we need to use a factory pattern if this is now compile time? why not just > put a few ifdefs in gaming_seat.h/cc and be done? No..It does not need to be like this, but I would want all the ifdef here (don't want to do it in server.cc). So I kind of have to wrap the constructor (cause gaming_seat_ozone does not need a thread) and expose an unified interface to server.cc. I don't see any performance issue here. https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... File components/exo/gaming_seat_joydev_unittest.cc (right): https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... components/exo/gaming_seat_joydev_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/05/31 22:42:23, reveman wrote: > why would we just be testing the joydev version? doesn't the same test apply to > the ozone impl? I have added test for device connect/disconnect. Most part of the ozone impl just forward the event to arc++.
https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... File components/exo/gaming_seat.h (right): https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... components/exo/gaming_seat.h:23: static GamingSeat* CreateGamingSeat(GamingSeatDelegate* gaming_seat_delegate, On 2017/06/01 at 19:43:15, jkwang wrote: > On 2017/05/31 22:42:23, reveman wrote: > > do we need to use a factory pattern if this is now compile time? why not just > > put a few ifdefs in gaming_seat.h/cc and be done? > > No..It does not need to be like this, but I would want all the ifdef here (don't want to do it in server.cc). So I kind of have to wrap the constructor (cause gaming_seat_ozone does not need a thread) and expose an unified interface to server.cc. I don't see any performance issue here. Performance is not a concern of mine. Making the code clean and easy to understand is important to me. If this is a compile time option then please have simple non-factory mechanism that server.cc can use to create an instance and either ifdef everything in gaming_seat.cc/h or add gaming_seat_IMPL.cc files if the amount of changes are large enough to justify separate files. If you're using separate files or not should not affect the actual implementation and is just a choice between using ifdefs or BUILD.gn to control what is compiled. You can follow base/memory/shared_memory.h/cc as an example of how to do this.
On 2017/06/01 22:24:52, reveman wrote: > https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... > File components/exo/gaming_seat.h (right): > > https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... > components/exo/gaming_seat.h:23: static GamingSeat* > CreateGamingSeat(GamingSeatDelegate* gaming_seat_delegate, > On 2017/06/01 at 19:43:15, jkwang wrote: > > On 2017/05/31 22:42:23, reveman wrote: > > > do we need to use a factory pattern if this is now compile time? why not > just > > > put a few ifdefs in gaming_seat.h/cc and be done? > > > > No..It does not need to be like this, but I would want all the ifdef here > (don't want to do it in server.cc). So I kind of have to wrap the constructor > (cause gaming_seat_ozone does not need a thread) and expose an unified interface > to server.cc. I don't see any performance issue here. > > Performance is not a concern of mine. Making the code clean and easy to > understand is important to me. If this is a compile time option then please have > simple non-factory mechanism that server.cc can use to create an instance and > either ifdef everything in gaming_seat.cc/h or add gaming_seat_IMPL.cc files if > the amount of changes are large enough to justify separate files. If you're > using separate files or not should not affect the actual implementation and is > just a choice between using ifdefs or BUILD.gn to control what is compiled. You > can follow base/memory/shared_memory.h/cc as an example of how to do this. I am not sure I understand you, so yell at me if I am wrong. The case is a little bit different to shared_memory.h/cc. For shared_memory.h/cc: 1. Code from one impl won't compile when it is compiled to other platform. 2. Different impl share same basic structure and some functions can be reused. For gaming_seat, code can compile together and gaming_seat_ozone and gaming_seat_joydev are almost completely different. If I do the "shared_memory way", I might end up with: class GamingSeat { #ifdef ozone ... All functions from Current GamingSeatOzone #else ... All functions from Current GamingSeatJoydev #endif } I would think it's a little bit hard to read and just get code jammed together.
On 2017/06/02 at 22:32:49, jkwang wrote: > On 2017/06/01 22:24:52, reveman wrote: > > https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... > > File components/exo/gaming_seat.h (right): > > > > https://codereview.chromium.org/2900773003/diff/20001/components/exo/gaming_s... > > components/exo/gaming_seat.h:23: static GamingSeat* > > CreateGamingSeat(GamingSeatDelegate* gaming_seat_delegate, > > On 2017/06/01 at 19:43:15, jkwang wrote: > > > On 2017/05/31 22:42:23, reveman wrote: > > > > do we need to use a factory pattern if this is now compile time? why not > > just > > > > put a few ifdefs in gaming_seat.h/cc and be done? > > > > > > No..It does not need to be like this, but I would want all the ifdef here > > (don't want to do it in server.cc). So I kind of have to wrap the constructor > > (cause gaming_seat_ozone does not need a thread) and expose an unified interface > > to server.cc. I don't see any performance issue here. > > > > Performance is not a concern of mine. Making the code clean and easy to > > understand is important to me. If this is a compile time option then please have > > simple non-factory mechanism that server.cc can use to create an instance and > > either ifdef everything in gaming_seat.cc/h or add gaming_seat_IMPL.cc files if > > the amount of changes are large enough to justify separate files. If you're > > using separate files or not should not affect the actual implementation and is > > just a choice between using ifdefs or BUILD.gn to control what is compiled. You > > can follow base/memory/shared_memory.h/cc as an example of how to do this. > > I am not sure I understand you, so yell at me if I am wrong. > The case is a little bit different to shared_memory.h/cc. > For shared_memory.h/cc: > 1. Code from one impl won't compile when it is compiled to other platform. > 2. Different impl share same basic structure and some functions can be reused. > > For gaming_seat, code can compile together and gaming_seat_ozone and gaming_seat_joydev are almost completely different. If I do the "shared_memory way", I might end up with: > class GamingSeat { > #ifdef ozone > ... All functions from Current GamingSeatOzone > #else > ... All functions from Current GamingSeatJoydev > #endif > } > > I would think it's a little bit hard to read and just get code jammed together. This is fine for gaming_seat.h as you of course want the public API to be the same and you want to minimize the difference in general. Implementations can go into separate files if that makes the code easier to read and understand. I'd recommend having non-ozone impl in gaming_seat.cc and ozone impl in gaming_seat_ozone.cc if multiple files are needed.
I am leaving the flag to be false. A later patch could easily flip it and turn on the ozone gamepad.
I like this approach. Keeps it simple and makes removing the old joydev code trivial in the future https://codereview.chromium.org/2900773003/diff/60001/chrome/common/features.gni File chrome/common/features.gni (right): https://codereview.chromium.org/2900773003/diff/60001/chrome/common/features.... chrome/common/features.gni:54: enable_ozone_gamepad_in_exo = false nit: enable_exo_ozone_gamepad? https://codereview.chromium.org/2900773003/diff/60001/components/exo/BUILD.gn File components/exo/BUILD.gn (right): https://codereview.chromium.org/2900773003/diff/60001/components/exo/BUILD.gn... components/exo/BUILD.gn:73: if (use_ozone && enable_ozone_gamepad_in_exo) { hm, is use_ozone needed? isn't that automatic from _ozone file name prefix? anyhow, probably shouldn't be setting enable_ozone_gamepad_in_exo to true unless when building for ozone https://codereview.chromium.org/2900773003/diff/60001/components/exo/BUILD.gn... components/exo/BUILD.gn:157: "../../ui/events/ozone/gamepad/gamepad_event.cc", I think you can include this unconditionally when use_ozone is true https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... File components/exo/gaming_seat.h (right): https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:16: #include "components/exo/gaming_seat.h" avoid including yourself https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:52: static base::Thread* CreatePollingThreadIfNeeded(); please remove this and always create this thread as before. thread has minimal cost when not used. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:57: base::Thread* polling_thread); "base::SingleThreadTaskRunner* polling_task_runner" as before https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:59: #if !defined(USE_OZONE_GAMEPAD) Let's just remove this testing only ctor a related tests as ozone gamepad is the future and the code that we care about testing. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:69: // Overridden WMHelper::FocusObserver: while here, please change to: // Overridden from WMHelper::FocusObserver: https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:77: // Overriden ui::GamepadObserver. These functions are overrided as private s/Overriden/Overridden/ and make it "// Overridden from ui::GamepadObserver:" without a follow up comment. Also, please don't make this private as you can always cast this to ui::GamepadObserver and then it's not private. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:85: // This map maps device id to gamepad delegates. nit: describe what is stored in the map instead of what it's used for. E.g. "Contains the delegate for each gamepad device." https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:86: std::map<int, GamepadDelegate*> gamepads_; base::flat_map https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:88: ui::GamepadProviderOzone* gamepad_provider_; nit: short comment above this for consistency https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:89: nit: remove this blank line https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:111: DISALLOW_COPY_AND_ASSIGN(GamingSeat); nit: blank line before this https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... File components/exo/gaming_seat_ozone.cc (right): https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:12: namespace { nit: blank line after this https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:13: constexpr int kMaxGamepadCount = 4; nit: s/constexpr/const/ as that's more consistent with exo code and please add a short comment above this to explain the constant https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:14: } // namespace nit: blank line before this https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:48: if (!target) { hm, why is this scope needed? https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:62: std::vector<ui::InputDevice> gamepad_devices = hm, would be nice if GetGamepadDevices could return a reference instead of making a copy of a vector https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:65: std::map<int, GamepadDelegate*> old_gamepad_map; nit: old_gamepads and I would prefer new_gamepads and reverse the logic below to swap at the end instead https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:70: auto ite = old_gamepad_map.find(device.id); nit: s/ite/it/ https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:77: // Send event for the disconected gamepads. nit: s/Send event for the/Remove each/ nit: s/disconected gamepads/disconnected gamepad/ https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:78: for (auto& entry : old_gamepad_map) { nit: no need for {} https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:84: if (gamepads_.size() >= kMaxGamepadCount) { nit: no need for {} why do we need this kMaxGamepadCount limit? https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:87: if (gamepads_.find(device.id) == gamepads_.end()) { nit: no need for {} https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:94: auto ite = gamepads_.find(event.device_id()); nit: s/ite/it/ https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:95: if (ite == gamepads_.end()) DCHECK after removing kMaxGamepadCount if possible https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:111: void GamingSeat::Enable(bool enable) { Do we need this function? Your only calling it from one place and I think the code would be easier to understand without it. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... File components/exo/gaming_seat_ozone_unittest.cc (right): https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Can we just make this the default unit tests and keep the gaming_seat_unittest.cc file name? I don't care about testing the old implementation if we're phasing it out. If you can write tests that don't depend on implementation details than that would be even better!
https://codereview.chromium.org/2900773003/diff/60001/chrome/common/features.gni File chrome/common/features.gni (right): https://codereview.chromium.org/2900773003/diff/60001/chrome/common/features.... chrome/common/features.gni:54: enable_ozone_gamepad_in_exo = false On 2017/06/05 22:48:03, reveman wrote: > nit: enable_exo_ozone_gamepad? Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/BUILD.gn File components/exo/BUILD.gn (right): https://codereview.chromium.org/2900773003/diff/60001/components/exo/BUILD.gn... components/exo/BUILD.gn:73: if (use_ozone && enable_ozone_gamepad_in_exo) { On 2017/06/05 22:48:03, reveman wrote: > hm, is use_ozone needed? isn't that automatic from _ozone file name prefix? > anyhow, probably shouldn't be setting enable_ozone_gamepad_in_exo to true unless > when building for ozone Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/BUILD.gn... components/exo/BUILD.gn:157: "../../ui/events/ozone/gamepad/gamepad_event.cc", On 2017/06/05 22:48:03, reveman wrote: > I think you can include this unconditionally when use_ozone is true Won't compile. The test case is specific for one impl. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... File components/exo/gaming_seat.h (right): https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:16: #include "components/exo/gaming_seat.h" On 2017/06/05 22:48:04, reveman wrote: > avoid including yourself Sorry. Copy/paste mistake. Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:52: static base::Thread* CreatePollingThreadIfNeeded(); On 2017/06/05 22:48:03, reveman wrote: > please remove this and always create this thread as before. thread has minimal > cost when not used. Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:57: base::Thread* polling_thread); On 2017/06/05 22:48:03, reveman wrote: > "base::SingleThreadTaskRunner* polling_task_runner" as before Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:59: #if !defined(USE_OZONE_GAMEPAD) On 2017/06/05 22:48:03, reveman wrote: > Let's just remove this testing only ctor a related tests as ozone gamepad is the > future and the code that we care about testing. Make sense. If we are not going to change code of the old gaming_seat impl. Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:69: // Overridden WMHelper::FocusObserver: On 2017/06/05 22:48:03, reveman wrote: > while here, please change to: // Overridden from WMHelper::FocusObserver: Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:77: // Overriden ui::GamepadObserver. These functions are overrided as private On 2017/06/05 22:48:03, reveman wrote: > s/Overriden/Overridden/ and make it "// Overridden from ui::GamepadObserver:" > without a follow up comment. > > Also, please don't make this private as you can always cast this to > ui::GamepadObserver and then it's not private. Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:85: // This map maps device id to gamepad delegates. On 2017/06/05 22:48:03, reveman wrote: > nit: describe what is stored in the map instead of what it's used for. E.g. > "Contains the delegate for each gamepad device." Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:86: std::map<int, GamepadDelegate*> gamepads_; On 2017/06/05 22:48:04, reveman wrote: > base::flat_map Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:88: ui::GamepadProviderOzone* gamepad_provider_; On 2017/06/05 22:48:04, reveman wrote: > nit: short comment above this for consistency Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:89: On 2017/06/05 22:48:04, reveman wrote: > nit: remove this blank line Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat.h:111: DISALLOW_COPY_AND_ASSIGN(GamingSeat); On 2017/06/05 22:48:03, reveman wrote: > nit: blank line before this Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... File components/exo/gaming_seat_ozone.cc (right): https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:12: namespace { On 2017/06/05 22:48:04, reveman wrote: > nit: blank line after this Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:13: constexpr int kMaxGamepadCount = 4; On 2017/06/05 22:48:04, reveman wrote: > nit: s/constexpr/const/ as that's more consistent with exo code and please add a > short comment above this to explain the constant Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:14: } // namespace On 2017/06/05 22:48:04, reveman wrote: > nit: blank line before this Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:48: if (!target) { On 2017/06/05 22:48:04, reveman wrote: > hm, why is this scope needed? To be honest, I don't know. I am reusing the same logic from old gaming_seat. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:62: std::vector<ui::InputDevice> gamepad_devices = On 2017/06/05 22:48:04, reveman wrote: > hm, would be nice if GetGamepadDevices could return a reference instead of > making a copy of a vector Yes, it should! I have added it to my bug. But I will fix it in a following patch cause I have to update ozone part. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:65: std::map<int, GamepadDelegate*> old_gamepad_map; On 2017/06/05 22:48:04, reveman wrote: > nit: old_gamepads and I would prefer new_gamepads and reverse the logic below to > swap at the end instead Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:70: auto ite = old_gamepad_map.find(device.id); On 2017/06/05 22:48:04, reveman wrote: > nit: s/ite/it/ Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:77: // Send event for the disconected gamepads. On 2017/06/05 22:48:04, reveman wrote: > nit: s/Send event for the/Remove each/ > nit: s/disconected gamepads/disconnected gamepad/ Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:78: for (auto& entry : old_gamepad_map) { On 2017/06/05 22:48:04, reveman wrote: > nit: no need for {} Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:84: if (gamepads_.size() >= kMaxGamepadCount) { On 2017/06/05 22:48:04, reveman wrote: > nit: no need for {} > > why do we need this kMaxGamepadCount limit? /device/gamepad support up to 4 gamepads. So android part only support 4 gamepads. I added a todo item to check if we should remove this and change android later. Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:87: if (gamepads_.find(device.id) == gamepads_.end()) { On 2017/06/05 22:48:04, reveman wrote: > nit: no need for {} Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:94: auto ite = gamepads_.find(event.device_id()); On 2017/06/05 22:48:04, reveman wrote: > nit: s/ite/it/ Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:95: if (ite == gamepads_.end()) On 2017/06/05 22:48:04, reveman wrote: > DCHECK after removing kMaxGamepadCount if possible Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:111: void GamingSeat::Enable(bool enable) { On 2017/06/05 22:48:04, reveman wrote: > Do we need this function? Your only calling it from one place and I think the > code would be easier to understand without it. Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... File components/exo/gaming_seat_ozone_unittest.cc (right): https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/06/05 22:48:04, reveman wrote: > Can we just make this the default unit tests and keep the > gaming_seat_unittest.cc file name? I don't care about testing the old > implementation if we're phasing it out. If you can write tests that don't depend > on implementation details than that would be even better! Done.
https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... File components/exo/gaming_seat_ozone.cc (right): https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:12: namespace { On 2017/06/05 22:48:04, reveman wrote: > nit: blank line after this Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:13: constexpr int kMaxGamepadCount = 4; On 2017/06/05 22:48:04, reveman wrote: > nit: s/constexpr/const/ as that's more consistent with exo code and please add a > short comment above this to explain the constant Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:14: } // namespace On 2017/06/05 22:48:04, reveman wrote: > nit: blank line before this Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:48: if (!target) { On 2017/06/05 22:48:04, reveman wrote: > hm, why is this scope needed? To be honest, I don't know. I am reusing the same logic from old gaming_seat. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:62: std::vector<ui::InputDevice> gamepad_devices = On 2017/06/05 22:48:04, reveman wrote: > hm, would be nice if GetGamepadDevices could return a reference instead of > making a copy of a vector Yes, it should! I have added it to my bug. But I will fix it in a following patch cause I have to update ozone part. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:65: std::map<int, GamepadDelegate*> old_gamepad_map; On 2017/06/05 22:48:04, reveman wrote: > nit: old_gamepads and I would prefer new_gamepads and reverse the logic below to > swap at the end instead Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:70: auto ite = old_gamepad_map.find(device.id); On 2017/06/05 22:48:04, reveman wrote: > nit: s/ite/it/ Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:77: // Send event for the disconected gamepads. On 2017/06/05 22:48:04, reveman wrote: > nit: s/Send event for the/Remove each/ > nit: s/disconected gamepads/disconnected gamepad/ Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:84: if (gamepads_.size() >= kMaxGamepadCount) { On 2017/06/05 22:48:04, reveman wrote: > nit: no need for {} > > why do we need this kMaxGamepadCount limit? /device/gamepad support up to 4 gamepads. So android part only support 4 gamepads. I added a todo item to check if we should remove this and change android later. Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:87: if (gamepads_.find(device.id) == gamepads_.end()) { On 2017/06/05 22:48:04, reveman wrote: > nit: no need for {} Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:94: auto ite = gamepads_.find(event.device_id()); On 2017/06/05 22:48:04, reveman wrote: > nit: s/ite/it/ Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:95: if (ite == gamepads_.end()) On 2017/06/05 22:48:04, reveman wrote: > DCHECK after removing kMaxGamepadCount if possible Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:111: void GamingSeat::Enable(bool enable) { On 2017/06/05 22:48:04, reveman wrote: > Do we need this function? Your only calling it from one place and I think the > code would be easier to understand without it. Done. https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... File components/exo/gaming_seat_ozone_unittest.cc (right): https://codereview.chromium.org/2900773003/diff/60001/components/exo/gaming_s... components/exo/gaming_seat_ozone_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/06/05 22:48:04, reveman wrote: > Can we just make this the default unit tests and keep the > gaming_seat_unittest.cc file name? I don't care about testing the old > implementation if we're phasing it out. If you can write tests that don't depend > on implementation details than that would be even better! Done.
nice! just a few nits and questions https://codereview.chromium.org/2900773003/diff/80001/chrome/common/features.gni File chrome/common/features.gni (right): https://codereview.chromium.org/2900773003/diff/80001/chrome/common/features.... chrome/common/features.gni:54: enable_exo_ozone_gamepad = use_ozone && false so how to we enable this in a build now? https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... File components/exo/gaming_seat.h (left): https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:27: // This class represents one gaming seat, it uses a background thread Please keep a class description here in some form. https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... File components/exo/gaming_seat.h (right): https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:40: // TODO(jkwang) always use ozone_gamepad when ozone is default for all chrome os nit: s/TODO(jkwang)/TODO(jkwang):", s/chrome os/Chrome OS/ and add a bug number. Also why are we not just limiting gamepad interface support to ozone builds? https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:41: // build. nit: s/build/builds/ https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:49: // This class will monitor gamepad connection change and manage gamepad s/change/changes/ https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:75: ui::GamepadProviderOzone* gamepad_provider_; why do we need this? please avoid caching a simple pointer that if possible so we don't have to worry about it becoming invalid https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... File components/exo/gaming_seat_ozone.cc (right): https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:17: const int kMaxGamepadCount = 4; can we just not have this in the first place? I'm failing to see why that would not work. The code below seem to handle it correctly, or not?
https://codereview.chromium.org/2900773003/diff/80001/chrome/common/features.gni File chrome/common/features.gni (right): https://codereview.chromium.org/2900773003/diff/80001/chrome/common/features.... chrome/common/features.gni:54: enable_exo_ozone_gamepad = use_ozone && false On 2017/06/06 21:58:55, reveman wrote: > so how to we enable this in a build now? What I was thinking is to change this to use_ozone && is_chromeos later. https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... File components/exo/gaming_seat.h (left): https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:27: // This class represents one gaming seat, it uses a background thread On 2017/06/06 21:58:55, reveman wrote: > Please keep a class description here in some form. Done. https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... File components/exo/gaming_seat.h (right): https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:40: // TODO(jkwang) always use ozone_gamepad when ozone is default for all chrome os On 2017/06/06 21:58:55, reveman wrote: > nit: s/TODO(jkwang)/TODO(jkwang):", s/chrome os/Chrome OS/ and add a bug number. > Also why are we not just limiting gamepad interface support to ozone builds? Done. https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:41: // build. On 2017/06/06 21:58:55, reveman wrote: > nit: s/build/builds/ Done. https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:49: // This class will monitor gamepad connection change and manage gamepad On 2017/06/06 21:58:55, reveman wrote: > s/change/changes/ Done. https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:75: ui::GamepadProviderOzone* gamepad_provider_; On 2017/06/06 21:58:55, reveman wrote: > why do we need this? please avoid caching a simple pointer that if possible so > we don't have to worry about it becoming invalid It is suggested here. https://cs.chromium.org/chromium/src/base/memory/singleton.h?l=215 https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... File components/exo/gaming_seat_ozone.cc (right): https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:17: const int kMaxGamepadCount = 4; On 2017/06/06 21:58:55, reveman wrote: > can we just not have this in the first place? I'm failing to see why that would > not work. The code below seem to handle it correctly, or not? The code below, yes. But the android counter part only support 4 gamepads. (And the android part does not yet support connect/disconnect gamepad device dynamically) As far as I know, call into gamepad added won't fail. Which will cause unexpected behavior. For example, if I connect 5 gamepad with id 0,1,2,3,4. Gamepad added is called for each of them but android only handle 0,1,2,3. Later, when I disconnect 0, android will only handle 1,2,3 but will not handle 4.
https://codereview.chromium.org/2900773003/diff/80001/chrome/common/features.gni File chrome/common/features.gni (right): https://codereview.chromium.org/2900773003/diff/80001/chrome/common/features.... chrome/common/features.gni:54: enable_exo_ozone_gamepad = use_ozone && false On 2017/06/07 at 18:03:18, jkwang wrote: > On 2017/06/06 21:58:55, reveman wrote: > > so how to we enable this in a build now? > > What I was thinking is to change this to use_ozone && is_chromeos later. Why not make it so we can enable this using a gn flag instead of having to land another patch? https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... File components/exo/gaming_seat.h (right): https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:75: ui::GamepadProviderOzone* gamepad_provider_; On 2017/06/07 at 18:03:19, jkwang wrote: > On 2017/06/06 21:58:55, reveman wrote: > > why do we need this? please avoid caching a simple pointer that if possible so > > we don't have to worry about it becoming invalid > It is suggested here. > https://cs.chromium.org/chromium/src/base/memory/singleton.h?l=215 16ns on my P4/2.8GHz is not worth micro optimizing for imo. https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... File components/exo/gaming_seat_ozone.cc (right): https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:17: const int kMaxGamepadCount = 4; On 2017/06/07 at 18:03:19, jkwang wrote: > On 2017/06/06 21:58:55, reveman wrote: > > can we just not have this in the first place? I'm failing to see why that would > > not work. The code below seem to handle it correctly, or not? > > The code below, yes. But the android counter part only support 4 gamepads. (And the android part does not yet support connect/disconnect gamepad device dynamically) As far as I know, call into gamepad added won't fail. Which will cause unexpected behavior. For example, if I connect 5 gamepad with id 0,1,2,3,4. Gamepad added is called for each of them but android only handle 0,1,2,3. Later, when I disconnect 0, android will only handle 1,2,3 but will not handle 4. Let's not have code here that makes assumptions about the client implementation. Especially not code that makes the implementation more complicated. Please fix the android side instead. https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... File components/exo/gaming_seat_ozone.cc (right): https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:23: GamingSeat::GamingSeat(GamingSeatDelegate* delegate, Add: //////////////////////////////////////////////////////////////////////////////// // GamingSeat, public: above this (blank line before GamingSeat::...) https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:43: void GamingSeat::OnWindowFocused(aura::Window* gained_focus, //////////////////////////////////////////////////////////////////////////////// // WMHelper::FocusObserver overrides: above this https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:65: // GamingSeat, Override functions for GamepadObserver. // ui::GamepadObserver overrides: https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:66: void GamingSeat::OnGamepadDevicesUpdated() { nit: blank line before this https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... File components/exo/gaming_seat_unittest.cc (right): https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... components/exo/gaming_seat_unittest.cc:80: ui::GamepadProviderOzone* gamepad_provider_; nit: please avoid caching this here too. it's only a test so performance is definitely not a problem
https://codereview.chromium.org/2900773003/diff/80001/chrome/common/features.gni File chrome/common/features.gni (right): https://codereview.chromium.org/2900773003/diff/80001/chrome/common/features.... chrome/common/features.gni:54: enable_exo_ozone_gamepad = use_ozone && false Sounds good. How should I do that? https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... File components/exo/gaming_seat.h (right): https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat.h:75: ui::GamepadProviderOzone* gamepad_provider_; On 2017/06/07 20:29:45, reveman wrote: > On 2017/06/07 at 18:03:19, jkwang wrote: > > On 2017/06/06 21:58:55, reveman wrote: > > > why do we need this? please avoid caching a simple pointer that if possible > so > > > we don't have to worry about it becoming invalid > > It is suggested here. > > https://cs.chromium.org/chromium/src/base/memory/singleton.h?l=215 > > 16ns on my P4/2.8GHz is not worth micro optimizing for imo. Done. https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... File components/exo/gaming_seat_ozone.cc (right): https://codereview.chromium.org/2900773003/diff/80001/components/exo/gaming_s... components/exo/gaming_seat_ozone.cc:17: const int kMaxGamepadCount = 4; On 2017/06/07 20:29:45, reveman wrote: > On 2017/06/07 at 18:03:19, jkwang wrote: > > On 2017/06/06 21:58:55, reveman wrote: > > > can we just not have this in the first place? I'm failing to see why that > would > > > not work. The code below seem to handle it correctly, or not? > > > > The code below, yes. But the android counter part only support 4 gamepads. > (And the android part does not yet support connect/disconnect gamepad device > dynamically) As far as I know, call into gamepad added won't fail. Which will > cause unexpected behavior. For example, if I connect 5 gamepad with id > 0,1,2,3,4. Gamepad added is called for each of them but android only handle > 0,1,2,3. Later, when I disconnect 0, android will only handle 1,2,3 but will not > handle 4. > > Let's not have code here that makes assumptions about the client implementation. > Especially not code that makes the implementation more complicated. Please fix > the android side instead. Done. https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... File components/exo/gaming_seat_ozone.cc (right): https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:23: GamingSeat::GamingSeat(GamingSeatDelegate* delegate, On 2017/06/07 20:29:46, reveman wrote: > Add: > > //////////////////////////////////////////////////////////////////////////////// > // GamingSeat, public: > > above this (blank line before GamingSeat::...) Done. https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:43: void GamingSeat::OnWindowFocused(aura::Window* gained_focus, On 2017/06/07 20:29:46, reveman wrote: > //////////////////////////////////////////////////////////////////////////////// > // WMHelper::FocusObserver overrides: > > above this Done. https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:65: // GamingSeat, Override functions for GamepadObserver. On 2017/06/07 20:29:45, reveman wrote: > // ui::GamepadObserver overrides: Done. https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:66: void GamingSeat::OnGamepadDevicesUpdated() { On 2017/06/07 20:29:45, reveman wrote: > nit: blank line before this Done. https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... File components/exo/gaming_seat_unittest.cc (right): https://codereview.chromium.org/2900773003/diff/100001/components/exo/gaming_... components/exo/gaming_seat_unittest.cc:80: ui::GamepadProviderOzone* gamepad_provider_; On 2017/06/07 20:29:46, reveman wrote: > nit: please avoid caching this here too. it's only a test so performance is > definitely not a problem Done.
lgtm after updating the description to match the latest patch and fixing last set of comments https://codereview.chromium.org/2900773003/diff/80001/chrome/common/features.gni File chrome/common/features.gni (right): https://codereview.chromium.org/2900773003/diff/80001/chrome/common/features.... chrome/common/features.gni:54: enable_exo_ozone_gamepad = use_ozone && false On 2017/06/07 at 22:01:45, jkwang wrote: > Sounds good. How should I do that? I think we should just make it "enable_exo_ozone_gamepad = use_ozone" as why land this if we're not turning it on somewhere? alternative is "enable_exo_ozone_gamepad = false" and then require specific boards to turn it on but I don't see the reason why we'd do that as we want to turn this on for all boards https://codereview.chromium.org/2900773003/diff/120001/components/exo/gaming_... File components/exo/gaming_seat.cc (left): https://codereview.chromium.org/2900773003/diff/120001/components/exo/gaming_... components/exo/gaming_seat.cc:8: nit: keep this blank line https://codereview.chromium.org/2900773003/diff/120001/components/exo/gaming_... File components/exo/gaming_seat.h (right): https://codereview.chromium.org/2900773003/diff/120001/components/exo/gaming_... components/exo/gaming_seat.h:25: class GamepadProviderOzone; remove this as no longer needed. https://codereview.chromium.org/2900773003/diff/120001/components/exo/gaming_... components/exo/gaming_seat.h:30: class Thread; remove this unless actually needed https://codereview.chromium.org/2900773003/diff/120001/components/exo/gaming_... File components/exo/gaming_seat_ozone.cc (right): https://codereview.chromium.org/2900773003/diff/120001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:4: // remove "//" from this line https://codereview.chromium.org/2900773003/diff/120001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:15: // GamingSeat, plublic: public: https://codereview.chromium.org/2900773003/diff/120001/components/exo/gaming_... components/exo/gaming_seat_ozone.cc:29: for (auto& entry : gamepads_) { nit: no need for {}
Description was changed from
==========
Add gaming_seat_ozone to exo
This patch adds gaming_seat_ozone. It will use ozone/gamepad as backend
of gamepad. The old gaming_seat is renamed to gaming_seat_joydev. The choice of
gaming_seat_ozone or gaming_seat_joydev (using /device/gamepad) is controlled by
a boolean in gaming_seat.cc.
BUG=717246
==========
to
==========
Allow gaming_seat to use ozone gamepad as back-end
This patch enable gaming_seat to use ozone gamepad as back-end. It is
protected by build args. If enable_exo_ozone_gamepad is turned off,
gaming_seat will fallback use old joydev implementation.
BUG=717246
==========
The CQ bit was checked by jkwang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2900773003/#ps140001 (title: "Allow gaming_seat to use ozone gamepad as backend")
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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by jkwang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2900773003/#ps160001 (title: "Allow gaming_seat to use ozone gamepad as backend")
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by jkwang@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2900773003/#ps180001 (title: "Allow gaming_seat to use ozone gamepad as backend")
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": 180001, "attempt_start_ts": 1497300069459300,
"parent_rev": "57f1f0bb48b70875fa118be2266104b7249db921", "commit_rev":
"f72a28a67a141af1e5c29f608e30453c7cc94a5a"}
Message was sent while issue was closed.
Description was changed from
==========
Allow gaming_seat to use ozone gamepad as back-end
This patch enable gaming_seat to use ozone gamepad as back-end. It is
protected by build args. If enable_exo_ozone_gamepad is turned off,
gaming_seat will fallback use old joydev implementation.
BUG=717246
==========
to
==========
Allow gaming_seat to use ozone gamepad as back-end
This patch enable gaming_seat to use ozone gamepad as back-end. It is
protected by build args. If enable_exo_ozone_gamepad is turned off,
gaming_seat will fallback use old joydev implementation.
BUG=717246
Review-Url: https://codereview.chromium.org/2900773003
Cr-Commit-Position: refs/heads/master@{#478834}
Committed:
https://chromium.googlesource.com/chromium/src/+/f72a28a67a141af1e5c29f608e30...
==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/f72a28a67a141af1e5c29f608e30... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
