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

services/ui/ws/platform_display_default.cc

 // Copyright 2016 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 <utility>
+
 #include "base/memory/ptr_util.h"
 #include "gpu/ipc/client/gpu_channel_host.h"
 #include "services/ui/display/screen_manager.h"
 #include "services/ui/ws/display_client_compositor_frame_sink.h"
 #include "services/ui/ws/server_window.h"
 #include "ui/base/cursor/image_cursors.h"
 #include "ui/display/display.h"
 #include "ui/events/event.h"
 #include "ui/events/event_utils.h"
 #include "ui/platform_window/platform_ime_controller.h"
 #include "ui/platform_window/platform_window.h"
 
 #if defined(OS_WIN)
 #include "ui/platform_window/win/win_window.h"
 #elif defined(USE_X11) && !defined(OS_CHROMEOS)
 #include "ui/platform_window/x11/x11_window.h"
 #elif defined(OS_ANDROID)
 #include "ui/platform_window/android/platform_window_android.h"
 #elif defined(USE_OZONE)
 #include "ui/ozone/public/ozone_platform.h"
 #endif
 
 namespace ui {
 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.
   delegate_ = nullptr;
 
   frame_generator_.reset();
   // Destroy the PlatformWindow early on as it may call us back during
   // destruction and we want to be in a known state. But destroy the surface
   // first because it can still be using the platform window.
   platform_window_.reset();
 }
 
 EventSink* PlatformDisplayDefault::GetEventSink() {
   return delegate_->GetEventSink();
 }
 
 void PlatformDisplayDefault::Init(PlatformDisplayDelegate* delegate) {
+  DCHECK(delegate);
   delegate_ = delegate;
 
   const gfx::Rect& bounds = metrics_.bounds_in_pixels;
   DCHECK(!bounds.size().IsEmpty());
 
 #if defined(OS_WIN)
   platform_window_ = base::MakeUnique<ui::WinWindow>(this, bounds);
 #elif defined(USE_X11) && !defined(OS_CHROMEOS)
   platform_window_ = base::MakeUnique<ui::X11Window>(this, bounds);
 #elif defined(OS_ANDROID)
   platform_window_ = base::MakeUnique<ui::PlatformWindowAndroid>(this);
   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) {
   platform_window_->SetBounds(gfx::Rect(size));
 }
 
 void PlatformDisplayDefault::SetTitle(const base::string16& title) {
   platform_window_->SetTitle(title);
 }
 
 void PlatformDisplayDefault::SetCapture() {
   platform_window_->SetCapture();
 }
 
 void PlatformDisplayDefault::ReleaseCapture() {
   platform_window_->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.
   //
   // We probably also need to deal with different DPIs.
   ui::Cursor cursor(static_cast<int32_t>(cursor_id));
   image_cursors_->SetPlatformCursor(&cursor);
   platform_window_->SetCursor(cursor.platform());
-#endif
 }
 
 void PlatformDisplayDefault::UpdateTextInputState(
     const ui::TextInputState& state) {
   ui::PlatformImeController* ime = platform_window_->GetPlatformImeController();
   if (ime)
     ime->UpdateTextInputState(state);
 }
 
 void PlatformDisplayDefault::SetImeVisibility(bool visible) {
@@ skipping 64 matching lines @@
     SendEventToSink(&pointer_event);
   } else if (event->IsMouseEvent()) {
     ui::PointerEvent pointer_event(*event->AsMouseEvent());
     SendEventToSink(&pointer_event);
   } else if (event->IsTouchEvent()) {
     ui::PointerEvent pointer_event(*event->AsTouchEvent());
     SendEventToSink(&pointer_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() {
   // TODO(tonikitoo): Handle a close request in external window mode. The window
   // should be closed by the WS and it shouldn't involve ScreenManager.
   const int64_t display_id = delegate_->GetDisplay().id();
   display::ScreenManager::GetInstance()->RequestCloseDisplay(display_id);
 }
 
 void PlatformDisplayDefault::OnClosed() {}
@@ skipping 39 matching lines @@
 }
 
 void PlatformDisplayDefault::OnAcceleratedWidgetDestroyed() {
   NOTREACHED();
 }
 
 void PlatformDisplayDefault::OnActivationChanged(bool active) {}
 
 }  // namespace ws
 }  // namespace ui