Adding key press detection in the browser process.

Adding key press detection in the browser process.
It works like this on the browser side: 
A new object UserInputMonitor is created on BrowserMainLoop and passed to AudioInputRendererHost to pass to AudioInputController.
AudioInputController::DoRecord calls UserInputMonitor::AddKeyStrokeListener --> UserInputMonitor listens to system key events (only implemented on Linux) --> AudioInputController::OnKeyPressed is called and sets key_pressed_ --> When AudioInputController::OnData called, it writes key_pressed_ to shared memory along with the audio data buffer.
On the renderer side a new param "key_pressed" is added through the code path of passing the flag to the webrtc voice engine.
This CL includes all these changes except the implementation of UserInputMonitor for Windows and Mac. The impl of UserInputMonitor is mostly copied from remoting/host/local_input_monitor_linux.cc


BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217768

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217844

[jiayl, 2013-08-01 22:06:31]

Dale&Tommi, please review media/ and */media/
sky, please review content/browser/
Thanks!

[DaleCurtis, 2013-08-01 23:20:29]

Kind of confusing to see keyboard data paired with audio data, but I don't have
a more efficient delivery mechanism offhand.

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

https://codereview.chromium.org/21183002/diff/13001/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:345: base::AutoLock
auto_lock(key_pressed_lock_);
Just use the lock_ above ?

https://codereview.chromium.org/21183002/diff/13001/media/audio/key_press_mon...
File media/audio/key_press_monitor.h (right):

https://codereview.chromium.org/21183002/diff/13001/media/audio/key_press_mon...
media/audio/key_press_monitor.h:25: virtual ~KeyPressListener() {}
protected:

https://codereview.chromium.org/21183002/diff/13001/media/audio/key_press_mon...
media/audio/key_press_monitor.h:42: typedef
std::unordered_set<KeyPressListener*> ListenerSet;
Use base::ObserverList instead.

[jiayl, 2013-08-01 23:35:22]

PTAL. Thanks!

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

https://codereview.chromium.org/21183002/diff/13001/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:345: base::AutoLock
auto_lock(key_pressed_lock_);
On 2013/08/01 23:20:29, DaleCurtis wrote:
> Just use the lock_ above ?

Done.

https://codereview.chromium.org/21183002/diff/13001/media/audio/key_press_mon...
File media/audio/key_press_monitor.h (right):

https://codereview.chromium.org/21183002/diff/13001/media/audio/key_press_mon...
media/audio/key_press_monitor.h:25: virtual ~KeyPressListener() {}
On 2013/08/01 23:20:29, DaleCurtis wrote:
> protected:

Done.

https://codereview.chromium.org/21183002/diff/13001/media/audio/key_press_mon...
media/audio/key_press_monitor.h:42: typedef
std::unordered_set<KeyPressListener*> ListenerSet;
On 2013/08/01 23:20:29, DaleCurtis wrote:
> Use base::ObserverList instead.

Done.

[DaleCurtis, 2013-08-02 01:01:17]

I think it might make more sense to land this after you've landed a
KeyPressMonitor implementation.  Otherwise we have a useless object lingering
around.  When do you expect to complete that portion of the work?

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

https://codereview.chromium.org/21183002/diff/18001/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:339: key_pressed = key_pressed_;
You could initialize key_pressed to false above and then use std::swap() here. 
Up to you, I find it preferable to an uninitialized variable.

https://codereview.chromium.org/21183002/diff/18001/media/audio/key_press_mon...
File media/audio/key_press_monitor.h (right):

https://codereview.chromium.org/21183002/diff/18001/media/audio/key_press_mon...
media/audio/key_press_monitor.h:42: typedef ObserverList<KeyPressListener>
ListenerList;
Typedef seems a bit overkill.  AudioInputController will always call Add/Remove
on the audio thread, you should be able to remove the Lock and just use a
base::ThreadChecker for sanity.  Is anyone else expected to call these methods?

[jiayl, 2013-08-02 20:32:11]

On 2013/08/02 01:01:17, DaleCurtis wrote:
> I think it might make more sense to land this after you've landed a
> KeyPressMonitor implementation.  Otherwise we have a useless object lingering
> around.  When do you expect to complete that portion of the work?
> 

I'm working on that now. Probably will add that to the same CL.

[jiayl, 2013-08-02 20:32:20]

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

https://codereview.chromium.org/21183002/diff/18001/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:339: key_pressed = key_pressed_;
On 2013/08/02 01:01:17, DaleCurtis wrote:
> You could initialize key_pressed to false above and then use std::swap() here.

> Up to you, I find it preferable to an uninitialized variable.

Done.

https://codereview.chromium.org/21183002/diff/18001/media/audio/key_press_mon...
File media/audio/key_press_monitor.h (right):

https://codereview.chromium.org/21183002/diff/18001/media/audio/key_press_mon...
media/audio/key_press_monitor.h:42: typedef ObserverList<KeyPressListener>
ListenerList;
On 2013/08/02 01:01:17, DaleCurtis wrote:
> Typedef seems a bit overkill.  AudioInputController will always call
Add/Remove
> on the audio thread, you should be able to remove the Lock and just use a
> base::ThreadChecker for sanity.  Is anyone else expected to call these
methods?

Done.

[DaleCurtis, 2013-08-02 21:04:46]

https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
File media/audio/key_press_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:11:
KeyPressMonitor::KeyPressMonitor(base::MessageLoopProxy* message_loop)
const scoped_refptr<base::MessageLoopProxy>&

https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
File media/audio/key_press_monitor.h (right):

https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
media/audio/key_press_monitor.h:20: // loop is passed to the ctor.
What about construction and destruction?  Should destruction check
listeners_.size() == 0?  Can it be done in a thread safe manner?

The way this is setup KeyPressMonitor will be destroyed before AudioManager
finishes processing all of the messages on the audio message loop.  I suspect
that will lead to some crashes...

You'll need to be careful to make sure KeypressMonitor outlives AudioManager and
that KeypressMonitor avoids spamming the audio loop in a way that would prevent
shutdown.  I.e., if I can hang browser shutdown by typing quickly, we've got a
problem :)

[jiayl, 2013-08-02 21:45:44]

https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
File media/audio/key_press_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:11:
KeyPressMonitor::KeyPressMonitor(base::MessageLoopProxy* message_loop)
On 2013/08/02 21:04:47, DaleCurtis wrote:
> const scoped_refptr<base::MessageLoopProxy>&

Done.

https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
File media/audio/key_press_monitor.h (right):

https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
media/audio/key_press_monitor.h:20: // loop is passed to the ctor.
On 2013/08/02 21:04:47, DaleCurtis wrote:
> What about construction and destruction?  Should destruction check
> listeners_.size() == 0?  Can it be done in a thread safe manner?
> 
> The way this is setup KeyPressMonitor will be destroyed before AudioManager
> finishes processing all of the messages on the audio message loop.  I suspect
> that will lead to some crashes...
> 
> You'll need to be careful to make sure KeypressMonitor outlives AudioManager
and
> that KeypressMonitor avoids spamming the audio loop in a way that would
prevent
> shutdown.  I.e., if I can hang browser shutdown by typing quickly, we've got a
> problem :)

Changed it to RefCountedThreadSafe. AudioInputController now holds a reference
to KeyPressMonitor to make sure it's valid to call AddListener/RemoveListener.
When AudioInputController goes away, it releases KeyPressMonitor, which release
the message loop. There should be no circular reference or hang. Sounds right?

[DaleCurtis, 2013-08-02 21:55:28]

https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
File media/audio/key_press_monitor.h (right):

