Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577573002:
Removes mouse listeners from UserInputMonitor. (Closed)
DescriptionRemoves 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}
Committed: https://chromium.googlesource.com/chromium/src/+/fdd566ad2d6598b104ea75b4ae2395a96083ca20
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addresses Wez's #7 comments. #
Total comments: 23
Patch Set 3 : Addresses Wez's #13 comments. #Patch Set 4 : Updates win's Start/Stop Monitor. #
Total comments: 4
Patch Set 5 : Addresses Wez's #15 comments. #
Total comments: 4
Patch Set 6 : Addresses Wez's #17,18 comments. #
Total comments: 4
Patch Set 7 : Addresses Wez's #20 comments. #Patch Set 8 : Updates test. #Patch Set 9 : Udpate test inclusion. #Patch Set 10 : Fix typo. #Patch Set 11 : Lint. #
Total comments: 13
Patch Set 12 : Addresses comment #45/46's concern about inlining the constructor. #Patch Set 13 : Addresses Wez's #48 comment. #
Total comments: 4
Patch Set 14 : Addresses Wez's #50 comment. #Patch Set 15 : Updates test. #Patch Set 16 : Typos in _win. #Patch Set 17 : Merge branch 'real_master' into UserInputMonitor #Patch Set 18 : Syntax error. #Patch Set 19 : Removes reset function. #Patch Set 20 : Updates win to not use reset. #Patch Set 21 : Merge branch 'real_master' into UserInputMonitor #
Total comments: 1
Patch Set 22 : format. #Patch Set 23 : Fixes GetRawDevice. #
Total comments: 4
Patch Set 24 : Format. #Patch Set 25 : Fix win by removing std::move. #
Messages
Total messages: 130 (72 generated)
lethalantidote@chromium.org changed reviewers: + wez@chromium.org
PTAL
CL description nits: - user_input_monitor -> UserInputMonitor (i.e. refer to the class name rather than the file name form). - typo in MouseEventListner. The Windows and Mac implementations will also need the relevant bits stripping out. :)
Description was changed from ========== Removes mouse listeners from user_input_monitor. MouseEventListner was found to be unused, and thus removed. BUG=673494 ========== to ========== Removes mouse listeners from UserInputMonitor. MouseEventListener was found to be unused, and thus removed. BUG=673494 ==========
On 2016/12/14 01:24:45, Wez wrote: > CL description nits: > - user_input_monitor -> UserInputMonitor (i.e. refer to the class name rather > than the file name form). > - typo in MouseEventListner. > > The Windows and Mac implementations will also need the relevant bits stripping > out. :) CL description updated. Windows and Mac in different CL ok?
On 2016/12/14 23:48:34, CJ wrote: > On 2016/12/14 01:24:45, Wez wrote: > > CL description nits: > > - user_input_monitor -> UserInputMonitor (i.e. refer to the class name rather > > than the file name form). > > - typo in MouseEventListner. > > > > The Windows and Mac implementations will also need the relevant bits stripping > > out. :) > > CL description updated. > > Windows and Mac in different CL ok? No, I think you'll need to remove the Windows and Mac code at the same time, otherwise they build will fail because the Start/StopMouseMonitoring() methods are no longer there for them to override. If you prefer to land separate patches then I'd suggest having this patch keeping Start/StopMouseMonitoring(), but preceding them with a comment to explain they'll be removed, and give them no-op default impls (so that the Linux build will still compile).
https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... File media/base/user_input_monitor.cc (right): https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor.cc:19: UserInputMonitor::UserInputMonitor() : key_press_counter_references_(0) {} nit: You could move this initialization into the header if you line, in-line in the class. https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... File media/base/user_input_monitor.h (right): https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor.h:14: #include "base/observer_list_threadsafe.h" nit: This header include is no longer necessary. https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor.h:26: // Monitors and notifies about mouse movements and keyboard events. nit: Update this to remove mention of mouse. https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor_linux.cc:45: MOUSE_EVENT, nit: You can get rid of this now :) https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor_linux.cc:75: XRecordRange* x_record_range_[2]; nit: Don't need an arrange of XRecordRange, just a single one, now. https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor_linux.cc:170: if (type == MOUSE_EVENT) { This needs stripping out, as well - we now only want to listen for keyboard input. :) https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor_linux.cc:272: if (event->u.u.type != MotionNotify) { nit: Looks like you don't need this at all - just get rid of this if entirely - you should no longer be registering for motion information.
https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... File media/base/user_input_monitor.cc (right): https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor.cc:19: UserInputMonitor::UserInputMonitor() : key_press_counter_references_(0) {} On 2016/12/16 02:06:43, Wez wrote: > nit: You could move this initialization into the header if you line, in-line in > the class. Done. https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... File media/base/user_input_monitor.h (right): https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor.h:14: #include "base/observer_list_threadsafe.h" On 2016/12/16 02:06:43, Wez wrote: > nit: This header include is no longer necessary. Done. https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor.h:26: // Monitors and notifies about mouse movements and keyboard events. On 2016/12/16 02:06:43, Wez wrote: > nit: Update this to remove mention of mouse. Done. https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor_linux.cc:45: MOUSE_EVENT, On 2016/12/16 02:06:43, Wez wrote: > nit: You can get rid of this now :) Done. https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor_linux.cc:75: XRecordRange* x_record_range_[2]; On 2016/12/16 02:06:43, Wez wrote: > nit: Don't need an arrange of XRecordRange, just a single one, now. Done. https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor_linux.cc:170: if (type == MOUSE_EVENT) { On 2016/12/16 02:06:43, Wez wrote: > This needs stripping out, as well - we now only want to listen for keyboard > input. :) Done. https://codereview.chromium.org/2577573002/diff/1/media/base/user_input_monit... media/base/user_input_monitor_linux.cc:272: if (event->u.u.type != MotionNotify) { On 2016/12/16 02:06:43, Wez wrote: > nit: Looks like you don't need this at all - just get rid of this if entirely - > you should no longer be registering for motion information. Done.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor.h (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor.h:25: // added. nit: Remove the comment about listeners, since there is no listener interface any more. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_linux.cc:170: XRecordRange** record_range_to_use = &x_record_range_; nit: You can just use &x_record_range_ directly in the XRecordCreateContext call, I think. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_linux.cc:195: if (!x_record_range_) { This is the wrong way around, and by the time you reach here, x_record_range_ is definitely non-null (see line 155) - you just want to watch the record FD unconditionally. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_linux.cc:256: ui::EventType type; nit: We only need this (and the corresponding code in the Windows version) because we use ui::EventType as the |type| parameter to OnKeyboardEvent, whereas in practice OnKeyboardEvent only cares about whether the event is a press or a release - it would simplify both callers and the KeyboardEventCounter itself if we switched the API to take a bool |down| parameter instead. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor_unittest.cc (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_unittest.cc:25: TEST(UserInputMonitorTest, KeyPressCounter) { nit: Really strange that this test is bundled with UserInputMonitorTest, when it's actually a unit-test of the KeyboardEventCounter. Also not clear why it is tied to OS_LINUX || OS_WIN..? https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_win.cc:105: StopMonitor(MOUSE_EVENT_MASK); No need for this any more. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_win.cc:116: if (events_monitored_ & type) nit: You can replace events_monitored_ with a bool and kill the EventBitMask |type|. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_win.cc:134: GetRawInputDevices(type, RIDEV_INPUTSINK)); You're removing mouse event support so you can replace GetRawInputDevices and hard-wire the keyboard RawInputDevice info in here. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_win.cc:198: if (input->header.dwType == RIM_TYPEMOUSE && input->header.hDevice != NULL) { This entire case can go away since we never listen for mouse input any more.
https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor.h (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor.h:25: // added. On 2017/01/03 23:59:40, Wez wrote: > nit: Remove the comment about listeners, since there is no listener interface > any more. Done. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_linux.cc:170: XRecordRange** record_range_to_use = &x_record_range_; On 2017/01/03 23:59:40, Wez wrote: > nit: You can just use &x_record_range_ directly in the XRecordCreateContext > call, I think. Done. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_linux.cc:195: if (!x_record_range_) { On 2017/01/03 23:59:40, Wez wrote: > This is the wrong way around, and by the time you reach here, x_record_range_ is > definitely non-null (see line 155) - you just want to watch the record FD > unconditionally. Done. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_linux.cc:256: ui::EventType type; On 2017/01/03 23:59:40, Wez wrote: > nit: We only need this (and the corresponding code in the Windows version) > because we use ui::EventType as the |type| parameter to OnKeyboardEvent, whereas > in practice OnKeyboardEvent only cares about whether the event is a press or a > release - it would simplify both callers and the KeyboardEventCounter itself if > we switched the API to take a bool |down| parameter instead. Done. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor_unittest.cc (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_unittest.cc:25: TEST(UserInputMonitorTest, KeyPressCounter) { On 2017/01/03 23:59:40, Wez wrote: > nit: Really strange that this test is bundled with UserInputMonitorTest, when > it's actually a unit-test of the KeyboardEventCounter. Also not clear why it is > tied to OS_LINUX || OS_WIN..? Done. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_win.cc:105: StopMonitor(MOUSE_EVENT_MASK); On 2017/01/03 23:59:40, Wez wrote: > No need for this any more. Done. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_win.cc:116: if (events_monitored_ & type) On 2017/01/03 23:59:40, Wez wrote: > nit: You can replace events_monitored_ with a bool and kill the EventBitMask > |type|. Done. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_win.cc:134: GetRawInputDevices(type, RIDEV_INPUTSINK)); On 2017/01/03 23:59:40, Wez wrote: > You're removing mouse event support so you can replace GetRawInputDevices and > hard-wire the keyboard RawInputDevice info in here. Left in function to make the keyboard RawInputDevice (removing type and mouse related code), but done. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_win.cc:198: if (input->header.dwType == RIM_TYPEMOUSE && input->header.hDevice != NULL) { On 2017/01/03 23:59:40, Wez wrote: > This entire case can go away since we never listen for mouse input any more. Done.
https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor.h (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor.h:25: // added. On 2017/01/07 00:12:35, CJ wrote: > On 2017/01/03 23:59:40, Wez wrote: > > nit: Remove the comment about listeners, since there is no listener interface > > any more. > > Done. nit: We still need the note about being thread-safe, since it seems that this class still gets called from various media threads :-/ https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_linux.cc:256: ui::EventType type; On 2017/01/07 00:12:35, CJ wrote: > On 2017/01/03 23:59:40, Wez wrote: > > nit: We only need this (and the corresponding code in the Windows version) > > because we use ui::EventType as the |type| parameter to OnKeyboardEvent, > whereas > > in practice OnKeyboardEvent only cares about whether the event is a press or a > > release - it would simplify both callers and the KeyboardEventCounter itself > if > > we switched the API to take a bool |down| parameter instead. > > Done. nit: Consider leaving a DCHECK in place to verify that type is KeyPress or KeyRelease. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_win.cc:134: GetRawInputDevices(type, RIDEV_INPUTSINK)); On 2017/01/07 00:12:36, CJ wrote: > On 2017/01/03 23:59:40, Wez wrote: > > You're removing mouse event support so you can replace GetRawInputDevices and > > hard-wire the keyboard RawInputDevice info in here. > > Left in function to make the keyboard RawInputDevice (removing type and mouse > related code), but done. Acknowledged. https://codereview.chromium.org/2577573002/diff/60001/media/base/user_input_m... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/60001/media/base/user_input_m... media/base/user_input_monitor_win.cc:161: if (events_monitored_ == 0) { No need for this check, any more. However, note that because you only have a single event type now, you could use whether or not |window_| is null to test for |events_monitored_|, rather than maintaining a separate bool.
https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor.h (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor.h:25: // added. On 2017/01/09 22:38:43, Wez wrote: > On 2017/01/07 00:12:35, CJ wrote: > > On 2017/01/03 23:59:40, Wez wrote: > > > nit: Remove the comment about listeners, since there is no listener > interface > > > any more. > > > > Done. > > nit: We still need the note about being thread-safe, since it seems that this > class still gets called from various media threads :-/ Done. https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/2577573002/diff/20001/media/base/user_input_m... media/base/user_input_monitor_linux.cc:256: ui::EventType type; On 2017/01/09 22:38:43, Wez wrote: > On 2017/01/07 00:12:35, CJ wrote: > > On 2017/01/03 23:59:40, Wez wrote: > > > nit: We only need this (and the corresponding code in the Windows version) > > > because we use ui::EventType as the |type| parameter to OnKeyboardEvent, > > whereas > > > in practice OnKeyboardEvent only cares about whether the event is a press or > a > > > release - it would simplify both callers and the KeyboardEventCounter itself > > if > > > we switched the API to take a bool |down| parameter instead. > > > > Done. > > nit: Consider leaving a DCHECK in place to verify that type is KeyPress or > KeyRelease. Done. https://codereview.chromium.org/2577573002/diff/60001/media/base/user_input_m... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/60001/media/base/user_input_m... media/base/user_input_monitor_win.cc:161: if (events_monitored_ == 0) { On 2017/01/09 22:38:44, Wez wrote: > No need for this check, any more. > > However, note that because you only have a single event type now, you could use > whether or not |window_| is null to test for |events_monitored_|, rather than > maintaining a separate bool. Done.
lgtm https://codereview.chromium.org/2577573002/diff/80001/media/base/user_input_m... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/80001/media/base/user_input_m... media/base/user_input_monitor_win.cc:118: window_.reset(new base::win::MessageWindow()); nit: This should become window_ = base::MakeUnique<base::win::MessageWindow>(); Note that if you were to assign the new window to a local unique_ptr<> window then you needn't reset() it in the failure cases - you instead just std::move() it into window_ when you've passed all the potentially-failing steps, below. https://codereview.chromium.org/2577573002/diff/80001/media/base/user_input_m... media/base/user_input_monitor_win.cc:152: window_.reset(); nit: window_ = nullptr;
Ooops, not LGTM re test. https://codereview.chromium.org/2577573002/diff/60001/media/base/user_input_m... File media/base/user_input_monitor_unittest.cc (left): https://codereview.chromium.org/2577573002/diff/60001/media/base/user_input_m... media/base/user_input_monitor_unittest.cc:54: TEST(UserInputMonitorTest, CreatePlatformSpecific) { Did you mean to remove this test? Doesn't look like it moved anywhere else?
https://codereview.chromium.org/2577573002/diff/60001/media/base/user_input_m... File media/base/user_input_monitor_unittest.cc (left): https://codereview.chromium.org/2577573002/diff/60001/media/base/user_input_m... media/base/user_input_monitor_unittest.cc:54: TEST(UserInputMonitorTest, CreatePlatformSpecific) { On 2017/01/10 02:00:27, Wez wrote: > Did you mean to remove this test? Doesn't look like it moved anywhere else? Done. https://codereview.chromium.org/2577573002/diff/80001/media/base/user_input_m... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/80001/media/base/user_input_m... media/base/user_input_monitor_win.cc:118: window_.reset(new base::win::MessageWindow()); On 2017/01/10 01:59:05, Wez wrote: > nit: This should become window_ = base::MakeUnique<base::win::MessageWindow>(); > > Note that if you were to assign the new window to a local unique_ptr<> window > then you needn't reset() it in the failure cases - you instead just std::move() > it into window_ when you've passed all the potentially-failing steps, below. Done. https://codereview.chromium.org/2577573002/diff/80001/media/base/user_input_m... media/base/user_input_monitor_win.cc:152: window_.reset(); On 2017/01/10 01:59:05, Wez wrote: > nit: window_ = nullptr; Done.
LGTM once the mouse stuff is removed from the create test, and the window_ syntax fixed. https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... File media/base/user_input_monitor_unittest.cc (right): https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... media/base/user_input_monitor_unittest.cc:24: class MockMouseListener : public UserInputMonitor::MouseEventListener { We've removed the MouseEventListener, so you can get rid of this, and the relevant calls in the code below. https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... media/base/user_input_monitor_win.cc:133: window_(std::move(window)); window_ = std::move(window) - I don't think this syntax will actually compile?
https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... File media/base/user_input_monitor_unittest.cc (right): https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... media/base/user_input_monitor_unittest.cc:24: class MockMouseListener : public UserInputMonitor::MouseEventListener { On 2017/01/10 02:45:10, Wez wrote: > We've removed the MouseEventListener, so you can get rid of this, and the > relevant calls in the code below. Done. https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... media/base/user_input_monitor_win.cc:133: window_(std::move(window)); On 2017/01/10 02:45:10, Wez wrote: > window_ = std::move(window) - I don't think this syntax will actually compile? Done.
https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... File media/base/user_input_monitor_unittest.cc (right): https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... media/base/user_input_monitor_unittest.cc:24: class MockMouseListener : public UserInputMonitor::MouseEventListener { On 2017/01/10 02:45:10, Wez wrote: > We've removed the MouseEventListener, so you can get rid of this, and the > relevant calls in the code below. Done. https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/100001/media/base/user_input_... media/base/user_input_monitor_win.cc:133: window_(std::move(window)); On 2017/01/10 02:45:10, Wez wrote: > window_ = std::move(window) - I don't think this syntax will actually compile? Done.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps120001 (title: "Addresses Wez's #20 comments.")
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps140001 (title: "Updates test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps160001 (title: "Udpate test inclusion.")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps180001 (title: "Fix typo.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lethalantidote@chromium.org changed reviewers: + xhwang@chromium.org
+xhwang, OWNERS
lgtm with nits https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... media/base/keyboard_event_counter.cc:22: void KeyboardEventCounter::OnKeyboardEvent(bool down, personally I found an explicit enum is more readable than a boolean, especially at the caller site, e.g. people won't understand what this means without looking at the declaration of the function: OnKeyboardEvent(true, ...); If you still prefer this, the function probably should be named OnKeyPressedOrRelased(bool pressed). https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... File media/base/keyboard_event_counter.h (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... media/base/keyboard_event_counter.h:30: // OnKeyboardEvent. Can we use a ThreadChecker to enforce this? https://codereview.chromium.org/2577573002/diff/200001/media/base/user_input_... File media/base/user_input_monitor.h (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/user_input_... media/base/user_input_monitor.h:27: UserInputMonitor() : key_press_counter_references_(0) {} wez: The style guide recommends inlining constructors when the class only contains POD members. In this case, we do have a base::Lock member. Is it a concern? https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...
https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... media/base/keyboard_event_counter.cc:22: void KeyboardEventCounter::OnKeyboardEvent(bool down, On 2017/01/10 20:13:15, xhwang wrote: > personally I found an explicit enum is more readable than a boolean, especially > at the caller site, e.g. people won't understand what this means without looking > at the declaration of the function: > > OnKeyboardEvent(true, ...); > > If you still prefer this, the function probably should be named > OnKeyPressedOrRelased(bool pressed). Yes, strictly it's preferable to keep this as an enum; the down-side of using ui::EventType is that it includes a load of type values that are never valid. :-/ So OK, yes, sorry, let's revert to using ui::EventType, but let's try to keep that code trivial, e.g: key_is_down ? KeyDown : KeyUp to generate the OnKeyboardEvent() parameter. https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... File media/base/keyboard_event_counter.h (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... media/base/keyboard_event_counter.h:30: // OnKeyboardEvent. On 2017/01/10 20:13:15, xhwang wrote: > Can we use a ThreadChecker to enforce this? +1, if this constraint is valid. However, it's not clear why this constraint makes sense, given that the caller is using GetKeyPressCount() on some other thread - so they will get a bogus result if the key-capture thread calls Reset between two GetKeyPressCount() calls. It looks like Reset() is only used in the Linux StartMonitor() implementation, and there is a race-condition between it being called on the IO thread and the calling thread (or some other thread) continuing on and calling GetKeyPressCount(). Given that the UserInputMonitor's keypress count API doesn't guarantee that keypress counts are zero-based, can we just get rid of Reset() entirely? https://codereview.chromium.org/2577573002/diff/200001/media/base/user_input_... File media/base/user_input_monitor.h (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/user_input_... media/base/user_input_monitor.h:27: UserInputMonitor() : key_press_counter_references_(0) {} On 2017/01/10 20:13:16, xhwang wrote: > wez: The style guide recommends inlining constructors when the class only > contains POD members. In this case, we do have a base::Lock member. Is it a > concern? I agree, it's probably best to keep the constructor in the .cc.
https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... media/base/keyboard_event_counter.cc:22: void KeyboardEventCounter::OnKeyboardEvent(bool down, On 2017/01/10 23:19:24, Wez wrote: > On 2017/01/10 20:13:15, xhwang wrote: > > personally I found an explicit enum is more readable than a boolean, > especially > > at the caller site, e.g. people won't understand what this means without > looking > > at the declaration of the function: > > > > OnKeyboardEvent(true, ...); > > > > If you still prefer this, the function probably should be named > > OnKeyPressedOrRelased(bool pressed). > > Yes, strictly it's preferable to keep this as an enum; the down-side of using > ui::EventType is that it includes a load of type values that are never valid. > :-/ > > So OK, yes, sorry, let's revert to using ui::EventType, but let's try to keep > that code trivial, e.g: > > key_is_down ? KeyDown : KeyUp > > to generate the OnKeyboardEvent() parameter. Confused as how the key_is_down ? KeyDown : KeyUp expression is involved. Do we want this function to take in a ui::EventType or a bool? https://codereview.chromium.org/2577573002/diff/200001/media/base/user_input_... File media/base/user_input_monitor.h (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/user_input_... media/base/user_input_monitor.h:27: UserInputMonitor() : key_press_counter_references_(0) {} On 2017/01/10 23:19:24, Wez wrote: > On 2017/01/10 20:13:16, xhwang wrote: > > wez: The style guide recommends inlining constructors when the class only > > contains POD members. In this case, we do have a base::Lock member. Is it a > > concern? > > I agree, it's probably best to keep the constructor in the .cc. Done.
https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... media/base/keyboard_event_counter.cc:22: void KeyboardEventCounter::OnKeyboardEvent(bool down, On 2017/01/11 21:40:27, CJ wrote: > On 2017/01/10 23:19:24, Wez wrote: > > On 2017/01/10 20:13:15, xhwang wrote: > > > personally I found an explicit enum is more readable than a boolean, > > especially > > > at the caller site, e.g. people won't understand what this means without > > looking > > > at the declaration of the function: > > > > > > OnKeyboardEvent(true, ...); > > > > > > If you still prefer this, the function probably should be named > > > OnKeyPressedOrRelased(bool pressed). > > > > Yes, strictly it's preferable to keep this as an enum; the down-side of using > > ui::EventType is that it includes a load of type values that are never valid. > > :-/ > > > > So OK, yes, sorry, let's revert to using ui::EventType, but let's try to keep > > that code trivial, e.g: > > > > key_is_down ? KeyDown : KeyUp > > > > to generate the OnKeyboardEvent() parameter. > > Confused as how the key_is_down ? KeyDown : KeyUp expression is involved. > > Do we want this function to take in a ui::EventType or a bool? I just meant that instead of restoring this: ui::EventType type; 288 if (event->u.u.type == KeyPress) { 289 type = ui::ET_KEY_PRESSED; 290 } else if (event->u.u.type == KeyRelease) { 291 type = ui::ET_KEY_RELEASED; 292 } else { 293 NOTREACHED(); 294 return; 295 } let's reduce it to: ui::EventType type = (event->u.u.type == KeyPress) ? ui::EventType::KeyDown : ui::EventType::KeyUp;
https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... media/base/keyboard_event_counter.cc:22: void KeyboardEventCounter::OnKeyboardEvent(bool down, On 2017/01/11 21:54:22, Wez wrote: > On 2017/01/11 21:40:27, CJ wrote: > > On 2017/01/10 23:19:24, Wez wrote: > > > On 2017/01/10 20:13:15, xhwang wrote: > > > > personally I found an explicit enum is more readable than a boolean, > > > especially > > > > at the caller site, e.g. people won't understand what this means without > > > looking > > > > at the declaration of the function: > > > > > > > > OnKeyboardEvent(true, ...); > > > > > > > > If you still prefer this, the function probably should be named > > > > OnKeyPressedOrRelased(bool pressed). > > > > > > Yes, strictly it's preferable to keep this as an enum; the down-side of > using > > > ui::EventType is that it includes a load of type values that are never > valid. > > > :-/ > > > > > > So OK, yes, sorry, let's revert to using ui::EventType, but let's try to > keep > > > that code trivial, e.g: > > > > > > key_is_down ? KeyDown : KeyUp > > > > > > to generate the OnKeyboardEvent() parameter. > > > > Confused as how the key_is_down ? KeyDown : KeyUp expression is involved. > > > > Do we want this function to take in a ui::EventType or a bool? > > I just meant that instead of restoring this: > ui::EventType type; > 288 if (event->u.u.type == KeyPress) { > 289 type = ui::ET_KEY_PRESSED; > 290 } else if (event->u.u.type == KeyRelease) { > 291 type = ui::ET_KEY_RELEASED; > 292 } else { > 293 NOTREACHED(); > 294 return; > 295 } > > let's reduce it to: > > ui::EventType type = (event->u.u.type == KeyPress) ? ui::EventType::KeyDown : > ui::EventType::KeyUp; Done.
LGTM https://codereview.chromium.org/2577573002/diff/240001/media/base/user_input_... File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/2577573002/diff/240001/media/base/user_input_... media/base/user_input_monitor_linux.cc:304: return base::WrapUnique(new UserInputMonitorLinux(io_task_runner)); nit: Consider migrating these WrapUnique calls (here and in the other platform impls) to base::MakeUnique<Foo>(...) https://codereview.chromium.org/2577573002/diff/240001/media/base/user_input_... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/240001/media/base/user_input_... media/base/user_input_monitor_win.cc:187: counter_.OnKeyboardEvent(!(input->data.keyboard.Flags & RI_KEY_BREAK), Looks like this needs migrating back to the ui::EventType style?
https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... File media/base/keyboard_event_counter.h (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... media/base/keyboard_event_counter.h:30: // OnKeyboardEvent. On 2017/01/10 23:19:24, Wez wrote: > On 2017/01/10 20:13:15, xhwang wrote: > > Can we use a ThreadChecker to enforce this? > > +1, if this constraint is valid. > > However, it's not clear why this constraint makes sense, given that the caller > is using GetKeyPressCount() on some other thread - so they will get a bogus > result if the key-capture thread calls Reset between two GetKeyPressCount() > calls. > > It looks like Reset() is only used in the Linux StartMonitor() implementation, > and there is a race-condition between it being called on the IO thread and the > calling thread (or some other thread) continuing on and calling > GetKeyPressCount(). > > Given that the UserInputMonitor's keypress count API doesn't guarantee that > keypress counts are zero-based, can we just get rid of Reset() entirely? Waiting on the result of this conversation. https://codereview.chromium.org/2577573002/diff/240001/media/base/user_input_... File media/base/user_input_monitor_linux.cc (right): https://codereview.chromium.org/2577573002/diff/240001/media/base/user_input_... media/base/user_input_monitor_linux.cc:304: return base::WrapUnique(new UserInputMonitorLinux(io_task_runner)); On 2017/01/11 23:19:19, Wez wrote: > nit: Consider migrating these WrapUnique calls (here and in the other platform > impls) to base::MakeUnique<Foo>(...) Done. https://codereview.chromium.org/2577573002/diff/240001/media/base/user_input_... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/240001/media/base/user_input_... media/base/user_input_monitor_win.cc:187: counter_.OnKeyboardEvent(!(input->data.keyboard.Flags & RI_KEY_BREAK), On 2017/01/11 23:19:19, Wez wrote: > Looks like this needs migrating back to the ui::EventType style? Thanks for catching this.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...) linux_chromium_chromeos_ozone_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 lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps300001 (title: "Typos in _win.")
The CQ bit was unchecked by lethalantidote@chromium.org
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... File media/base/keyboard_event_counter.h (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... media/base/keyboard_event_counter.h:30: // OnKeyboardEvent. On 2017/01/11 23:41:55, CJ wrote: > On 2017/01/10 23:19:24, Wez wrote: > > On 2017/01/10 20:13:15, xhwang wrote: > > > Can we use a ThreadChecker to enforce this? > > > > +1, if this constraint is valid. > > > > However, it's not clear why this constraint makes sense, given that the caller > > is using GetKeyPressCount() on some other thread - so they will get a bogus > > result if the key-capture thread calls Reset between two GetKeyPressCount() > > calls. > > > > It looks like Reset() is only used in the Linux StartMonitor() implementation, > > and there is a race-condition between it being called on the IO thread and the > > calling thread (or some other thread) continuing on and calling > > GetKeyPressCount(). > > > > Given that the UserInputMonitor's keypress count API doesn't guarantee that > > keypress counts are zero-based, can we just get rid of Reset() entirely? > > Waiting on the result of this conversation. Removing Reset(). In the implementation, |total_key_presses_| is cast to an base::subtle::AtomicWord. Does that at all help with the raise case u were mentioning (this is just for my understanding).
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps360001 (title: "Removes reset function.")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... File media/base/keyboard_event_counter.h (right): https://codereview.chromium.org/2577573002/diff/200001/media/base/keyboard_ev... media/base/keyboard_event_counter.h:30: // OnKeyboardEvent. On 2017/01/12 23:59:37, CJ wrote: > On 2017/01/11 23:41:55, CJ wrote: > > On 2017/01/10 23:19:24, Wez wrote: > > > On 2017/01/10 20:13:15, xhwang wrote: > > > > Can we use a ThreadChecker to enforce this? > > > > > > +1, if this constraint is valid. > > > > > > However, it's not clear why this constraint makes sense, given that the > caller > > > is using GetKeyPressCount() on some other thread - so they will get a bogus > > > result if the key-capture thread calls Reset between two GetKeyPressCount() > > > calls. > > > > > > It looks like Reset() is only used in the Linux StartMonitor() > implementation, > > > and there is a race-condition between it being called on the IO thread and > the > > > calling thread (or some other thread) continuing on and calling > > > GetKeyPressCount(). > > > > > > Given that the UserInputMonitor's keypress count API doesn't guarantee that > > > keypress counts are zero-based, can we just get rid of Reset() entirely? > > > > Waiting on the result of this conversation. > > Removing Reset(). In the implementation, |total_key_presses_| is cast to an > base::subtle::AtomicWord. Does that at all help with the raise case u were > mentioning (this is just for my understanding). No, but good question. The base::subtle::Atomic stuff is about ensuring that values are written atomically to memory - e.g. if you write 0xff0000ff to a location that currently contains 0x00ffff00 then it is possible (though usually unlikely) for some other thread reading that location mid-write to "see" a value like 0xff00ff00, because it's reading while the other thread is mid-write! There is a separate aspect, memory "barriers", which is basically to do with whether other threads might still see a stale value because of caching. The race-condition I was talking about, though, is to do with the fact that the caller is expecting to call GetKeyEventCount() on e.g. the UI thread, while OnKeyboardEvent() and Reset() are called on some other background thread - the caller might start listening for keyboard events and immediately afterwards check the count and get a non-zero value like 5, because that's what the count had previously got up to, but then the next time it calls it sees zero, because Reset() took place on the other thread in the meantime. Looking at the use of atomics in the implementation, I'm surprised that we're doing the casts, rather than just using base::subtle::Atomic32 etc as the counter type directly, and just casting it to a size_t when we want to return it.
The CQ bit was checked by lethalantidote@chromium.org
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by lethalantidote@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps380001 (title: "Updates win to not use reset.")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lethalantidote@chromium.org
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps400001 (title: "Merge branch 'real_master' into UserInputMonitor")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lethalantidote@chromium.org
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2577573002/diff/400001/media/base/user_input_... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/400001/media/base/user_input_... media/base/user_input_monitor_win.cc:125: std::unique_ptr<RAWINPUTDEVICE> device(GetRawInputDevices(RIDEV_INPUTSINK)); OK, the issue breaking the tests is that GetRawInputDevices actually depends on |window_| being set (it calls |window_->hwnd()|). So it seems that we need to set |window_| before we call this, OR we need to pass the HWND explicitly as a parameter to GetRawInputDevices - the latter would be my preference, and it would let you move GetRawInputDevices() to be a static helper function in this .cc rather than a class method.
The CQ bit was checked by lethalantidote@chromium.org to run a CQ dry run
On 2017/01/18 20:41:12, Wez wrote: > https://codereview.chromium.org/2577573002/diff/400001/media/base/user_input_... > File media/base/user_input_monitor_win.cc (right): > > https://codereview.chromium.org/2577573002/diff/400001/media/base/user_input_... > media/base/user_input_monitor_win.cc:125: std::unique_ptr<RAWINPUTDEVICE> > device(GetRawInputDevices(RIDEV_INPUTSINK)); > OK, the issue breaking the tests is that GetRawInputDevices actually depends on > |window_| being set (it calls |window_->hwnd()|). > > So it seems that we need to set |window_| before we call this, OR we need to > pass the HWND explicitly as a parameter to GetRawInputDevices - the latter would > be my preference, and it would let you move GetRawInputDevices() to be a static > helper function in this .cc rather than a class method. Updated. PTAL
Dry run: 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
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps440001 (title: "Fixes GetRawDevice.")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
This looks good to me - left comments on the build breakers. FWIW you can upload & send your patch to a specific try-bot via the codereview user interface, rather than keeping sending it to loads of bots. Alternatively, once you have a new patch to try, upload it and I'll build it locally. :) https://codereview.chromium.org/2577573002/diff/440001/media/base/user_input_... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/440001/media/base/user_input_... media/base/user_input_monitor_win.cc:32: DCHECK(ui_task_runner_->BelongsToCurrentThread()); Remove this - you're no longer in a class member, so the task runner member is not accessible here, and this is an internal helper function in any case, so you can assume it's called correctly given the thread checks elsewhere. https://codereview.chromium.org/2577573002/diff/440001/media/base/user_input_... media/base/user_input_monitor_win.cc:156: GetRawInputDevices(RIDEV_REMOVE)); You put the window_->hwnd() before GetRawInputDevices, rather than as the first parameter to it, by mistake. :)
https://codereview.chromium.org/2577573002/diff/440001/media/base/user_input_... File media/base/user_input_monitor_win.cc (right): https://codereview.chromium.org/2577573002/diff/440001/media/base/user_input_... media/base/user_input_monitor_win.cc:32: DCHECK(ui_task_runner_->BelongsToCurrentThread()); On 2017/01/19 01:59:56, Wez wrote: > Remove this - you're no longer in a class member, so the task runner member is > not accessible here, and this is an internal helper function in any case, so you > can assume it's called correctly given the thread checks elsewhere. Done. https://codereview.chromium.org/2577573002/diff/440001/media/base/user_input_... media/base/user_input_monitor_win.cc:156: GetRawInputDevices(RIDEV_REMOVE)); On 2017/01/19 01:59:56, Wez wrote: > You put the window_->hwnd() before GetRawInputDevices, rather than as the first > parameter to it, by mistake. :) Done.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps460001 (title: "Format.")
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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
On 2017/01/21 00:52:59, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_clang on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...) The win_clang error is super confusing; IIUC you actually want to just "return device" without the std::move() in there, because the compiler would move the content of |device| (copy elision) even without it.
The CQ bit was checked by lethalantidote@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/2577573002/#ps480001 (title: "Fix win by removing std::move.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 480001, "attempt_start_ts": 1484965698345510,
"parent_rev": "799d1b122128aa137ee7c4a66f7e65eaf39b0485", "commit_rev":
"fdd566ad2d6598b104ea75b4ae2395a96083ca20"}
Message was sent while issue was closed.
Description was changed from ========== Removes mouse listeners from UserInputMonitor. MouseEventListener was found to be unused, and thus removed. BUG=673494 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/fdd566ad2d6598b104ea75b4ae23... ==========
Message was sent while issue was closed.
Committed patchset #25 (id:480001) as https://chromium.googlesource.com/chromium/src/+/fdd566ad2d6598b104ea75b4ae23... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
