Adds the UserInputMonitor implementation for Mac.

media/base/user_input_monitor_mac.mm

Index: media/base/user_input_monitor_mac.mm
diff --git a/media/base/user_input_monitor_mac.mm b/media/base/user_input_monitor_mac.mm
index 4ffad42a72d9b479d1611e4a10fea96ae41e0e5d..21bfbf96007973b2d809c0c7c07c5093baad44b6 100644
--- a/media/base/user_input_monitor_mac.mm
+++ b/media/base/user_input_monitor_mac.mm
@@ -4,13 +4,240 @@
 
 #include "media/base/user_input_monitor.h"
 
+#import <AppKit/AppKit.h>

[Mark Mentovai, 2013/08/21 18:32:43]

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.

[jiayl, 2013/08/21 22:58:22]

removed

+#include <ApplicationServices/ApplicationServices.h>
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/mac/scoped_cftyperef.h"
+#include "base/message_loop/message_loop.h"
+#include "base/single_thread_task_runner.h"
+#include "base/threading/non_thread_safe.h"
+#include "base/time/time.h"
+#include "third_party/skia/include/core/SkPoint.h"
+#import "third_party/GTM/AppKit/GTMCarbonEvent.h"

[Mark Mentovai, 2013/08/21 18:32:43]

I can’t tell what you’ve included this for either. Same thing as the previous
comment.

[jiayl, 2013/08/21 22:58:22]

removed