https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
media/audio/key_press_monitor.h:20: // loop is passed to the ctor.
On 2013/08/02 21:45:45, jiayl wrote:
> On 2013/08/02 21:04:47, DaleCurtis wrote:
> > What about construction and destruction?  Should destruction check
> > listeners_.size() == 0?  Can it be done in a thread safe manner?
> > 
> > The way this is setup KeyPressMonitor will be destroyed before AudioManager
> > finishes processing all of the messages on the audio message loop.  I
suspect
> > that will lead to some crashes...
> > 
> > You'll need to be careful to make sure KeypressMonitor outlives AudioManager
> and
> > that KeypressMonitor avoids spamming the audio loop in a way that would
> prevent
> > shutdown.  I.e., if I can hang browser shutdown by typing quickly, we've got
a
> > problem :)
> 
> Changed it to RefCountedThreadSafe. AudioInputController now holds a reference
> to KeyPressMonitor to make sure it's valid to call AddListener/RemoveListener.
> When AudioInputController goes away, it releases KeyPressMonitor, which
release
> the message loop. There should be no circular reference or hang. Sounds right?

Ref counting is a last resort and likely overkill here.  The ownership semantics
are now quite messy.  If you just destruct KeypressMonitor after AudioManager,
you shouldn't have any problems.  Is there a reason you chose not to do that?

[jiayl, 2013-08-02 22:17:51]

On 2013/08/02 21:55:28, DaleCurtis wrote:
>
https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
> File media/audio/key_press_monitor.h (right):
> 
>
https://codereview.chromium.org/21183002/diff/33001/media/audio/key_press_mon...
> media/audio/key_press_monitor.h:20: // loop is passed to the ctor.
> On 2013/08/02 21:45:45, jiayl wrote:
> > On 2013/08/02 21:04:47, DaleCurtis wrote:
> > > What about construction and destruction?  Should destruction check
> > > listeners_.size() == 0?  Can it be done in a thread safe manner?
> > > 
> > > The way this is setup KeyPressMonitor will be destroyed before
AudioManager
> > > finishes processing all of the messages on the audio message loop.  I
> suspect
> > > that will lead to some crashes...
> > > 
> > > You'll need to be careful to make sure KeypressMonitor outlives
AudioManager
> > and
> > > that KeypressMonitor avoids spamming the audio loop in a way that would
> > prevent
> > > shutdown.  I.e., if I can hang browser shutdown by typing quickly, we've
got
> a
> > > problem :)
> > 
> > Changed it to RefCountedThreadSafe. AudioInputController now holds a
reference
> > to KeyPressMonitor to make sure it's valid to call
AddListener/RemoveListener.
> > When AudioInputController goes away, it releases KeyPressMonitor, which
> release
> > the message loop. There should be no circular reference or hang. Sounds
right?
> 
> Ref counting is a last resort and likely overkill here.  The ownership
semantics
> are now quite messy.  If you just destruct KeypressMonitor after AudioManager,
> you shouldn't have any problems.  Is there a reason you chose not to do that?

I thought AudioInputController could live longer than AudioManager... New patch
uploaded.

[DaleCurtis, 2013-08-02 23:34:42]

media lgtm, i defer to tommi for the webrtc stuff -- perhaps he'll even see a
way we can avoid all this plumbing.

[sky, 2013-08-05 16:38:54]

content/browser LGTM

https://codereview.chromium.org/21183002/diff/47001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_renderer_host.h (right):

https://codereview.chromium.org/21183002/diff/47001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_renderer_host.h:158:
media::KeyPressMonitor* key_press_monitor_;
Document ownership.

[jiayl, 2013-08-05 23:00:53]

Updated to add the implementation of UserInputMonitor for Linux. PTAL.
Thanks!

[tommi (sloooow) - chröme, 2013-08-06 10:57:46]

This approach is as we had discussed doing this and I think this is probably as
little plumbing as we can get away with.  The IPC approach would probably
require more.

The cl looks good.  I just have a couple of questions/ramblings.

https://codereview.chromium.org/21183002/diff/68001/content/renderer/media/we...
File content/renderer/media/webaudio_capturer_source.cc (right):

https://codereview.chromium.org/21183002/diff/68001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.cc:99:
callback_->Capture(capture_bus_.get(), 0, 1.0, false);
Can you add a todo or some comment that explains that this behavior is possibly
wrong?

If keypress detection has been applied at the getUserMedia level and WebAudio is
using (or supposed to be using) that mediastream, then this is disregarding the
constraints, right?

Then again, this will probably go away once audio processing has been moved out
of peerconnection and into chrome or rather the audio track implementation. 
Hmm... I guess there's no action here unless maybe to somehow explain this in a
comment?

related bug: crbug.com/268820

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

https://codereview.chromium.org/21183002/diff/68001/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:341: std::swap(key_pressed, key_pressed_);
this resets key_pressed_.  I'm assuming that's intentional but I'm wondering if
that's correct (and perhaps it is as correct and efficient as we can make it
with the current implementation).
In the webrtc implementation the whole keyboard was polled for every audio
callback which while not efficient, behaves differently than this approach and
might tickle the algorithm differently.  I'm just wondering about the background
here...  I guess the question I'm trying to ask is: does it work? :)

https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
File media/audio/key_press_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:33: FOR_EACH_OBSERVER(KeyPressListener,
listeners_, OnKeyPressed());
Would it make sense to have two events, OnKeyPressed(key) and
OnKeyReleased(key), to keep track of whether or not a key is being pressed?  I
guess some sort of reference counting would need to be used, but that might
perhaps be closer to the behavior that the webrtc code used to do.

Actually... since you already have a map of pressed keys, what about having an
event to indicate that no keys are being pressed and use that to clear the
key_pressed_ flag?

[jiayl, 2013-08-06 17:24:10]

Thanks for the comments!

https://codereview.chromium.org/21183002/diff/68001/content/renderer/media/we...
File content/renderer/media/webaudio_capturer_source.cc (right):

https://codereview.chromium.org/21183002/diff/68001/content/renderer/media/we...
content/renderer/media/webaudio_capturer_source.cc:99:
callback_->Capture(capture_bus_.get(), 0, 1.0, false);
On 2013/08/06 10:57:47, tommi wrote:
> Can you add a todo or some comment that explains that this behavior is
possibly
> wrong?
> 
> If keypress detection has been applied at the getUserMedia level and WebAudio
is
> using (or supposed to be using) that mediastream, then this is disregarding
the
> constraints, right?
> 
> Then again, this will probably go away once audio processing has been moved
out
> of peerconnection and into chrome or rather the audio track implementation. 
> Hmm... I guess there's no action here unless maybe to somehow explain this in
a
> comment?
> 
> related bug: crbug.com/268820

I'm not sure I understand this...the current plan is to report the state of
"overwhelming typing noise detected" back to the JS through peerconnection stats
(i.e. result of getStats),  instead of muting the stream automatically. Then JS
should decide what to do with the stream.
Since webaudio is not associated with a peerconnection, it has no way to get the
stats update anyway. Would we ever want to enable typing detection for webaudio
in this case?

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

https://codereview.chromium.org/21183002/diff/68001/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:341: std::swap(key_pressed, key_pressed_);
On 2013/08/06 10:57:47, tommi wrote:
> this resets key_pressed_.  I'm assuming that's intentional but I'm wondering
if
> that's correct (and perhaps it is as correct and efficient as we can make it
> with the current implementation).
> In the webrtc implementation the whole keyboard was polled for every audio
> callback which while not efficient, behaves differently than this approach and
> might tickle the algorithm differently.  I'm just wondering about the
background
> here...  I guess the question I'm trying to ask is: does it work? :)

I might miss something, but AFAIK this logic produces the same result as the
webrtc implementation.

Take Linux for example, the webrtc implementation is
https://cs.corp.google.com/#perforce10///depot/googleclient/third_party/webrt...

for (i = 0; i < sizeof(szKey); i++)
    state |= (szKey[i] ^ _oldKeyState[i]) & szKey[i];
return state != 0;
where szKey is the current key states, and oldKeyState is the key states of the
previous sampling period.
So |state| is non-zero when a key is pressed in this sampling period but not
pressed in the last sampling period.

In the impl of this CL, key_pressed_ is set true whenever a key is pressed
(holding a key will only set key_pressed_ once). Then we need to reset
key_pressed_ after each sampling so that a key pressed in the last sampling
period will not be carried to the next samplings.

Make sense?

