chromeos: run callbacks if event is discarded by a rewriter

If an EventRewriter discards the event, callbacks should be run immediately.
Without this, the callback never gets run.

BUG=898658
TEST=covered by tests

Change-Id: Ia2644890750fe328506d4fce3f575f327c06cd1f
Reviewed-on: https://chromium-review.googlesource.com/c/1371216
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615636}

ash/host/ash_window_tree_host_platform.cc

@@ -201,11 +201,12 @@ void AshWindowTreeHostPlatform::CommonInit() {
   SetSharedInputMethod(input_method_.get());
 }
 
-void AshWindowTreeHostPlatform::DispatchEventFromQueue(ui::Event* event) {
+ui::EventDispatchDetails AshWindowTreeHostPlatform::DispatchEventFromQueue(
+    ui::Event* event) {
   TRACE_EVENT0("input", "AshWindowTreeHostPlatform::DispatchEvent");
   if (event->IsLocatedEvent())
     TranslateLocatedEvent(static_cast<ui::LocatedEvent*>(event));
-  SendEventToSink(event);
+  return SendEventToSink(event);
 }
 
 void AshWindowTreeHostPlatform::SetTapToClickPaused(bool state) {

ash/host/ash_window_tree_host_platform.h

@@ -82,7 +82,7 @@ class ASH_EXPORT AshWindowTreeHostPlatform
                         ui::mojom::TextInputStatePtr state) override;
 
   // ws::HostEventDispatcher:
-  void DispatchEventFromQueue(ui::Event* event) override;
+  ui::EventDispatchDetails DispatchEventFromQueue(ui::Event* event) override;
 
  private:
   // All constructors call into this.

services/ws/event_queue.cc

@@ -55,16 +55,17 @@ std::unique_ptr<HostEventQueue> EventQueue::RegisterHostEventDispatcher(
 }
 
 // static
-void EventQueue::DispatchOrQueueEvent(WindowService* service,
-                                      aura::WindowTreeHost* window_tree_host,
-                                      ui::Event* event,
-                                      bool honor_rewriters) {
+base::Optional<ui::EventDispatchDetails> EventQueue::DispatchOrQueueEvent(
+    WindowService* service,
+    aura::WindowTreeHost* window_tree_host,
+    ui::Event* event,
+    bool honor_rewriters) {
   DCHECK(window_tree_host);
   HostEventQueue* host_event_queue =
       service->event_queue()->GetHostEventQueueForDisplay(
           window_tree_host->GetDisplayId());
   DCHECK(host_event_queue);
-  host_event_queue->DispatchOrQueueEvent(event, honor_rewriters);
+  return host_event_queue->DispatchOrQueueEvent(event, honor_rewriters);
 }
 
 bool EventQueue::ShouldQueueEvent(HostEventQueue* host_queue,

services/ws/event_queue.h

@@ -26,6 +26,7 @@ class WindowTreeHost;
 
 namespace ui {
 class Event;
+struct EventDispatchDetails;
 }
 
 namespace ws {
@@ -54,11 +55,13 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) EventQueue
   // is true, the event is passed through EventRewriters first. Events received
   // from the platform go through EventRewriters, so generally
   // |honor_rewriters| should be true, remote injection may need to circumvent
-  // that though.
-  static void DispatchOrQueueEvent(WindowService* service,
-                                   aura::WindowTreeHost* window_tree_host,
-                                   ui::Event* event,
-                                   bool honor_rewriters);
+  // that though. If the event was not queued, the return value contains the
+  // result of the event processing.
+  static base::Optional<ui::EventDispatchDetails> DispatchOrQueueEvent(
+      WindowService* service,
+      aura::WindowTreeHost* window_tree_host,
+      ui::Event* event,
+      bool honor_rewriters);
 
   // Returns true if |event| should be queued at this time.
   bool ShouldQueueEvent(HostEventQueue* host, const ui::Event& event);

services/ws/host_event_dispatcher.h

@@ -10,6 +10,7 @@
 
 namespace ui {
 class Event;
+struct EventDispatchDetails;
 }
 
 namespace ws {
@@ -20,7 +21,7 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) HostEventDispatcher {
   // NOTE: as with other event dispatch related functions, the *caller* owns
   // |event|, but HostEventDispatcher may modify |event| as necessary (but not
   // delete it).
-  virtual void DispatchEventFromQueue(ui::Event* event) = 0;
+  virtual ui::EventDispatchDetails DispatchEventFromQueue(ui::Event* event) = 0;
 
  protected:
   virtual ~HostEventDispatcher() = default;

services/ws/host_event_queue.cc

@@ -25,22 +25,22 @@ HostEventQueue::~HostEventQueue() {
     event_queue_->OnHostEventQueueDestroyed(this);
 }
 
-void HostEventQueue::DispatchOrQueueEvent(ui::Event* event,
-                                          bool honor_rewriters) {
-  if (event_queue_ && event_queue_->ShouldQueueEvent(this, *event))
+base::Optional<ui::EventDispatchDetails> HostEventQueue::DispatchOrQueueEvent(
+    ui::Event* event,
+    bool honor_rewriters) {
+  if (event_queue_ && event_queue_->ShouldQueueEvent(this, *event)) {
     event_queue_->QueueEvent(this, *event, honor_rewriters);
-  else
-    DispatchEventDontQueue(event, honor_rewriters);
+    return base::nullopt;
+  }
+  return DispatchEventDontQueue(event, honor_rewriters);
 }
 
-void HostEventQueue::DispatchEventDontQueue(ui::Event* event,
-                                            bool honor_rewriters) {
-  if (honor_rewriters) {
-    host_event_dispatcher_->DispatchEventFromQueue(event);
-  } else {
-    // No need to do anything with the result of sending the event.
-    ignore_result(window_tree_host_->event_sink()->OnEventFromSource(event));
-  }
+ui::EventDispatchDetails HostEventQueue::DispatchEventDontQueue(
+    ui::Event* event,
+    bool honor_rewriters) {
+  if (honor_rewriters)
+    return host_event_dispatcher_->DispatchEventFromQueue(event);
+  return window_tree_host_->event_sink()->OnEventFromSource(event);
 }
 
 }  // namespace ws

services/ws/host_event_queue.h

@@ -8,6 +8,7 @@
 #include "base/component_export.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 
 namespace aura {
 class WindowTreeHost;
@@ -15,6 +16,7 @@ class WindowTreeHost;
 
 namespace ui {
 class Event;
+struct EventDispatchDetails;
 }
 
 namespace ws {
@@ -37,8 +39,12 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) HostEventQueue {
   ~HostEventQueue();
 
   // If necessary, queues the event. If the event need not be queued,
-  // HostEventDispatcher::DispatchEventFromQueue() is called synchronously.
-  void DispatchOrQueueEvent(ui::Event* event, bool honor_rewriters = true);
+  // HostEventDispatcher::DispatchEventFromQueue() is called synchronously. If
+  // the event was not queued, the return value contains the result of the
+  // event processing.
+  base::Optional<ui::EventDispatchDetails> DispatchOrQueueEvent(
+      ui::Event* event,
+      bool honor_rewriters = true);
 
   aura::WindowTreeHost* window_tree_host() { return window_tree_host_; }
 
@@ -51,7 +57,8 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) HostEventQueue {
 
   // Dispatches an event directly, circumventing any queuing. This is private as
   // it's only useful internally.
-  void DispatchEventDontQueue(ui::Event* event, bool honor_rewriters);
+  ui::EventDispatchDetails DispatchEventDontQueue(ui::Event* event,
+                                                  bool honor_rewriters);
 
   // Because of shutdown ordering, HostEventQueue may be deleted *after*
   // EventQueue.

services/ws/injected_event_handler.cc

@@ -57,13 +57,19 @@ void InjectedEventHandler::Inject(std::unique_ptr<ui::Event> event,
   auto this_ref = weak_factory_.GetWeakPtr();
   pre_target_register_ = std::make_unique<ScopedPreTargetRegister>(
       window_service_->delegate()->GetGlobalEventTarget(), this);
-  EventQueue::DispatchOrQueueEvent(window_service_, window_tree_host_,
-                                   event.get(), /* honors_rewriters */ true);
+  auto result = EventQueue::DispatchOrQueueEvent(window_service_,
+                                                 window_tree_host_, event.get(),
+                                                 /* honors_rewriters */ true);
   if (!this_ref)
     return;
   // |pre_target_register_| needs to be a member to ensure it's destroyed
   // if |this| is destroyed.
   pre_target_register_.reset();
+
+  if (result && result->event_discarded) {
+    DCHECK(!event_id_);
+    NotifyCallback();
+  }
 }
 
 void InjectedEventHandler::NotifyCallback() {

services/ws/injected_event_handler.h

@@ -34,12 +34,13 @@ class WindowService;
 //
 // Implementation note: an event may be handled in any of the following
 // ways:
-// . It may be dispatched to remote client. This is detected by way of
+// . It may be dispatched to a remote client. This is detected by way of
 //   OnWillSendEventToClient().
 // . It may be handled locally.
 // . It may be held and processed later on. This happens if pointer events are
 //   being held if WindowEventDispatcher. See
 //   WindowEventDispatcher::HoldPointerMoves() for details.
+// . It may be discarded by an EventRewriter.
 // For the case of the event being sent to a remote client this has to ensure
 // if the remote client, or WindowTreeHost is destroyed, then no ack is
 // received.

services/ws/injected_event_handler_unittest.cc

@@ -21,6 +21,9 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/aura/test/window_event_dispatcher_test_api.h"
 #include "ui/aura/window.h"
+#include "ui/events/event.h"
+#include "ui/events/event_rewriter.h"
+#include "ui/events/keycodes/dom/dom_code.h"
 #include "ui/platform_window/platform_window_init_properties.h"
 
 namespace ws {
@@ -150,4 +153,59 @@ TEST(InjectedEventHandlerTest, HeldEvent) {
   EXPECT_TRUE(was_callback_run);
 }
 
+class DiscardingEventRewriter : public ui::EventRewriter {
+ public:
+  DiscardingEventRewriter() = default;
+  ~DiscardingEventRewriter() override = default;
+
+  bool got_rewrite_event() const { return got_rewrite_event_; }
+
+  // ui::EventRewriter:
+  ui::EventRewriteStatus RewriteEvent(
+      const ui::Event& event,
+      std::unique_ptr<ui::Event>* new_event) override {
+    got_rewrite_event_ = true;
+    return ui::EVENT_REWRITE_DISCARD;
+  }
+  ui::EventRewriteStatus NextDispatchEvent(
+      const ui::Event& last_event,
+      std::unique_ptr<ui::Event>* new_event) override {
+    return ui::EVENT_REWRITE_DISPATCH_ANOTHER;
+  }
+
+ private:
+  bool got_rewrite_event_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(DiscardingEventRewriter);
+};
+
+TEST(InjectedEventHandlerTest, RewriterDiscards) {
+  // Create a single window and give it focus.
+  DiscardingEventRewriter rewriter;
+  WindowServiceTestSetup test_setup;
+  test_setup.aura_test_helper()->root_window()->GetHost()->AddEventRewriter(
+      &rewriter);
+  aura::Window* top_level =
+      test_setup.window_tree_test_helper()->NewTopLevelWindow();
+  ASSERT_TRUE(top_level);
+  top_level->SetBounds(gfx::Rect(0, 0, 100, 100));
+  top_level->Show();
+  top_level->Focus();
+  EXPECT_TRUE(top_level->HasFocus());
+
+  // Dispatch an event. Because the rewriter discards the event, the callback
+  // should be run immediately.
+  bool was_callback_run = false;
+  InjectedEventHandler injected_event_handler(test_setup.service(),
+                                              test_setup.root()->GetHost());
+
+  ui::KeyEvent char_event('A', ui::VKEY_A, ui::DomCode::NONE, 0);
+  injected_event_handler.Inject(
+      ui::Event::Clone(char_event),
+      base::BindOnce([](bool* was_run) { *was_run = true; },
+                     &was_callback_run));
+  EXPECT_TRUE(was_callback_run);
+  EXPECT_TRUE(rewriter.got_rewrite_event());
+}
+
 }  // namespace ws

services/ws/test_host_event_dispatcher.cc

@@ -18,9 +18,9 @@ TestHostEventDispatcher::TestHostEventDispatcher(
 
 TestHostEventDispatcher::~TestHostEventDispatcher() = default;
 
-void TestHostEventDispatcher::DispatchEventFromQueue(ui::Event* event) {
-  ignore_result(
-      ui::EventSourceTestApi(window_tree_host_).SendEventToSink(event));
+ui::EventDispatchDetails TestHostEventDispatcher::DispatchEventFromQueue(
+    ui::Event* event) {
+  return ui::EventSourceTestApi(window_tree_host_).SendEventToSink(event);
 }
 
 }  // namespace ws

services/ws/test_host_event_dispatcher.h

@@ -21,7 +21,7 @@ class TestHostEventDispatcher : public HostEventDispatcher {
   ~TestHostEventDispatcher() override;
 
   // HostEventDispatcher:
-  void DispatchEventFromQueue(ui::Event* event) override;
+  ui::EventDispatchDetails DispatchEventFromQueue(ui::Event* event) override;
 
  private:
   aura::WindowTreeHost* window_tree_host_;

testing/buildbot/filters/chromeos.single_process_mash.interactive_ui_tests.filter

@@ -21,11 +21,6 @@
 -SelectToSpeakTest.SmoothlyReadsAcrossMultipleLines
 -SelectToSpeakTest.SpeakStatusTray
 -SelectToSpeakTest.WorksWithStickyKeys
--StickyKeysBrowserTest.CtrlClickHomeButton
--StickyKeysBrowserTest.OpenNewTabs
--StickyKeysBrowserTest.OpenTrayMenu
--StickyKeysBrowserTest.OverlayShown
--StickyKeysBrowserTest.SearchLeftOmnibox
 -TestAsNormalAndGuestUser/SpokenFeedbackTest.ChromeVoxNextTabRecovery/0
 -TestAsNormalAndGuestUser/SpokenFeedbackTest.ChromeVoxNextTabRecovery/1
 -TestAsNormalAndGuestUser/SpokenFeedbackTest.ChromeVoxShiftSearch/0
@@ -37,6 +32,10 @@
 -TestAsNormalAndGuestUser/SpokenFeedbackTest.OverviewMode/0
 -TestAsNormalAndGuestUser/SpokenFeedbackTest.OverviewMode/1
 
+# crbug.com/913549
+-StickyKeysBrowserTest.OpenTrayMenu
+-StickyKeysBrowserTest.SearchLeftOmnibox
+
 # This test is flaky. https://crbug.com/897879
 -ExtensionApiTest.DisplayModeWindowIsInFullscreen
 

ui/events/event_dispatcher.h

@@ -18,11 +18,11 @@ class EventDispatcher;
 class EventTarget;
 
 struct EventDispatchDetails {
-  EventDispatchDetails()
-      : dispatcher_destroyed(false),
-        target_destroyed(false) {}
-  bool dispatcher_destroyed;
-  bool target_destroyed;
+  bool dispatcher_destroyed = false;
+  bool target_destroyed = false;
+
+  // Set to true if an EventRewriter discards the event.
+  bool event_discarded = false;
 };
 
 class EVENTS_EXPORT EventDispatcherDelegate {

ui/events/event_source.cc

@@ -71,7 +71,9 @@ EventDispatchDetails EventSource::SendEventToSinkFromRewriter(
     status = (*it)->RewriteEvent(*event_for_rewriting, &rewritten_event);
     if (status == EVENT_REWRITE_DISCARD) {
       CHECK(!rewritten_event);
-      return EventDispatchDetails();
+      EventDispatchDetails details;
+      details.event_discarded = true;
+      return details;
     }
     if (status == EVENT_REWRITE_CONTINUE) {
       CHECK(!rewritten_event);