Issue 22801007: Adds the UserInputMonitor implementation for Mac.

jiayl

7 years, 4 months ago (2013-08-19 22:56:52 UTC) #1

DaleCurtis

I don't know Obj-C well enough to give you a stamp there, either sergeyu@ does ...

7 years, 4 months ago (2013-08-19 23:49:00 UTC) #2

jiayl

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc File media/base/user_input_monitor.cc (right): https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc#newcode109 media/base/user_input_monitor.cc:109: void UserInputMonitor::PollKeyState() { On 2013/08/19 23:49:00, DaleCurtis wrote: > ...

7 years, 4 months ago (2013-08-20 00:21:15 UTC) #3

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc
File media/base/user_input_monitor.cc (right):

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:109: void UserInputMonitor::PollKeyState() {
On 2013/08/19 23:49:00, DaleCurtis wrote:
> Seems like this should go in UserInputMonitorMac, no?  Instead of
reimplementing
> your KeyPress detection, can it just call OnKeyboardEvent() with the relevant
> event and key_code?

I would need another pressed_key_map in UserInputMonitorMac to keep track of the
pressed keys and fire Key_released notification while UserInputMonitor already
has a pressed_keys_. I put PollKeyState in UserInputMonitor to avoid the
duplication. If you think it's worthwhile for readability, I can move it into
UserInputMonitorMac.

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:121: key_index);
On 2013/08/19 23:49:00, DaleCurtis wrote:
> Indent is off.