+
 namespace media {
+namespace {
+
+class UserInputMonitorMacCore;
+
+class UserInputMonitorMac : public base::NonThreadSafe,

[Mark Mentovai, 2013/08/21 18:32:43]

…so this is the implementation class?

[jiayl, 2013/08/21 22:58:22]

Merged with *Core now.

+                            public UserInputMonitor {
+ public:
+  typedef const base::Callback<void(const SkIPoint&)> MouseCallback;
+
+  UserInputMonitorMac(
+      scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner);
+
+  virtual ~UserInputMonitorMac();
+
+ private:
+  virtual void StartMouseMonitoring() OVERRIDE;
+  virtual void StopMouseMonitoring() OVERRIDE;
+  virtual void StartKeyboardMonitoring() OVERRIDE;
+  virtual void StopKeyboardMonitoring() OVERRIDE;
+
+  void OnMouseMoved(const SkIPoint& position);
+  void OnKeyStroke();
+
+  // Task runner on which X Window events are received.

[Mark Mentovai, 2013/08/21 18:32:43]

X Window events? Really?

[jiayl, 2013/08/21 22:58:22]

fixed

+  scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner_;
+
+  scoped_ptr<UserInputMonitorMacCore> core_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserInputMonitorMac);
+};
+
+// The class implements the event listening. Must be called on the UI thread.

[Mark Mentovai, 2013/08/21 18:32:43]

…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.

[Mark Mentovai, 2013/08/21 18:32:43]

You need to assert what this comment says with DCHECKs in the code.

[jiayl, 2013/08/21 22:58:22]

Done.

[jiayl, 2013/08/21 22:58:22]

Done.

[Wez, 2013/08/21 23:19:53]

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.

+class UserInputMonitorMacCore {
+ public:
+  UserInputMonitorMacCore(
+      const UserInputMonitorMac::MouseCallback& mouse_callback,
+      const base::Closure& key_stroke_callback);
+  virtual ~UserInputMonitorMacCore();

[Mark Mentovai, 2013/08/21 18:32:43]

This destructor doesn’t need to be virtual.

[jiayl, 2013/08/21 22:58:22]

Done.

+
+  void StartMonitoringMouse();
+  void StopMonitoringMouse();
+  void StartPollingKeyState();
+  void StopPollingKeyState();
+
+  void OnMouseMoved(const SkIPoint& position);
+
+  base::WeakPtr<UserInputMonitorMacCore> GetAsWeakPtr();
+
+ private:
+  static CGEventRef MouseMoved(CGEventTapProxy proxy,
+                               CGEventType type,
+                               CGEventRef event,
+                               void* context);
+  void PollKeyState();
+
+  UserInputMonitorMac::MouseCallback mouse_callback_;
+  base::Closure key_stroke_callback_;
+
+  CFRunLoopSourceRef input_run_loop_source_;
+  base::ScopedCFTypeRef<CFMachPortRef> input_mach_port_;

[Mark Mentovai, 2013/08/21 18:32:43]

Why is this in a scoper but input_run_loop_source_ bare? Their lifetimes seem to
be identical. I happen to like the scoper.

[jiayl, 2013/08/21 22:58:22]

Done.

+
+  bool polling_key_state_;
+  // 0x5d is key "9", after that comes function keys.

[Mark Mentovai, 2013/08/21 18:32:43]

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.

[jiayl, 2013/08/21 22:58:22]

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.

+  bool prev_key_state_[0x5d];
+
+  base::WeakPtrFactory<UserInputMonitorMacCore> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserInputMonitorMacCore);
+};
+
+UserInputMonitorMac::UserInputMonitorMac(
+    scoped_refptr<base::SingleThreadTaskRunner> ui_task_runner)
+    : ui_task_runner_(ui_task_runner),
+      core_(new UserInputMonitorMacCore(
+          base::Bind(&UserInputMonitorMac::OnMouseMoved,
+                     base::Unretained(this)),
+          base::Bind(&UserInputMonitorMac::OnKeyStroke,
+                     base::Unretained(this)))) {}
+
+UserInputMonitorMac::~UserInputMonitorMac() {
+  ui_task_runner_->DeleteSoon(FROM_HERE, core_.release());
+}
+
+void UserInputMonitorMac::StartMouseMonitoring() {
+  ui_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&UserInputMonitorMacCore::StartMonitoringMouse,
+                            core_->GetAsWeakPtr()));
+}
+
+void UserInputMonitorMac::StopMouseMonitoring() {
+  ui_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&UserInputMonitorMacCore::StopMonitoringMouse,
+                            core_->GetAsWeakPtr()));
+}
+
+void UserInputMonitorMac::StartKeyboardMonitoring() {
+  ui_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&UserInputMonitorMacCore::StartPollingKeyState,
+                            core_->GetAsWeakPtr()));
+}
+
+void UserInputMonitorMac::StopKeyboardMonitoring() {
+  ui_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&UserInputMonitorMacCore::StopPollingKeyState,
+                            core_->GetAsWeakPtr()));
+}
+
+void UserInputMonitorMac::OnMouseMoved(const SkIPoint& position) {
+  UserInputMonitor::OnMouseEvent(position);
+}
+
+void UserInputMonitorMac::OnKeyStroke() {
+  UserInputMonitor::OnKeyboardEvent(ui::ET_KEY_PRESSED, ui::VKEY_UNKNOWN);
+}
+
+UserInputMonitorMacCore::UserInputMonitorMacCore(
+    const UserInputMonitorMac::MouseCallback& mouse_callback,
+    const base::Closure& key_stroke_callback)
+    : mouse_callback_(mouse_callback),
+      key_stroke_callback_(key_stroke_callback),
+      polling_key_state_(false),
+      weak_factory_(this) {}
+
+UserInputMonitorMacCore::~UserInputMonitorMacCore() {
+  DCHECK(!input_mach_port_);
+  DCHECK(!input_run_loop_source_);
+  DCHECK(!polling_key_state_);
+}
+
+void UserInputMonitorMacCore::StartMonitoringMouse() {

[Mark Mentovai, 2013/08/21 18:32:43]

Some or all of these functions need to run on the main thread. You should DCHECK
that you’re on the right thread.

[jiayl, 2013/08/21 22:58:22]

Done.

+  input_mach_port_.reset(CGEventTapCreate(
+      kCGSessionEventTap, kCGHeadInsertEventTap, kCGEventTapOptionListenOnly,
+      CGEventMaskBit(kCGEventMouseMoved), MouseMoved, this));
+  if (input_mach_port_) {
+    input_run_loop_source_ = CFMachPortCreateRunLoopSource(
+        NULL, input_mach_port_, 0);
+    CFRunLoopAddSource(
+        CFRunLoopGetMain(), input_run_loop_source_, kCFRunLoopCommonModes);
+  } else {
+    LOG(ERROR) << "CGEventTapCreate failed.";
+  }
+}
+
+void UserInputMonitorMacCore::StopMonitoringMouse() {
+  if (input_run_loop_source_) {
+    CFMachPortInvalidate(input_mach_port_);
+    CFRunLoopRemoveSource(
+        CFRunLoopGetMain(), input_run_loop_source_, kCFRunLoopCommonModes);
+    CFRelease(input_run_loop_source_);
+    input_mach_port_.reset();
+    input_run_loop_source_ = NULL;
+  }
+}
+
+void UserInputMonitorMacCore::StartPollingKeyState() {
+  polling_key_state_ = true;
+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&UserInputMonitorMacCore::PollKeyState, GetAsWeakPtr()),
+      base::TimeDelta::FromMilliseconds(10));

