mash: Don't synthesize extra key press events in the window server

This fixes a bug where pressing tab would move focus forward by two UI
elements.

There was a workaround in PlatformDisplayDefault that would synthesize a
duplicate ET_KEY_PRESSED for every key down in order to emulate Windows
WM_CHAR synthesis. See https://codereview.chromium.org/1234623004
The workaround isn't needed on mash, so I removed it.

I added a unit test, which required some small changes to
PlatformDisplayDefault and OzonePlatform.

Allowed the ui::ImageCursors parameter to be null because the test harness
does not provide the resource bundle needed for cursors.

Revert a workaround in InputMethodBridge for these synthesized events,
see https://codereview.chromium.org/2576973003/

BUG=706574,707055
TEST=added to mus_ws_unittests

Review-Url: https://codereview.chromium.org/2795503002
Cr-Commit-Position: refs/heads/master@{#463036}

chrome/browser/ui/views/ime_driver/input_method_bridge_chromeos.cc

@@ -37,10 +37,7 @@ void InputMethodBridge::ProcessKeyEvent(
     input_method_chromeos_->DispatchKeyEvent(
         key_event, base::MakeUnique<base::Callback<void(bool)>>(callback));
   } else {
-    // On Linux (include ChromeOS), the mus emulates the WM_CHAR generation
-    // behaviour of Windows. But for ChromeOS, we don't expect those char
-    // events, so we filter them out.
-    const bool handled = true;
+    const bool handled = false;
     callback.Run(handled);
   }
 }

services/ui/service.cc