https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
File media/audio/key_press_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:33: FOR_EACH_OBSERVER(KeyPressListener,
listeners_, OnKeyPressed());
On 2013/08/06 10:57:47, tommi wrote:
> Would it make sense to have two events, OnKeyPressed(key) and
> OnKeyReleased(key), to keep track of whether or not a key is being pressed?  I
> guess some sort of reference counting would need to be used, but that might
> perhaps be closer to the behavior that the webrtc code used to do.
> 
> Actually... since you already have a map of pressed keys, what about having an
> event to indicate that no keys are being pressed and use that to clear the
> key_pressed_ flag?

It doesn't seem necessary if my understanding of the existing webrtc impl is
correct (see my reply to the other comment). If you are concerned about the case
of holding down a key without releasing it, KeyPressMonitor already takes care
of that case and only notifies the listeners once. IMO the clients of
KeyPressMonitor don't need to know which keys are pressed or released; these
logic should be hidden in KeyPressMonitor.

[jiayl, 2013-08-06 22:08:16]

Sergey,

could you take a look at the impl of UserInputMonitor?


On Tue, Aug 6, 2013 at 10:24 AM, <jiayl@chromium.org> wrote:

> Thanks for the comments!
>
>
>
> https://codereview.chromium.**org/21183002/diff/68001/**
>
content/renderer/media/**webaudio_capturer_source.cc<https://codereview.chromium.org/21183002/diff/68001/content/renderer/media/webaudio_capturer_source.cc>
> File content/renderer/media/**webaudio_capturer_source.cc (right):
>
> https://codereview.chromium.**org/21183002/diff/68001/**
>
content/renderer/media/**webaudio_capturer_source.cc#**newcode99<https://codereview.chromium.org/21183002/diff/68001/content/renderer/media/webaudio_capturer_source.cc#newcode99>
> content/renderer/media/**webaudio_capturer_source.cc:**99:
> callback_->Capture(capture_**bus_.get(), 0, 1.0, false);
> On 2013/08/06 10:57:47, tommi wrote:
>
>> Can you add a todo or some comment that explains that this behavior is
>>
> possibly
>
>> wrong?
>>
>
>  If keypress detection has been applied at the getUserMedia level and
>>
> WebAudio is
>
>> using (or supposed to be using) that mediastream, then this is
>>
> disregarding the
>
>> constraints, right?
>>
>
>  Then again, this will probably go away once audio processing has been
>>
> moved out
>
>> of peerconnection and into chrome or rather the audio track
>>
> implementation.
>
>> Hmm... I guess there's no action here unless maybe to somehow explain
>>
> this in a
>
>> comment?
>>
>
>  related bug: crbug.com/268820
>>
>
> I'm not sure I understand this...the current plan is to report the state
> of "overwhelming typing noise detected" back to the JS through
> peerconnection stats (i.e. result of getStats),  instead of muting the
> stream automatically. Then JS should decide what to do with the stream.
> Since webaudio is not associated with a peerconnection, it has no way to
> get the stats update anyway. Would we ever want to enable typing
> detection for webaudio in this case?
>
>
> https://codereview.chromium.**org/21183002/diff/68001/media/**
>
audio/audio_input_controller.**cc<https://codereview.chromium.org/21183002/diff/68001/media/audio/audio_input_controller.cc>
> File media/audio/audio_input_**controller.cc (right):
>
> https://codereview.chromium.**org/21183002/diff/68001/media/**
>
audio/audio_input_controller.**cc#newcode341<https://codereview.chromium.org/21183002/diff/68001/media/audio/audio_input_controller.cc#newcode341>
> media/audio/audio_input_**controller.cc:341: std::swap(key_pressed,
> key_pressed_);
> On 2013/08/06 10:57:47, tommi wrote:
>
>> this resets key_pressed_.  I'm assuming that's intentional but I'm
>>
> wondering if
>
>> that's correct (and perhaps it is as correct and efficient as we can
>>
> make it
>
>> with the current implementation).
>> In the webrtc implementation the whole keyboard was polled for every
>>
> audio
>
>> callback which while not efficient, behaves differently than this
>>
> approach and
>
>> might tickle the algorithm differently.  I'm just wondering about the
>>
> background
>
>> here...  I guess the question I'm trying to ask is: does it work? :)
>>
>
> I might miss something, but AFAIK this logic produces the same result as
> the webrtc implementation.
>
> Take Linux for example, the webrtc implementation is
> https://cs.corp.google.com/#**perforce10///depot/**
> googleclient/third_party/**webrtc/nightly/files/webrtc/**
>
modules/audio_device/linux/**audio_device_pulse_linux.cc&l=**3112<https://cs.corp.google.com/#perforce10///depot/googleclient/third_party/webrtc/nightly/files/webrtc/modules/audio_device/linux/audio_device_pulse_linux.cc&l=3112>
>
> for (i = 0; i < sizeof(szKey); i++)
>     state |= (szKey[i] ^ _oldKeyState[i]) & szKey[i];
> return state != 0;
> where szKey is the current key states, and oldKeyState is the key states
> of the previous sampling period.
> So |state| is non-zero when a key is pressed in this sampling period but
> not pressed in the last sampling period.
>
> In the impl of this CL, key_pressed_ is set true whenever a key is
> pressed (holding a key will only set key_pressed_ once). Then we need to
> reset key_pressed_ after each sampling so that a key pressed in the last
> sampling period will not be carried to the next samplings.
>
> Make sense?
>
>
> https://codereview.chromium.**org/21183002/diff/68001/media/**
>
audio/key_press_monitor.cc<https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_monitor.cc>
> File media/audio/key_press_monitor.**cc (right):
>
> https://codereview.chromium.**org/21183002/diff/68001/media/**
>
audio/key_press_monitor.cc#**newcode33<https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_monitor.cc#newcode33>
> media/audio/key_press_monitor.**cc:33: FOR_EACH_OBSERVER(**
> KeyPressListener,
> listeners_, OnKeyPressed());
> On 2013/08/06 10:57:47, tommi wrote:
>
>> Would it make sense to have two events, OnKeyPressed(key) and
>> OnKeyReleased(key), to keep track of whether or not a key is being
>>
> pressed?  I
>
>> guess some sort of reference counting would need to be used, but that
>>
> might
>
>> perhaps be closer to the behavior that the webrtc code used to do.
>>
>
>  Actually... since you already have a map of pressed keys, what about
>>
> having an
>
>> event to indicate that no keys are being pressed and use that to clear
>>
> the
>
>> key_pressed_ flag?
>>
>
> It doesn't seem necessary if my understanding of the existing webrtc
> impl is correct (see my reply to the other comment). If you are
> concerned about the case of holding down a key without releasing it,
> KeyPressMonitor already takes care of that case and only notifies the
> listeners once. IMO the clients of KeyPressMonitor don't need to know
> which keys are pressed or released; these logic should be hidden in
> KeyPressMonitor.
>
>
https://codereview.chromium.**org/21183002/<https://codereview.chromium.org/2...
>

[tommi (sloooow) - chröme, 2013-08-07 11:42:55]

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

https://codereview.chromium.org/21183002/diff/68001/media/audio/audio_input_c...
media/audio/audio_input_controller.cc:341: std::swap(key_pressed, key_pressed_);
On 2013/08/06 17:24:11, jiayl wrote:
> On 2013/08/06 10:57:47, tommi wrote:
> > this resets key_pressed_.  I'm assuming that's intentional but I'm wondering
> if
> > that's correct (and perhaps it is as correct and efficient as we can make it
> > with the current implementation).
> > In the webrtc implementation the whole keyboard was polled for every audio
> > callback which while not efficient, behaves differently than this approach
and
> > might tickle the algorithm differently.  I'm just wondering about the
> background
> > here...  I guess the question I'm trying to ask is: does it work? :)
> 
> I might miss something, but AFAIK this logic produces the same result as the
> webrtc implementation.
> 
> Take Linux for example, the webrtc implementation is
>
https://cs.corp.google.com/#perforce10///depot/googleclient/third_party/webrt...
> 
> for (i = 0; i < sizeof(szKey); i++)
>     state |= (szKey[i] ^ _oldKeyState[i]) & szKey[i];
> return state != 0;
> where szKey is the current key states, and oldKeyState is the key states of
the
> previous sampling period.
> So |state| is non-zero when a key is pressed in this sampling period but not
> pressed in the last sampling period.
> 
> In the impl of this CL, key_pressed_ is set true whenever a key is pressed
> (holding a key will only set key_pressed_ once). Then we need to reset
> key_pressed_ after each sampling so that a key pressed in the last sampling
> period will not be carried to the next samplings.
> 
> Make sense?

The implementation in this CL behaves slightly differently.
Consider pressing a key and hold it down for a whole second.  In the webrtc
example, keypressed will be true for the whole period while this implementation
will report keypressed as true for only the first audio data callback, then
clear the keypressed variable and report false for the remainder of the period
and then ignore the key-released event.

However, I don't think this really matters for our purposes right now, so the
current approach lgtm.

[tommi (sloooow) - chröme, 2013-08-07 11:46:43]

On 2013/08/07 11:42:55, tommi wrote:
>
https://codereview.chromium.org/21183002/diff/68001/media/audio/audio_input_c...
> File media/audio/audio_input_controller.cc (right):
> 
>
https://codereview.chromium.org/21183002/diff/68001/media/audio/audio_input_c...
> media/audio/audio_input_controller.cc:341: std::swap(key_pressed,
key_pressed_);
> On 2013/08/06 17:24:11, jiayl wrote:
> > On 2013/08/06 10:57:47, tommi wrote:
> > > this resets key_pressed_.  I'm assuming that's intentional but I'm
wondering
> > if
> > > that's correct (and perhaps it is as correct and efficient as we can make
it
> > > with the current implementation).
> > > In the webrtc implementation the whole keyboard was polled for every audio
> > > callback which while not efficient, behaves differently than this approach
> and
> > > might tickle the algorithm differently.  I'm just wondering about the
> > background
> > > here...  I guess the question I'm trying to ask is: does it work? :)
> > 
> > I might miss something, but AFAIK this logic produces the same result as the
> > webrtc implementation.
> > 
> > Take Linux for example, the webrtc implementation is
> >
>
https://cs.corp.google.com/#perforce10///depot/googleclient/third_party/webrt...
> > 
> > for (i = 0; i < sizeof(szKey); i++)
> >     state |= (szKey[i] ^ _oldKeyState[i]) & szKey[i];
> > return state != 0;
> > where szKey is the current key states, and oldKeyState is the key states of
> the
> > previous sampling period.
> > So |state| is non-zero when a key is pressed in this sampling period but not
> > pressed in the last sampling period.
> > 
> > In the impl of this CL, key_pressed_ is set true whenever a key is pressed
> > (holding a key will only set key_pressed_ once). Then we need to reset
> > key_pressed_ after each sampling so that a key pressed in the last sampling
> > period will not be carried to the next samplings.
> > 
> > Make sense?
> 
> The implementation in this CL behaves slightly differently.
> Consider pressing a key and hold it down for a whole second.  In the webrtc
> example, keypressed will be true for the whole period while this
implementation
> will report keypressed as true for only the first audio data callback, then
> clear the keypressed variable and report false for the remainder of the period
> and then ignore the key-released event.
> 
> However, I don't think this really matters for our purposes right now, so the
> current approach lgtm.

Ah, sorry, I re-read that and you're completely right. This behaves the same
way.
Somehow I had forgotten about _oldKeyState and just assumed I knew what I was
talking about :)

[DaleCurtis, 2013-08-07 17:50:39]

https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
File media/audio/key_press_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:13: const
scoped_refptr<base::MessageLoopProxy>& io_message_loop,
Not really necessary to pass all this in, you could just use
BrowserThread::GetMessageLoopForThread().

https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:36: class UserInputMonitorLinux : public
base::NonThreadSafe,
Instead of copying this code from LocalInputMonitor, is it possible to work with
the remoting team to create a generic version that both components can use?  It
looks like they've already created OSX and Windows variants too.

[jiayl, 2013-08-07 17:59:17]

On 2013/08/07 17:50:39, DaleCurtis wrote:
>
https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
> File media/audio/key_press_monitor.cc (right):
> 
>
https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
> media/audio/key_press_monitor.cc:13: const
> scoped_refptr<base::MessageLoopProxy>& io_message_loop,
> Not really necessary to pass all this in, you could just use
> BrowserThread::GetMessageLoopForThread().
...then I cannot tell if the methods are called on the right thread.

>
https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
> File media/base/user_input_monitor_linux.cc (right):
> 
>
https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
> media/base/user_input_monitor_linux.cc:36: class UserInputMonitorLinux :
public
> base::NonThreadSafe,
> Instead of copying this code from LocalInputMonitor, is it possible to work
with
> the remoting team to create a generic version that both components can use? 
It
> looks like they've already created OSX and Windows variants too.

Yes, the new UserInputMonitor is intended to be used by both audio and remoting.
Remoting will be migrated to the new API in a future CL.

[DaleCurtis, 2013-08-07 18:58:01]

In general it seems weird to me to have to pass around so many message loops,
especially so since there seems to be 'wrong' choices; i.e., linux seems to have
some special dependency on the IO loop and mac/win on the UI loop. If there's
only one correct way to use the class, it seems weird to offer an interface that
lets you choose a wrong answer.

https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
File media/audio/key_press_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:13: const
scoped_refptr<base::MessageLoopProxy>& io_message_loop,
On 2013/08/07 17:50:39, DaleCurtis wrote:
> Not really necessary to pass all this in, you could just use
> BrowserThread::GetMessageLoopForThread().

I forgot this is in media/ so you can't do this after all.

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

https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
media/base/user_input_monitor.h:31: };
Needs protected virtual destructor with empty impl.

https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
media/base/user_input_monitor.h:42: virtual ~UserInputMonitor() {}
protected.

https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
media/base/user_input_monitor.h:45: // |input_task_runner| is the task runner
for an IO thread to process events
You mention mac / linux / win here, but it's a generic platform agnostic
interface. It sounds like there should just be an input_task_runner which the
caller specifies as IO on linux and UI on mac / win?

https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
media/base/user_input_monitor.h:55: scoped_refptr<base::SingleThreadTaskRunner>
input_task_runner,
const & ? ditto for the other two.

https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
media/base/user_input_monitor.h:58: uint8 events_of_interest,
input_event_mask ?

[jiayl, 2013-08-07 23:38:13]

PTAL.

On 2013/08/07 18:58:01, DaleCurtis wrote:
> In general it seems weird to me to have to pass around so many message loops,
> especially so since there seems to be 'wrong' choices; i.e., linux seems to
have
> some special dependency on the IO loop and mac/win on the UI loop. If there's
> only one correct way to use the class, it seems weird to offer an interface
that
> lets you choose a wrong answer.
> 

The comments for the interfaces might be misleading. The platform-specific
dependencies 
should really be hidden in the implementation and the callers should not worry
about it.
I changed the comments to make it clearer hopefully. 
The type of the message loop is required by the OS APIs, not a choice we can
change.

>
https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
> File media/audio/key_press_monitor.cc (right):
> 
>
https://codereview.chromium.org/21183002/diff/68001/media/audio/key_press_mon...
> media/audio/key_press_monitor.cc:13: const
> scoped_refptr<base::MessageLoopProxy>& io_message_loop,
> On 2013/08/07 17:50:39, DaleCurtis wrote:
> > Not really necessary to pass all this in, you could just use
> > BrowserThread::GetMessageLoopForThread().
> 
> I forgot this is in media/ so you can't do this after all.
> 
>
https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
> File media/base/user_input_monitor.h (right):
> 
>
https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
> media/base/user_input_monitor.h:31: };
> Needs protected virtual destructor with empty impl.
Done
 
>
https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
> media/base/user_input_monitor.h:42: virtual ~UserInputMonitor() {}
> protected.
I cannot since it does not compile as scoped_ptr needs to call delete.

>
https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
> media/base/user_input_monitor.h:45: // |input_task_runner| is the task runner
> for an IO thread to process events
> You mention mac / linux / win here, but it's a generic platform agnostic
> interface. It sounds like there should just be an input_task_runner which the
> caller specifies as IO on linux and UI on mac / win?
I changed the comments so that the callers don't need to know about the
platform-specific 
dependency. I don't think we should let the caller decide which thread to pass
based on platform,
since it's an implementation detail the caller should not care about.

>
https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
> media/base/user_input_monitor.h:55:
scoped_refptr<base::SingleThreadTaskRunner>
> input_task_runner,
> const & ? ditto for the other two.
Done.

>
https://codereview.chromium.org/21183002/diff/68001/media/base/user_input_mon...
> media/base/user_input_monitor.h:58: uint8 events_of_interest,
> input_event_mask ?
Done.

[Sergey Ulanov, 2013-08-08 01:25:48]

I have some style nits. There is also issue with const ref pointers.

https://codereview.chromium.org/21183002/diff/84001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/21183002/diff/84001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_renderer_host.cc:64:
key_press_monitor_(key_press_monitor) {}
move } to the next line.

https://codereview.chromium.org/21183002/diff/84001/media/audio/key_press_mon...
File media/audio/key_press_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/84001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:18: weak_ptr_factory_(this) {}
Move } to the next line.

https://codereview.chromium.org/21183002/diff/84001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:22: void KeyPressMonitor::OnMouseMoved(const
SkIPoint& position) { NOTREACHED(); }
Wrap NOTREACHED();.  Putting } for a function on the same line as { is allowed
only when whole function definition fits one line.

https://codereview.chromium.org/21183002/diff/84001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:33: DCHECK(event == ui::ET_KEY_RELEASED);
DCHECK_EQ

https://codereview.chromium.org/21183002/diff/84001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:44:
monitor_.reset(UserInputMonitor::Create(io_message_loop_,
monitor_ = UserInputMonitor::Create(....).

Don't need reset() and relese() when Create() returns scoped_ptr<>.

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

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor.h:41: MOUSE_MOVEMENT = 0x1,
MOUSE_EVENT (enum name and the other already have Event suffix)

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:77: const
scoped_refptr<base::SingleThreadTaskRunner>& input_task_runner_;
Ouch. This shouldn't be a const ref. It will crash in the worst conceivable way
if you use const ref here. (it still can be passed as const ref to the
constructor)

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:83: const
base::WeakPtr<UserInputObserver>& observer_;
same here. It should be weak ptr member. Not a const ref.

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:194: x_record_context_ =
XRecordCreateContext(x_record_display_,
nit: IMO it's more readable when there are more than one argument per line:
  x_record_context_ = XRecordCreateContext(
      x_record_display_, 0, &client_spec, 1, x_record_range_,
      arraysize(x_record_range_));

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:294: type = ui::ET_KEY_PRESSED;
add {} for multi-line if statement.

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

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
nit: Lawyercat says you don't need (c).
http://www.chromium.org/developers/coding-style

Same with other files.

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor_mac.mm:15: return
scoped_ptr<UserInputMonitor>(NULL);
nit: remove NULL.
nit: add TODO to implement this?

[DaleCurtis, 2013-08-08 01:40:28]

lgtm

Some thoughts:

1. It stinks to pass around so many message loops.
2. KeyPressMonitor isn't really doing much besides acting as a proxy for
UserInputMonitor events.
3. UserInputMonitor sounds like there should only be once instance that doles
out keyboard or mouse events depending on which listeners are attached.

Instead of having both KeyPressMonitor and UserInputMonitor, should we just have
one? I.e. BrowserMainLoop owns a UserInputMonitor and AudioInputRendererHost
passes it to AudioInputController and let it decide to ask for key presses.

WDYT?

[DaleCurtis, 2013-08-08 01:41:23]

Whoops, accidentally hit looks good to me. not lgtm w/o some more discussion.

[DaleCurtis, 2013-08-08 01:49:50]

(One wrinkle with combining them though is that user_input_monitor seems to have
entirely different implementations per platform, so the small work that
KeyPressMonitor is doing would see duplication).

I defer to sergeyu@ for the final call on UserInputMonitor though since he's
most familiar with this code and its usage pattern inside of remoting as well as
the problems historically seen there.

[jiayl, 2013-08-08 17:35:33]

How about this:

Remove KeyPressMonitor, merge the impl into UserInputMonitor;

UserInputMonitor has a single instance owned by BrowserMainLoop:
class UserInputMonitor {
 public:
  /* definition of MouseEventListener, KeyboardEventListener,
KeyPressListener */
  ...
  // Create platform-specific instances.
  scoped_ptr Create(io_task_runner, ui_task_runner);

  // Event monitoring is not started until listeners are added.
  void AddListner(MouseEventListener*);
  void AddListner(KeyboardEventListener*);
  void AddListner(KeyPressListener*);
  /* RemoveListener */
  ...
 protected:
  // Implemented by the base class to call the callbacks. Platform-specific
