Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2076013002:
exo: Implement wayland gamepad support (Closed)
|
Created:
4 years, 6 months ago by denniskempin Modified:
4 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@serv Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL implements the gamepads wayland API. Since js device nodes
on linux can be opened multiple times, we are not required to go
through the content::GamepadService, but can directly use our own
device::GamepadPlatformDataFetcher in our own polling thread.
The polling thread will compare the newly fetched data to the previous
data and send any changes as events to the wayland client.
BUG=620977
Committed: https://crrev.com/68de182439eed91857dafbb4348f1e4857b18c69
Cr-Commit-Position: refs/heads/master@{#405356}
Patch Set 1 #Patch Set 2 : using new device/gamepad api #Patch Set 3 : minor adjustments to protocol #
Total comments: 15
Patch Set 4 : switched to single gamepad support #Patch Set 5 : atexo: Implement wayland gamepad support #Patch Set 6 : removed old gamepads.* files #Patch Set 7 : nasty little missing comma... #Patch Set 8 : do not depend on webkit but device/gamepad #
Total comments: 1
Patch Set 9 : fixed polling thread. #
Total comments: 53
Patch Set 10 : removed id from Gamepad and updated wording #
Total comments: 4
Patch Set 11 : fixed nits. only sends gamepad updates if an exo window is in focus. #
Total comments: 18
Patch Set 12 : Focus handling. Process gamepad delta on origin thread. Add focus unittest. #
Total comments: 37
Patch Set 13 : limit polling to when exo windows are focused #
Total comments: 20
Patch Set 14 : moved polling thread resources into separate class. pass task_runner into Gamepad. #
Total comments: 46
Patch Set 15 : fixed nits and smaller issues #
Total comments: 21
Patch Set 16 : fixed nits #Patch Set 17 : changed back callback to store by copy instead of reference. #Patch Set 18 : rebase #Patch Set 19 : rebase #Patch Set 20 : added dep to //base/test:test_support #Patch Set 21 : added dep.. this time to the right target hopefully #Patch Set 22 : ... looks like both targets want this dep. #Patch Set 23 : more missing deps. I wonder why these do not come up during pre-submit #
Depends on Patchset: Messages
Total messages: 69 (28 generated)
Description was changed from ========== WIP: Implement wayland gamepad support BUG= ========== to ========== WIP: Implement wayland gamepad support BUG= ==========
Description was changed from ========== WIP: Implement wayland gamepad support BUG= ========== to ========== exo: Implement wayland gamepad support This CL implements the gamepads wayland API. Since js device nodes on linux can be opened multiple times, we are not required to go through the content::GamepadService, but can directly use our own device::GamepadPlatformDataFetcher in our own polling thread. The polling thread will compare the newly fetched data to the previous data and send any changes as events to the wayland client. BUG=620977 ==========
Description was changed from ========== exo: Implement wayland gamepad support This CL implements the gamepads wayland API. Since js device nodes on linux can be opened multiple times, we are not required to go through the content::GamepadService, but can directly use our own device::GamepadPlatformDataFetcher in our own polling thread. The polling thread will compare the newly fetched data to the previous data and send any changes as events to the wayland client. BUG=620977 ========== to ========== This CL implements the gamepads wayland API. Since js device nodes on linux can be opened multiple times, we are not required to go through the content::GamepadService, but can directly use our own device::GamepadPlatformDataFetcher in our own polling thread. The polling thread will compare the newly fetched data to the previous data and send any changes as events to the wayland client. BUG=620977 ==========
denniskempin@google.com changed reviewers: + bajones@chromium.org, reveman@chromium.org
Sent this out a little prematurely. Still need to polish and document this. But if you see any big issues please let me know!
On 2016/06/23 21:31:39, denniskempin wrote: > Sent this out a little prematurely. Still need to polish and document this. But > if you see any big issues please let me know! Allright. This CL is good to go (after rebasing to the submitted version of bajones CL of course)
lg. some comments but please take a look at my comments on the wayland interface change before this. https://codereview.chromium.org/2076013002/diff/40001/components/exo.gypi File components/exo.gypi (right): https://codereview.chromium.org/2076013002/diff/40001/components/exo.gypi#new... components/exo.gypi:19: # todo(denniskempin): add gyp dependency to /device/gamepad once it exists. nit: not sure this is needed as it's pretty obvious that it should be added once we start depending on compiled code in device/gamepad https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads.cc File components/exo/gamepads.cc (right): https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads... components/exo/gamepads.cc:20: namespace { nit: blank line after this. blank line before is not needed. https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads... components/exo/gamepads.cc:22: bool almostEquals(double a, double b) { nit: AlmostEqual but might also be good to explain what it's used for in the name. e.g. GamepadButtonValuesAreEqual. https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads... components/exo/gamepads.cc:64: fetcher_.reset(new device::GamepadPlatformDataFetcher()); Would be nice if this was always passed to the ctor as a raw pointer where the lifetime of it is managed elsewhere. That way we don't need any special function for testing purposes. Is that possible? https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads.h File components/exo/gamepads.h (right): https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads... components/exo/gamepads.h:26: class Gamepads { I've made a similar comment on the interface change already. Why not singular Gamepad to be consistent with Pointer, Keyboard, Touch? https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads... components/exo/gamepads.h:35: std::unique_ptr<device::GamepadDataFetcher> fetcher); Add SetGamepadDataFetcherForTesting function would e preferred to make it more clear that this is only for testing purposes. https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads... components/exo/gamepads.h:51: GamepadsDelegate* delegate_; nit: GamepadsDelegate* const delegate_; https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads... components/exo/gamepads.h:57: std::unique_ptr<base::Thread> polling_thread_; Do we need to add a thread for this? Can we use an existing thread instead? Maybe just pass a gamepad_data_fetch_task_runner to exo::Display ctor and have the code here be agnostic to what thread that is.. https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads... components/exo/gamepads.h:66: DISALLOW_COPY_AND_ASSIGN(Gamepads); nit: blank line before this
reveman, I fixed or commented on your notes.
This CL is also updates to use a single gamepad. Eventually we will want to
extend exo to support multiple seats for multiple gamepads as we discussed.
One note, I am still getting the following presubmit errors that I do not know
how to fix. I have added those dependencies to gyp, gn and DEPS.
** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
components/exo/gamepad.cc
Illegal include: "device/gamepad/gamepad_data_fetcher.h"
Because of no rule applying. \
components/exo/gamepad.cc
Illegal include: "device/gamepad/gamepad_platform_data_fetcher.h"
Because of no rule applying. \
components/exo/gamepad.h
Illegal include: "third_party/WebKit/public/platform/WebGamepads.h"
Because of no rule applying. \
components/exo/gamepad_unittest.cc
Illegal include: "device/gamepad/gamepad_test_helpers.h"
Because of no rule applying. \
components/exo/gamepads.cc
Illegal include: "device/gamepad/gamepad_data_fetcher.h"
Because of no rule applying. \
components/exo/gamepads.cc
Illegal include: "device/gamepad/gamepad_platform_data_fetcher.h"
Because of no rule applying. \
components/exo/gamepads.h
Illegal include: "third_party/WebKit/public/platform/WebGamepads.h"
Because of no rule applying. \
components/exo/gamepads_unittest.cc
Illegal include: "device/gamepad/gamepad_test_helpers.h"
Because of no rule applying.
https://codereview.chromium.org/2076013002/diff/40001/components/exo.gypi
File components/exo.gypi (right):
https://codereview.chromium.org/2076013002/diff/40001/components/exo.gypi#new...
components/exo.gypi:19: # todo(denniskempin): add gyp dependency to
/device/gamepad once it exists.
On 2016/06/27 17:30:34, reveman wrote:
> nit: not sure this is needed as it's pretty obvious that it should be added
once
> we start depending on compiled code in device/gamepad
done. I was just waiting for a gyp build file to be added to bajones@s CL.
https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads.h
File components/exo/gamepads.h (right):
https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads...
components/exo/gamepads.h:26: class Gamepads {
On 2016/06/27 17:30:35, reveman wrote:
> I've made a similar comment on the interface change already. Why not singular
> Gamepad to be consistent with Pointer, Keyboard, Touch?
Done.
https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads...
components/exo/gamepads.h:35: std::unique_ptr<device::GamepadDataFetcher>
fetcher);
On 2016/06/27 17:30:35, reveman wrote:
> Add SetGamepadDataFetcherForTesting function would e preferred to make it more
> clear that this is only for testing purposes.
Not sure we can do that since this class will initialize a platform-based
fetcher in the constructor, which we don't want it to do for tests. I am
following the same layout as content/browser/GamepadService here.
We could make this constructor private and add a static method along the lines
of CreateWithGamepadDataFetcherForTesting
Or another alternative.. don't initialize in the constructor but provide a
separate Initialize() method that allow us to use
SetGamepadDataFetcherForTesting before initialization. But generally I'd like to
follow the practice of having the constructor initialize.
Let me know what you prefer.
https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads...
components/exo/gamepads.h:51: GamepadsDelegate* delegate_;
On 2016/06/27 17:30:35, reveman wrote:
> nit: GamepadsDelegate* const delegate_;
Done.
https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads...
components/exo/gamepads.h:57: std::unique_ptr<base::Thread> polling_thread_;
On 2016/06/27 17:30:35, reveman wrote:
> Do we need to add a thread for this? Can we use an existing thread instead?
> Maybe just pass a gamepad_data_fetch_task_runner to exo::Display ctor and have
> the code here be agnostic to what thread that is..
I've been following the code of GamepadService here as well. I think it's a good
idea to use a separate thread for two reasons:
a) consistency of polling. We don't want the poll to get delayed too much
because something else is running on the task runner.
b) eventually I'd like to move towards blocking IO (or selecting fd's) instead
of polling. The gamepads api can't do that since not all platforms allow
blocking IO for gamepads, but since we run on linux only, we are free to do so.
https://codereview.chromium.org/2076013002/diff/40001/components/exo/gamepads...
components/exo/gamepads.h:66: DISALLOW_COPY_AND_ASSIGN(Gamepads);
On 2016/06/27 17:30:35, reveman wrote:
> nit: blank line before this
Done.
There was a comma missing in the DEPS file, that solves the presubmit issue. Also I am now depending directly on "+third_party/WebKit/public/platform/WebGamepads.h" instead of all of WebKit/public.
https://codereview.chromium.org/2076013002/diff/140001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/140001/components/exo/gamepad... components/exo/gamepad.cc:41: origin_task_runner_(base::ThreadTaskRunnerHandle::Get()) { reveman: An update on this. Eventually we want to replace this either with the GamepadService, which already has a polling thread... or change the underlying gamepad api to allow for blocking IO in our own thread, so we don't need to poll.
mostly nits https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:20: namespace { nit: blank line after this. blank line before is not needed https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:21: constexpr double kEpsilon = 0.001; nit: kGamepadButtonValueEpsilon and blank line after this line https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:22: bool almostEquals(double a, double b) { GamepadButtonValuesAreEqual()? https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:26: } // anonymous namespace nit: "} // namespace" to be consistent with rest of exo/ https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:60: base::TimeDelta::FromMilliseconds(16)); 16? Please add a constant and a comment for this magic value https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:75: // Update state with data from gamepad data fetcher s/fetcher/fetcher./ https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:85: DCHECK(base::MessageLoop::current() == polling_thread_->message_loop()); Can you use the thread checker here instead? https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:94: // Update connection state nit: s/state/state./ https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:98: base::Unretained(delegate_), new_pad.connected)); Is base::Unretained safe here and below? what if the gamepad is destroyed before this is processed? https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:104: // Notify delegate of updated axes nit: s/axes/axes./ https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:116: // Notify delegate of updated buttons nit: s/buttons/buttons./ https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:121: pad.buttons[button].pressed != new_pad.buttons[button].pressed) { check "pressed" first as it's cheaper https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.h:19: // This class represents a gamepad, it implements a background thread s/a gamepad/one or more gamepads/ as discussed https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.h:26: Gamepad(size_t id, GamepadDelegate* delegate); remove id as discussed https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.h:30: Gamepad(size_t id, remove id as discussed https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.h:52: size_t id_; remove as discussed https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.h:61: std::unique_ptr<base::Thread> polling_thread_; fyi, one thread per gamepad interface instances is far from ideal but fine for now as the current usage is one client and one instance. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:21: nit: remove blank line https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:23: nit: remove blank line https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:29: class GamepadTests : public testing::Test, public device::GamepadTestHelper { s/GamepadTests/GamepadTest/ https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:70: // Gamepad connected s/connected/connected./ https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:78: // Gamepad disconnected ditto https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:97: // Gamepad connected and here https://codereview.chromium.org/2076013002/diff/160001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2076013002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2745: // zwp_gamepad_v1: nit: convention is "gaming_input_interface:". not great but best to be consistent https://codereview.chromium.org/2076013002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2755: nit: remove this blank line https://codereview.chromium.org/2076013002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2803: auto gamepad = new Gamepad(0, new WaylandGamepadDelegate(gamepad_resource)); nit: remove this temporary variable for consistency https://codereview.chromium.org/2076013002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2811: void bing_gaming_input(wl_client* client, s/bing/bind/ https://codereview.chromium.org/2076013002/diff/180001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/180001/components/exo/gamepad... components/exo/gamepad.h:26: Gamepad(GamepadDelegate* delegate); nit: explicit https://codereview.chromium.org/2076013002/diff/180001/components/exo/gamepad... components/exo/gamepad.h:27: nit: remove this blank line to ctors and dtor in one group
Gamepad will now check the focus before sending updates to the client https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:20: namespace { On 2016/06/29 21:55:44, reveman wrote: > nit: blank line after this. blank line before is not needed Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:21: constexpr double kEpsilon = 0.001; On 2016/06/29 21:55:44, reveman wrote: > nit: kGamepadButtonValueEpsilon and blank line after this line Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:22: bool almostEquals(double a, double b) { On 2016/06/29 21:55:44, reveman wrote: > GamepadButtonValuesAreEqual()? Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:26: } // anonymous namespace On 2016/06/29 21:55:44, reveman wrote: > nit: "} // namespace" to be consistent with rest of exo/ Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:60: base::TimeDelta::FromMilliseconds(16)); On 2016/06/29 21:55:44, reveman wrote: > 16? Please add a constant and a comment for this magic value Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:75: // Update state with data from gamepad data fetcher On 2016/06/29 21:55:44, reveman wrote: > s/fetcher/fetcher./ Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:85: DCHECK(base::MessageLoop::current() == polling_thread_->message_loop()); On 2016/06/29 21:55:44, reveman wrote: > Can you use the thread checker here instead? It's a different thread. This runs in the polling thread. The thread checker checks for the origin thread. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:94: // Update connection state On 2016/06/29 21:55:44, reveman wrote: > nit: s/state/state./ Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:104: // Notify delegate of updated axes On 2016/06/29 21:55:44, reveman wrote: > nit: s/axes/axes./ Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:116: // Notify delegate of updated buttons On 2016/06/29 21:55:44, reveman wrote: > nit: s/buttons/buttons./ Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.cc:121: pad.buttons[button].pressed != new_pad.buttons[button].pressed) { On 2016/06/29 21:55:44, reveman wrote: > check "pressed" first as it's cheaper Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.h:19: // This class represents a gamepad, it implements a background thread On 2016/06/29 21:55:44, reveman wrote: > s/a gamepad/one or more gamepads/ as discussed Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.h:26: Gamepad(size_t id, GamepadDelegate* delegate); On 2016/06/29 21:55:45, reveman wrote: > remove id as discussed Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.h:30: Gamepad(size_t id, On 2016/06/29 21:55:45, reveman wrote: > remove id as discussed Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.h:52: size_t id_; On 2016/06/29 21:55:45, reveman wrote: > remove as discussed Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad.h:61: std::unique_ptr<base::Thread> polling_thread_; On 2016/06/29 21:55:45, reveman wrote: > fyi, one thread per gamepad interface instances is far from ideal but fine for > now as the current usage is one client and one instance. I agree. We will need some kind of shared service between the gamepads for multi-seat support. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:21: On 2016/06/29 21:55:45, reveman wrote: > nit: remove blank line Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:23: On 2016/06/29 21:55:45, reveman wrote: > nit: remove blank line Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:29: class GamepadTests : public testing::Test, public device::GamepadTestHelper { On 2016/06/29 21:55:45, reveman wrote: > s/GamepadTests/GamepadTest/ Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:70: // Gamepad connected On 2016/06/29 21:55:45, reveman wrote: > s/connected/connected./ Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:78: // Gamepad disconnected On 2016/06/29 21:55:45, reveman wrote: > ditto Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/gamepad... components/exo/gamepad_unittest.cc:97: // Gamepad connected On 2016/06/29 21:55:45, reveman wrote: > and here Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2076013002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2745: // zwp_gamepad_v1: On 2016/06/29 21:55:45, reveman wrote: > nit: convention is "gaming_input_interface:". not great but best to be > consistent Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2755: On 2016/06/29 21:55:45, reveman wrote: > nit: remove this blank line Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2803: auto gamepad = new Gamepad(0, new WaylandGamepadDelegate(gamepad_resource)); On 2016/06/29 21:55:45, reveman wrote: > nit: remove this temporary variable for consistency Done. https://codereview.chromium.org/2076013002/diff/160001/components/exo/wayland... components/exo/wayland/server.cc:2811: void bing_gaming_input(wl_client* client, On 2016/06/29 21:55:45, reveman wrote: > s/bing/bind/ sorry about the trademark infringement ;) done. https://codereview.chromium.org/2076013002/diff/180001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/180001/components/exo/gamepad... components/exo/gamepad.h:26: Gamepad(GamepadDelegate* delegate); On 2016/06/29 21:55:45, reveman wrote: > nit: explicit Done. https://codereview.chromium.org/2076013002/diff/180001/components/exo/gamepad... components/exo/gamepad.h:27: On 2016/06/29 21:55:45, reveman wrote: > nit: remove this blank line to ctors and dtor in one group Done.
https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:32: constexpr unsigned kPollingTimeDelta = 16; nit: s/TimeDelta/Interval/? Chromium usually adds Ms or Milliseconds suffix to make the unit clear This wakeup interval worries me and the effect it might have on battery usage. Adding 60 wakeups per second on ChromeOS seems really bad. How do we avoid this when the gamepad is not used? Will the client just not instantiate a gamepad in that case? https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:46: exo_focused_(false), nit: all exo code is now providing default init of members where declared https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:70: base::AutoLock _l(lock_); nit: base::AutoLock lock(lock_); It's hard to follow what happens to events in flight when focus changes. Did you consider just doing all this filtering on the origin thread and not have the polling thread aware of focus? I think that would be easier to understand and maybe there wouldn't even be a need for a mutex. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:138: delegate_->GetWeakPtr(), new_pad.connected)); GetWeakPtr() can't be called and the weak ptr returned can't be validated on a different thread than the weak ptr factory was created on. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.h:69: bool exo_focused_; I don't think this is enough. How do we know that the surface actually belongs to the client that owns this gamepad instance? I think this needs to be "Surface* focus_ = nullptr;" and the same logic as keyboard impl. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.h:77: base::ThreadChecker thread_checker_; I think a thread checker is a good idea but I'm not seeing it used. Did you forget to update the cc file? https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... File components/exo/gamepad_delegate.h (right): https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad_delegate.h:36: virtual base::WeakPtr<GamepadDelegate> GetWeakPtr() = 0; Please handle this in the Gamepad implementation instead. The delegate is supposed to be as simple as possible.
https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:32: constexpr unsigned kPollingTimeDelta = 16; On 2016/06/30 01:19:05, reveman wrote: > nit: s/TimeDelta/Interval/? Chromium usually adds Ms or Milliseconds suffix to > make the unit clear > > This wakeup interval worries me and the effect it might have on battery usage. > Adding 60 wakeups per second on ChromeOS seems really bad. How do we avoid this > when the gamepad is not used? Will the client just not instantiate a gamepad in > that case? Unfortunately we cannot do anything about this, with the current gamepad api we have to poll to even find out when devices are connected. The same thing is already happening in the gamepad service. Their polling thread is always on, polling at 60Hz as well. Unfortunately even if we use the GamepadService later, we still need to poll but we could restrict the polling to when gamepads are active. I am considering customizing the code more deeply for our purposes and reimplement the gamepad data fetcher in a blocking mode that will wait for devices and block on io, so we can get rid of the polling. This cannot be done easily in the gamepad api since some platforms have to use polling for gamepads. Unfortunately for now.. this is the best we can do. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:70: base::AutoLock _l(lock_); On 2016/06/30 01:19:05, reveman wrote: > nit: base::AutoLock lock(lock_); > > It's hard to follow what happens to events in flight when focus changes. Did you > consider just doing all this filtering on the origin thread and not have the > polling thread aware of focus? I think that would be easier to understand and > maybe there wouldn't even be a need for a mutex. I am not quite sure how that would look like. Events in flight will still be transmitted with this solution. As soon as the next poll comes around the polling thread will know of the lost focus and stop. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.h:69: bool exo_focused_; On 2016/06/30 01:19:05, reveman wrote: > I don't think this is enough. How do we know that the surface actually belongs > to the client that owns this gamepad instance? I think this needs to be > "Surface* focus_ = nullptr;" and the same logic as keyboard impl. I can do the CanAcceptKeyboardEventsForSurface equivalent here, but we can still just store a boolean for focus or not focus after checking if the delegate accepts events on that surface.
https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:70: base::AutoLock _l(lock_); On 2016/06/30 03:23:43, denniskempin wrote: > On 2016/06/30 01:19:05, reveman wrote: > > nit: base::AutoLock lock(lock_); > > > > It's hard to follow what happens to events in flight when focus changes. Did > you > > consider just doing all this filtering on the origin thread and not have the > > polling thread aware of focus? I think that would be easier to understand and > > maybe there wouldn't even be a need for a mutex. > > I am not quite sure how that would look like. Events in flight will still be > transmitted with this solution. As soon as the next poll comes around the > polling thread will know of the lost focus and stop. Actually I take that back. I thought a bit about it and made some changes (will upload tomorrow after I compiled and tested it). The new version will use the polling thread only to check if gamepad data updated and then post a task to the origin thread with the new data. The origin thread will then determine changes and call the delegate directly for all changes. That gets rid of the need for a weak_ptr. It also gets rid of the mutex since the polling thread has it's own separate data. It will also make for a very easy refactoring to extract the polling thread for multi-seat support.
We actually do still need a weak pointer factory to post tasks from the polling thread to the origin thread. The polling thread will be shutdown before Gamepad is destroyed, but there might still be a task on the origin task runner left after it was destroyed. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:32: constexpr unsigned kPollingTimeDelta = 16; On 2016/06/30 03:23:43, denniskempin wrote: > On 2016/06/30 01:19:05, reveman wrote: > > nit: s/TimeDelta/Interval/? Chromium usually adds Ms or Milliseconds suffix to > > make the unit clear > > > > This wakeup interval worries me and the effect it might have on battery usage. > > Adding 60 wakeups per second on ChromeOS seems really bad. How do we avoid > this > > when the gamepad is not used? Will the client just not instantiate a gamepad > in > > that case? > > Unfortunately we cannot do anything about this, with the current gamepad api we > have to poll to even find out when devices are connected. > > The same thing is already happening in the gamepad service. Their polling thread > is always on, polling at 60Hz as well. > Unfortunately even if we use the GamepadService later, we still need to poll but > we could restrict the polling to when gamepads are active. > > I am considering customizing the code more deeply for our purposes and > reimplement the gamepad data fetcher in a blocking mode that will wait for > devices and block on io, so we can get rid of the polling. > > This cannot be done easily in the gamepad api since some platforms have to use > polling for gamepads. > > Unfortunately for now.. this is the best we can do. Done on the nit. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:46: exo_focused_(false), On 2016/06/30 01:19:05, reveman wrote: > nit: all exo code is now providing default init of members where declared Done. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:70: base::AutoLock _l(lock_); On 2016/06/30 04:56:46, denniskempin wrote: > On 2016/06/30 03:23:43, denniskempin wrote: > > On 2016/06/30 01:19:05, reveman wrote: > > > nit: base::AutoLock lock(lock_); > > > > > > It's hard to follow what happens to events in flight when focus changes. Did > > you > > > consider just doing all this filtering on the origin thread and not have the > > > polling thread aware of focus? I think that would be easier to understand > and > > > maybe there wouldn't even be a need for a mutex. > > > > I am not quite sure how that would look like. Events in flight will still be > > transmitted with this solution. As soon as the next poll comes around the > > polling thread will know of the lost focus and stop. > > Actually I take that back. I thought a bit about it and made some changes (will > upload tomorrow after I compiled and tested it). > > The new version will use the polling thread only to check if gamepad data > updated and then post a task to the origin thread with the new data. The origin > thread will then determine changes and call the delegate directly for all > changes. > > That gets rid of the need for a weak_ptr. It also gets rid of the mutex since > the polling thread has it's own separate data. > > It will also make for a very easy refactoring to extract the polling thread for > multi-seat support. Done. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.cc:138: delegate_->GetWeakPtr(), new_pad.connected)); On 2016/06/30 01:19:05, reveman wrote: > GetWeakPtr() can't be called and the weak ptr returned can't be validated on a > different thread than the weak ptr factory was created on. no longer needed https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.h:69: bool exo_focused_; On 2016/06/30 03:23:43, denniskempin wrote: > On 2016/06/30 01:19:05, reveman wrote: > > I don't think this is enough. How do we know that the surface actually belongs > > to the client that owns this gamepad instance? I think this needs to be > > "Surface* focus_ = nullptr;" and the same logic as keyboard impl. > > I can do the CanAcceptKeyboardEventsForSurface equivalent here, but we can still > just store a boolean for focus or not focus after checking if the delegate > accepts events on that surface. Done. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad.h:77: base::ThreadChecker thread_checker_; On 2016/06/30 01:19:05, reveman wrote: > I think a thread checker is a good idea but I'm not seeing it used. Did you > forget to update the cc file? I used to. With the latest update I changed it to have one thread checker for the origin and another for the polling thread. https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... File components/exo/gamepad_delegate.h (right): https://codereview.chromium.org/2076013002/diff/200001/components/exo/gamepad... components/exo/gamepad_delegate.h:36: virtual base::WeakPtr<GamepadDelegate> GetWeakPtr() = 0; On 2016/06/30 01:19:05, reveman wrote: > Please handle this in the Gamepad implementation instead. The delegate is > supposed to be as simple as possible. Done. Also no longer needed.
https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.cc:32: constexpr unsigned kPollingTimeIntervalMs = 16; I'm still not sure we can land this unless we can limit this to when an app is running that might need it somehow. Constantly polling at this interval on all ChromeOS devices is not OK I think. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.cc:85: exo_focused_ = target && delegate_->CanAcceptGamepadEventsForSurface(target); What if the target is destroyed? Don't we need to keep track of target as we do for other input devices? https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.h:62: // True if an exosphere window is currently focused. s/an exosphere window/a client surface/ https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.h:63: bool exo_focused_{false}; nit: remove exo_ prefix https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.h:87: base::WeakPtr<Gamepad> weak_this_; Chromium code will typically keep a callbacks that reference a weak ptr instead. Can you do that in this case too? https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:35: class GamepadTest : public test::ExoTestBase { Consider avoiding this test fixture to be consistent with other input device unit tests. The down side is of course a lot of copy and paste but that's something we might want to address in a clean consistent way across all exo unit tests. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:39: void FocusNonExo() { nit: FocusNonClientSurface. However, I think it's even better to remove this function and move the code into the test with a comment. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:45: void FocusExo() { ditto https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:71: mock_data_fetcher_ = new device::MockGamepadDataFetcher(initial_data); nit: can you refactor the Gamepad code so it doesn't look like we're leaking memory here? ie. have the Gamepad ctor take a raw pointer and keep the ownership here? https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:86: // to be processed. It's strongly discouraged to have tests that depend timers firing. Can the polling thread be instrumented in such a way that we better control it for testing purposes. E.g. have the polling thread implemented outside the gamepad code and just have Gamepad class operate on a task runner? This would allow us to use a TestSimpleTaskRunner here which is the recommended way of testing multi-threaded code like this. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:106: TEST_F(GamepadTest, ConnectDisconnect) { Can you name the tests based on the delegate function they are testing? That's the convention used for other input device classes. In this case I think it would be s/ConnectDisconnect/OnStateChange/ https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:129: TEST_F(GamepadTest, AxisMove) { s/AxisMove/OnAxis/? https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:152: TEST_F(GamepadTest, FocusHandling) { s/FocusHandling/CanAcceptGamepadEventsForSurface/ ? https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:189: } // namespace Can you add a simple test for OnButton too? https://codereview.chromium.org/2076013002/diff/220001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/wayland... components/exo/wayland/server.cc:2780: base::WeakPtr<GamepadDelegate> GetWeakPtr() { no longer used https://codereview.chromium.org/2076013002/diff/220001/components/exo/wayland... components/exo/wayland/server.cc:2793: base::WeakPtrFactory<GamepadDelegate> weak_factory_; unused
https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.cc:32: constexpr unsigned kPollingTimeIntervalMs = 16; On 2016/06/30 18:06:11, reveman wrote: > I'm still not sure we can land this unless we can limit this to when an app is > running that might need it somehow. Constantly polling at this interval on all > ChromeOS devices is not OK I think. I've done some experimenting. What we can do is keep the thread idle. Only when an exo client window is focused, it will initialize the gamepad data fetcher, open the devices and start polling. One disadvantage is that we cannot properly keep the state consistent across focus switches (i.e. if a button is down while switching, the app won't know). But for gamepads that's a non-issue I would say. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.cc:85: exo_focused_ = target && delegate_->CanAcceptGamepadEventsForSurface(target); On 2016/06/30 18:06:11, reveman wrote: > What if the target is destroyed? Don't we need to keep track of target as we do > for other input devices? Shouldn't we get an OnWindowFocused event in that case? On other devices you need to know since you are actively using the target. We are not passing the target surface into the delegate. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.h:62: // True if an exosphere window is currently focused. On 2016/06/30 18:06:11, reveman wrote: > s/an exosphere window/a client surface/ Done. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.h:63: bool exo_focused_{false}; On 2016/06/30 18:06:11, reveman wrote: > nit: remove exo_ prefix Done. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.h:87: base::WeakPtr<Gamepad> weak_this_; On 2016/06/30 18:06:11, reveman wrote: > Chromium code will typically keep a callbacks that reference a weak ptr instead. > Can you do that in this case too? Done. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:35: class GamepadTest : public test::ExoTestBase { On 2016/06/30 18:06:12, reveman wrote: > Consider avoiding this test fixture to be consistent with other input device > unit tests. The down side is of course a lot of copy and paste but that's > something we might want to address in a clean consistent way across all exo unit > tests. I am not quite sure I understand your reasoning to do so. What is the benefit of copy and pasting the test fixture methods into the test cases below other than consistency? Even when this is being addressed later, it will ease the refactoring since for this test case there is only one place to adjust the common code. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:39: void FocusNonExo() { On 2016/06/30 18:06:12, reveman wrote: > nit: FocusNonClientSurface. However, I think it's even better to remove this > function and move the code into the test with a comment. no longer needed. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:45: void FocusExo() { On 2016/06/30 18:06:12, reveman wrote: > ditto no longer needed. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:71: mock_data_fetcher_ = new device::MockGamepadDataFetcher(initial_data); On 2016/06/30 18:06:11, reveman wrote: > nit: can you refactor the Gamepad code so it doesn't look like we're leaking > memory here? ie. have the Gamepad ctor take a raw pointer and keep the ownership > here? Done. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:86: // to be processed. On 2016/06/30 18:06:11, reveman wrote: > It's strongly discouraged to have tests that depend timers firing. Can the > polling thread be instrumented in such a way that we better control it for > testing purposes. E.g. have the polling thread implemented outside the gamepad > code and just have Gamepad class operate on a task runner? This would allow us > to use a TestSimpleTaskRunner here which is the recommended way of testing > multi-threaded code like this. This is not based on timers, but based on the polling. The mock data fetcher receives a signal when polling happened, so we can wait for that to happen and do not rely on timing. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:106: TEST_F(GamepadTest, ConnectDisconnect) { On 2016/06/30 18:06:12, reveman wrote: > Can you name the tests based on the delegate function they are testing? That's > the convention used for other input device classes. In this case I think it > would be s/ConnectDisconnect/OnStateChange/ Done. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:129: TEST_F(GamepadTest, AxisMove) { On 2016/06/30 18:06:12, reveman wrote: > s/AxisMove/OnAxis/? Done. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:152: TEST_F(GamepadTest, FocusHandling) { On 2016/06/30 18:06:12, reveman wrote: > s/FocusHandling/CanAcceptGamepadEventsForSurface/ ? I had to remove the focus handling test since I am not sure how to test this properly without any race conditions. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:189: } // namespace On 2016/06/30 18:06:12, reveman wrote: > Can you add a simple test for OnButton too? Done. https://codereview.chromium.org/2076013002/diff/220001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/wayland... components/exo/wayland/server.cc:2780: base::WeakPtr<GamepadDelegate> GetWeakPtr() { On 2016/06/30 18:06:12, reveman wrote: > no longer used Done. https://codereview.chromium.org/2076013002/diff/220001/components/exo/wayland... components/exo/wayland/server.cc:2793: base::WeakPtrFactory<GamepadDelegate> weak_factory_; On 2016/06/30 18:06:12, reveman wrote: > unused Done.
https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:86: // to be processed. On 2016/07/01 02:34:36, denniskempin wrote: > On 2016/06/30 18:06:11, reveman wrote: > > It's strongly discouraged to have tests that depend timers firing. Can the > > polling thread be instrumented in such a way that we better control it for > > testing purposes. E.g. have the polling thread implemented outside the gamepad > > code and just have Gamepad class operate on a task runner? This would allow us > > to use a TestSimpleTaskRunner here which is the recommended way of testing > > multi-threaded code like this. > > This is not based on timers, but based on the polling. The mock data fetcher > receives a signal when polling happened, so we can wait for that to happen and > do not rely on timing. I've been looking into just providing a TaskRunner to the class. This would add a large amount of complexity to the Gamepad class. Right now the polling thread is owned by the Gamepad class and forced to shut down before Gamepad is destroyed. If we move the thread out, the thread will outlive the gamepad class which opens up the possibility that Gamepad is destroyed while the polling thread is executing. Nothing that can't be solved.. but it adds quite complex synchronization logic that I would like to prevent as it might open up possibilities for missing weird edge cases. Right now the relationship between the origin and polling thread is very clear and easy to synchronize.
https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.cc:32: constexpr unsigned kPollingTimeIntervalMs = 16; On 2016/07/01 at 02:34:35, denniskempin wrote: > On 2016/06/30 18:06:11, reveman wrote: > > I'm still not sure we can land this unless we can limit this to when an app is > > running that might need it somehow. Constantly polling at this interval on all > > ChromeOS devices is not OK I think. > > I've done some experimenting. > > What we can do is keep the thread idle. Only when an exo client window is focused, it will initialize the gamepad data fetcher, open the devices and start polling. > > One disadvantage is that we cannot properly keep the state consistent across focus switches (i.e. if a button is down while switching, the app won't know). But for gamepads that's a non-issue I would say. Thanks for looking into this. This sgtm. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad.cc:85: exo_focused_ = target && delegate_->CanAcceptGamepadEventsForSurface(target); On 2016/07/01 at 02:34:35, denniskempin wrote: > On 2016/06/30 18:06:11, reveman wrote: > > What if the target is destroyed? Don't we need to keep track of target as we do > > for other input devices? > > Shouldn't we get an OnWindowFocused event in that case? On other devices you need to know since you are actively using the target. We are not passing the target surface into the delegate. Probably fine for now but it would be good in the future if the we add enter/leave events to guarantee that events are not delivered after surface is destroyed. No need to change this for now. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:35: class GamepadTest : public test::ExoTestBase { On 2016/07/01 at 02:34:36, denniskempin wrote: > On 2016/06/30 18:06:12, reveman wrote: > > Consider avoiding this test fixture to be consistent with other input device > > unit tests. The down side is of course a lot of copy and paste but that's > > something we might want to address in a clean consistent way across all exo unit > > tests. > > I am not quite sure I understand your reasoning to do so. What is the benefit of copy and pasting the test fixture methods into the test cases below other than consistency? > Even when this is being addressed later, it will ease the refactoring since for this test case there is only one place to adjust the common code. Consistency would be the benefit and imo a sufficient one. When someone does the work to make all unit test code less copy-n-paste-y then this new gamepad code is not going to require any additional work. I'm not going to push back on this as it's my fault that the current amount of code-n-paste is needed for these tests. https://codereview.chromium.org/2076013002/diff/220001/components/exo/gamepad... components/exo/gamepad_unittest.cc:86: // to be processed. On 2016/07/01 at 17:16:50, denniskempin wrote: > On 2016/07/01 02:34:36, denniskempin wrote: > > On 2016/06/30 18:06:11, reveman wrote: > > > It's strongly discouraged to have tests that depend timers firing. Can the > > > polling thread be instrumented in such a way that we better control it for > > > testing purposes. E.g. have the polling thread implemented outside the gamepad > > > code and just have Gamepad class operate on a task runner? This would allow us > > > to use a TestSimpleTaskRunner here which is the recommended way of testing > > > multi-threaded code like this. > > > > This is not based on timers, but based on the polling. The mock data fetcher > > receives a signal when polling happened, so we can wait for that to happen and > > do not rely on timing. Does changing the polling frequency to 16 seconds instead of 16ms affect how long it takes to run these tests? That's what I meant by rely on timing. > > I've been looking into just providing a TaskRunner to the class. This would add a large amount of complexity to the Gamepad class. Right now the polling thread is owned by the Gamepad class and forced to shut down before Gamepad is destroyed. > If we move the thread out, the thread will outlive the gamepad class which opens up the possibility that Gamepad is destroyed while the polling thread is executing. > Nothing that can't be solved.. but it adds quite complex synchronization logic that I would like to prevent as it might open up possibilities for missing weird edge cases. > Right now the relationship between the origin and polling thread is very clear and easy to synchronize. Is it just not a matter of using a weak ptr as we're already doing for messages to the origin thread? Anyhow, if this is indeed complicated then avoiding it in this patch is fine as long as we're not adding tests that take 16ms to run. https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.cc:130: if (external_fetcher_) { hm, when I suggested a refactor to avoid confusing ownership in tests I was expecting a refactor that wouldn't result it different logic for testing and real usage. I'd like to avoid any unnecessary complexity because of tests and make sure we're not entering a different code path during tests compared to real usage. Is there a better way to instrument this class for tests? Maybe add a virtual GetDataFetcher() with a comment that it is virtual for testing purposes and then subclass Gamepad as TestGamepad for unit testing. https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:39: bool is_paused() { return is_paused_; } where is this used? please add const modifier if we need this https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:42: // Pauses polling thread. Has no effect is the thread is already paused. s/is/if/ https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:44: void PausePolling(); nit: Common style in chromium is to add a suffix "OnXThread" to the function name to make it clear what thread it should run on. Can you add a "OnPollingThread" suffix here? https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:46: // Resume polling. Hs no effect if the thread is already active. s/Hs/Has/ https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:48: void ResumePolling(); nit: add OnPollingThread suffix? btw, would having one function EnablePollingOnPollingThread(bool enabled) reduce the amount of code instead of having PausePolling+ResumePolling? https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:52: void ScheduleOnPoll(); nit: SchedulePollOnPollingThread https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:56: void OnPoll(); nit: PollOnPollingThread https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:71: // ThreadChecker for the origin thread nit: s/origin thread/origin thread./ https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad_unittest.cc:73: for (int i = 0; i < 1; ++i) { this loop doesn't seem to be doing anything as i < 1 will just be true once. the comment above also seem to indicate something else. what is this supposed to do? https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad_unittest.cc:82: // Pointer owned by the gamepad instance. this doesn't seem correct anymore
https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.cc:130: if (external_fetcher_) { On 2016/07/07 22:14:02, reveman wrote: > hm, when I suggested a refactor to avoid confusing ownership in tests I was > expecting a refactor that wouldn't result it different logic for testing and > real usage. I'd like to avoid any unnecessary complexity because of tests and > make sure we're not entering a different code path during tests compared to real > usage. > > Is there a better way to instrument this class for tests? Maybe add a virtual > GetDataFetcher() with a comment that it is virtual for testing purposes and then > subclass Gamepad as TestGamepad for unit testing. Done. https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:39: bool is_paused() { return is_paused_; } On 2016/07/07 22:14:03, reveman wrote: > where is this used? please add const modifier if we need this Done. https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:42: // Pauses polling thread. Has no effect is the thread is already paused. On 2016/07/07 22:14:02, reveman wrote: > s/is/if/ Done. https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:44: void PausePolling(); On 2016/07/07 22:14:03, reveman wrote: > nit: Common style in chromium is to add a suffix "OnXThread" to the function > name to make it clear what thread it should run on. Can you add a > "OnPollingThread" suffix here? Done. https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:46: // Resume polling. Hs no effect if the thread is already active. On 2016/07/07 22:14:02, reveman wrote: > s/Hs/Has/ Done. https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:48: void ResumePolling(); On 2016/07/07 22:14:03, reveman wrote: > nit: add OnPollingThread suffix? > > btw, would having one function EnablePollingOnPollingThread(bool enabled) reduce > the amount of code instead of having PausePolling+ResumePolling? Done. https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:52: void ScheduleOnPoll(); On 2016/07/07 22:14:02, reveman wrote: > nit: SchedulePollOnPollingThread Done. https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:56: void OnPoll(); On 2016/07/07 22:14:03, reveman wrote: > nit: PollOnPollingThread Done. https://codereview.chromium.org/2076013002/diff/240001/components/exo/gamepad... components/exo/gamepad.h:71: // ThreadChecker for the origin thread On 2016/07/07 22:14:03, reveman wrote: > nit: s/origin thread/origin thread./ Done.
mostly nits https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:42: class Gamepad::PollingThreadResources nit: this class contains more than some resources. it has all the logic for fetching gamepad changes afaict. can we rename this to Gamepad::ThreadSafeGamepadChangeFetcher to make this more clear? https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:43: : public base::RefCounted<Gamepad::PollingThreadResources> { Does this need to be base::ThreadSafeRefCounted? https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:46: PostGamepadChangesCallback; nit: using HandleGamepadChangesCallback = .. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:51: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) nit: different parts of chrome does this differently but I prefer to avoid ref-counting chrun using "base::SingleThreadTaskRunner* task_runner" instead of "const scoped_refptr<X>&" https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:69: virtual ~PollingThreadResources() {} nit: blank line before this https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:75: if (enabled == is_enabled_) nit: is this check needed? https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:86: if (!is_enabled_) { is this needed? if it is, then shouldn't the conditional on l.79 be removed? or maybe better, do fetcher_.reset() from there? and in that case also create the fetcher from there. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:111: nit: move this blank line above l.110 instead https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:133: GamepadDataFetcherFactory fetcher_factory_; nit: create_fetcher_callback_ https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:148: bool has_poll_scheduled_{false}; nit: "has_poll_scheduled_ = false" is preferred in exo and chromium code https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:151: bool is_enabled_{true}; nit: is_enabled_ = true https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:211: if (focused) { nit: polling_thread_resources_->EnablePolling(focused) instead of this if statement. maybe even remove the temporary |focused| variable. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:22: typedef base::Callback<std::unique_ptr<device::GamepadDataFetcher>()> nit: using CreateGamepadDataFetcherCallback = base::Callback<... "using" is preferred and when it's just a single callback I prefer the "Create*Callback" name instead of *Factory https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:33: scoped_refptr<base::SingleThreadTaskRunner> polling_task_runner); nit: this will result in unnecessary ref-count churn. please use "base::SingleThreadTaskRunner* polling_task_runner" instead. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:37: scoped_refptr<base::SingleThreadTaskRunner> polling_task_runner, nit: "base::SingleThreadTaskRunner* polling_task_runner" instead https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:49: scoped_refptr<PollingThreadResources> polling_thread_resources_; nit: member variables below types and functions https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:53: void PostGamepadChanges(const blink::WebGamepad new_pad); nit: "Handle" or "Process" instead of "Post" as post is typically used for asynchronous processing of tasks and that's not the case here. Please update comment too. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:58: GamepadPlatformDataFetcherFactory(); nit: either move this into anonymouse namespace in .cc file or remove the ctor that doesn't take a create fetcher callback https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad_unittest.cc:48: void InitializeGamepad(MockGamepadDelegate* delegate) { Would be nice to have one test that verifies that multiple gamepad instances work correctly but this seems to limit testing to one instance. I'm OK with having that not be tested for now but something to think about for the future. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad_unittest.cc:62: // Process tasks until polling is shut down. are the following lines really needed? https://codereview.chromium.org/2076013002/diff/260001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2076013002/diff/260001/components/exo/wayland... components/exo/wayland/server.cc:2841: base::Thread* gamepad_thread = GetUserDataAs<base::Thread>(resource); nit: gaming_input_thread https://codereview.chromium.org/2076013002/diff/260001/components/exo/wayland... components/exo/wayland/server.cc:2859: std::unique_ptr<base::Thread> gamepad_thread( nit: gaming_input_thread https://codereview.chromium.org/2076013002/diff/260001/components/exo/wayland... components/exo/wayland/server.cc:2860: new base::Thread("Exo gamepad polling thread.")); nit: "Exo gaming input polling thread."
all done https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:42: class Gamepad::PollingThreadResources On 2016/07/11 10:50:44, reveman wrote: > nit: this class contains more than some resources. it has all the logic for > fetching gamepad changes afaict. can we rename this to > Gamepad::ThreadSafeGamepadChangeFetcher to make this more clear? Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:43: : public base::RefCounted<Gamepad::PollingThreadResources> { On 2016/07/11 10:50:44, reveman wrote: > Does this need to be base::ThreadSafeRefCounted? Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:46: PostGamepadChangesCallback; On 2016/07/11 10:50:44, reveman wrote: > nit: using HandleGamepadChangesCallback = .. Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:51: const scoped_refptr<base::SingleThreadTaskRunner>& task_runner) On 2016/07/11 10:50:44, reveman wrote: > nit: different parts of chrome does this differently but I prefer to avoid > ref-counting chrun using "base::SingleThreadTaskRunner* task_runner" instead of > "const scoped_refptr<X>&" Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:69: virtual ~PollingThreadResources() {} On 2016/07/11 10:50:44, reveman wrote: > nit: blank line before this Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:75: if (enabled == is_enabled_) On 2016/07/11 10:50:44, reveman wrote: > nit: is this check needed? Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:86: if (!is_enabled_) { On 2016/07/11 10:50:44, reveman wrote: > is this needed? if it is, then shouldn't the conditional on l.79 be removed? or > maybe better, do fetcher_.reset() from there? and in that case also create the > fetcher from there. Done. I've moved the creation/destruction of the fetcher to the EnablePollingOnPollingThread method. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:111: On 2016/07/11 10:50:44, reveman wrote: > nit: move this blank line above l.110 instead Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:133: GamepadDataFetcherFactory fetcher_factory_; On 2016/07/11 10:50:44, reveman wrote: > nit: create_fetcher_callback_ Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:148: bool has_poll_scheduled_{false}; On 2016/07/11 10:50:44, reveman wrote: > nit: "has_poll_scheduled_ = false" is preferred in exo and chromium code Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:151: bool is_enabled_{true}; On 2016/07/11 10:50:44, reveman wrote: > nit: is_enabled_ = true Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.cc:211: if (focused) { On 2016/07/11 10:50:44, reveman wrote: > nit: polling_thread_resources_->EnablePolling(focused) instead of this if > statement. maybe even remove the temporary |focused| variable. Done. Yes.. well this was just dumb ;) https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:22: typedef base::Callback<std::unique_ptr<device::GamepadDataFetcher>()> On 2016/07/11 10:50:44, reveman wrote: > nit: using CreateGamepadDataFetcherCallback = base::Callback<... > > "using" is preferred and when it's just a single callback I prefer the > "Create*Callback" name instead of *Factory Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:33: scoped_refptr<base::SingleThreadTaskRunner> polling_task_runner); On 2016/07/11 10:50:44, reveman wrote: > nit: this will result in unnecessary ref-count churn. please use > "base::SingleThreadTaskRunner* polling_task_runner" instead. Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:37: scoped_refptr<base::SingleThreadTaskRunner> polling_task_runner, On 2016/07/11 10:50:44, reveman wrote: > nit: "base::SingleThreadTaskRunner* polling_task_runner" instead Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:49: scoped_refptr<PollingThreadResources> polling_thread_resources_; On 2016/07/11 10:50:44, reveman wrote: > nit: member variables below types and functions Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:53: void PostGamepadChanges(const blink::WebGamepad new_pad); On 2016/07/11 10:50:44, reveman wrote: > nit: "Handle" or "Process" instead of "Post" as post is typically used for > asynchronous processing of tasks and that's not the case here. Please update > comment too. Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad.h:58: GamepadPlatformDataFetcherFactory(); On 2016/07/11 10:50:44, reveman wrote: > nit: either move this into anonymouse namespace in .cc file or remove the ctor > that doesn't take a create fetcher callback Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad_unittest.cc:48: void InitializeGamepad(MockGamepadDelegate* delegate) { On 2016/07/11 10:50:44, reveman wrote: > Would be nice to have one test that verifies that multiple gamepad instances > work correctly but this seems to limit testing to one instance. I'm OK with > having that not be tested for now but something to think about for the future. I think that would be a good thing to do when we add support for multiple gamepads. https://codereview.chromium.org/2076013002/diff/260001/components/exo/gamepad... components/exo/gamepad_unittest.cc:62: // Process tasks until polling is shut down. On 2016/07/11 10:50:44, reveman wrote: > are the following lines really needed? Since polling is happening independently of the Gamepad instance and might continue if not properly shut down by the Gamepad destructor, I'd like to make sure this is happening so we are not messing up following tests. https://codereview.chromium.org/2076013002/diff/260001/components/exo/wayland... File components/exo/wayland/server.cc (right): https://codereview.chromium.org/2076013002/diff/260001/components/exo/wayland... components/exo/wayland/server.cc:2841: base::Thread* gamepad_thread = GetUserDataAs<base::Thread>(resource); On 2016/07/11 10:50:45, reveman wrote: > nit: gaming_input_thread Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/wayland... components/exo/wayland/server.cc:2859: std::unique_ptr<base::Thread> gamepad_thread( On 2016/07/11 10:50:45, reveman wrote: > nit: gaming_input_thread Done. https://codereview.chromium.org/2076013002/diff/260001/components/exo/wayland... components/exo/wayland/server.cc:2860: new base::Thread("Exo gamepad polling thread.")); On 2016/07/11 10:50:45, reveman wrote: > nit: "Exo gaming input polling thread." Done.
lgtm with some more nits https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.cc:116: has_poll_scheduled_ = false; nit: move this before if statement so l.133 can be removed https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.cc:138: ProcessGamepadChangesCallback post_gamepad_changes_; nit: process_gamepad_changes_callback_ https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.cc:141: CreateGamepadDataFetcherCallback create_fetcher_callback_; nit: const CreateGamepadDataFetcherCallback& https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.cc:193: // can safely have it shut down after Gamepad has been destroyed. nit: fix formatting of this comment https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.h:25: // This class represents one or more gamepads, it implements a background thread nit: s/implements/uses/ https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad_unittest.cc:71: // Run origin task runner to invoke delegate nit: "invoke delegate." https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad_unittest.cc:182: // Create surface and focus on it nit: "focus on it." Maybe "and move focus to it."? https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad_unittest.cc:200: polling_task_runner_->RunPendingTasks(); why twice? looks like a typo. add a comment to explain why or remove.. https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad_unittest.cc:203: // Remove focus from exo window. nit: s/exo window/window/ https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad_unittest.cc:210: polling_task_runner_->RunPendingTasks(); same here. why twice?
device/gamepad/ LGTM
all done. I think we are good to go here! https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.cc:116: has_poll_scheduled_ = false; On 2016/07/11 18:45:01, reveman wrote: > nit: move this before if statement so l.133 can be removed Done. https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.cc:138: ProcessGamepadChangesCallback post_gamepad_changes_; On 2016/07/11 18:45:01, reveman wrote: > nit: process_gamepad_changes_callback_ Done. https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.cc:141: CreateGamepadDataFetcherCallback create_fetcher_callback_; On 2016/07/11 18:45:01, reveman wrote: > nit: const CreateGamepadDataFetcherCallback& Done. https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.cc:193: // can safely have it shut down after Gamepad has been destroyed. On 2016/07/11 18:45:01, reveman wrote: > nit: fix formatting of this comment Done. https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad.h File components/exo/gamepad.h (right): https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.h:25: // This class represents one or more gamepads, it implements a background thread On 2016/07/11 18:45:01, reveman wrote: > nit: s/implements/uses/ Done. https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... File components/exo/gamepad_unittest.cc (right): https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad_unittest.cc:71: // Run origin task runner to invoke delegate On 2016/07/11 18:45:01, reveman wrote: > nit: "invoke delegate." Done. https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad_unittest.cc:182: // Create surface and focus on it On 2016/07/11 18:45:01, reveman wrote: > nit: "focus on it." Maybe "and move focus to it."? Done. https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad_unittest.cc:200: polling_task_runner_->RunPendingTasks(); On 2016/07/11 18:45:01, reveman wrote: > why twice? looks like a typo. add a comment to explain why or remove.. added comment and turned into a loop. I just want to check a few times to make sure we are actually polling indefinitely. https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad_unittest.cc:203: // Remove focus from exo window. On 2016/07/11 18:45:01, reveman wrote: > nit: s/exo window/window/ Done. https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad_unittest.cc:210: polling_task_runner_->RunPendingTasks(); On 2016/07/11 18:45:01, reveman wrote: > same here. why twice? added comment. First round executes EnablePollingOnPollingThread, only then polling will stop in the next round.
Ran a couple of last tests.. played some GTA. Let's submit this! ;) https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad.cc File components/exo/gamepad.cc (right): https://codereview.chromium.org/2076013002/diff/280001/components/exo/gamepad... components/exo/gamepad.cc:141: CreateGamepadDataFetcherCallback create_fetcher_callback_; On 2016/07/11 18:45:01, reveman wrote: > nit: const CreateGamepadDataFetcherCallback& actually I have to change this back. the callback docstring recommends storing callbacks by copy, especially since we are passing in temporary instances.
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2076013002/#ps320001 (title: "changed back callback to store by copy instead of reference.")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2093803002 Patch 120001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2076013002/#ps360001 (title: "rebase")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2093803002 Patch 160001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by denniskempin@google.com
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2093803002 Patch 160001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
On 2016/07/13 15:53:03, commit-bot: I haz the power wrote: > This CL has an open dependency (Issue 2093803002 Patch 160001). Please resolve > the dependency and try again. > If you are sure that there is no real dependency, please use one of the options > listed in https://goo.gl/9Es4OR to land the CL. I guess the CQ won't apply both patches, so I'll have to wait for the protocol change to merge first.
The CQ bit was checked by denniskempin@google.com
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: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2076013002/#ps380001 (title: "added dep to //base/test:test_support")
The CQ bit was checked by denniskempin@google.com
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-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2076013002/#ps400001 (title: "added dep.. this time to the right target hopefully")
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-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2076013002/#ps420001 (title: "... looks like both targets want this dep.")
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-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I found out about gn check.. this should have all the deps now.
The CQ bit was checked by denniskempin@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2076013002/#ps440001 (title: "more missing deps. I wonder why these do not come up during pre-submit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== This CL implements the gamepads wayland API. Since js device nodes on linux can be opened multiple times, we are not required to go through the content::GamepadService, but can directly use our own device::GamepadPlatformDataFetcher in our own polling thread. The polling thread will compare the newly fetched data to the previous data and send any changes as events to the wayland client. BUG=620977 ========== to ========== This CL implements the gamepads wayland API. Since js device nodes on linux can be opened multiple times, we are not required to go through the content::GamepadService, but can directly use our own device::GamepadPlatformDataFetcher in our own polling thread. The polling thread will compare the newly fetched data to the previous data and send any changes as events to the wayland client. BUG=620977 ==========
Message was sent while issue was closed.
Committed patchset #23 (id:440001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== This CL implements the gamepads wayland API. Since js device nodes on linux can be opened multiple times, we are not required to go through the content::GamepadService, but can directly use our own device::GamepadPlatformDataFetcher in our own polling thread. The polling thread will compare the newly fetched data to the previous data and send any changes as events to the wayland client. BUG=620977 ========== to ========== This CL implements the gamepads wayland API. Since js device nodes on linux can be opened multiple times, we are not required to go through the content::GamepadService, but can directly use our own device::GamepadPlatformDataFetcher in our own polling thread. The polling thread will compare the newly fetched data to the previous data and send any changes as events to the wayland client. BUG=620977 Committed: https://crrev.com/68de182439eed91857dafbb4348f1e4857b18c69 Cr-Commit-Position: refs/heads/master@{#405356} ==========
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/68de182439eed91857dafbb4348f1e4857b18c69 Cr-Commit-Position: refs/heads/master@{#405356} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
