Clean up UserInputMonitor implementations |
||||||
Issue descriptionIssues with the UserInputMonitorLinux implementation: 1. It is an owned object on which certain methods may be called from multiple threads, making thread-safety the responsibility of the caller, and hard to reason about. 2. The multi-threading requirement necessitates use of WeakPtrs when posting registrations to the UIMLCore, on the I/O thread (since registrations from threads other than the UIML caller thread will not be serialized with teardown). 3. UIML destruction uses DeleteSoon() on the Core, but falls-back to in-line delete if that returns false, which: - Violates the same-thread/sequence requirement of the WeakPtr[Factory]. - Risks leaking the UIMLCore in the race-condition case of DeleteSoon() returning true, but the I/O thread then quitting before seeing the delete task. 4. As per issue 279486, there should be a suite of unit-tests for this code. 5. UserInputMonitor is a base-class from which implementations derive, rather than separating the two into the common core logic and a Delegate. 6. It looks like UIM is used solely to get a key-press count, to suppress echos; the mouse monitoring functionality does not appear to be used in Chrome.
,
Dec 14 2016
Mouse monitoring seems unimplemented. There is a notify that calls OnMouseMoved, but nothing (outside of test) implements this function.
,
Dec 14 2016
,
Dec 14 2016
Re #2: OnMouseMoved() is called e.g. in the Linux implementation: https://cs.chromium.org/chromium/src/media/base/user_input_monitor_linux.cc?q=UserInputMonitorLinuxCore&sq=package:chromium&l=284 My point in #6 is just that I don't see anything that ever calls UserInputMonitorLinux::AddMouseListener in the first place.
,
Dec 14 2016
Once that's done we should in principle be able to simplify the API contract to something like:
class KeyPressMonitor {
virtual size_t GetKeyPressCount() = 0;
};
class KeyPressMonitorFactory {
virtual unique_ptr<KeyPressMonitor> Create() = 0;
};
We could then create the factory during browser/test startup, and create monitor instances on a per-AudioStream basis, so that each stream can cleanly own a monitor instance (though the implementation can internally share a single keypress-monitoring resource).
,
Jan 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fdd566ad2d6598b104ea75b4ae2395a96083ca20 commit fdd566ad2d6598b104ea75b4ae2395a96083ca20 Author: lethalantidote <lethalantidote@chromium.org> Date: Sat Jan 21 04:20:14 2017 Removes mouse listeners from UserInputMonitor. MouseEventListener was found to be unused, and thus removed. BUG=673494 Review-Url: https://codereview.chromium.org/2577573002 Cr-Commit-Position: refs/heads/master@{#445270} [modify] https://crrev.com/fdd566ad2d6598b104ea75b4ae2395a96083ca20/media/base/BUILD.gn [modify] https://crrev.com/fdd566ad2d6598b104ea75b4ae2395a96083ca20/media/base/keyboard_event_counter.cc [modify] https://crrev.com/fdd566ad2d6598b104ea75b4ae2395a96083ca20/media/base/keyboard_event_counter.h [add] https://crrev.com/fdd566ad2d6598b104ea75b4ae2395a96083ca20/media/base/keyboard_event_counter_unittest.cc [modify] https://crrev.com/fdd566ad2d6598b104ea75b4ae2395a96083ca20/media/base/user_input_monitor.cc [modify] https://crrev.com/fdd566ad2d6598b104ea75b4ae2395a96083ca20/media/base/user_input_monitor.h [modify] https://crrev.com/fdd566ad2d6598b104ea75b4ae2395a96083ca20/media/base/user_input_monitor_linux.cc [modify] https://crrev.com/fdd566ad2d6598b104ea75b4ae2395a96083ca20/media/base/user_input_monitor_mac.cc [modify] https://crrev.com/fdd566ad2d6598b104ea75b4ae2395a96083ca20/media/base/user_input_monitor_unittest.cc [modify] https://crrev.com/fdd566ad2d6598b104ea75b4ae2395a96083ca20/media/base/user_input_monitor_win.cc
,
Mar 14 2017
Gonna take this off me for now. Focusing on product work.
,
Apr 6 2018
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 16 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/968fd020751671ca139878c6ffe6132b5f32b285 commit 968fd020751671ca139878c6ffe6132b5f32b285 Author: Marina Ciocea <marinaciocea@chromium.org> Date: Mon Apr 16 22:44:38 2018 Add user_input_monitor_unittest.cc back to BUILD. Looks like it was accidentally removed in https://crrev.com/2577573002. Bug: 673494 Change-Id: Ibb73e8ff0c8a1b3a222446b002eb5b7560e9fe31 Reviewed-on: https://chromium-review.googlesource.com/1012079 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Commit-Queue: Marina Ciocea <marinaciocea@chromium.org> Cr-Commit-Position: refs/heads/master@{#551155} [modify] https://crrev.com/968fd020751671ca139878c6ffe6132b5f32b285/media/base/BUILD.gn [modify] https://crrev.com/968fd020751671ca139878c6ffe6132b5f32b285/media/base/user_input_monitor_unittest.cc
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/968fd020751671ca139878c6ffe6132b5f32b285 commit 968fd020751671ca139878c6ffe6132b5f32b285 Author: Marina Ciocea <marinaciocea@chromium.org> Date: Mon Apr 16 22:44:38 2018 Add user_input_monitor_unittest.cc back to BUILD. Looks like it was accidentally removed in https://crrev.com/2577573002. Bug: 673494 Change-Id: Ibb73e8ff0c8a1b3a222446b002eb5b7560e9fe31 Reviewed-on: https://chromium-review.googlesource.com/1012079 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Commit-Queue: Marina Ciocea <marinaciocea@chromium.org> Cr-Commit-Position: refs/heads/master@{#551155} [modify] https://crrev.com/968fd020751671ca139878c6ffe6132b5f32b285/media/base/BUILD.gn [modify] https://crrev.com/968fd020751671ca139878c6ffe6132b5f32b285/media/base/user_input_monitor_unittest.cc
,
Apr 23 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by w...@chromium.org
, Dec 13 2016Status: Assigned (was: Untriaged)