Prevent menus from blocking user activity detection

This is a retry CL for https://codereview.chromium.org/1024583003/ with
a slight change.

UserActivityDetector used to be an EventHandler triggered when the pre-
target event handlers are triggered. When a menu is present, the
MenuEventDispatcher doesn't trigger pre-target handlers and hence user
activity is not detected.
With this change the UserActivityDetector is made a PlatformEventObserver
to make sure it's always triggered.

R=sadrul@chromium.org, jamescook@chromium.org
BUG=462735
TEST=ui_base_unittests --gtest_filter=UserActivityDetectorTest.*

Review URL: https://codereview.chromium.org/1074543002

Cr-Commit-Position: refs/heads/master@{#324366}

ash/shell.cc

@@ -678,7 +678,6 @@ Shell::~Shell() {
   RemovePreTargetHandler(speech_feedback_handler_.get());
   speech_feedback_handler_.reset();
 #endif
-  RemovePreTargetHandler(user_activity_detector_.get());
   RemovePreTargetHandler(overlay_filter_.get());
   RemovePreTargetHandler(input_method_filter_.get());
   RemovePreTargetHandler(accelerator_filter_.get());
@@ -922,7 +921,6 @@ void Shell::Init(const ShellInitParams& init_params) {
   // ui::UserActivityDetector passes events to observers, so let them get
   // rewritten first.
   user_activity_detector_.reset(new ui::UserActivityDetector);
-  AddPreTargetHandler(user_activity_detector_.get());
 
   overlay_filter_.reset(new OverlayEventFilter);
   AddPreTargetHandler(overlay_filter_.get());

extensions/shell/browser/shell_desktop_controller_aura.cc

@@ -279,8 +279,6 @@ void ShellDesktopControllerAura::InitWindowManager() {
   aura::client::SetCursorClient(host_->window(), cursor_manager_.get());
 
   user_activity_detector_.reset(new ui::UserActivityDetector);
-  host_->event_processor()->GetRootTarget()->AddPreTargetHandler(
-      user_activity_detector_.get());
 #if defined(OS_CHROMEOS)
   user_activity_notifier_.reset(
       new ui::UserActivityPowerManagerNotifier(user_activity_detector_.get()));
@@ -324,10 +322,6 @@ void ShellDesktopControllerAura::DestroyRootWindow() {
   host_->RemoveObserver(this);
   if (input_method_filter_)
     root_window_event_filter_->RemoveHandler(input_method_filter_.get());
-  if (user_activity_detector_) {
-    host_->event_processor()->GetRootTarget()->RemovePreTargetHandler(
-        user_activity_detector_.get());
-  }
   wm::FocusController* focus_controller =
       static_cast<wm::FocusController*>(focus_client_.get());
   if (focus_controller) {

ui/base/user_activity/user_activity_detector.cc

@@ -8,7 +8,8 @@
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "ui/base/user_activity/user_activity_observer.h"
-#include "ui/events/event.h"
+#include "ui/events/event_utils.h"
+#include "ui/events/platform/platform_event_source.h"
 
 namespace ui {
 
@@ -48,9 +49,24 @@ const int UserActivityDetector::kDisplayPowerChangeIgnoreMouseMs = 1000;
 UserActivityDetector::UserActivityDetector() {
   CHECK(!g_instance);
   g_instance = this;
+
+  ui::PlatformEventSource* platform_event_source =
+      ui::PlatformEventSource::GetInstance();
+#if defined(OS_CHROMEOS) || defined(OS_LINUX)
+  CHECK(platform_event_source);
+#endif
+  if (platform_event_source)
+    platform_event_source->AddPlatformEventObserver(this);
 }
 
 UserActivityDetector::~UserActivityDetector() {
+  ui::PlatformEventSource* platform_event_source =
+      ui::PlatformEventSource::GetInstance();
+#if defined(OS_CHROMEOS) || defined(OS_LINUX)
+  CHECK(platform_event_source);
+#endif
+  if (platform_event_source)
+    platform_event_source->RemovePlatformEventObserver(this);
   g_instance = nullptr;
 }
 
@@ -77,36 +93,31 @@ void UserActivityDetector::OnDisplayPowerChanging() {
       base::TimeDelta::FromMilliseconds(kDisplayPowerChangeIgnoreMouseMs);
 }
 
-void UserActivityDetector::OnKeyEvent(ui::KeyEvent* event) {
-  HandleActivity(event);
+void UserActivityDetector::DidProcessEvent(
+    const PlatformEvent& platform_event) {
+  scoped_ptr<ui::Event> event(ui::EventFromNative(platform_event));
+  ProcessReceivedEvent(event.get());
 }
 
-void UserActivityDetector::OnMouseEvent(ui::MouseEvent* event) {
-  if (event->flags() & ui::EF_IS_SYNTHESIZED)
-    return;
-  if (!honor_mouse_events_time_.is_null() &&
-      GetCurrentTime() < honor_mouse_events_time_)
-    return;
-
-  HandleActivity(event);
+base::TimeTicks UserActivityDetector::GetCurrentTime() const {
+  return !now_for_test_.is_null() ? now_for_test_ : base::TimeTicks::Now();
 }
 
-void UserActivityDetector::OnScrollEvent(ui::ScrollEvent* event) {
-  HandleActivity(event);
-}
+void UserActivityDetector::ProcessReceivedEvent(const ui::Event* event) {
+  if (!event)
+    return;
 
-void UserActivityDetector::OnTouchEvent(ui::TouchEvent* event) {
-  HandleActivity(event);
-}
+  if (event->IsMouseEvent() || event->IsMouseWheelEvent()) {
+    if (event->flags() & ui::EF_IS_SYNTHESIZED)
+      return;
+    if (!honor_mouse_events_time_.is_null()
+        && GetCurrentTime() < honor_mouse_events_time_)
+      return;
+  }
 
-void UserActivityDetector::OnGestureEvent(ui::GestureEvent* event) {
   HandleActivity(event);
 }
 
-base::TimeTicks UserActivityDetector::GetCurrentTime() const {
-  return !now_for_test_.is_null() ? now_for_test_ : base::TimeTicks::Now();
-}
-
 void UserActivityDetector::HandleActivity(const ui::Event* event) {
   base::TimeTicks now = GetCurrentTime();
   last_activity_time_ = now;

ui/base/user_activity/user_activity_detector.h

@@ -10,14 +10,15 @@
 #include "base/observer_list.h"
 #include "base/time/time.h"
 #include "ui/base/ui_base_export.h"
-#include "ui/events/event_handler.h"
+#include "ui/events/event.h"
+#include "ui/events/platform/platform_event_observer.h"
 
 namespace ui {
 
 class UserActivityObserver;
 
 // Watches for input events and notifies observers that the user is active.
-class UI_BASE_EXPORT UserActivityDetector : public ui::EventHandler {
+class UI_BASE_EXPORT UserActivityDetector : public ui::PlatformEventObserver {
  public:
   // Minimum amount of time between notifications to observers.
   static const int kNotifyIntervalMs;
@@ -43,17 +44,19 @@ class UI_BASE_EXPORT UserActivityDetector : public ui::EventHandler {
   // Called when displays are about to be turned on or off.
   void OnDisplayPowerChanging();
 
-  // ui::EventHandler implementation.
-  void OnKeyEvent(ui::KeyEvent* event) override;
-  void OnMouseEvent(ui::MouseEvent* event) override;
-  void OnScrollEvent(ui::ScrollEvent* event) override;
-  void OnTouchEvent(ui::TouchEvent* event) override;
-  void OnGestureEvent(ui::GestureEvent* event) override;
+  // ui::PlatformEventObserver:
+  void WillProcessEvent(const PlatformEvent& platform_event) override {}
+  void DidProcessEvent(const PlatformEvent& platform_event) override;
 
  private:
+  friend class UserActivityDetectorTest;
+
   // Returns |now_for_test_| if set or base::TimeTicks::Now() otherwise.
   base::TimeTicks GetCurrentTime() const;
 
+  // Processes the event after it has been converted from a PlatformEvent.
+  void ProcessReceivedEvent(const ui::Event* event);
+
   // Updates |last_activity_time_|.  Additionally notifies observers and
   // updates |last_observer_notification_time_| if enough time has passed
   // since the last notification.

ui/base/user_activity/user_activity_detector_unittest.cc

@@ -13,6 +13,7 @@
 #include "ui/events/event_constants.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/keycodes/keyboard_codes.h"
+#include "ui/events/platform/platform_event_source.h"
 #include "ui/gfx/geometry/point.h"
 
 namespace ui {
@@ -36,10 +37,22 @@ class TestUserActivityObserver : public UserActivityObserver {
   DISALLOW_COPY_AND_ASSIGN(TestUserActivityObserver);
 };
 
+// A test implementation of PlatformEventSource that we can instantiate to make
+// sure that the PlatformEventSource has an instance while in unit tests.
+class TestPlatformEventSource : public ui::PlatformEventSource {
+ public:
+  TestPlatformEventSource() {}
+  ~TestPlatformEventSource() override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestPlatformEventSource);
+};
+
 class UserActivityDetectorTest : public testing::Test {
  public:
   UserActivityDetectorTest()
-      : detector_(new UserActivityDetector),
+      : platform_event_source_(new TestPlatformEventSource),
+        detector_(new UserActivityDetector),
         observer_(new TestUserActivityObserver) {
     detector_->AddObserver(observer_.get());
     now_ = base::TimeTicks::Now();
@@ -57,6 +70,11 @@ class UserActivityDetectorTest : public testing::Test {
     detector_->set_now_for_test(now_);
   }
 
+  void OnEvent(const ui::Event* event) {
+    detector_->ProcessReceivedEvent(event);
+  }
+
+  scoped_ptr<TestPlatformEventSource> platform_event_source_;
   scoped_ptr<UserActivityDetector> detector_;
   scoped_ptr<TestUserActivityObserver> observer_;
 
@@ -70,7 +88,7 @@ class UserActivityDetectorTest : public testing::Test {
 // events.
 TEST_F(UserActivityDetectorTest, Basic) {
   ui::KeyEvent key_event(ui::ET_KEY_PRESSED, ui::VKEY_A, ui::EF_NONE);
-  detector_->OnKeyEvent(&key_event);
+  OnEvent(&key_event);
   EXPECT_FALSE(key_event.handled());
   EXPECT_EQ(now_.ToInternalValue(),
             detector_->last_activity_time().ToInternalValue());
@@ -82,7 +100,7 @@ TEST_F(UserActivityDetectorTest, Basic) {
   AdvanceTime(advance_delta);
   ui::MouseEvent mouse_event(ui::ET_MOUSE_MOVED, gfx::Point(), gfx::Point(),
                              ui::EventTimeForNow(), ui::EF_NONE, ui::EF_NONE);
-  detector_->OnMouseEvent(&mouse_event);
+  OnEvent(&mouse_event);
   EXPECT_FALSE(mouse_event.handled());
   EXPECT_EQ(now_.ToInternalValue(),
             detector_->last_activity_time().ToInternalValue());
@@ -93,7 +111,7 @@ TEST_F(UserActivityDetectorTest, Basic) {
 
   // Temporarily ignore mouse events when displays are turned on or off.
   detector_->OnDisplayPowerChanging();
-  detector_->OnMouseEvent(&mouse_event);
+  OnEvent(&mouse_event);
   EXPECT_FALSE(mouse_event.handled());
   EXPECT_EQ(time_before_ignore.ToInternalValue(),
             detector_->last_activity_time().ToInternalValue());
@@ -104,7 +122,7 @@ TEST_F(UserActivityDetectorTest, Basic) {
       base::TimeDelta::FromMilliseconds(
           UserActivityDetector::kDisplayPowerChangeIgnoreMouseMs);
   AdvanceTime(kIgnoreMouseTime / 2);
-  detector_->OnMouseEvent(&mouse_event);
+  OnEvent(&mouse_event);
   EXPECT_FALSE(mouse_event.handled());
   EXPECT_EQ(time_before_ignore.ToInternalValue(),
             detector_->last_activity_time().ToInternalValue());
@@ -113,7 +131,7 @@ TEST_F(UserActivityDetectorTest, Basic) {
 
   // After enough time has passed, mouse events should be reported again.
   AdvanceTime(std::max(kIgnoreMouseTime, advance_delta));
-  detector_->OnMouseEvent(&mouse_event);
+  OnEvent(&mouse_event);
   EXPECT_FALSE(mouse_event.handled());
   EXPECT_EQ(now_.ToInternalValue(),
             detector_->last_activity_time().ToInternalValue());
@@ -123,7 +141,7 @@ TEST_F(UserActivityDetectorTest, Basic) {
   AdvanceTime(advance_delta);
   ui::TouchEvent touch_event(
       ui::ET_TOUCH_PRESSED, gfx::Point(), 0, base::TimeDelta());
-  detector_->OnTouchEvent(&touch_event);
+  OnEvent(&touch_event);
   EXPECT_FALSE(touch_event.handled());
   EXPECT_EQ(now_.ToInternalValue(),
             detector_->last_activity_time().ToInternalValue());
@@ -137,7 +155,7 @@ TEST_F(UserActivityDetectorTest, Basic) {
       ui::EF_NONE,
       base::TimeDelta::FromMilliseconds(base::Time::Now().ToDoubleT() * 1000),
       ui::GestureEventDetails(ui::ET_GESTURE_TAP));
-  detector_->OnGestureEvent(&gesture_event);
+  OnEvent(&gesture_event);
   EXPECT_FALSE(gesture_event.handled());
   EXPECT_EQ(now_.ToInternalValue(),
             detector_->last_activity_time().ToInternalValue());
@@ -149,14 +167,14 @@ TEST_F(UserActivityDetectorTest, Basic) {
 TEST_F(UserActivityDetectorTest, RateLimitNotifications) {
   // The observer should be notified about a key event.
   ui::KeyEvent event(ui::ET_KEY_PRESSED, ui::VKEY_A, ui::EF_NONE);
-  detector_->OnKeyEvent(&event);
+  OnEvent(&event);
   EXPECT_FALSE(event.handled());
   EXPECT_EQ(1, observer_->num_invocations());
   observer_->reset_stats();
 
   // It shouldn't be notified if a second event occurs in the same instant in
   // time.
-  detector_->OnKeyEvent(&event);
+  OnEvent(&event);
   EXPECT_FALSE(event.handled());
   EXPECT_EQ(0, observer_->num_invocations());
   observer_->reset_stats();
@@ -165,7 +183,7 @@ TEST_F(UserActivityDetectorTest, RateLimitNotifications) {
   AdvanceTime(
       base::TimeDelta::FromMilliseconds(
           UserActivityDetector::kNotifyIntervalMs - 100));
-  detector_->OnKeyEvent(&event);
+  OnEvent(&event);
   EXPECT_FALSE(event.handled());
   EXPECT_EQ(0, observer_->num_invocations());
   observer_->reset_stats();
@@ -175,7 +193,7 @@ TEST_F(UserActivityDetectorTest, RateLimitNotifications) {
   AdvanceTime(base::TimeDelta::FromMilliseconds(
       UserActivityDetector::kNotifyIntervalMs));
 
-  detector_->OnKeyEvent(&event);
+  OnEvent(&event);
   EXPECT_FALSE(event.handled());
   EXPECT_EQ(1, observer_->num_invocations());
 }
@@ -185,7 +203,7 @@ TEST_F(UserActivityDetectorTest, IgnoreSyntheticMouseEvents) {
   ui::MouseEvent mouse_event(ui::ET_MOUSE_MOVED, gfx::Point(), gfx::Point(),
                              ui::EventTimeForNow(), ui::EF_IS_SYNTHESIZED,
                              ui::EF_NONE);
-  detector_->OnMouseEvent(&mouse_event);
+  OnEvent(&mouse_event);
   EXPECT_FALSE(mouse_event.handled());
   EXPECT_EQ(base::TimeTicks().ToInternalValue(),
             detector_->last_activity_time().ToInternalValue());

ui/ozone/platform/test/ozone_platform_test.cc

@@ -25,6 +25,17 @@ namespace ui {
 
 namespace {
 
+// A test implementation of PlatformEventSource that we can instantiate to make
+// sure that the PlatformEventSource has an instance while in unit tests.
+class TestPlatformEventSource : public ui::PlatformEventSource {
+ public:
+  TestPlatformEventSource() {}
+  ~TestPlatformEventSource() override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestPlatformEventSource);
+};
+
 // OzonePlatform for testing
 //
 // This platform dumps images to a file for testing purposes.
@@ -67,7 +78,7 @@ class OzonePlatformTest : public OzonePlatform {
     window_manager_->Initialize();
     // This unbreaks tests that create their own.
     if (!PlatformEventSource::GetInstance())
-      platform_event_source_ = PlatformEventSource::CreateDefault();
+      platform_event_source_.reset(new TestPlatformEventSource);
     KeyboardLayoutEngineManager::SetKeyboardLayoutEngine(
         make_scoped_ptr(new StubKeyboardLayoutEngine()));