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

services/ui/ws/platform_display_default.cc

Index: services/ui/ws/platform_display_default.cc
diff --git a/services/ui/ws/platform_display_default.cc b/services/ui/ws/platform_display_default.cc
index 6b2340880882179bce3f48e9c3caf74e71668a5f..08f8f9e26da0b08ec93fad0684ea8ee3cb6c8323 100644
--- a/services/ui/ws/platform_display_default.cc
+++ b/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)

[tonikitoo, 2017/04/07 17:48:54]

Sorry for being picky here, james. You have got the lgtm's needed, so feel free
to ignore the comment, as owners have precedence here. 
But...

While working on something else, I came across this piece of code:

31 void InputMethodBridge::ProcessKeyEvent(
32     std::unique_ptr<ui::Event> event,
33     const ProcessKeyEventCallback& callback) {
34   DCHECK(event->IsKeyEvent());
35   ui::KeyEvent* key_event = event->AsKeyEvent();
36   if (!key_event->is_char()) {
37     input_method_chromeos_->DispatchKeyEvent(
38         key_event, base::MakeUnique<base::Callback<void(bool)>>(callback));
39   } else {
40     // On Linux (include ChromeOS), the mus emulates the WM_CHAR generation
41     // behaviour of Windows. But for ChromeOS, we don't expect those char
42     // events, so we filter them out.
43     const bool handled = true;
44     callback.Run(handled);
45   }
46 }

.. in
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/ime_driver/input...

Is the comment in line 40 still accurate?

Maybe a more conservative approach would be keep the linux behavior even on
chromeos off-device/linux builds(if it is really needed as the comment says),
and add a IsRunningAsSystemCompositor() check to not run this code if
OS_CHROMEOS and is running on real ChromeOS device?

[James Cook, 2017/04/07 21:17:51]

tonikitoo, good catch! I talked to penghuang@ and the workaround in
InputMethodBridge isn't necessary if I remove this code. So I removed that
workaround as well. Updated CL description.

Thanks for catching this.

-  // 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() {