[Mark Mentovai, 2013/08/21 18:32:43]

And why can’t you use an event tap for this?

[jiayl, 2013/08/21 22:58:22]

Due to permission issues stated before.

+}
+
+void UserInputMonitorMacCore::StopPollingKeyState() {
+  polling_key_state_ = false;
+}
+
+base::WeakPtr<UserInputMonitorMacCore> UserInputMonitorMacCore::GetAsWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
+void UserInputMonitorMacCore::OnMouseMoved(const SkIPoint& position) {
+  mouse_callback_.Run(position);
+}
+
+CGEventRef UserInputMonitorMacCore::MouseMoved(CGEventTapProxy proxy,
+                                               CGEventType type,
+                                               CGEventRef event,
+                                               void* context) {
+  int64_t pid = CGEventGetIntegerValueField(event, kCGEventSourceUnixProcessID);
+  if (pid == 0) {

[Mark Mentovai, 2013/08/21 18:32:43]

Why? What’s 0 mean here? You need a comment to explain this.

[jiayl, 2013/08/21 22:58:22]

Done.

+    DCHECK(type == kCGEventMouseMoved);

[Mark Mentovai, 2013/08/21 18:32:43]

DCHECK_EQ

[jiayl, 2013/08/21 22:58:22]

Done.

+    CGPoint cg_mouse_pos = CGEventGetLocation(event);
+    SkIPoint mouse_pos = SkIPoint::Make(cg_mouse_pos.x, cg_mouse_pos.y);
+    static_cast<UserInputMonitorMacCore*>(context)->OnMouseMoved(mouse_pos);
+  }
+  return NULL;
+}
+
+void UserInputMonitorMacCore::PollKeyState() {
+  if (!polling_key_state_)
+    return;
+
+  bool key_pressed = false;
+  for (uint key_index = 0;

[Mark Mentovai, 2013/08/21 18:32:43]

CGKeyCode, not uint.

[jiayl, 2013/08/21 22:58:22]

Done.

+      key_index <= sizeof(prev_key_state_)/sizeof(prev_key_state_[0]);

[Mark Mentovai, 2013/08/21 18:32:43]

This is arraysize, you don’t need to write the cumbersome expression on your
own.

[jiayl, 2013/08/21 22:58:22]

Done.

+      ++key_index) {
+    bool key_state = CGEventSourceKeyState(kCGEventSourceStateHIDSystemState,

[Mark Mentovai, 2013/08/21 18:32:43]

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.

[jiayl, 2013/08/21 22:58:22]

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).

+                                           key_index);
+
+    // A false -> true change in keymap means a key is pressed.
+    key_pressed |= (key_state && !prev_key_state_[key_index]);
+    // Save current state.
+    prev_key_state_[key_index] = key_state;
+  }
+
+  if (key_pressed)
+    key_stroke_callback_.Run();
+
+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&UserInputMonitorMacCore::PollKeyState, GetAsWeakPtr()),
+      base::TimeDelta::FromMilliseconds(10));

[Mark Mentovai, 2013/08/21 18:32:43]

1. I think this business of polling is nonsense.

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.

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.

[Wez, 2013/08/21 19:45:57]

Agreed. When did polling creep into this?

[jiayl, 2013/08/21 22:58:22]

Unfortunately I don't have a better solution.
Done.
Changed to 30ms. Not seeing misses when sweeping the keys quickly.

+}
+
+}  // namespace
 
-// TODO(jiayl): add the implementation.
 scoped_ptr<UserInputMonitor> UserInputMonitor::Create(
     const scoped_refptr<base::SingleThreadTaskRunner>& input_task_runner,
     const scoped_refptr<base::SingleThreadTaskRunner>& ui_task_runner) {
-  return scoped_ptr<UserInputMonitor>();
+  return scoped_ptr<UserInputMonitor>(new UserInputMonitorMac(ui_task_runner));
 }
 
 }  // namespace media