@@ -96,6 +96,10 @@ Service::~Service() {
   // WindowServer (or more correctly its Displays) may have state that needs to
   // be destroyed before GpuState as well.
   window_server_.reset();
+
+#if defined(USE_OZONE)
+  OzonePlatform::Shutdown();
+#endif
 }
 
 void Service::InitializeResources(service_manager::Connector* connector) {

services/ui/ws/BUILD.gn

@@ -262,6 +262,12 @@ service_test("mus_ws_unittests") {
     sources += [ "window_manager_client_unittest.cc" ]
   }
 
+  # TODO(jamescook): Run this test on non-ozone platforms. This will require
+  # initializing all the platform-specific windowing support.
+  if (use_ozone) {
+    sources += [ "platform_display_default_unittest.cc" ]
+  }
+
   catalog = ":mus_ws_unittests_catalog"
 
   deps = [

services/ui/ws/display.cc

@@ -29,6 +29,10 @@
 #include "ui/base/cursor/cursor.h"
 #include "ui/display/screen.h"
 
+#if defined(USE_OZONE)
+#include "ui/ozone/public/ozone_platform.h"
+#endif
+
 namespace ui {
 namespace ws {
 
@@ -276,6 +280,14 @@ void Display::OnNativeCaptureLost() {
     display_root->window_manager_state()->SetCapture(nullptr, kInvalidClientId);
 }
 
+OzonePlatform* Display::GetOzonePlatform() {
+#if defined(USE_OZONE)
+  return OzonePlatform::GetInstance();
+#else
+  return nullptr;
+#endif
+}
+
 void Display::OnViewportMetricsChanged(
     const display::ViewportMetrics& metrics) {
   platform_display_->UpdateViewportMetrics(metrics);

services/ui/ws/display.h

@@ -183,6 +183,7 @@ class Display : public PlatformDisplayDelegate,
   EventSink* GetEventSink() override;
   void OnAcceleratedWidgetAvailable() override;
   void OnNativeCaptureLost() override;
+  OzonePlatform* GetOzonePlatform() override;
 
   // FocusControllerDelegate:
   bool CanHaveActiveChildren(ServerWindow* window) const override;

services/ui/ws/platform_display.cc

@@ -8,6 +8,7 @@
 #include "services/ui/ws/platform_display_default.h"
 #include "services/ui/ws/platform_display_factory.h"
 #include "services/ui/ws/server_window.h"
+#include "ui/base/cursor/image_cursors.h"
 
 namespace ui {
 namespace ws {
@@ -22,7 +23,13 @@ std::unique_ptr<PlatformDisplay> PlatformDisplay::Create(
   if (factory_)
     return factory_->CreatePlatformDisplay(root, metrics);
 
-  return base::MakeUnique<PlatformDisplayDefault>(root, metrics);
+#if defined(OS_ANDROID)
+  return base::MakeUnique<PlatformDisplayDefault>(root, metrics,
+                                                  nullptr /* image_cursors */);
+#else
+  return base::MakeUnique<PlatformDisplayDefault>(
+      root, metrics, base::MakeUnique<ImageCursors>());
+#endif
 }
 
 }  // namespace ws

services/ui/ws/platform_display_default.cc

@@ -4,6 +4,8 @@
 
 #include "services/ui/ws/platform_display_default.h"
 
+#include <utility>
+
 #include "base/memory/ptr_util.h"
 #include "gpu/ipc/client/gpu_channel_host.h"
 #include "services/ui/display/screen_manager.h"
@@ -31,14 +33,12 @@ namespace ws {
 
 PlatformDisplayDefault::PlatformDisplayDefault(
     ServerWindow* root_window,
-    const display::ViewportMetrics& metrics)
+    const display::ViewportMetrics& metrics,
+    std::unique_ptr<ImageCursors> image_cursors)
     : root_window_(root_window),
-#if !defined(OS_ANDROID)
-      image_cursors_(new ImageCursors),
-#endif
+      image_cursors_(std::move(image_cursors)),
       metrics_(metrics),
-      widget_(gfx::kNullAcceleratedWidget) {
-}
+      widget_(gfx::kNullAcceleratedWidget) {}
 
 PlatformDisplayDefault::~PlatformDisplayDefault() {
   // Don't notify the delegate from the destructor.
@@ -56,6 +56,7 @@ EventSink* PlatformDisplayDefault::GetEventSink() {
 }
 
 void PlatformDisplayDefault::Init(PlatformDisplayDelegate* delegate) {
+  DCHECK(delegate);
   delegate_ = delegate;
 
   const gfx::Rect& bounds = metrics_.bounds_in_pixels;
@@ -70,16 +71,16 @@ void PlatformDisplayDefault::Init(PlatformDisplayDelegate* delegate) {
   platform_window_->SetBounds(bounds);
 #elif defined(USE_OZONE)
   platform_window_ =
-      ui::OzonePlatform::GetInstance()->CreatePlatformWindow(this, bounds);
+      delegate_->GetOzonePlatform()->CreatePlatformWindow(this, bounds);
 #else
   NOTREACHED() << "Unsupported platform";
 #endif
 
   platform_window_->Show();
-#if !defined(OS_ANDROID)
-  image_cursors_->SetDisplay(delegate_->GetDisplay(),
-                             metrics_.device_scale_factor);
-#endif
+  if (image_cursors_) {
+    image_cursors_->SetDisplay(delegate_->GetDisplay(),
+                               metrics_.device_scale_factor);
+  }
 }
 
 void PlatformDisplayDefault::SetViewportSize(const gfx::Size& size) {
@@ -99,7 +100,9 @@ void PlatformDisplayDefault::ReleaseCapture() {
 }
 
 void PlatformDisplayDefault::SetCursorById(mojom::CursorType cursor_id) {
-#if !defined(OS_ANDROID)
+  if (!image_cursors_)
+    return;
+
   // TODO(erg): This still isn't sufficient, and will only use native cursors
   // that chrome would use, not custom image cursors. For that, we should
   // delegate to the window manager to load images from resource packs.
@@ -108,7 +111,6 @@ void PlatformDisplayDefault::SetCursorById(mojom::CursorType cursor_id) {
   ui::Cursor cursor(static_cast<int32_t>(cursor_id));
   image_cursors_->SetPlatformCursor(&cursor);
   platform_window_->SetCursor(cursor.platform());
-#endif
 }
 
 void PlatformDisplayDefault::UpdateTextInputState(
@@ -193,32 +195,6 @@ void PlatformDisplayDefault::DispatchEvent(ui::Event* event) {
   } else {
     SendEventToSink(event);
   }
-
-#if defined(USE_X11) || defined(USE_OZONE)
-  // We want to emulate the WM_CHAR generation behaviour of Windows.
-  //
-  // On Linux, we've previously inserted characters by having
-  // InputMethodAuraLinux take all key down events and send a character event
-  // to the TextInputClient. This causes a mismatch in code that has to be
-  // shared between Windows and Linux, including blink code. Now that we're
-  // trying to have one way of doing things, we need to standardize on and
-  // emulate Windows character events.
-  //
-  // This is equivalent to what we're doing in the current Linux port, but
-  // done once instead of done multiple times in different places.
-  if (event->type() == ui::ET_KEY_PRESSED) {
-    ui::KeyEvent* key_press_event = event->AsKeyEvent();
-    ui::KeyEvent char_event(key_press_event->GetCharacter(),
-                            key_press_event->key_code(),
-                            key_press_event->flags());
-    // We don't check that GetCharacter() is equal because changing a key event
-    // with an accelerator to a character event can change the character, for
-    // example, from 'M' to '^M'.
-    DCHECK_EQ(key_press_event->key_code(), char_event.key_code());
-    DCHECK_EQ(key_press_event->flags(), char_event.flags());
-    SendEventToSink(&char_event);
-  }
-#endif
 }
 
 void PlatformDisplayDefault::OnCloseRequest() {

services/ui/ws/platform_display_default.h

@@ -29,8 +29,10 @@ namespace ws {
 class PlatformDisplayDefault : public PlatformDisplay,
                                public ui::PlatformWindowDelegate {
  public:
+  // |image_cursors| may be null, for example on Android or in tests.
   PlatformDisplayDefault(ServerWindow* root_window,
-                         const display::ViewportMetrics& metrics);
+                         const display::ViewportMetrics& metrics,
+                         std::unique_ptr<ImageCursors> image_cursors);
   ~PlatformDisplayDefault() override;
 
   // EventSource::
@@ -72,9 +74,7 @@ class PlatformDisplayDefault : public PlatformDisplay,
 
   ServerWindow* root_window_;
 
-#if !defined(OS_ANDROID)
   std::unique_ptr<ui::ImageCursors> image_cursors_;
-#endif
 
   PlatformDisplayDelegate* delegate_ = nullptr;
   std::unique_ptr<FrameGenerator> frame_generator_;

services/ui/ws/platform_display_default_unittest.cc

+// Copyright 2017 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 "services/ui/ws/platform_display_default.h"
+
+#include "base/time/time.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/cursor/image_cursors.h"
+#include "ui/display/types/native_display_delegate.h"
+#include "ui/events/event.h"
+#include "ui/events/event_sink.h"
+#include "ui/gfx/geometry/point.h"
+#include "ui/ozone/public/ozone_platform.h"
+#include "ui/ozone/public/system_input_injector.h"
+#include "ui/platform_window/platform_window.h"
+#include "ui/platform_window/stub/stub_window.h"
+
+namespace ui {
+namespace ws {
+namespace {
+
+// An EventSink that records events sent to it.
+class TestEventSink : public EventSink {
+ public:
+  TestEventSink() { Reset(); }
+  ~TestEventSink() override = default;
+
+  void Reset() {
+    count_ = 0;
+    last_event_type_ = ET_UNKNOWN;
+  }
+
+  // EventSink:
+  EventDispatchDetails OnEventFromSource(Event* event) override {
+    count_++;
+    last_event_type_ = event->type();
+    return EventDispatchDetails();
+  }
+
+  int count_;
+  EventType last_event_type_;
+};
+
+// A PlatformDisplayDelegate to connect the PlatformDisplay to a TestEventSink.
+class TestPlatformDisplayDelegate : public PlatformDisplayDelegate {
+ public:
+  TestPlatformDisplayDelegate(TestEventSink* sink, OzonePlatform* platform)
+      : event_sink_(sink), ozone_platform_(platform) {}
+  ~TestPlatformDisplayDelegate() override = default;
+
+  // PlatformDisplayDelegate:
+  const display::Display& GetDisplay() override { return stub_display_; }
+  ServerWindow* GetRootWindow() override { return nullptr; }
+  EventSink* GetEventSink() override { return event_sink_; }
+  void OnAcceleratedWidgetAvailable() override {}
+  void OnNativeCaptureLost() override {}
+  OzonePlatform* GetOzonePlatform() override { return ozone_platform_; }
+
+ private:
+  TestEventSink* event_sink_;
+  OzonePlatform* ozone_platform_;
+  display::Display stub_display_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestPlatformDisplayDelegate);
+};
+
+// An OzonePlatform that creates StubWindows.
+class TestOzonePlatform : public OzonePlatform {
+ public:
+  TestOzonePlatform() = default;
+  ~TestOzonePlatform() override = default;
+
+  // OzonePlatform:
+  ui::SurfaceFactoryOzone* GetSurfaceFactoryOzone() override { return nullptr; }
+  ui::OverlayManagerOzone* GetOverlayManager() override { return nullptr; }
+  ui::CursorFactoryOzone* GetCursorFactoryOzone() override { return nullptr; }
+  ui::InputController* GetInputController() override { return nullptr; }
+  ui::GpuPlatformSupportHost* GetGpuPlatformSupportHost() override {
+    return nullptr;
+  }
+  std::unique_ptr<SystemInputInjector> CreateSystemInputInjector() override {
+    return nullptr;
+  }
+  std::unique_ptr<PlatformWindow> CreatePlatformWindow(
+      PlatformWindowDelegate* delegate,
+      const gfx::Rect& bounds) override {
+    return base::MakeUnique<StubWindow>(
+        delegate, false /* use_default_accelerated_widget */);
+  }
+  std::unique_ptr<display::NativeDisplayDelegate> CreateNativeDisplayDelegate()
+      override {
+    return nullptr;
+  }
+  void InitializeUI(const InitParams& params) override {}
+  void InitializeGPU(const InitParams& params) override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestOzonePlatform);
+};
+
+TEST(PlatformDisplayDefaultTest, EventDispatch) {
+  // Setup ozone so the display can be initialized.
+  TestOzonePlatform platform;
+
+  // Create the display.
+  display::ViewportMetrics metrics;
+  metrics.bounds_in_pixels = gfx::Rect(1024, 768);
+  metrics.device_scale_factor = 1.f;
+  metrics.ui_scale_factor = 1.f;
+  PlatformDisplayDefault display(nullptr, metrics,
+                                 std::unique_ptr<ImageCursors>());
+
+  // Initialize the display with a test EventSink so we can sense events.
+  TestEventSink event_sink;
+  TestPlatformDisplayDelegate delegate(&event_sink, &platform);
+  display.Init(&delegate);
+
+  // Event dispatch is handled at the PlatformWindowDelegate level.
+  PlatformWindowDelegate* display_for_dispatch =
+      static_cast<PlatformWindowDelegate*>(&display);
+
+  // Mouse events are converted to pointer events.
+  MouseEvent mouse(ET_MOUSE_PRESSED, gfx::Point(1, 2), gfx::Point(1, 2),
+                   base::TimeTicks(), EF_NONE, 0);
+  display_for_dispatch->DispatchEvent(&mouse);
+  EXPECT_EQ(ET_POINTER_DOWN, event_sink.last_event_type_);
+  event_sink.Reset();
+
+  // Touch events are converted to pointer events.
+  TouchEvent touch(ET_TOUCH_PRESSED, gfx::Point(3, 4), base::TimeTicks(),
+                   PointerDetails(EventPointerType::POINTER_TYPE_TOUCH, 0));
+  display_for_dispatch->DispatchEvent(&touch);
+  EXPECT_EQ(ET_POINTER_DOWN, event_sink.last_event_type_);
+  event_sink.Reset();
+
+  // Pressing a key dispatches exactly one event.
+  KeyEvent key_pressed(ET_KEY_PRESSED, VKEY_A, EF_NONE);
+  display_for_dispatch->DispatchEvent(&key_pressed);
+  EXPECT_EQ(1, event_sink.count_);
+  EXPECT_EQ(ET_KEY_PRESSED, event_sink.last_event_type_);
+  event_sink.Reset();
+
+  // Releasing the key dispatches exactly one event.
+  KeyEvent key_released(ET_KEY_RELEASED, VKEY_A, EF_NONE);
+  display_for_dispatch->DispatchEvent(&key_released);
+  EXPECT_EQ(1, event_sink.count_);
+  EXPECT_EQ(ET_KEY_RELEASED, event_sink.last_event_type_);
+}
+
+}  // namespace
+}  // namespace ws
+}  // namespace ui

services/ui/ws/platform_display_delegate.h

@@ -12,6 +12,7 @@ class Display;
 namespace ui {
 
 class EventSink;
+class OzonePlatform;
 
 namespace ws {
 
@@ -37,6 +38,10 @@ class PlatformDisplayDelegate {
   // Called when the Display loses capture.
   virtual void OnNativeCaptureLost() = 0;
 
+  // Allows the OzonePlatform to be overridden, e.g. for tests. Returns null
+  // for non-Ozone platforms.
+  virtual OzonePlatform* GetOzonePlatform() = 0;
+
  protected:
   virtual ~PlatformDisplayDelegate() {}
 };

ui/aura/env.cc

@@ -49,6 +49,11 @@ Env::~Env() {
 
   for (EnvObserver& observer : observers_)
     observer.OnWillDestroyEnv();
+
+#if defined(USE_OZONE)
+  ui::OzonePlatform::Shutdown();
+#endif
+
   DCHECK_EQ(this, lazy_tls_ptr.Pointer()->Get());
   lazy_tls_ptr.Pointer()->Set(NULL);
 }

ui/ozone/public/ozone_platform.cc

@@ -2,14 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/command_line.h"
+#include "ui/ozone/public/ozone_platform.h"
+
 #include "base/logging.h"
 #include "base/trace_event/trace_event.h"
 #include "ui/events/devices/device_data_manager.h"
 #include "ui/ozone/platform_object.h"
 #include "ui/ozone/platform_selection.h"
-#include "ui/ozone/public/ozone_platform.h"
-#include "ui/ozone/public/ozone_switches.h"
 
 namespace ui {
 
@@ -18,7 +17,7 @@ namespace {
 bool g_platform_initialized_ui = false;
 bool g_platform_initialized_gpu = false;
 
-}
+}  // namespace
 
 OzonePlatform::OzonePlatform() {
   DCHECK(!instance_) << "There should only be a single OzonePlatform.";
@@ -59,6 +58,12 @@ void OzonePlatform::InitializeForGPU(const InitParams& args) {
   instance_->InitializeGPU(args);
 }
 
+// static
+void OzonePlatform::Shutdown() {
+  delete instance_;
+  // Destructor resets pointer.
+}
+
 // static
 OzonePlatform* OzonePlatform::GetInstance() {
   DCHECK(instance_) << "OzonePlatform is not initialized";
@@ -83,7 +88,7 @@ OzonePlatform* OzonePlatform::EnsureInstance() {
 }
 
 // static
-OzonePlatform* OzonePlatform::instance_;
+OzonePlatform* OzonePlatform::instance_ = nullptr;
 
 IPC::MessageFilter* OzonePlatform::GetGpuMessageFilter() {
   return nullptr;

ui/ozone/public/ozone_platform.h

@@ -93,6 +93,10 @@ class OZONE_EXPORT OzonePlatform {
   // provided by |args| as with InitalizeForUI.
   static void InitializeForGPU(const InitParams& args);
 
+  // Deletes the instance. Does nothing if OzonePlatform has not yet been
+  // initialized.
+  static void Shutdown();
+
   static OzonePlatform* GetInstance();
 
   // Factory getters to override in subclasses. The returned objects will be