sub-classes are supposed to call these.
  void OnMouseEvent(...);  // call mouse event callbacks.
  void OnKeyboardEvent(...);  // call keyboard event callbacks; update
pressed_keys_ and maybe call key press callbacks.

 private:
  /* listener lists and pressed_keys_ */
 ...
}
UserInputMonitor maintains three ObserverListThreadSafe for
mouse/keyboard/keypress respectively. It also tracks the pressed keys as
what KeyPressMonitor is doing.
ObserverListThreadSafe makes sure that callbacks are called on the same
thread where the callbacks are added, so no need to pass in the observer
task runner.


On Wed, Aug 7, 2013 at 6:49 PM, <dalecurtis@chromium.org> wrote:

> (One wrinkle with combining them though is that user_input_monitor seems
> to have
> entirely different implementations per platform, so the small work that
> KeyPressMonitor is doing would see duplication).
>
> I defer to sergeyu@ for the final call on UserInputMonitor though since
> he's
> most familiar with this code and its usage pattern inside of remoting as
> well as
> the problems historically seen there.
>
>
https://codereview.chromium.**org/21183002/<https://codereview.chromium.org/2...
>

[DaleCurtis, 2013-08-08 21:13:11]

sgtm % avoiding overloaded methods per the style guide and a naming nit:
AddKeyboardEventListener and AddKeyPressListener are pretty similar to each
other.  Can you think of a better name which makes it easier to differentiate
them at a glance?