Done.

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:123: if (key_state &&
pressed_keys_.find(key_index) == pressed_keys_.end()) {
On 2013/08/19 23:49:00, DaleCurtis wrote:
> Save result so you're not calling find() twice.

Done.

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:127: pressed_keys_.find(key_index) !=
pressed_keys_.end()) {
On 2013/08/19 23:49:00, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:138:
base::MessageLoop::current()->PostDelayedTask(
On 2013/08/19 23:49:00, DaleCurtis wrote:
> Which loop is this running on? It looks like it'll happen on whoever calls
> AddKeyStrokeListener() first.  Instead you should probably have it execute on
> the IO or UI loop.  Also this class is not RefCounted, so you need to ensure
the
> callback is not outstanding during destruction.  Usually you'd do this using a
> CancellableClosure, but you may have difficulty due to your threading
situation.

Yeh, this is gross. But I don't want to run it on Browser IO/UI thread since
it's pretty expensive. Ideally it should run on a worker thread. How about
passing the audio thread to UserInputMonitor ctor and run on that? :)

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
File media/base/user_input_monitor_mac.mm (right):

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor_mac.mm:75: // Must be called when the
UserInputMonitorManager is no longer to be used.
On 2013/08/19 23:49:00, DaleCurtis wrote:
> UserInputMonitorManager?
Renamed

https://codereview.chromium.org/22801007/diff/1/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/22801007/diff/1/media/media.gyp#newcode788
media/media.gyp:788: 'sources': [
On 2013/08/19 23:49:00, DaleCurtis wrote:
> Seems like there should be a .gyp for the GTM project that media can add a a
> dependency on instead of this manual source inclusion?

But third_party/GTM has no .gyp file. Where to put it?

DaleCurtis

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc File media/base/user_input_monitor.cc (right): https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc#newcode109 media/base/user_input_monitor.cc:109: void UserInputMonitor::PollKeyState() { On 2013/08/20 00:21:15, jiayl wrote: > ...

7 years, 4 months ago (2013-08-20 00:35:53 UTC) #4

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc
File media/base/user_input_monitor.cc (right):

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:109: void UserInputMonitor::PollKeyState() {
On 2013/08/20 00:21:15, jiayl wrote:
> On 2013/08/19 23:49:00, DaleCurtis wrote:
> > Seems like this should go in UserInputMonitorMac, no?  Instead of
> reimplementing
> > your KeyPress detection, can it just call OnKeyboardEvent() with the
relevant
> > event and key_code?
> 
> I would need another pressed_key_map in UserInputMonitorMac to keep track of
the
> pressed keys and fire Key_released notification while UserInputMonitor already
> has a pressed_keys_. I put PollKeyState in UserInputMonitor to avoid the
> duplication. If you think it's worthwhile for readability, I can move it into
> UserInputMonitorMac.

If you allow OnKeyboardEvent() to be called w/ ET_KEY_RELEASED for keys not in
the pressed_keys_ set you can just call it instead of managing two maps.

I'd recommend introducing a protected method call OnKeyboardEvent_Locked() in
this case so you're not constantly reacquiring the AutoLock.

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:138:
base::MessageLoop::current()->PostDelayedTask(
On 2013/08/20 00:21:15, jiayl wrote:
> On 2013/08/19 23:49:00, DaleCurtis wrote:
> > Which loop is this running on? It looks like it'll happen on whoever calls
> > AddKeyStrokeListener() first.  Instead you should probably have it execute
on
> > the IO or UI loop.  Also this class is not RefCounted, so you need to ensure
> the
> > callback is not outstanding during destruction.  Usually you'd do this using
a
> > CancellableClosure, but you may have difficulty due to your threading
> situation.
> 
> Yeh, this is gross. But I don't want to run it on Browser IO/UI thread since
> it's pretty expensive. Ideally it should run on a worker thread. How about
> passing the audio thread to UserInputMonitor ctor and run on that? :)

Well not only is it gross, it doesn't work: consider the case where the message
loop the listener was added on goes away.  Also you say that you don't want it
on the UI or IO thread, but there's no guarantee current() isn't one of those.

How long does it take to scan all the keyboard codes? Also, you say you can't
get key events w/o assistive tech enabled, but what about:

https://developer.apple.com/library/mac/DOCUMENTATION/Cocoa/Conceptual/EventO...

Specifically
https://developer.apple.com/library/mac/DOCUMENTATION/Cocoa/Reference/Applica...

DaleCurtis

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc File media/base/user_input_monitor.cc (right): https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc#newcode138 media/base/user_input_monitor.cc:138: base::MessageLoop::current()->PostDelayedTask( On 2013/08/20 00:35:54, DaleCurtis wrote: > On 2013/08/20 ...

7 years, 4 months ago (2013-08-20 00:40:00 UTC) #5

jiayl

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc File media/base/user_input_monitor.cc (right): https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc#newcode109 media/base/user_input_monitor.cc:109: void UserInputMonitor::PollKeyState() { On 2013/08/20 00:35:54, DaleCurtis wrote: > ...

7 years, 4 months ago (2013-08-20 00:42:59 UTC) #6

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc
File media/base/user_input_monitor.cc (right):

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:109: void UserInputMonitor::PollKeyState() {
On 2013/08/20 00:35:54, DaleCurtis wrote:
> On 2013/08/20 00:21:15, jiayl wrote:
> > On 2013/08/19 23:49:00, DaleCurtis wrote:
> > > Seems like this should go in UserInputMonitorMac, no?  Instead of
> > reimplementing
> > > your KeyPress detection, can it just call OnKeyboardEvent() with the
> relevant
> > > event and key_code?
> > 
> > I would need another pressed_key_map in UserInputMonitorMac to keep track of
> the
> > pressed keys and fire Key_released notification while UserInputMonitor
already
> > has a pressed_keys_. I put PollKeyState in UserInputMonitor to avoid the
> > duplication. If you think it's worthwhile for readability, I can move it
into
> > UserInputMonitorMac.
> 
> If you allow OnKeyboardEvent() to be called w/ ET_KEY_RELEASED for keys not in
> the pressed_keys_ set you can just call it instead of managing two maps.
> 
> I'd recommend introducing a protected method call OnKeyboardEvent_Locked() in
> this case so you're not constantly reacquiring the AutoLock.

So you mean firing Key_release for all keys not in a pressed state? That seems
too spamy, since you'll get callback for all keys in every poll if no typing is
happening.

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:138:
base::MessageLoop::current()->PostDelayedTask(
On 2013/08/20 00:35:54, DaleCurtis wrote:
> On 2013/08/20 00:21:15, jiayl wrote:
> > On 2013/08/19 23:49:00, DaleCurtis wrote:
> > > Which loop is this running on? It looks like it'll happen on whoever calls
> > > AddKeyStrokeListener() first.  Instead you should probably have it execute
> on
> > > the IO or UI loop.  Also this class is not RefCounted, so you need to
ensure
> > the
> > > callback is not outstanding during destruction.  Usually you'd do this
using
> a
> > > CancellableClosure, but you may have difficulty due to your threading
> > situation.
> > 
> > Yeh, this is gross. But I don't want to run it on Browser IO/UI thread since
> > it's pretty expensive. Ideally it should run on a worker thread. How about
> > passing the audio thread to UserInputMonitor ctor and run on that? :)
> 
> Well not only is it gross, it doesn't work: consider the case where the
message
> loop the listener was added on goes away.  Also you say that you don't want it
> on the UI or IO thread, but there's no guarantee current() isn't one of those.
> 
> How long does it take to scan all the keyboard codes? Also, you say you can't
> get key events w/o assistive tech enabled, but what about:
> 
>
https://developer.apple.com/library/mac/DOCUMENTATION/Cocoa/Conceptual/EventO...
> 
> Specifically
>
https://developer.apple.com/library/mac/DOCUMENTATION/Cocoa/Reference/Applica...

Right, the document says
A global event monitor looks for user-input events dispatched to applications
other than the one in which it is installed. The monitor cannot modify an event
or prevent its normal delivery. And it may only monitor key events *if
accessibility is enabled or if the application is trusted for accessibility*.

which is not true for Chrome on Mac, by default.
I think HenrikG mentioned that it took a few milliseconds to poll all keys.

DaleCurtis

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc File media/base/user_input_monitor.cc (right): https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc#newcode109 media/base/user_input_monitor.cc:109: void UserInputMonitor::PollKeyState() { On 2013/08/20 00:42:59, jiayl wrote: > ...

7 years, 4 months ago (2013-08-20 00:52:18 UTC) #7

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc
File media/base/user_input_monitor.cc (right):

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:109: void UserInputMonitor::PollKeyState() {
On 2013/08/20 00:42:59, jiayl wrote:
> On 2013/08/20 00:35:54, DaleCurtis wrote:
> > On 2013/08/20 00:21:15, jiayl wrote:
> > > On 2013/08/19 23:49:00, DaleCurtis wrote:
> > > > Seems like this should go in UserInputMonitorMac, no?  Instead of
> > > reimplementing
> > > > your KeyPress detection, can it just call OnKeyboardEvent() with the
> > relevant
> > > > event and key_code?
> > > 
> > > I would need another pressed_key_map in UserInputMonitorMac to keep track
of
> > the
> > > pressed keys and fire Key_released notification while UserInputMonitor
> already
> > > has a pressed_keys_. I put PollKeyState in UserInputMonitor to avoid the
> > > duplication. If you think it's worthwhile for readability, I can move it
> into
> > > UserInputMonitorMac.
> > 
> > If you allow OnKeyboardEvent() to be called w/ ET_KEY_RELEASED for keys not
in
> > the pressed_keys_ set you can just call it instead of managing two maps.
> > 
> > I'd recommend introducing a protected method call OnKeyboardEvent_Locked()
in
> > this case so you're not constantly reacquiring the AutoLock.
> 
> So you mean firing Key_release for all keys not in a pressed state? That seems
> too spamy, since you'll get callback for all keys in every poll if no typing
is
> happening.

OnKeyboardEvent() doesn't fire anything for ET_KEY_RELEASED.  What do you mean?

https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
media/base/user_input_monitor.cc:138:
base::MessageLoop::current()->PostDelayedTask(
On 2013/08/20 00:42:59, jiayl wrote:
> On 2013/08/20 00:35:54, DaleCurtis wrote:
> > On 2013/08/20 00:21:15, jiayl wrote:
> > > On 2013/08/19 23:49:00, DaleCurtis wrote:
> > > > Which loop is this running on? It looks like it'll happen on whoever
calls
> > > > AddKeyStrokeListener() first.  Instead you should probably have it
execute
> > on
> > > > the IO or UI loop.  Also this class is not RefCounted, so you need to
> ensure
> > > the
> > > > callback is not outstanding during destruction.  Usually you'd do this
> using
> > a
> > > > CancellableClosure, but you may have difficulty due to your threading
> > > situation.
> > > 
> > > Yeh, this is gross. But I don't want to run it on Browser IO/UI thread
since
> > > it's pretty expensive. Ideally it should run on a worker thread. How about
> > > passing the audio thread to UserInputMonitor ctor and run on that? :)
> > 
> > Well not only is it gross, it doesn't work: consider the case where the
> message
> > loop the listener was added on goes away.  Also you say that you don't want
it
> > on the UI or IO thread, but there's no guarantee current() isn't one of
those.
> > 
> > How long does it take to scan all the keyboard codes? Also, you say you
can't
> > get key events w/o assistive tech enabled, but what about:
> > 
> >
>
https://developer.apple.com/library/mac/DOCUMENTATION/Cocoa/Conceptual/EventO...
> > 
> > Specifically
> >
>
https://developer.apple.com/library/mac/DOCUMENTATION/Cocoa/Reference/Applica...
> 
> Right, the document says
> A global event monitor looks for user-input events dispatched to applications
> other than the one in which it is installed. The monitor cannot modify an
event
> or prevent its normal delivery. And it may only monitor key events *if
> accessibility is enabled or if the application is trusted for accessibility*.
> 
> which is not true for Chrome on Mac, by default.
> I think HenrikG mentioned that it took a few milliseconds to poll all keys.
> 

Can we ask users for assistive access?  FYI, on OSX the Audio Thread is the UI
thread, so I don't think that'll work either if you don't want this running on
the UI thread.

Sergey Ulanov

Sorry, but I'm not an expect in Objective C either. https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor_mac.mm File media/base/user_input_monitor_mac.mm (right): https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor_mac.mm#newcode27 ...

7 years, 4 months ago (2013-08-20 00:55:40 UTC) #8

Robert Sesek

Have you considered using the NSEvent API for this instead? It's considerably easier to work ...

7 years, 4 months ago (2013-08-20 13:03:33 UTC) #9

jiayl

On 2013/08/20 00:52:18, DaleCurtis wrote: > https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc > File media/base/user_input_monitor.cc (right): > > https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc#newcode109 > ...

7 years, 4 months ago (2013-08-20 16:39:19 UTC) #10

On 2013/08/20 00:52:18, DaleCurtis wrote:
>
https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor.cc
> File media/base/user_input_monitor.cc (right):
> 
>
https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
> media/base/user_input_monitor.cc:109: void UserInputMonitor::PollKeyState() {
> On 2013/08/20 00:42:59, jiayl wrote:
> > On 2013/08/20 00:35:54, DaleCurtis wrote:
> > > On 2013/08/20 00:21:15, jiayl wrote:
> > > > On 2013/08/19 23:49:00, DaleCurtis wrote:
> > > > > Seems like this should go in UserInputMonitorMac, no?  Instead of
> > > > reimplementing
> > > > > your KeyPress detection, can it just call OnKeyboardEvent() with the
> > > relevant
> > > > > event and key_code?
> > > > 
> > > > I would need another pressed_key_map in UserInputMonitorMac to keep
track
> of
> > > the
> > > > pressed keys and fire Key_released notification while UserInputMonitor
> > already
> > > > has a pressed_keys_. I put PollKeyState in UserInputMonitor to avoid the
> > > > duplication. If you think it's worthwhile for readability, I can move it
> > into
> > > > UserInputMonitorMac.
> > > 
> > > If you allow OnKeyboardEvent() to be called w/ ET_KEY_RELEASED for keys
not
> in
> > > the pressed_keys_ set you can just call it instead of managing two maps.
> > > 
> > > I'd recommend introducing a protected method call OnKeyboardEvent_Locked()
> in
> > > this case so you're not constantly reacquiring the AutoLock.
> > 
> > So you mean firing Key_release for all keys not in a pressed state? That
seems
> > too spamy, since you'll get callback for all keys in every poll if no typing
> is
> > happening.
> 
> OnKeyboardEvent() doesn't fire anything for ET_KEY_RELEASED.  What do you
mean?
> 

I mean OnKeyboardEvent itself will be called too often unnecessarily.

>
https://codereview.chromium.org/22801007/diff/1/media/base/user_input_monitor...
> media/base/user_input_monitor.cc:138:
> base::MessageLoop::current()->PostDelayedTask(
> On 2013/08/20 00:42:59, jiayl wrote:
> > On 2013/08/20 00:35:54, DaleCurtis wrote:
> > > On 2013/08/20 00:21:15, jiayl wrote:
> > > > On 2013/08/19 23:49:00, DaleCurtis wrote:
> > > > > Which loop is this running on? It looks like it'll happen on whoever
> calls
> > > > > AddKeyStrokeListener() first.  Instead you should probably have it
> execute
> > > on
> > > > > the IO or UI loop.  Also this class is not RefCounted, so you need to
> > ensure
> > > > the
> > > > > callback is not outstanding during destruction.  Usually you'd do this
> > using
> > > a
> > > > > CancellableClosure, but you may have difficulty due to your threading
> > > > situation.
> > > > 
> > > > Yeh, this is gross. But I don't want to run it on Browser IO/UI thread
> since
> > > > it's pretty expensive. Ideally it should run on a worker thread. How
about
> > > > passing the audio thread to UserInputMonitor ctor and run on that? :)
> > > 
> > > Well not only is it gross, it doesn't work: consider the case where the
> > message
> > > loop the listener was added on goes away.  Also you say that you don't
want
> it
> > > on the UI or IO thread, but there's no guarantee current() isn't one of
> those.
> > > 
> > > How long does it take to scan all the keyboard codes? Also, you say you
> can't
> > > get key events w/o assistive tech enabled, but what about:
> > > 
> > >
> >
>
https://developer.apple.com/library/mac/DOCUMENTATION/Cocoa/Conceptual/EventO...
> > > 
> > > Specifically
> > >
> >
>
https://developer.apple.com/library/mac/DOCUMENTATION/Cocoa/Reference/Applica...
> > 
> > Right, the document says
> > A global event monitor looks for user-input events dispatched to
applications
> > other than the one in which it is installed. The monitor cannot modify an
> event
> > or prevent its normal delivery. And it may only monitor key events *if
> > accessibility is enabled or if the application is trusted for
accessibility*.
> > 
> > which is not true for Chrome on Mac, by default.
> > I think HenrikG mentioned that it took a few milliseconds to poll all keys.
> > 
> 
> Can we ask users for assistive access?  FYI, on OSX the Audio Thread is the UI
> thread, so I don't think that'll work either if you don't want this running on
> the UI thread.

So what do you suggest? Creating a new thread? 

Asking users for access will add new complexity: what's the UI to prompt the
user? How to instruct the user navigating through Mac system configuration? How
to cache user's choice (e.g. "don't ask again")? ... And we still need the
polling code as a fallback when users fail to allow the access. IMO it will add
little value since most users would just ignore the prompt.

jiayl

On 2013/08/20 13:03:33, rsesek wrote: > Have you considered using the NSEvent API for this ...

7 years, 4 months ago (2013-08-20 16:47:55 UTC) #11

Robert Sesek

On 2013/08/20 16:47:55, jiayl wrote: > On 2013/08/20 13:03:33, rsesek wrote: > > Have you ...

7 years, 4 months ago (2013-08-20 16:55:46 UTC) #12

jiayl

Moved polling into UserInputMonitorMac and running on the UI thread. jam, The polling runs every ...

7 years, 4 months ago (2013-08-20 23:49:04 UTC) #13

Robert Sesek

Is the polling only done when a WebRTC stream needs to know about key events? ...

7 years, 4 months ago (2013-08-21 00:19:30 UTC) #15

jiayl

On 2013/08/21 00:19:30, rsesek wrote: > Is the polling only done when a WebRTC stream ...

7 years, 4 months ago (2013-08-21 00:50:48 UTC) #16

Robert Sesek

https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm File media/base/user_input_monitor_mac.mm (right): https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm#newcode90 media/base/user_input_monitor_mac.mm:90: static CGEventRef inputEvent(CGEventTapProxy proxy, This should be a static ...

7 years, 4 months ago (2013-08-21 14:43:23 UTC) #17

jiayl

On 2013/08/21 14:43:23, rsesek wrote: > https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm > File media/base/user_input_monitor_mac.mm (right): > > https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm#newcode90 > ...

7 years, 4 months ago (2013-08-21 16:23:16 UTC) #18

Robert Sesek

On 2013/08/21 16:23:16, jiayl wrote: > On 2013/08/21 14:43:23, rsesek wrote: > > > https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm ...

7 years, 4 months ago (2013-08-21 16:29:58 UTC) #19

Wez

On 21 August 2013 09:29, <rsesek@chromium.org> wrote: > On 2013/08/21 16:23:16, jiayl wrote: > >> ...

7 years, 4 months ago (2013-08-21 17:03:02 UTC) #20

On 21 August 2013 09:29, <rsesek@chromium.org> wrote:

> On 2013/08/21 16:23:16, jiayl wrote:
>
>> On 2013/08/21 14:43:23, rsesek wrote:
>> >
>>
>
> https://codereview.chromium.**org/22801007/diff/26001/media/**
>
base/user_input_monitor_mac.mm<https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm>
>
>> > File
media/base/user_input_monitor_**mac.mm<http://user_input_monitor_mac.mm>(right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/22801007/diff/26001/media/**
>
base/user_input_monitor_mac.**mm#newcode90<https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm#newcode90>
>
>> >
media/base/user_input_monitor_**mac.mm:90<http://user_input_monitor_mac.mm:90>:
>> static CGEventRef
>> > inputEvent(CGEventTapProxy proxy,
>> > This should be a static member function.
>> >
>> The style guide doesn't say static member function is preferred to
>> nonmember
>> function in a namespace. And it doesn't depend on any static class data
>>
> member.
>
>> Why is static member function better? :)
>>
>
> It is the pattern to use for CG and CF callbacks in C++ code. The function
> is
> directly related to the class and it should be part of it.
>
>
>  >
>>
>
> https://codereview.chromium.**org/22801007/diff/26001/media/**
>
base/user_input_monitor_mac.**mm#newcode115<https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm#newcode115>
>
>> >
media/base/user_input_monitor_**mac.mm:115<http://user_input_monitor_mac.mm:115>:
>> void
>> > UserInputMonitorMac::**StartMouseMonitoring() {
>> > I'm wondering if this could be simplified s.t. rather than having two
>>
> classes,
>
>> > you just perform this check:
>> >
>> > void UserInputMonitorMac::**StartMouseMonitoring() {
>> >   if (!ui_task_runner_->**BelongsToCurentThread()) {
>> >     ui_task_runner_->PostTask(**FROM_HERE,
>> >         base::Bind(&**UserInputMonitorMac::**StartMouseMonitoring,
>> this));
>> >     return;
>> >   }
>> >
>> >   // Now running on UI thread.
>> > }
>> >
>> > I don't know what the best pattern here is.
>>
>
>  I think the current pattern makes it clear that UserInputMonitorMacCore
>> is a
>> single threaded class running on the UI thread and so minimizes the
>> chance of
>> accidental thread violation.
>>
>
> Searching the code base, what I suggested actually appears to be fairly
> common:
>
> https://code.google.com/p/**chromium/codesearch#search/&q=**
>
%22belongstocurrentthread())%**20%7B%22&sq=package:chromium&**type=cs<https://code.google.com/p/chromium/codesearch#search/&q=%22belongstocurrentthread())%20%7B%22&sq=package:chromium&type=cs>


The arrangement you describe requires that UserInputMonitorMac becomes
itself ref-counted rather than singly owned, which is not desirable.

Since in this implementation UserInputMonitorMac::Core just does work on
behalf of the main class, but on a different thread, it should be possible
for it to be owned by UserInputMonitorMac rather than ref-counted:
- UserInputMonitorMac holds a scoped_ptr<> to the Core.
- The Core uses a WeakPtrFactory to create WeakPtrs to itself to use with
Bind.
- In its destructor, UserInputMonitorMac calls
ui_task_runner->DeleteSoon(core_->release()) to teardown the Core safely.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

jiayl

On 2013/08/21 17:03:02, Wez wrote: > On 21 August 2013 09:29, <mailto:rsesek@chromium.org> wrote: > > ...

7 years, 4 months ago (2013-08-21 17:47:35 UTC) #21

On 2013/08/21 17:03:02, Wez wrote:
> On 21 August 2013 09:29, <mailto:rsesek@chromium.org> wrote:
> 
> > On 2013/08/21 16:23:16, jiayl wrote:
> >
> >> On 2013/08/21 14:43:23, rsesek wrote:
> >> >
> >>
> >
> > https://codereview.chromium.**org/22801007/diff/26001/media/**
> >
>
base/user_input_monitor_mac.mm<https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm>
> >
> >> > File
>
media/base/user_input_monitor_**mac.mm<http://user_input_monitor_mac.mm>(right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.**org/22801007/diff/26001/media/**
> >
>
base/user_input_monitor_mac.**mm#newcode90<https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm#newcode90>
> >
> >> >
>
media/base/user_input_monitor_**mac.mm:90<http://user_input_monitor_mac.mm:90>:
> >> static CGEventRef
> >> > inputEvent(CGEventTapProxy proxy,
> >> > This should be a static member function.
> >> >
> >> The style guide doesn't say static member function is preferred to
> >> nonmember
> >> function in a namespace. And it doesn't depend on any static class data
> >>
> > member.
> >
> >> Why is static member function better? :)
> >>
> >
> > It is the pattern to use for CG and CF callbacks in C++ code. The function
> > is
> > directly related to the class and it should be part of it.
> >
> >
> >  >
> >>
> >
> > https://codereview.chromium.**org/22801007/diff/26001/media/**
> >
>
base/user_input_monitor_mac.**mm#newcode115<https://codereview.chromium.org/22801007/diff/26001/media/base/user_input_monitor_mac.mm#newcode115>
> >
> >> >
>
media/base/user_input_monitor_**mac.mm:115<http://user_input_monitor_mac.mm:115>:
> >> void
> >> > UserInputMonitorMac::**StartMouseMonitoring() {
> >> > I'm wondering if this could be simplified s.t. rather than having two
> >>
> > classes,
> >
> >> > you just perform this check:
> >> >
> >> > void UserInputMonitorMac::**StartMouseMonitoring() {
> >> >   if (!ui_task_runner_->**BelongsToCurentThread()) {
> >> >     ui_task_runner_->PostTask(**FROM_HERE,
> >> >         base::Bind(&**UserInputMonitorMac::**StartMouseMonitoring,
> >> this));
> >> >     return;
> >> >   }
> >> >
> >> >   // Now running on UI thread.
> >> > }
> >> >
> >> > I don't know what the best pattern here is.
> >>
> >
> >  I think the current pattern makes it clear that UserInputMonitorMacCore
> >> is a
> >> single threaded class running on the UI thread and so minimizes the
> >> chance of
> >> accidental thread violation.
> >>
> >
> > Searching the code base, what I suggested actually appears to be fairly
> > common:
> >
> > https://code.google.com/p/**chromium/codesearch#search/&q=**
> >
>
%22belongstocurrentthread())%**20%7B%22&sq=package:chromium&**type=cs<https://code.google.com/p/chromium/codesearch#search/&q=%22belongstocurrentthread())%20%7B%22&sq=package:chromium&type=cs>
> 
> 
> The arrangement you describe requires that UserInputMonitorMac becomes
> itself ref-counted rather than singly owned, which is not desirable.
> 
> Since in this implementation UserInputMonitorMac::Core just does work on
> behalf of the main class, but on a different thread, it should be possible
> for it to be owned by UserInputMonitorMac rather than ref-counted:
> - UserInputMonitorMac holds a scoped_ptr<> to the Core.
> - The Core uses a WeakPtrFactory to create WeakPtrs to itself to use with
> Bind.
> - In its destructor, UserInputMonitorMac calls
> ui_task_runner->DeleteSoon(core_->release()) to teardown the Core safely.
> 
Nice! Done.

> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Mark Mentovai

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_monitor.cc File media/base/user_input_monitor.cc (right): https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_monitor.cc#newcode89 media/base/user_input_monitor.cc:89: DCHECK(false); Oh, so NOTREACHED? You still should write “return;” ...

7 years, 4 months ago (2013-08-21 18:32:42 UTC) #23

Wez

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_monitor_mac.mm File media/base/user_input_monitor_mac.mm (right): https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_monitor_mac.mm#newcode232 media/base/user_input_monitor_mac.mm:232: base::TimeDelta::FromMilliseconds(10)); On 2013/08/21 18:32:43, Mark Mentovai wrote: > 1. ...

7 years, 4 months ago (2013-08-21 19:45:56 UTC) #24

jiayl

On 2013/08/21 19:45:56, Wez wrote: > https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_monitor_mac.mm > File media/base/user_input_monitor_mac.mm (right): > > https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_monitor_mac.mm#newcode232 > ...

7 years, 4 months ago (2013-08-21 20:08:08 UTC) #25

Wez

https://codereview.chromium.org/22801007/diff/43001/media/base/user_input_monitor_mac.cc File media/base/user_input_monitor_mac.cc (right): https://codereview.chromium.org/22801007/diff/43001/media/base/user_input_monitor_mac.cc#newcode247 media/base/user_input_monitor_mac.cc:247: CGEventSourceKeyState(kCGEventSourceStateHIDSystemState, key_index); This should be *SessionState, not *SystemState, I ...

7 years, 4 months ago (2013-08-21 22:41:56 UTC) #26

jiayl

PTAL https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_monitor.cc File media/base/user_input_monitor.cc (right): https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_monitor.cc#newcode89 media/base/user_input_monitor.cc:89: DCHECK(false); On 2013/08/21 18:32:43, Mark Mentovai wrote: > ...

7 years, 4 months ago (2013-08-21 22:58:22 UTC) #27

PTAL

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
File media/base/user_input_monitor.cc (right):

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor.cc:89: DCHECK(false);
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> Oh, so NOTREACHED?
> 
> You still should write “return;” after this. DCHECKs aren’t anything in
release
> builds, and even in debug builds, the compiler doesn’t know that the rest of
the
> block is unreachable. There’s a slight object code size reduction from writing
> the return.

Done.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
File media/base/user_input_monitor_mac.mm (right):

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:7: #import <AppKit/AppKit.h>
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> I can’t tell what you’ve included this for. In fact, there doesn’t seem to be
> any Objective-C in this file at all. It could be a .cc.
removed

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:18: #import
"third_party/GTM/AppKit/GTMCarbonEvent.h"
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> I can’t tell what you’ve included this for either. Same thing as the previous
> comment.
removed

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:25: class UserInputMonitorMac : public
base::NonThreadSafe,
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> …so this is the implementation class?
Merged with *Core now.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:44: // Task runner on which X Window events
are received.
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> X Window events? Really?
fixed

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:52: // The class implements the event
listening. Must be called on the UI thread.
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> …no, wait, THIS is the implementation class? Why? Why did you split this up
into
> two classes? They’re both private to this file and one simply proxies things
to
> the other. Why are there two? Just to make sure that things happen on the
right
> thread? What if the calls come in from the right thread to begin with? Then
> you’re doing unnecessary work to dispatch a new task from the message loop.

Done.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:52: // The class implements the event
listening. Must be called on the UI thread.
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> You need to assert what this comment says with DCHECKs in the code.

Done.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:58: virtual ~UserInputMonitorMacCore();
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> This destructor doesn’t need to be virtual.

Done.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:80: base::ScopedCFTypeRef<CFMachPortRef>
input_mach_port_;
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> Why is this in a scoper but input_run_loop_source_ bare? Their lifetimes seem
to
> be identical. I happen to like the scoper.

Done.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:83: // 0x5d is key "9", after that comes
function keys.
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> And why don’t you care about function keys?
> 
> "9" is 0x5c. 0x5d comes after 9, so the array bound needs to be [0x5d] in
order
> to accommodate "9". The comment is wrong.
> 
> However, this is only appropriate for “ANSI” keyboards (such as US keyboards).
> It is ignorant of other keyboard types. For example, I see from
> Carbon/HIToolbox/Events.h that JIS (Japanese) keyboards generate virtual codes
> higher than 0x5c for printable characters.

I'd be consistent with the webrtc code:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

I filed https://code.google.com/p/webrtc/issues/detail?id=2282 to see if this
should be changed. If so, both places should be fixed.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:150: void
UserInputMonitorMacCore::StartMonitoringMouse() {
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> Some or all of these functions need to run on the main thread. You should
DCHECK
> that you’re on the right thread.

Done.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:180:
base::TimeDelta::FromMilliseconds(10));
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> And why can’t you use an event tap for this?

Due to permission issues stated before.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:200: if (pid == 0) {
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> Why? What’s 0 mean here? You need a comment to explain this.

Done.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:201: DCHECK(type == kCGEventMouseMoved);
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:214: for (uint key_index = 0;
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> CGKeyCode, not uint.

Done.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:215: key_index <=
sizeof(prev_key_state_)/sizeof(prev_key_state_[0]);
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> This is arraysize, you don’t need to write the cumbersome expression on your
> own.

Done.

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:217: bool key_state =
CGEventSourceKeyState(kCGEventSourceStateHIDSystemState,
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> This is a lot of IPC to the window server you’re planning on doing really
often.
> Have you investigated optimizing it by calling
CGEventSourceCounterForEventType
> to see if the counter has changed? If unchanged, there’s no reason to enter
this
> loop, you know no new keys are pressed.

I tried both CGEventSourceCounterForEventType and
CGEventSourceSecondsSinceLastEventType, but they both have a problem in not
reporting existence of event if the same key is pressed consecutively (not held
down).

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:232:
base::TimeDelta::FromMilliseconds(10));
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> 1. I think this business of polling is nonsense.
> 
Unfortunately I don't have a better solution.

> 2. I think that if you’re going to poll, a repeating timer is probably better
> than something that just fires new delayed tasks from each task.
> 
Done.

> 3. If you’re going to fire new delayed tasks from each task, you should do
> something to unify how you fire the first one and subsequent ones. Why can’t
you
> just call StartPollingKeyState from here, for example? It’s the same exact
> thing. At the very least, use a constant to keep the 10s uniform.
> 

> 4. 10ms seems way too fast for what you’re doing.
Changed to 30ms. Not seeing misses when sweeping the keys quickly.

https://codereview.chromium.org/22801007/diff/43001/media/base/user_input_mon...
File media/base/user_input_monitor_mac.cc (right):

https://codereview.chromium.org/22801007/diff/43001/media/base/user_input_mon...
media/base/user_input_monitor_mac.cc:247:
CGEventSourceKeyState(kCGEventSourceStateHIDSystemState, key_index);
On 2013/08/21 22:41:57, Wez wrote:
> This should be *SessionState, not *SystemState, I think.
The webrtc impl uses *SystemState:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

We should only care about hardware events.

Wez

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_monitor_mac.mm File media/base/user_input_monitor_mac.mm (right): https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_monitor_mac.mm#newcode52 media/base/user_input_monitor_mac.mm:52: // The class implements the event listening. Must be ...

7 years, 4 months ago (2013-08-21 23:19:52 UTC) #28

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
File media/base/user_input_monitor_mac.mm (right):

https://codereview.chromium.org/22801007/diff/33001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:52: // The class implements the event
listening. Must be called on the UI thread.
On 2013/08/21 18:32:43, Mark Mentovai wrote:
> …no, wait, THIS is the implementation class? Why? Why did you split this up
into
> two classes? They’re both private to this file and one simply proxies things
to
> the other. Why are there two? Just to make sure that things happen on the
right
> thread? What if the calls come in from the right thread to begin with? Then
> you’re doing unnecessary work to dispatch a new task from the message loop.

The UserInputMonitor[Mac] is owned by code on the calling thread, and may be
deleted by that caller at any time, so if work needs to be done on a background
thread then none of that work can touch the UserInputMonitorMac, since there's
no guarantee that it hasn't been deleted in the meantime.

https://codereview.chromium.org/22801007/diff/43001/media/base/user_input_mon...
File media/base/user_input_monitor_mac.cc (right):

https://codereview.chromium.org/22801007/diff/43001/media/base/user_input_mon...
media/base/user_input_monitor_mac.cc:247:
CGEventSourceKeyState(kCGEventSourceStateHIDSystemState, key_index);
On 2013/08/21 22:58:23, jiayl wrote:
> On 2013/08/21 22:41:57, Wez wrote:
> > This should be *SessionState, not *SystemState, I think.
> The webrtc impl uses *SystemState:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> We should only care about hardware events.

Monitoring session events is correct in terms of scope - if the user session is
switched out then you don't want to be monitoring the console keyboard for
events - and non-hardware keyboard events are probably not common enough for
capturing them as well to be a problem, surely?

https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_moni...
File media/base/user_input_monitor_mac.cc (right):

https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_moni...
media/base/user_input_monitor_mac.cc:86: base::Unretained(this)));
This code will now crash if the calling thread e.g. calls
StartMouseMonitoring(), then deletes the UserInputMonitor, since tasks will be
executed against it on the UI thread after it has been deleted.  You need the
wrapper/core separation.

jiayl

https://codereview.chromium.org/22801007/diff/43001/media/base/user_input_monitor_mac.cc File media/base/user_input_monitor_mac.cc (right): https://codereview.chromium.org/22801007/diff/43001/media/base/user_input_monitor_mac.cc#newcode247 media/base/user_input_monitor_mac.cc:247: CGEventSourceKeyState(kCGEventSourceStateHIDSystemState, key_index); On 2013/08/21 23:19:53, Wez wrote: > On ...

7 years, 4 months ago (2013-08-21 23:32:59 UTC) #29

https://codereview.chromium.org/22801007/diff/43001/media/base/user_input_mon...
File media/base/user_input_monitor_mac.cc (right):

https://codereview.chromium.org/22801007/diff/43001/media/base/user_input_mon...
media/base/user_input_monitor_mac.cc:247:
CGEventSourceKeyState(kCGEventSourceStateHIDSystemState, key_index);
On 2013/08/21 23:19:53, Wez wrote:
> On 2013/08/21 22:58:23, jiayl wrote:
> > On 2013/08/21 22:41:57, Wez wrote:
> > > This should be *SessionState, not *SystemState, I think.
> > The webrtc impl uses *SystemState:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> > 
> > We should only care about hardware events.
> 
> Monitoring session events is correct in terms of scope - if the user session
is
> switched out then you don't want to be monitoring the console keyboard for
> events - and non-hardware keyboard events are probably not common enough for
> capturing them as well to be a problem, surely?

I'm not sure. What's the state of audio capturing when a user session is
switched out? If it's still capturing, we should keep monitoring the keyboard. 
I tried locking my workstation while in a video call and audio/video was still
being recorded.

https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_moni...
File media/base/user_input_monitor_mac.cc (right):

https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_moni...
media/base/user_input_monitor_mac.cc:86: base::Unretained(this)));
On 2013/08/21 23:19:53, Wez wrote:
> This code will now crash if the calling thread e.g. calls
> StartMouseMonitoring(), then deletes the UserInputMonitor, since tasks will be
> executed against it on the UI thread after it has been deleted.  You need the
> wrapper/core separation.
UserInputMonitor should always outlive the UI/IO threads due to their
declaration order in BrowserMainLoop. This is also documented in the comment in
user_input_monitor.h. So there is no crash.
The wrapper/core separation does not help actually because the core previously
called into the wrapper's callback with the wrapper "Unretained".

Wez

https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_monitor_mac.cc File media/base/user_input_monitor_mac.cc (right): https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_monitor_mac.cc#newcode86 media/base/user_input_monitor_mac.cc:86: base::Unretained(this))); On 2013/08/21 23:32:59, jiayl wrote: > On 2013/08/21 ...

7 years, 4 months ago (2013-08-21 23:43:53 UTC) #30

jiayl

On 2013/08/21 23:43:53, Wez wrote: > https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_monitor_mac.cc > File media/base/user_input_monitor_mac.cc (right): > > https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_monitor_mac.cc#newcode86 > ...

7 years, 4 months ago (2013-08-21 23:47:33 UTC) #31

Wez

On 2013/08/21 23:47:33, jiayl wrote: > On 2013/08/21 23:43:53, Wez wrote: > > > https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_monitor_mac.cc ...

7 years, 4 months ago (2013-08-22 00:11:50 UTC) #32

jiayl

The BrowserMainLoop owned instance is always released on the main thread, but the API does ...

7 years, 4 months ago (2013-08-22 00:23:27 UTC) #33

The BrowserMainLoop owned instance is always released on the main thread,
but the API does not restrict that.

Even the API requires it to be released on the UI thread, it won't work on
Linux where the callbacks are made on the IO thread, i.e. the requirement
of outliving IO thread is still needed.

If Chroming removes all listeners first and then post to UI/IO thread to
delete UserInputMonitor, it should also work.


On Wed, Aug 21, 2013 at 5:11 PM, <wez@chromium.org> wrote:

> On 2013/08/21 23:47:33, jiayl wrote:
>
>> On 2013/08/21 23:43:53, Wez wrote:
>> >
>>
>
> https://codereview.chromium.**org/22801007/diff/9006/media/**
>
base/user_input_monitor_mac.cc<https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_monitor_mac.cc>
>
>> > File media/base/user_input_monitor_**mac.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/22801007/diff/9006/media/**
>
base/user_input_monitor_mac.**cc#newcode86<https://codereview.chromium.org/22801007/diff/9006/media/base/user_input_monitor_mac.cc#newcode86>
>
>> > media/base/user_input_monitor_**mac.cc:86: base::Unretained(this)));
>> > On 2013/08/21 23:32:59, jiayl wrote:
>> > > On 2013/08/21 23:19:53, Wez wrote:
>> > > > This code will now crash if the calling thread e.g. calls
>> > > > StartMouseMonitoring(), then deletes the UserInputMonitor, since
>> tasks
>> will
>> > be
>> > > > executed against it on the UI thread after it has been deleted.  You
>>
> need
>
>> > the
>> > > > wrapper/core separation.
>> > > UserInputMonitor should always outlive the UI/IO threads due to their
>> > > declaration order in BrowserMainLoop. This is also documented in the
>>
> comment
>
>> > in
>> > > user_input_monitor.h. So there is no crash.
>> >
>> > That's specific to the BrowserMainLoop caller - I thought we intended
>> this
>>
> to
>
>> be
>> > re-usable for Chromoting as well?
>> >
>> I think for Chromting we also need to find a host that can meet this
>> requirement.
>>
>
> Chromoting's threading model is a little different from Chrome's and we
> try hard
> not to rely on objects outliving threads for provide safe teardown.
>
>
>  > > The wrapper/core separation does not help actually because the core
>> previously
>> > > called into the wrapper's callback with the wrapper "Unretained".
>> >
>> > You'd use a WeakPtr back to the wrapper in that case.
>> But there is threading issue: UserInputMonitor can be deleted on any
>> thread,
>>
> not
>
>> guaranteed on the UI thread. Invalidating and dereferencing WeakPtr on
>>
> different
>
>> threads does not work.
>>
>
> Oh. That seems strange. How come it can be deleted on any thread?
>
>
>
https://codereview.chromium.**org/22801007/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Mark Mentovai

Taking a step back… If all you really need to know is when a key ...

7 years, 4 months ago (2013-08-22 00:48:22 UTC) #34

jiayl

Example: step 1: press and release a key; step 2: after 1 second of step ...

7 years, 4 months ago (2013-08-22 00:59:19 UTC) #35

Mark Mentovai

It works correctly for me: mark@cougar bash$ cat tidle.cc #include <ApplicationServices/ApplicationServices.h> #include <stdio.h> int main(int ...

7 years, 4 months ago (2013-08-22 01:37:05 UTC) #36

jiayl

Thanks for the info! You are right; it was something else causing the missing events ...

7 years, 4 months ago (2013-08-22 16:45:02 UTC) #37

Mark Mentovai

I still think you may be overcomplicating this, perhaps to fit into the current interface ...

7 years, 4 months ago (2013-08-22 17:02:13 UTC) #38

I still think you may be overcomplicating this, perhaps to fit into the
current interface that’s in Chrome that you’re trying to code to.

The high-level problem as I understand it is “find out if the user is
interacting with the keyboard or mouse, and if they are, mute the
microphone.”

In order to implement this in the most simple and reliable way, I don’t
think you need to distinguish keyboard events from mouse events, and I
don’t think you need to do anything to count events. You don’t care what
the keys are, you don’t care whether they produce characters or move
insertion points or act as modifiers, and you don’t care where the mouse
cursor is pointing. I think that all you need to do is periodically check
to see if the user’s performing any input activity.

Really, all you need is a periodic call to
CGEventSourceSecondsSinceLastEventType,
and all you need to look for is kCGAnyInputEventType. If the result is near
zero, the user is actively doing something, and you can auto-mute the
microphone.

This business about distinguishing key repeats from distinct keypresses
seems very nitpicky to me. A key that’s down is still input that the user
is providing. I don’t think it’s a problem to treat it as input for the
purposes of this interface.

If you take a step back and imagine the best way to implement this on the
Mac with the primitives available, and use that as a jumping-off point for
your implementation, I think that the result will be much more elegant.
Chrome’s current internal interfaces for this may not make sense on the
Mac. That’s fine, we can change Chrome code.

On Thu, Aug 22, 2013 at 12:44 PM, Jiayang Liu <jiayl@chromium.org> wrote:

> Thanks for the info! You are right; it was something else causing the
> missing events in my previous testing.
>
> I tested CGEventSourceCounterForEventType
> and CGEventSourceSecondsSinceLastEventType and they both work fine for
> repeatedly pressing one key.
>
> I think CGEventSourceCounterForEventType will serve us better since it
> does not count auto-repeated events, i.e. holding down a key is counted as
> one key_down event, while CGEventSourceSecondsSinceLastEventType resets the
> interval for every auto-repeated key presses.
>
> Since we don't need to query all key states anymore, we could call
> CGEventSourceCounterForEventType in every audio callback instead of
> periodical polling. This should give us more accurate results since the key
> press state is now sync'd with the audio data.
>
>
>
>
> On Wed, Aug 21, 2013 at 6:37 PM, <mark@chromium.org> wrote:
>
>> It works correctly for me:
>>
>> mark@cougar bash$ cat tidle.cc
>> #include <ApplicationServices/**ApplicationServices.h>
>> #include <stdio.h>
>>
>> int main(int argc, char* argv[]) {
>>   while (true) {
>>     CFTimeInterval i = CGEventSourceSecondsSinceLastE**ventType(
>>         kCGEventSourceStateCombinedSes**sionState,
>>         kCGAnyInputEventType);
>>     printf("%.1fs\n", i);
>>     sleep(1);
>>   }
>>
>>   return 0;
>> }
>> mark@cougar bash$ clang++ tidle.cc -o tidle -framework
>> ApplicationServices
>> mark@cougar bash$ ./tidle
>> 0.0s
>> 0.9s
>> 1.9s
>> 2.9s
>> a0.4s
>> 1.4s
>> a0.4s
>> 1.4s
>> 2.4s
>> a0.5s
>> a0.4s
>> a0.4s
>> a0.2s
>> 1.2s
>> 2.3s
>> 3.3s
>> b0.9s
>> a0.1s
>> 1.1s
>> ^C
>> mark@cougar bash$
>>
>> You can see above where I typed letters, and when I hit the same key
>> repeatedly,
>> the time-since-last-event went back to 0.
>>
>>
>>
https://codereview.chromium.**org/22801007/<https://codereview.chromium.org/2...
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

jiayl

I don't agree. The typing detection algorithm in the voice engine expects the input to ...

7 years, 4 months ago (2013-08-22 17:22:27 UTC) #39

I don't agree. The typing detection algorithm in the voice engine expects
the input to be "whether the user has pressed a key in the last sampling
period" and if so it tries to find a volume peak in the audio data to
decide whether the typing activity has affected recording. We don't want
false positives when the user is talking while moving the mouse or when
typing is not affecting recording.

Calling CGEventSourceSecondsSinceLastEventType much less often than audio
sampling will lead to inaccurate results too, since trying to match the
volume peak in last 8ms audio data while the key press might happen 100ms
ago has no point.





On Thu, Aug 22, 2013 at 10:02 AM, Mark Mentovai <mark@chromium.org> wrote:

> I still think you may be overcomplicating this, perhaps to fit into the
> current interface that’s in Chrome that you’re trying to code to.
>
> The high-level problem as I understand it is “find out if the user is
> interacting with the keyboard or mouse, and if they are, mute the
> microphone.”
>
> In order to implement this in the most simple and reliable way, I don’t
> think you need to distinguish keyboard events from mouse events, and I
> don’t think you need to do anything to count events. You don’t care what
> the keys are, you don’t care whether they produce characters or move
> insertion points or act as modifiers, and you don’t care where the mouse
> cursor is pointing. I think that all you need to do is periodically check
> to see if the user’s performing any input activity.
>
> Really, all you need is a periodic call to
CGEventSourceSecondsSinceLastEventType,
> and all you need to look for is kCGAnyInputEventType. If the result is
> near zero, the user is actively doing something, and you can auto-mute the
> microphone.
>
> This business about distinguishing key repeats from distinct keypresses
> seems very nitpicky to me. A key that’s down is still input that the user
> is providing. I don’t think it’s a problem to treat it as input for the
> purposes of this interface.
>
> If you take a step back and imagine the best way to implement this on the
> Mac with the primitives available, and use that as a jumping-off point for
> your implementation, I think that the result will be much more elegant.
> Chrome’s current internal interfaces for this may not make sense on the
> Mac. That’s fine, we can change Chrome code.
>
>
> On Thu, Aug 22, 2013 at 12:44 PM, Jiayang Liu <jiayl@chromium.org> wrote:
>
>> Thanks for the info! You are right; it was something else causing the
>> missing events in my previous testing.
>>
>> I tested CGEventSourceCounterForEventType
>> and CGEventSourceSecondsSinceLastEventType and they both work fine for
>> repeatedly pressing one key.
>>
>> I think CGEventSourceCounterForEventType will serve us better since it
>> does not count auto-repeated events, i.e. holding down a key is counted as
>> one key_down event, while CGEventSourceSecondsSinceLastEventType resets the
>> interval for every auto-repeated key presses.
>>
>> Since we don't need to query all key states anymore, we could call
>> CGEventSourceCounterForEventType in every audio callback instead of
>> periodical polling. This should give us more accurate results since the key
>> press state is now sync'd with the audio data.
>>
>>
>>
>>
>> On Wed, Aug 21, 2013 at 6:37 PM, <mark@chromium.org> wrote:
>>
>>> It works correctly for me:
>>>
>>> mark@cougar bash$ cat tidle.cc
>>> #include <ApplicationServices/**ApplicationServices.h>
>>> #include <stdio.h>
>>>
>>> int main(int argc, char* argv[]) {
>>>   while (true) {
>>>     CFTimeInterval i = CGEventSourceSecondsSinceLastE**ventType(
>>>         kCGEventSourceStateCombinedSes**sionState,
>>>         kCGAnyInputEventType);
>>>     printf("%.1fs\n", i);
>>>     sleep(1);
>>>   }
>>>
>>>   return 0;
>>> }
>>> mark@cougar bash$ clang++ tidle.cc -o tidle -framework
>>> ApplicationServices
>>> mark@cougar bash$ ./tidle
>>> 0.0s
>>> 0.9s
>>> 1.9s
>>> 2.9s
>>> a0.4s
>>> 1.4s
>>> a0.4s
>>> 1.4s
>>> 2.4s
>>> a0.5s
>>> a0.4s
>>> a0.4s
>>> a0.2s
>>> 1.2s
>>> 2.3s
>>> 3.3s
>>> b0.9s
>>> a0.1s
>>> 1.1s
>>> ^C
>>> mark@cougar bash$
>>>
>>> You can see above where I typed letters, and when I hit the same key
>>> repeatedly,
>>> the time-since-last-event went back to 0.
>>>
>>>
>>>
https://codereview.chromium.**org/22801007/<https://codereview.chromium.org/2...
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

jiayl

PTAL. The change is reduced a lot and does not touch UserInputMonitor at all.

7 years, 4 months ago (2013-08-22 17:37:26 UTC) #40

Mark Mentovai

Yes, this is substantially clearer now. https://codereview.chromium.org/22801007/diff/62001/media/audio/audio_input_controller.cc File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/22801007/diff/62001/media/audio/audio_input_controller.cc#newcode343 media/audio/audio_input_controller.cc:343: uint32_t current_count = ...

7 years, 4 months ago (2013-08-22 18:27:51 UTC) #41

jiayl

PTAL. Thanks! https://codereview.chromium.org/22801007/diff/62001/media/audio/audio_input_controller.cc File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/22801007/diff/62001/media/audio/audio_input_controller.cc#newcode343 media/audio/audio_input_controller.cc:343: uint32_t current_count = CGEventSourceCounterForEventType( On 2013/08/22 18:27:52, ...

7 years, 4 months ago (2013-08-22 18:51:16 UTC) #42

Mark Mentovai

LGTM but obviously you should get a media OWNER to weigh in on this again, ...

7 years, 4 months ago (2013-08-22 19:00:25 UTC) #43

DaleCurtis

My concern with this is three fold: 1. You're calling into the Quartz API during ...

7 years, 4 months ago (2013-08-22 19:48:51 UTC) #44

Robert Sesek

I was somehow from this review after comment #39. Please do not remove reviewers in ...

7 years, 4 months ago (2013-08-22 20:45:15 UTC) #45

jiayl

Fixed Dale's comments by adding the GetKeyPressCount API for all platforms. PTAL.

7 years, 4 months ago (2013-08-22 21:41:18 UTC) #46

DaleCurtis

You'll need mark@ and rsesek@ approval for the OSX stuff you added back. Of which, ...

7 years, 4 months ago (2013-08-22 22:07:34 UTC) #47

jiayl

On 2013/08/22 22:07:34, DaleCurtis wrote: > You'll need mark@ and rsesek@ approval for the OSX ...

7 years, 4 months ago (2013-08-22 22:23:01 UTC) #48

jiayl

On 2013/08/22 22:07:34, DaleCurtis wrote: > You'll need mark@ and rsesek@ approval for the OSX ...

7 years, 4 months ago (2013-08-22 22:23:03 UTC) #49

jiayl

PTAL https://codereview.chromium.org/22801007/diff/68001/media/audio/audio_input_controller.cc File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/22801007/diff/68001/media/audio/audio_input_controller.cc#newcode246 media/audio/audio_input_controller.cc:246: user_input_monitor_->AddKeyPressCounterReference(); On 2013/08/22 22:07:34, DaleCurtis wrote: > How ...

7 years, 4 months ago (2013-08-22 22:23:21 UTC) #50

DaleCurtis

https://codereview.chromium.org/22801007/diff/68001/media/audio/audio_input_controller.cc File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/22801007/diff/68001/media/audio/audio_input_controller.cc#newcode246 media/audio/audio_input_controller.cc:246: user_input_monitor_->AddKeyPressCounterReference(); On 2013/08/22 22:23:22, jiayl wrote: > On 2013/08/22 ...

7 years, 4 months ago (2013-08-23 01:13:37 UTC) #51

https://codereview.chromium.org/22801007/diff/68001/media/audio/audio_input_c...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/22801007/diff/68001/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:246:
user_input_monitor_->AddKeyPressCounterReference();
On 2013/08/22 22:23:22, jiayl wrote:
> On 2013/08/22 22:07:34, DaleCurtis wrote:
> > How about just EnableKeyPressMonitoring(), callers don't need to know that
> this
> > occurs via a reference counting system.
> 
> I don't want to give the caller a wrong idea that the monitoring is started or
> stopped by its call, but shared with other callers. 

KeyPressCounterReference is opaque to a reader without looking at
UserInputMonitor.  Seeing EnableKeyPressMonitoring() is much clearer.

I don't think it's important to callers to know what UserInputMonitor is doing
behind the scenes.  Especially since Chrome already uses the concept of lazy
operators all over (scoped_refptr, etc).

https://codereview.chromium.org/22801007/diff/68001/media/base/user_input_mon...
File media/base/user_input_monitor.h (left):

https://codereview.chromium.org/22801007/diff/68001/media/base/user_input_mon...
media/base/user_input_monitor.h:34: class MEDIA_EXPORT MouseEventListener {
On 2013/08/22 22:23:22, jiayl wrote:
> On 2013/08/22 22:07:34, DaleCurtis wrote:
> > Have you sat down with sergeyu@ and wez@ to figure out if UserInputMonitor
is
> > going to work for their use case?  If not, please do so offline, as there's
> > quite a lot of unused code behind the mouse movement monitoring.
> 
> Sergey reviewed the last CL that added these APIs.

Not really the same thing. :) Since you introduced these classes and the usage
model, I'd like to see you take ownership of the larger problem.  Part of that
means coordinating with remoting/ to figure out a long term plan.

At the very least there should be a bug filed.

https://codereview.chromium.org/22801007/diff/72001/media/base/user_input_mon...
File media/base/user_input_monitor_mac.cc (right):

https://codereview.chromium.org/22801007/diff/72001/media/base/user_input_mon...
media/base/user_input_monitor_mac.cc:7: #include
<ApplicationServices/ApplicationServices.h>
Remove?

jiayl

PTAL https://codereview.chromium.org/22801007/diff/68001/media/audio/audio_input_controller.cc File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/22801007/diff/68001/media/audio/audio_input_controller.cc#newcode246 media/audio/audio_input_controller.cc:246: user_input_monitor_->AddKeyPressCounterReference(); On 2013/08/23 01:13:38, DaleCurtis wrote: > On ...

7 years, 4 months ago (2013-08-23 01:49:32 UTC) #52

PTAL

https://codereview.chromium.org/22801007/diff/68001/media/audio/audio_input_c...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/22801007/diff/68001/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:246:
user_input_monitor_->AddKeyPressCounterReference();
On 2013/08/23 01:13:38, DaleCurtis wrote:
> On 2013/08/22 22:23:22, jiayl wrote:
> > On 2013/08/22 22:07:34, DaleCurtis wrote:
> > > How about just EnableKeyPressMonitoring(), callers don't need to know that
> > this
> > > occurs via a reference counting system.
> > 
> > I don't want to give the caller a wrong idea that the monitoring is started
or
> > stopped by its call, but shared with other callers. 
> 
> KeyPressCounterReference is opaque to a reader without looking at
> UserInputMonitor.  Seeing EnableKeyPressMonitoring() is much clearer.
> 
> I don't think it's important to callers to know what UserInputMonitor is doing
> behind the scenes.  Especially since Chrome already uses the concept of lazy
> operators all over (scoped_refptr, etc).

Done.

https://codereview.chromium.org/22801007/diff/68001/media/base/user_input_mon...
File media/base/user_input_monitor.h (left):

https://codereview.chromium.org/22801007/diff/68001/media/base/user_input_mon...
media/base/user_input_monitor.h:34: class MEDIA_EXPORT MouseEventListener {
On 2013/08/23 01:13:38, DaleCurtis wrote:
> On 2013/08/22 22:23:22, jiayl wrote:
> > On 2013/08/22 22:07:34, DaleCurtis wrote:
> > > Have you sat down with sergeyu@ and wez@ to figure out if UserInputMonitor
> is
> > > going to work for their use case?  If not, please do so offline, as
there's
> > > quite a lot of unused code behind the mouse movement monitoring.
> > 
> > Sergey reviewed the last CL that added these APIs.
> 
> Not really the same thing. :) Since you introduced these classes and the usage
> model, I'd like to see you take ownership of the larger problem.  Part of that
> means coordinating with remoting/ to figure out a long term plan.
> 
> At the very least there should be a bug filed.

Yes, there is a plan to migrate remoting to use this class too. Filed
https://code.google.com/p/chromium/issues/detail?id=277906

https://codereview.chromium.org/22801007/diff/72001/media/base/user_input_mon...
File media/base/user_input_monitor_mac.cc (right):

https://codereview.chromium.org/22801007/diff/72001/media/base/user_input_mon...
media/base/user_input_monitor_mac.cc:7: #include
<ApplicationServices/ApplicationServices.h>
On 2013/08/23 01:13:38, DaleCurtis wrote:
> Remove?
Still needed for the CGEvent API

DaleCurtis

lgtm % nits. https://codereview.chromium.org/22801007/diff/33002/media/audio/audio_input_controller.h File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/22801007/diff/33002/media/audio/audio_input_controller.h#newcode287 media/audio/audio_input_controller.h:287: uint32_t prev_key_down_count_; size_t https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_monitor.cc File media/base/user_input_monitor.cc ...

7 years, 4 months ago (2013-08-23 02:16:21 UTC) #53

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/22801007/33002

7 years, 4 months ago (2013-08-23 16:49:13 UTC) #55

Mark Mentovai

The interface/implementation confusion needs to be cleaned up for sure. The other comments are much ...

7 years, 4 months ago (2013-08-23 18:09:02 UTC) #56

The interface/implementation confusion needs to be cleaned up for sure. The
other comments are much more trivial.

https://codereview.chromium.org/22801007/diff/33002/media/audio/audio_input_c...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/22801007/diff/33002/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:348: key_pressed = (current_count !=
prev_key_down_count_);
Outer (parentheses) are unnecessary.

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
File media/base/user_input_monitor.h (right):

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
media/base/user_input_monitor.h:59: // The callers must call
EnableKeyPressMonitoring before calling
“A caller must call”

That clarifies that it’s OK for more than one caller to call Enable.

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
media/base/user_input_monitor.h:64: // Returns the number of Key presses since
the first KeyPressCounterReference
Let’s get a blank line before this, because otherwise it sort of bleeds into the
declarations that come before it.

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
media/base/user_input_monitor.h:64: // Returns the number of Key presses since
the first KeyPressCounterReference
Key is not a proper noun, it shouldn’t be capitalized.

Actually, “keypresses” is fine as a single compound word.

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
media/base/user_input_monitor.h:64: // Returns the number of Key presses since
the first KeyPressCounterReference
This comment introduces the concept of a KeyPressCounterReference but doesn’t
explain it, and that word isn’t found anywhere else in this file. Revise the
comment. It should probably say something about the first
EnableKeyPressMonitoring instead.

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
media/base/user_input_monitor.h:65: // is added. Auto-repeated key presses are
not counted.
It’s fine if the implementation doesn’t count auto-repeated keypresses, but I
don’t think that this is a very important feature of GetKeyPressCount, and I
don’t think that it should be mentioned here. It’d be totally fine, I think, if
an implementation did count auto-repeated keypresses.

This is the sort of thing that you can mention at the actual implementation
site.

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
media/base/user_input_monitor.h:82: mutable base::Lock lock_;
What sort of constness is changing that requires this now?

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
media/base/user_input_monitor.h:88: std::set<int> pressed_keys_;
This is irrelevant on at least one platform now.

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
File media/base/user_input_monitor_mac.cc (right):

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
media/base/user_input_monitor_mac.cc:12: class UserInputMonitorMac : public
UserInputMonitor {
This is a total mess.

You’ve got UserInputMonitor that’s partially an interface and partially an
implementation. On any given platform, some of the UserInputMonitor
implementation is active, and some is totally masked. This applies to both code
and data: on Mac, GetKeyPressCount also fully overrides the base version. The
base’s pressed_keys_ and total_key_presses_ member variables are entirely
unused. The base’s monitoring_mouse_ seems to be totally irrelevant because
although the code in the base that might change this member variable is
reachable, it will trigger a NOTREACHED any time the value might change.

There’s a code complexity penalty with this design, there’s a code size
complexity penalty with this design (both in terms of source and object code for
any given implementation), there’s a data size penalty with this design (because
you’re carrying around data that’s totally irrelevant and in some cases is even
totally unreachable). Most importantly, there’s a huge mental obstacle to
understand what this code is supposed to do and what it actually does.

If you need an interface class, that’s fine. If you need a sort-of-interface
class that delegates a bunch of implementation details to platform-specific
implementations but also provides some amount of common functionality, that’s
also fine. I think that {Enable,Disable}KeypresMonitoring, with their
relationship to {Start,Stop}KeyboardMonitoring, fall into this category. What
you can’t have is a class that’s part interface and part ultimate
implementation, but where in some but not all cases, and on some but not all
platforms, you ignore the implementation and treat it as pure interface. That’s
what you’ve done here. On Linux, UserInputMonitor::GetKeyPressCount is
implementation. On Mac, UserInputMonitor::GetKeyPressCount is only interface,
and the implementation is UserInputMonitorMac::GetKeyPressCount. Isn’t that
confusing? Similarly, having {Add,Remove}MouseListener be part of the
platform-independent external interface imposes a requirement that each
implementation provide {Start,Stop}MouseMonitoring, but on the Mac, you’ve made
that a no-op via NOTREACHED, meaning that you don’t actually expect anyone to
ever call {Add,Remove}MouseListener on the Mac, and if they do, the program can
crash. Having the NOTREACHED turns those operations into a run-time DCHECK, but
wouldn’t it be better if the check happened at compile time instead? Wouldn’t
that provide a stronger guarantee against accidentally calling code that ought
not be called?

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
media/base/user_input_monitor_mac.cc:38: kCGEventSourceStateHIDSystemState,
kCGEventKeyDown);
I believe I understand why you used this given the intended application of this
code, but you should add a comment clarifying why you did so instead of using
the more usual kCGEventSourceStateCombinedSessionState.

https://codereview.chromium.org/22801007/diff/33002/media/base/user_input_mon...
media/base/user_input_monitor_mac.cc:51: initial_key_press_count_ =
CGEventSourceCounterForEventType(
Since you shouldn’t reach this code without initial_key_press_count_ being 0
(which is something you can DCHECK_EQ), you can capture this with:

  initial_key_press_count_ = GetKeyPressCount();

That serves to synchronize any changes to the call to
CGEventSourceCounterForEventType between the two call sites. That way, if
someone decides to change one of the parameters, things won’t break if they
forget to change both call sites.

jiayl

Resolved Mark's comments. Extracted the logic of converting key events to keypress count into a ...

7 years, 4 months ago (2013-08-23 19:09:09 UTC) #57

Mark Mentovai

This has changed enough again that Dale should also have another look. https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event_counter.cc File media/base/keyboard_event_counter.cc ...

7 years, 4 months ago (2013-08-23 20:18:10 UTC) #58

jiayl

PTAL https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event_counter.cc File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event_counter.cc#newcode32 media/base/keyboard_event_counter.cc:32: DCHECK(pressed_keys_.find(key_code) != pressed_keys_.end()); On 2013/08/23 20:18:11, Mark Mentovai ...

7 years, 4 months ago (2013-08-23 23:47:56 UTC) #59

PTAL

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
File media/base/keyboard_event_counter.cc (right):

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
media/base/keyboard_event_counter.cc:32: DCHECK(pressed_keys_.find(key_code) !=
pressed_keys_.end());
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> There’s no possible way you can guarantee this condition unless you also have
a
> way to guarantee that no keys will be down when this object is constructed or
> when Reset is called.

removed.

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
media/base/keyboard_event_counter.cc:38: base::AutoLock auto_lock(lock_);
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> This probably isn’t a hot spot, but you could make lock_ protect only
> pressed_keys_, and use AtomicIncrement (base/atomicops.h) to manage
> total_key_presses_. That would allow you to avoid taking the lock here.

But the documentation in atomicops.h says that those routines should not be used
without solid evidence on performance improvement...so I didn't change here

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
File media/base/keyboard_event_counter.h (right):

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
media/base/keyboard_event_counter.h:9: #include "base/synchronization/lock.h"
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> Blank line before this.

Done.

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
media/base/keyboard_event_counter.h:15: // This class tracks the total number
key presses based on the OnKeyboardEvent
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> total number of keypresses

Done.

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
media/base/keyboard_event_counter.h:17: // Multiple key down events for the same
key is counted as one keypress until
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> is → are

Done.

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
media/base/keyboard_event_counter.h:27: // Returns the total number of
keypresses since its creation or last Reset()
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> Put blank lines before the comments that introduce new things to avoid having
> them vanish in between  unrelated declarations. Same on lines 30 and 36.

Done.

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
media/base/keyboard_event_counter.h:37: std::set<int> pressed_keys_;
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> ui::KeyboardCode, not int?

Done.

https://codereview.chromium.org/22801007/diff/91011/media/base/user_input_mon...
File media/base/user_input_monitor.cc (right):

https://codereview.chromium.org/22801007/diff/91011/media/base/user_input_mon...
media/base/user_input_monitor.cc:63: UserInputMonitor::UserInputMonitor()
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> I don’t know how this wound up here, but normally the constructor’s
> implementation gets placed higher up in the file, above the destructor, the
same
> as the order it’s declared in the class’ declaration in the header. Nobody’s
> gonna find it here buried between ordinary methods.

Done.

https://codereview.chromium.org/22801007/diff/91011/media/base/user_input_mon...
File media/base/user_input_monitor.h (right):

https://codereview.chromium.org/22801007/diff/91011/media/base/user_input_mon...
media/base/user_input_monitor.h:26: // will not access deleted object.
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> access a deleted object.

Done.

https://codereview.chromium.org/22801007/diff/91011/media/base/user_input_mon...
media/base/user_input_monitor.h:61: // for the first time.
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> This comment is still confusing, because Enable-Disable will still zero the
> count, so a subsequent Enable will start at 0 again despite not being “the
first
> time.”
> 
> I actually think that you don’t need to specify this so tightly. You can say
> that it returns the number of keypresses, and that the caller must have
enabled
> keypress monitoring for it to be valid. That would implicitly refer to the
> comment on the Enable/Disable calls above, so you shouldn’t need to say
anything
> more about it. It would also free you from mandating that you start the count
at
> 0 when Enable is called for the first time. As this code is used, the actual
> absolute count or zero reference doesn’t seem to matter, since you only ever
> check equality. In fact, the Mac implementation can just return the value it
> gets from the system routine directly, without having to subtract or track
> initial_key_press_count_ at all. Simpler, right?
> 
> Keypresses is not a proper noun and does not use a capital K.

Done.

https://codereview.chromium.org/22801007/diff/91011/media/base/user_input_mon...
File media/base/user_input_monitor_mac.cc (right):

https://codereview.chromium.org/22801007/diff/91011/media/base/user_input_mon...
media/base/user_input_monitor_mac.cc:14: explicit UserInputMonitorMac();
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> Doesn’t need to be explicit now, there are no arguments.

Done.

https://codereview.chromium.org/22801007/diff/91011/media/base/user_input_mon...
media/base/user_input_monitor_mac.cc:25: size_t initial_key_press_count_;
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> See previous comment about eliminating this member variable. I don’t think
it’s
> necessary.

Done.

https://codereview.chromium.org/22801007/diff/91011/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/22801007/diff/91011/media/media.gyp#newcode264
media/media.gyp:264: 'base/keyboard_event_counter.cc',
On 2013/08/23 20:18:11, Mark Mentovai wrote:
> You don’t need to build this file on Mac, and shouldn’t build it on Mac.
> 
> Not sure about other platforms, since I don’t know what your plans are, but as
> of right now you only need it on Linux.

Moved to linux only now. Windows will also need the class, so I didn't suffix it
with _linux.

Mark Mentovai

Dale should have another look again, since this has changed nontrivially since his last review ...

7 years, 3 months ago (2013-08-26 15:11:01 UTC) #60

jiayl

Dale, could you take another look? https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event_counter.cc File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event_counter.cc#newcode38 media/base/keyboard_event_counter.cc:38: base::AutoLock auto_lock(lock_); On ...

7 years, 3 months ago (2013-08-26 17:16:57 UTC) #61

Mark Mentovai

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event_counter.cc File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event_counter.cc#newcode38 media/base/keyboard_event_counter.cc:38: base::AutoLock auto_lock(lock_); jiayl wrote: > On 2013/08/26 15:11:01, Mark ...

7 years, 3 months ago (2013-08-26 19:07:54 UTC) #62

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
File media/base/keyboard_event_counter.cc (right):

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
media/base/keyboard_event_counter.cc:38: base::AutoLock auto_lock(lock_);
jiayl wrote:
> On 2013/08/26 15:11:01, Mark Mentovai wrote:
> > jiayl wrote:
> > > But the documentation in atomicops.h says that those routines should not
be
> > used
> > > without solid evidence on performance improvement...so I didn't change
here
> > 
> > Well, how often will GetKeyPressCount be called? It’s called from
> > AudioInputController::OnData, which seems like it might be called relatively
> > frequently. Removing a lock could be beneficial.
> 
> But *_AtomicIncrement does not have a version for unsigned int.

A cast would be fine, although you’d need to pick the right type to cast to for
a size_t: it’d be Atomic32 in 32-bit builds and Atomic64 in 64-bit builds.

https://codereview.chromium.org/22801007/diff/97001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/22801007/diff/97001/media/media.gyp#newcode688
media/media.gyp:688: 'audio/cras/audio_manager_cras.cc',
jiayl wrote:
> On 2013/08/26 15:11:01, Mark Mentovai wrote:
> > We normally use excludes for this:
> > 
> > List keyboard_event_counter.{cc,h} in the main 'sources' list, and then list
> > them again here in 'sources!' to exclude them from the build on non-Linux.
> 
> That won't work when the Windows impl is added. I would need to add a new
> condition as OS != "linux" && OS != "windows".

That’s correct, you’ll need to do that once you add Windows.

Adding sources inside OS-specific blocks is not done in Chrome code. We always
add all sources and then conditionally exclude them.

The reason we do this is to make all of the source files visible in development
environments. With the source files listed unconditionally in 'sources', Xcode
and Visual Studio, for example, will list those files. That way, it’s easy for
another developer to find the files, view them, and make changes, even if
they’re editing on a Mac or Windows system that won’t compile them.

jiayl

PTAL https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event_counter.cc File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event_counter.cc#newcode38 media/base/keyboard_event_counter.cc:38: base::AutoLock auto_lock(lock_); On 2013/08/26 19:07:55, Mark Mentovai wrote: ...

7 years, 3 months ago (2013-08-26 19:30:08 UTC) #63

PTAL

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
File media/base/keyboard_event_counter.cc (right):

https://codereview.chromium.org/22801007/diff/91011/media/base/keyboard_event...
media/base/keyboard_event_counter.cc:38: base::AutoLock auto_lock(lock_);
On 2013/08/26 19:07:55, Mark Mentovai wrote:
> jiayl wrote:
> > On 2013/08/26 15:11:01, Mark Mentovai wrote:
> > > jiayl wrote:
> > > > But the documentation in atomicops.h says that those routines should not
> be
> > > used
> > > > without solid evidence on performance improvement...so I didn't change
> here
> > > 
> > > Well, how often will GetKeyPressCount be called? It’s called from
> > > AudioInputController::OnData, which seems like it might be called
relatively
> > > frequently. Removing a lock could be beneficial.
> > 
> > But *_AtomicIncrement does not have a version for unsigned int.
> 
> A cast would be fine, although you’d need to pick the right type to cast to
for
> a size_t: it’d be Atomic32 in 32-bit builds and Atomic64 in 64-bit builds.

Done.

https://codereview.chromium.org/22801007/diff/97001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/22801007/diff/97001/media/media.gyp#newcode688
media/media.gyp:688: 'audio/cras/audio_manager_cras.cc',
On 2013/08/26 19:07:55, Mark Mentovai wrote:
> jiayl wrote:
> > On 2013/08/26 15:11:01, Mark Mentovai wrote:
> > > We normally use excludes for this:
> > > 
> > > List keyboard_event_counter.{cc,h} in the main 'sources' list, and then
list
> > > them again here in 'sources!' to exclude them from the build on non-Linux.
> > 
> > That won't work when the Windows impl is added. I would need to add a new
> > condition as OS != "linux" && OS != "windows".
> 
> That’s correct, you’ll need to do that once you add Windows.
> 
> Adding sources inside OS-specific blocks is not done in Chrome code. We always
> add all sources and then conditionally exclude them.
> 
> The reason we do this is to make all of the source files visible in
development
> environments. With the source files listed unconditionally in 'sources', Xcode
> and Visual Studio, for example, will list those files. That way, it’s easy for
> another developer to find the files, view them, and make changes, even if
> they’re editing on a Mac or Windows system that won’t compile them.

Done. Thanks for the explanation.

Mark Mentovai

https://codereview.chromium.org/22801007/diff/103001/media/base/keyboard_event_counter.cc File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/22801007/diff/103001/media/base/keyboard_event_counter.cc#newcode24 media/base/keyboard_event_counter.cc:24: base::AutoLock auto_lock(lock_); Is it true that pressed_keys_ will now ...

7 years, 3 months ago (2013-08-26 19:33:42 UTC) #64

jiayl

PTAL https://codereview.chromium.org/22801007/diff/103001/media/base/keyboard_event_counter.cc File media/base/keyboard_event_counter.cc (right): https://codereview.chromium.org/22801007/diff/103001/media/base/keyboard_event_counter.cc#newcode24 media/base/keyboard_event_counter.cc:24: base::AutoLock auto_lock(lock_); On 2013/08/26 19:33:43, Mark Mentovai wrote: ...

7 years, 3 months ago (2013-08-26 20:09:18 UTC) #65

DaleCurtis

lgtm % nits. Also, I'd like to see a set of platform agnostic unit tests ...

7 years, 3 months ago (2013-08-26 22:16:54 UTC) #67

jiayl

On 2013/08/26 22:16:54, DaleCurtis wrote: > lgtm % nits. Also, I'd like to see a ...

7 years, 3 months ago (2013-08-26 22:29:07 UTC) #68

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/22801007/116001

7 years, 3 months ago (2013-08-26 22:56:52 UTC) #69

commit-bot: I haz the power

Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=72019

7 years, 3 months ago (2013-08-27 03:43:03 UTC) #70

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/22801007/135001

7 years, 3 months ago (2013-08-27 16:43:02 UTC) #71

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jiayl@chromium.org/22801007/135001

7 years, 3 months ago (2013-08-28 00:36:45 UTC) #72

Message was sent while issue was closed.

Change committed as 219896

Issue 22801007: Adds the UserInputMonitor implementation for Mac. (Closed)

Description

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Patch Set 12 : #

Patch Set 13 : #

Patch Set 14 : #

Patch Set 15 : #

Patch Set 16 : #

Patch Set 17 : #

Patch Set 18 : #

Patch Set 19 : #

Patch Set 20 : sync #

Patch Set 21 : #

Messages