Adds the UserInputMonitor implementation for Mac.

media/base/user_input_monitor_mac.cc

Index: media/base/user_input_monitor_mac.cc
diff --git a/media/base/user_input_monitor_mac.cc b/media/base/user_input_monitor_mac.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b1ddfa3d001bc0059cd9b2b6b416146a3fe10ddf
--- /dev/null
+++ b/media/base/user_input_monitor_mac.cc
@@ -0,0 +1,67 @@
+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "media/base/user_input_monitor.h"
+
+#include <ApplicationServices/ApplicationServices.h>
+
+namespace media {
+namespace {
+
+class UserInputMonitorMac : public UserInputMonitor {

[Mark Mentovai, 2013/08/23 18:09:03]

This is a total mess.

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

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

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

+ public:
+  explicit UserInputMonitorMac();
+  virtual ~UserInputMonitorMac();
+
+  virtual size_t GetKeyPressCount() const OVERRIDE;
+
+ private:
+  virtual void StartMouseMonitoring() OVERRIDE;
+  virtual void StopMouseMonitoring() OVERRIDE;
+  virtual void StartKeyboardMonitoring() OVERRIDE;
+  virtual void StopKeyboardMonitoring() OVERRIDE;
+
+  size_t initial_key_press_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserInputMonitorMac);
+};
+
+UserInputMonitorMac::UserInputMonitorMac() : initial_key_press_count_(0) {}
+
+UserInputMonitorMac::~UserInputMonitorMac() {
+  DCHECK(!initial_key_press_count_);
+}
+
+size_t UserInputMonitorMac::GetKeyPressCount() const {
+  size_t total_count = CGEventSourceCounterForEventType(
+      kCGEventSourceStateHIDSystemState, kCGEventKeyDown);

[Mark Mentovai, 2013/08/23 18:09:03]

I believe I understand why you used this given the intended application of this
code, but you should add a comment clarifying why you did so instead of using
the more usual kCGEventSourceStateCombinedSessionState.

+  return total_count - initial_key_press_count_;
+}
+
+void UserInputMonitorMac::StartMouseMonitoring() {
+  NOTREACHED();
+}
+
+void UserInputMonitorMac::StopMouseMonitoring() {
+  NOTREACHED();
+}
+
+void UserInputMonitorMac::StartKeyboardMonitoring() {
+  initial_key_press_count_ = CGEventSourceCounterForEventType(

[Mark Mentovai, 2013/08/23 18:09:03]

Since you shouldn’t reach this code without initial_key_press_count_ being 0
(which is something you can DCHECK_EQ), you can capture this with:

  initial_key_press_count_ = GetKeyPressCount();

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

+      kCGEventSourceStateHIDSystemState, kCGEventKeyDown);
+}
+
+void UserInputMonitorMac::StopKeyboardMonitoring() {
+  initial_key_press_count_ = 0;
+}
+
+}  // namespace
+
+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>(new UserInputMonitorMac());
+}
+
+}  // namespace media