[Sergey Ulanov, 2013-08-08 23:32:35]

Do I understand it correctly that this CL enables automatic muting for all
WebRTC streams including ones that don't come from microphone? If so, then it
would break system audio capturer used by chromecast for screen casting (it was
recently added for ChromeOS).

[jiayl, 2013-08-08 23:40:21]

On 2013/08/08 23:32:35, Sergey Ulanov wrote:
> Do I understand it correctly that this CL enables automatic muting for all
> WebRTC streams including ones that don't come from microphone? If so, then it
> would break system audio capturer used by chromecast for screen casting (it
was
> recently added for ChromeOS).

Typing detection is now disabled in WebRTC for Chrome, so passing key_pressed to
WebRTC has no effect for now. And WebRTC typing detection is going to be changed
to not automatic mute in C++, but populate the typing state to JS and let JS
decide what to do. So there will not be any automatic muting.

[tommi (sloooow) - chröme, 2013-08-09 07:52:29]

key_pressed should also always be set to false for non microphone sources.

Recently we've made changes that make webrtc support audio tracks (using
mediastream terminology) that do not have audio processing applied (aec,
agc, noise suppression, keyboard detection).  This is a step towards a
different architecture where audio processing will be applied at the source
level and not in the WebRTC/PeerConnection API.  It'll be a while until
we're there but for now there is a way to support non-APM sources and
microphone sources over the same PeerConnection.  For mic sources, audio
processing is still an all-or-nothing deal since audio processing is either
applied or not (you can't have one mic with only aec, another with only
agc, etc).


On Fri, Aug 9, 2013 at 1:40 AM, <jiayl@chromium.org> wrote:

> On 2013/08/08 23:32:35, Sergey Ulanov wrote:
>
>> Do I understand it correctly that this CL enables automatic muting for all
>> WebRTC streams including ones that don't come from microphone? If so,
>> then it
>> would break system audio capturer used by chromecast for screen casting
>> (it
>>
> was
>
>> recently added for ChromeOS).
>>
>
> Typing detection is now disabled in WebRTC for Chrome, so passing
> key_pressed to
> WebRTC has no effect for now. And WebRTC typing detection is going to be
> changed
> to not automatic mute in C++, but populate the typing state to JS and let
> JS
> decide what to do. So there will not be any automatic muting.
>
>
https://codereview.chromium.**org/21183002/<https://codereview.chromium.org/2...
>

[jiayl, 2013-08-09 23:30:13]

Merged KeyPressMonitor into UserInputMonitor. PTAL.

https://codereview.chromium.org/21183002/diff/84001/media/audio/key_press_mon...
File media/audio/key_press_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/84001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:18: weak_ptr_factory_(this) {}
On 2013/08/08 01:25:48, Sergey Ulanov wrote:
> Move } to the next line.
This is auto formatted by "git-cl format".

https://codereview.chromium.org/21183002/diff/84001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:22: void KeyPressMonitor::OnMouseMoved(const
SkIPoint& position) { NOTREACHED(); }
On 2013/08/08 01:25:48, Sergey Ulanov wrote:
> Wrap NOTREACHED();.  Putting } for a function on the same line as { is allowed
> only when whole function definition fits one line.
This is auto formatted by "git-cl format".

https://codereview.chromium.org/21183002/diff/84001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:33: DCHECK(event == ui::ET_KEY_RELEASED);
On 2013/08/08 01:25:48, Sergey Ulanov wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/21183002/diff/84001/media/audio/key_press_mon...
media/audio/key_press_monitor.cc:44:
monitor_.reset(UserInputMonitor::Create(io_message_loop_,
On 2013/08/08 01:25:48, Sergey Ulanov wrote:
> monitor_ = UserInputMonitor::Create(....).
> 
> Don't need reset() and relese() when Create() returns scoped_ptr<>.

Done.

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

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor.h:41: MOUSE_MOVEMENT = 0x1,
On 2013/08/08 01:25:48, Sergey Ulanov wrote:
> MOUSE_EVENT (enum name and the other already have Event suffix)

Removed now.

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:77: const
scoped_refptr<base::SingleThreadTaskRunner>& input_task_runner_;
On 2013/08/08 01:25:48, Sergey Ulanov wrote:
> Ouch. This shouldn't be a const ref. It will crash in the worst conceivable
way
> if you use const ref here. (it still can be passed as const ref to the
> constructor)

Oops, find&replace error. Fixed.

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:83: const
base::WeakPtr<UserInputObserver>& observer_;
On 2013/08/08 01:25:48, Sergey Ulanov wrote:
> same here. It should be weak ptr member. Not a const ref.

Done.

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:194: x_record_context_ =
XRecordCreateContext(x_record_display_,
On 2013/08/08 01:25:48, Sergey Ulanov wrote:
> nit: IMO it's more readable when there are more than one argument per line:
>   x_record_context_ = XRecordCreateContext(
>       x_record_display_, 0, &client_spec, 1, x_record_range_,
>       arraysize(x_record_range_));

This is auto formatted by "git-cl format".

https://codereview.chromium.org/21183002/diff/84001/media/base/user_input_mon...
media/base/user_input_monitor_linux.cc:294: type = ui::ET_KEY_PRESSED;
On 2013/08/08 01:25:48, Sergey Ulanov wrote:
> add {} for multi-line if statement.

Done.

[DaleCurtis, 2013-08-10 00:13:12]

Also I just realized you can avoid plumbing UserInputMonitor everywhere by
having AudioSyncWriter just get it directly from BrowserMainLoop and inject the
value.  I'm not sure how I feel about that though, WDYT?

https://codereview.chromium.org/21183002/diff/106001/content/browser/browser_...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/21183002/diff/106001/content/browser/browser_...
content/browser/browser_main_loop.h:129: // user_input_monitor_ has to outlive
audio_manager_, so declared first.
Comment isn't true anymore?

https://codereview.chromium.org/21183002/diff/106001/media/DEPS
File media/DEPS (right):

https://codereview.chromium.org/21183002/diff/106001/media/DEPS#newcode10
media/DEPS:10: "+ui/base",
alphabetic order.

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
File media/base/user_input_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor.cc:26: if (keyboard_listeners_.size() == 1 &&
key_press_listeners_.size() == 0)
It'd be clearer to just track this through two bools set by their respective
functions; i.e., if (!monitoring_keyboard_) ... if (!monitoring_mouse_) etc.

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
File media/base/user_input_monitor.h (right):

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor.h:25: class MouseEventListener {
Should this be RawMouseEventListener then?

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor.h:59: // |input_task_runner| is the task runner
for an IO thread.
io_task_runner ?

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor.h:85: virtual void StartMonitorMouse() = 0;
StartMouseMonitoring, StopMouseMonitoring, etc for all of these.

Also it's kind of weird to see inherited methods as private, though I don't see
any style guide rules forbidding this...  Did you mean for these to be
protected?

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:169: void
UserInputMonitorLinux::SetOrVerifyCallingThread() {
Instead of trying to do this, which feels a bit sketchy, how about just letting
clients handle their own thread hopping?  I.e. you deliver the notification via
ObserverList and clients can post a task to whatever thread they want if
necessary.

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:270: base::MessageLoopForIO* message_loop
= base::MessageLoopForIO::current();
?? just use io_loop_ ?

[jiayl, 2013-08-10 00:37:55]

Does AudioSyncWriter run on the UI thread? BrowserMainLoop::Instance can only be
called on the UI thread.

https://codereview.chromium.org/21183002/diff/106001/content/browser/browser_...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/21183002/diff/106001/content/browser/browser_...
content/browser/browser_main_loop.h:129: // user_input_monitor_ has to outlive
audio_manager_, so declared first.
On 2013/08/10 00:13:12, DaleCurtis wrote:
> Comment isn't true anymore?
It's still true...

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
File media/base/user_input_monitor.h (right):

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor.h:25: class MouseEventListener {
On 2013/08/10 00:13:12, DaleCurtis wrote:
> Should this be RawMouseEventListener then?
I don't feel strong about it, but there is no un-raw mouse event so seems
unnecessary to me. :)

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:169: void
UserInputMonitorLinux::SetOrVerifyCallingThread() {
On 2013/08/10 00:13:12, DaleCurtis wrote:
> Instead of trying to do this, which feels a bit sketchy, how about just
letting
> clients handle their own thread hopping?  I.e. you deliver the notification
via
> ObserverList and clients can post a task to whatever thread they want if
> necessary.
If it's not done here, it needs to be done in the base class, since the
ObserverList should be accessed on one thread. I think moving it to the base
class is better.

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:270: base::MessageLoopForIO* message_loop
= base::MessageLoopForIO::current();
On 2013/08/10 00:13:12, DaleCurtis wrote:
> ?? just use io_loop_ ?
Core doesn't have a member as io_loop_. It's always running on the IO loop.

[Sergey Ulanov, 2013-08-10 00:48:18]

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
File media/base/user_input_monitor.h (right):

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor.h:25: class MouseEventListener {
please add comments for this interface and the ones below. I think it's better
to make them nested in UserInputMonitor.

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor.h:42: class KeyPressListener {
Why do we need this given that there is RawKeyboardEventListener?

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:169: void
UserInputMonitorLinux::SetOrVerifyCallingThread() {
On 2013/08/10 00:13:12, DaleCurtis wrote:
> Instead of trying to do this, which feels a bit sketchy, how about just
letting
> clients handle their own thread hopping?  I.e. you deliver the notification
via
> ObserverList and clients can post a task to whatever thread they want if
> necessary.

It would mean that the clients of this interface have to be thread-aware, which
isn't good.
I think ObserverListThreadSafe might be useful here - it will jump threads as
needed (and so it can be used to make UserInputMonitor usable on different
threads).

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:174:
CHECK(callback_task_runner_->BelongsToCurrentThread());
it's more common to use DCHECKs for thread checks.

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:270: base::MessageLoopForIO* message_loop
= base::MessageLoopForIO::current();
On 2013/08/10 00:37:56, jiayl wrote:
> On 2013/08/10 00:13:12, DaleCurtis wrote:
> > ?? just use io_loop_ ?
> Core doesn't have a member as io_loop_. It's always running on the IO loop.

Also, only ForIO version of MessageLoop has WatchFileDescriptor(), so io_loop_
(which is a task runner wrapping the message loop) would not be useful here.

[DaleCurtis, 2013-08-10 00:57:51]

BrowserMainLoop::GetInstance() can be called anywhere it looks like?  It just
returns a global static.

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:169: void
UserInputMonitorLinux::SetOrVerifyCallingThread() {
On 2013/08/10 00:37:56, jiayl wrote:
> On 2013/08/10 00:13:12, DaleCurtis wrote:
> > Instead of trying to do this, which feels a bit sketchy, how about just
> letting
> > clients handle their own thread hopping?  I.e. you deliver the notification
> via
> > ObserverList and clients can post a task to whatever thread they want if
> > necessary.
> If it's not done here, it needs to be done in the base class, since the
> ObserverList should be accessed on one thread. I think moving it to the base
> class is better.

Since UserInputMonitor is owned by BrowserMainLoop, multiple threads may now
register for user input... so registering yourself to a single callback task
runner seems a recipe for disaster. 

Actually, I think the only way around this is a lock or a requirement that
listeners are only added and removed on the UI or IO thread (depending on
whether linux or mac/win is used).

Since you don't actually need the ability for reentrant removal of observers,
you could switch them to std::lists protected by a single lock.

[jiayl, 2013-08-10 01:01:10]

On 2013/08/10 00:57:51, DaleCurtis wrote:
> BrowserMainLoop::GetInstance() can be called anywhere it looks like?  It just
> returns a global static.
> 
See
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

>
https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
> File media/base/user_input_monitor_linux.cc (right):
> 
>
https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
> media/base/user_input_monitor_linux.cc:169: void
> UserInputMonitorLinux::SetOrVerifyCallingThread() {
> On 2013/08/10 00:37:56, jiayl wrote:
> > On 2013/08/10 00:13:12, DaleCurtis wrote:
> > > Instead of trying to do this, which feels a bit sketchy, how about just
> > letting
> > > clients handle their own thread hopping?  I.e. you deliver the
notification
> > via
> > > ObserverList and clients can post a task to whatever thread they want if
> > > necessary.
> > If it's not done here, it needs to be done in the base class, since the
> > ObserverList should be accessed on one thread. I think moving it to the base
> > class is better.
> 
> Since UserInputMonitor is owned by BrowserMainLoop, multiple threads may now
> register for user input... so registering yourself to a single callback task
> runner seems a recipe for disaster. 
> 
> Actually, I think the only way around this is a lock or a requirement that
> listeners are only added and removed on the UI or IO thread (depending on
> whether linux or mac/win is used).
> 
> Since you don't actually need the ability for reentrant removal of observers,
> you could switch them to std::lists protected by a single lock.

OK, I'll make it thread safe

[DaleCurtis, 2013-08-10 01:05:21]

Ah, you're right, I totally missed that DCHECK! Sad. We could still get at it
via a BrowserThread::PostTask(UI) call, but that seems even hackier.

[jiayl, 2013-08-12 17:52:59]

On 2013/08/10 00:48:18, Sergey Ulanov wrote:
>
https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
> File media/base/user_input_monitor.h (right):
> 
>
https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
> media/base/user_input_monitor.h:25: class MouseEventListener {
> please add comments for this interface and the ones below. I think it's better
> to make them nested in UserInputMonitor.
> 
Will do.

>
https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
> media/base/user_input_monitor.h:42: class KeyPressListener {
> Why do we need this given that there is RawKeyboardEventListener?
> 
Two reasons:
1. Mac will not support RawKeyboradEventListener but only KeyPressListener due
to permission issue.
2. There is no better place to host the impl of converting keyboard events into
key press notifications. And for Mac, it's the polling of keyboard states,
instead of a simple conversion.

>
https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
> File media/base/user_input_monitor_linux.cc (right):
> 
>
https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
> media/base/user_input_monitor_linux.cc:169: void
> UserInputMonitorLinux::SetOrVerifyCallingThread() {
> On 2013/08/10 00:13:12, DaleCurtis wrote:
> > Instead of trying to do this, which feels a bit sketchy, how about just
> letting
> > clients handle their own thread hopping?  I.e. you deliver the notification
> via
> > ObserverList and clients can post a task to whatever thread they want if
> > necessary.
> 
> It would mean that the clients of this interface have to be thread-aware,
which
> isn't good.
> I think ObserverListThreadSafe might be useful here - it will jump threads as
> needed (and so it can be used to make UserInputMonitor usable on different
> threads).
> 
I'll change it to be thread safe. But ObserverListThreadSafe is too limited in
that there is no API to get the size/empty-ness of the list, which is needed to
decide when to stop monitoring.

>
https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
> media/base/user_input_monitor_linux.cc:174:
> CHECK(callback_task_runner_->BelongsToCurrentThread());
> it's more common to use DCHECKs for thread checks.
> 
callback_task_runner_ will be removed.

>
https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
> media/base/user_input_monitor_linux.cc:270: base::MessageLoopForIO*
message_loop
> = base::MessageLoopForIO::current();
> On 2013/08/10 00:37:56, jiayl wrote:
> > On 2013/08/10 00:13:12, DaleCurtis wrote:
> > > ?? just use io_loop_ ?
> > Core doesn't have a member as io_loop_. It's always running on the IO loop.
> 
> Also, only ForIO version of MessageLoop has WatchFileDescriptor(), so io_loop_
> (which is a task runner wrapping the message loop) would not be useful here.

[Sergey Ulanov, 2013-08-12 18:53:59]

On Mon, Aug 12, 2013 at 10:52 AM, <jiayl@chromium.org> wrote:

>
> https://codereview.chromium.**org/21183002/diff/106001/**
>
media/base/user_input_monitor.**h#newcode42<https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_monitor.h#newcode42>
>
>> media/base/user_input_monitor.**h:42: class KeyPressListener {
>> Why do we need this given that there is RawKeyboardEventListener?
>>
>
> Two reasons:
> 1. Mac will not support RawKeyboradEventListener but only KeyPressListener
> due
> to permission issue.
> 2. There is no better place to host the impl of converting keyboard events
> into
> key press notifications. And for Mac, it's the polling of keyboard states,
> instead of a simple conversion.
>
>
AudioInputController implements only KeyPressListener and
RawKeyboradEventListener doesn't seem to be used anywhere. So maybe you
just don't need RawKeyboradEventListener, given that it can't work
correctly on all platforms?


https://codereview.chromium.**org/21183002/diff/106001/**
>
media/base/user_input_monitor_**linux.cc#newcode169<https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_monitor_linux.cc#newcode169>
>
>> media/base/user_input_monitor_**linux.cc:169: void
>> UserInputMonitorLinux::**SetOrVerifyCallingThread() {
>> On 2013/08/10 00:13:12, DaleCurtis wrote:
>> > Instead of trying to do this, which feels a bit sketchy, how about just
>> letting
>> > clients handle their own thread hopping?  I.e. you deliver the
>> notification
>> via
>> > ObserverList and clients can post a task to whatever thread they want if
>> > necessary.
>>
>
>  It would mean that the clients of this interface have to be thread-aware,
>>
> which
>
>> isn't good.
>> I think ObserverListThreadSafe might be useful here - it will jump
>> threads as
>> needed (and so it can be used to make UserInputMonitor usable on different
>> threads).
>>
>
> I'll change it to be thread safe. But ObserverListThreadSafe is too
> limited in
> that there is no API to get the size/empty-ness of the list, which is
> needed to
> decide when to stop monitoring.


I think it should be easy to workaround with an explicit counter in
UserInputMonitor, just need to make sure it's always updated on the same
thread. But that probably not worth it given that we expect all observers
to run on the same thread.

[jiayl, 2013-08-12 19:05:58]

PTAL. UserInputMonitor is now thread safe with lock to synchronize
adding/removing/calling listeners. The callback can be made on any thread and
the caller should take care of it.

Sergey,
RawKeyboardListener is supported for remoting to detect the disconnect hotkey on
Linux. If you think remoting does not need that, I can remove it.

Thanks!

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/106001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:169: void
UserInputMonitorLinux::SetOrVerifyCallingThread() {
On 2013/08/10 00:57:51, DaleCurtis wrote:
> On 2013/08/10 00:37:56, jiayl wrote:
> > On 2013/08/10 00:13:12, DaleCurtis wrote:
> > > Instead of trying to do this, which feels a bit sketchy, how about just
> > letting
> > > clients handle their own thread hopping?  I.e. you deliver the
notification
> > via
> > > ObserverList and clients can post a task to whatever thread they want if
> > > necessary.
> > If it's not done here, it needs to be done in the base class, since the
> > ObserverList should be accessed on one thread. I think moving it to the base
> > class is better.
> 
> Since UserInputMonitor is owned by BrowserMainLoop, multiple threads may now
> register for user input... so registering yourself to a single callback task
> runner seems a recipe for disaster. 
> 
> Actually, I think the only way around this is a lock or a requirement that
> listeners are only added and removed on the UI or IO thread (depending on
> whether linux or mac/win is used).
> 
> Since you don't actually need the ability for reentrant removal of observers,
> you could switch them to std::lists protected by a single lock.

Is there any problem with using ObserverList with a lock? It's nice to reuse the
duplicated observer checking and such of ObserverList.

[Sergey Ulanov, 2013-08-12 19:27:13]

On Mon, Aug 12, 2013 at 12:05 PM, <jiayl@chromium.org> wrote:

> PTAL. UserInputMonitor is now thread safe with lock to synchronize
> adding/removing/calling listeners. The callback can be made on any thread
> and
> the caller should take care of it.
>
> Sergey,
> RawKeyboardListener is supported for remoting to detect the disconnect
> hotkey on
> Linux. If you think remoting does not need that, I can remove it.
>

Yeah, lets keep it out of this CL since you are not changing chromoting to
use the new code. It will be easy to add it back if necessary.


>
> Thanks!
>
>

[DaleCurtis, 2013-08-12 20:24:10]

https://codereview.chromium.org/21183002/diff/119001/media/audio/audio_input_...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/21183002/diff/119001/media/audio/audio_input_...
media/audio/audio_input_controller.cc:265: if (user_input_monitor_)
Should this be in the (state_ != kClosed) block? You might get a double removal
this way.

https://codereview.chromium.org/21183002/diff/119001/media/audio/audio_input_...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/21183002/diff/119001/media/audio/audio_input_...
media/audio/audio_input_controller.h:124: UserInputMonitor* user_input_monitor)
= 0;
You should document at the relevant call sites (i.e., here and all other public
interfaces) that this can be NULL.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
File media/base/user_input_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor.cc:51: DVLOG(2) << "Stoped keyboard monitoring.";
Stopped.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor.cc:69: DVLOG(2) << "Stoped keyboard monitoring.";
Stopped.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
File media/base/user_input_monitor.h (right):

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor.h:28: // The thread on which the listenters are
called is not guaranteed.
listeners.  Combine with previous line.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor.h:29: class UserInputMonitor {
Since callbacks are delivered on either the UI or IO thread, it might be nice to
document in UserInputMonitor that callers shouldn't do anything in the callbacks
that is intensive / blocks at all.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:194: if (type == MOUSE_EVENT) {
Add line above -- doesn't go with block above.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:252: XEvent ev;
Is a new XEvent necessary to stack allocate every time? Also instead of
duplicating this code section, just call OnFileCanReadWithoutBlocking()?

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:292: XEvent ev;
Ditto.

[jiayl, 2013-08-12 23:13:45]

PTAL. Thanks!

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
File media/base/user_input_monitor.cc (right):

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor.cc:51: DVLOG(2) << "Stoped keyboard monitoring.";
On 2013/08/12 20:24:11, DaleCurtis wrote:
> Stopped.

Done.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor.cc:69: DVLOG(2) << "Stoped keyboard monitoring.";
On 2013/08/12 20:24:11, DaleCurtis wrote:
> Stopped.

Done.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
File media/base/user_input_monitor.h (right):

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor.h:28: // The thread on which the listenters are
called is not guaranteed.
On 2013/08/12 20:24:11, DaleCurtis wrote:
> listeners.  Combine with previous line.

Done.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor.h:29: class UserInputMonitor {
On 2013/08/12 20:24:11, DaleCurtis wrote:
> Since callbacks are delivered on either the UI or IO thread, it might be nice
to
> document in UserInputMonitor that callers shouldn't do anything in the
callbacks
> that is intensive / blocks at all.

Done.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:194: if (type == MOUSE_EVENT) {
On 2013/08/12 20:24:11, DaleCurtis wrote:
> Add line above -- doesn't go with block above.

Done.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:252: XEvent ev;
On 2013/08/12 20:24:11, DaleCurtis wrote:
> Is a new XEvent necessary to stack allocate every time? Also instead of
> duplicating this code section, just call OnFileCanReadWithoutBlocking()?

Done.

https://codereview.chromium.org/21183002/diff/119001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:292: XEvent ev;
On 2013/08/12 20:24:11, DaleCurtis wrote:
> Ditto.

Done.

[DaleCurtis, 2013-08-12 23:24:32]

Using ObserverList is okay, but since you're not using any of its features, it's
really just overhead in what is likely to be a frequently called function.

Whether that overhead is actually measurable or not, I'm not sure.  In any case,
it's up to you, lgtm otherwise.  Thanks for your patience.

[Sergey Ulanov, 2013-08-13 20:12:50]

lgtm

https://codereview.chromium.org/21183002/diff/139001/media/base/user_input_mo...
File media/base/user_input_monitor_linux.cc (right):

https://codereview.chromium.org/21183002/diff/139001/media/base/user_input_mo...
media/base/user_input_monitor_linux.cc:46: MOUSE_EVENT = 0,
nit: no need to specify values here - 0 and 1 must be assigned by default
anyway.

[commit-bot: I haz the power, 2013-08-13 20:18:38]

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

[commit-bot: I haz the power, 2013-08-13 21:17:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-13 22:10:41]

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

[commit-bot: I haz the power, 2013-08-13 23:28:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-13 23:28:49]

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

[commit-bot: I haz the power, 2013-08-14 01:04:56]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, browser_tests, components_unittests,
content_browsertests, content_unittests, interactive_ui_tests, media_unittests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-14 01:12:41]

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

[commit-bot: I haz the power, 2013-08-14 02:25:57]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, browser_tests, components_unittests,
content_browsertests, content_unittests, interactive_ui_tests, media_unittests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-14 05:13:26]

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

[commit-bot: I haz the power, 2013-08-14 05:49:43]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, browser_tests, components_unittests,
content_browsertests, content_unittests, interactive_ui_tests, media_unittests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-14 16:01:39]

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

[commit-bot: I haz the power, 2013-08-14 16:02:04]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2013-08-14 16:25:30]

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

[commit-bot: I haz the power, 2013-08-14 17:34:11]

Retried try job too often on linux_chromeos for step(s) app_list_unittests,
ash_unittests, aura_unittests, browser_tests, components_unittests,
content_browsertests, content_unittests, interactive_ui_tests, media_unittests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-14 18:57:02]

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

[commit-bot: I haz the power, 2013-08-14 19:38:29]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-14 20:09:25]

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

[commit-bot: I haz the power, 2013-08-14 22:16:49]

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

[commit-bot: I haz the power, 2013-08-14 22:55:43]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-14 23:44:00]

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

[commit-bot: I haz the power, 2013-08-15 07:41:08]

Change committed as 217768

[commit-bot: I haz the power, 2013-08-15 19:42:57]

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

[commit-bot: I haz the power, 2013-08-15 22:02:45]

Change committed as 217844