exo: Move ash::ImeControllerImpl obverving to Keyboard.

Currently, WaylandKeyboardDelegate directly subscribes
ash::ImeControllerImpl. For better layering, this CL moves
it to Keyboard, so that WaylandKeyboardDelegate will be
just a Delegate for the Keyboard.

Along with the updating mock expectation in the unittest,
this CL also updates some use of Mocks not to have a warning
and verify the behavior precisely.

BUG=1123705
TEST=Ran exo_unittests.

Change-Id: I4d889807bd8cbef3cec2e40dd0b3546a123895e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2417672Reviewed-by: Jun Mukai <mukai@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808683}

ash/public/cpp/external_arc/message_center/arc_notification_content_view_unittest.cc

@@ -80,6 +80,10 @@ class MockKeyboardDelegate : public exo::KeyboardDelegate {
               OnKeyRepeatSettingsChanged,
               (bool, base::TimeDelta, base::TimeDelta),
               (override));
+  MOCK_METHOD(void,
+              OnKeyboardLayoutUpdated,
+              (const std::string& layout_name),
+              (override));
 };
 
 class FakeNotificationSurface : public exo::NotificationSurface {

components/exo/keyboard.cc

@@ -8,6 +8,7 @@
 #include "ash/keyboard/ui/keyboard_util.h"
 #include "ash/public/cpp/app_types.h"
 #include "ash/public/cpp/keyboard/keyboard_controller.h"
+#include "ash/shell.h"
 #include "base/bind.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/exo/input_trace.h"
@@ -173,7 +174,10 @@ Keyboard::Keyboard(std::unique_ptr<KeyboardDelegate> delegate, Seat* seat)
   AddEventHandler();
   seat_->AddObserver(this);
   ash::KeyboardController::Get()->AddObserver(this);
+  ash::ImeControllerImpl* ime_controller = ash::Shell::Get()->ime_controller();
+  ime_controller->AddObserver(this);
 
+  delegate_->OnKeyboardLayoutUpdated(ime_controller->keyboard_layout_name());
   OnSurfaceFocused(seat_->GetFocusedSurface());
   OnKeyRepeatSettingsChanged(
       ash::KeyboardController::Get()->GetKeyRepeatSettings());
@@ -184,9 +188,11 @@ Keyboard::~Keyboard() {
     observer.OnKeyboardDestroying(this);
   if (focus_)
     focus_->RemoveSurfaceObserver(this);
-  RemoveEventHandler();
-  seat_->RemoveObserver(this);
+
+  ash::Shell::Get()->ime_controller()->RemoveObserver(this);
   ash::KeyboardController::Get()->RemoveObserver(this);
+  seat_->RemoveObserver(this);
+  RemoveEventHandler();
 }
 
 bool Keyboard::HasDeviceConfigurationDelegate() const {
@@ -401,6 +407,15 @@ void Keyboard::OnKeyRepeatSettingsChanged(
                                         settings.interval);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+// ash::ImeControllerImpl::Observer overrides:
+
+void Keyboard::OnCapsLockChanged(bool enabled) {}
+
+void Keyboard::OnKeyboardLayoutNameChanged(const std::string& layout_name) {
+  delegate_->OnKeyboardLayoutUpdated(layout_name);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Keyboard, private:
 

components/exo/keyboard.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "ash/ime/ime_controller_impl.h"
 #include "ash/public/cpp/keyboard/keyboard_controller_observer.h"
 #include "base/containers/flat_map.h"
 #include "base/containers/flat_set.h"
@@ -34,7 +35,8 @@ class Surface;
 class Keyboard : public ui::EventHandler,
                  public SurfaceObserver,
                  public SeatObserver,
-                 public ash::KeyboardControllerObserver {
+                 public ash::KeyboardControllerObserver,
+                 public ash::ImeControllerImpl::Observer {
  public:
   Keyboard(std::unique_ptr<KeyboardDelegate> delegate, Seat* seat);
   ~Keyboard() override;
@@ -70,6 +72,10 @@ class Keyboard : public ui::EventHandler,
   void OnKeyRepeatSettingsChanged(
       const ash::KeyRepeatSettings& settings) override;
 
+  // Overridden from ash::ImeControllerImpl::Observer
+  void OnCapsLockChanged(bool enabled) override;
+  void OnKeyboardLayoutNameChanged(const std::string& layout_name) override;
+
  private:
   // Change keyboard focus to |surface|.
   void SetFocus(Surface* surface);

components/exo/keyboard_delegate.h

@@ -48,6 +48,11 @@ class KeyboardDelegate {
   virtual void OnKeyRepeatSettingsChanged(bool enabled,
                                           base::TimeDelta delay,
                                           base::TimeDelta interval) = 0;
+
+  // Called when keyboard layout is updated.
+  // TODO(hidehiko): Update the argument to pass the keymap
+  // when XkbTracker is moved out from WaylandKeyboardDelegate.
+  virtual void OnKeyboardLayoutUpdated(const std::string& layout_name) = 0;
 };
 
 }  // namespace exo

components/exo/keyboard_unittest.cc

@@ -52,6 +52,7 @@ class MockKeyboardDelegate : public KeyboardDelegate {
   MOCK_METHOD(void,
               OnKeyRepeatSettingsChanged,
               (bool, base::TimeDelta, base::TimeDelta));
+  MOCK_METHOD(void, OnKeyboardLayoutUpdated, (const std::string&));
 };
 using NiceMockKeyboardDelegate = ::testing::NiceMock<MockKeyboardDelegate>;
 
@@ -605,8 +606,7 @@ constexpr base::TimeDelta kDelta1000Ms =
     base::TimeDelta::FromMilliseconds(1000);
 
 TEST_F(KeyboardTest, KeyRepeatSettingsLoadDefaults) {
-  auto delegate = std::make_unique<MockKeyboardDelegate>();
-  EXPECT_CALL(*delegate, OnKeyRepeatSettingsChanged).Times(0);
+  auto delegate = std::make_unique<NiceMockKeyboardDelegate>();
   EXPECT_CALL(*delegate,
               OnKeyRepeatSettingsChanged(true, kDelta500Ms, kDelta50Ms));
 
@@ -620,39 +620,44 @@ TEST_F(KeyboardTest, KeyRepeatSettingsLoadInitially) {
   SetUserPref(email, ash::prefs::kXkbAutoRepeatDelay, base::Value(1000));
   SetUserPref(email, ash::prefs::kXkbAutoRepeatInterval, base::Value(1000));
 
-  auto delegate = std::make_unique<MockKeyboardDelegate>();
-  EXPECT_CALL(*delegate, OnKeyRepeatSettingsChanged).Times(0);
-  EXPECT_CALL(*delegate,
+  auto delegate = std::make_unique<NiceMockKeyboardDelegate>();
+  auto* delegate_ptr = delegate.get();
+  EXPECT_CALL(*delegate_ptr,
               OnKeyRepeatSettingsChanged(true, kDelta1000Ms, kDelta1000Ms));
-
   Seat seat;
   Keyboard keyboard(std::move(delegate), &seat);
+  // Verify before destroying keyboard to make sure the expected call
+  // is made on the methods above, rather than in the destructor.
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
 }
 
 TEST_F(KeyboardTest, KeyRepeatSettingsUpdateAtRuntime) {
-  auto delegate = std::make_unique<MockKeyboardDelegate>();
-  {
-    testing::InSequence s;
-
-    // Initially load defaults.
-    EXPECT_CALL(*delegate, OnKeyRepeatSettingsChanged)
-        .Times(testing::AtLeast(1));
-
-    // Respond to pref changes, in order
-    EXPECT_CALL(*delegate,
-                OnKeyRepeatSettingsChanged(false, testing::_, testing::_));
-    EXPECT_CALL(*delegate,
-                OnKeyRepeatSettingsChanged(false, kDelta1000Ms, testing::_));
-    EXPECT_CALL(*delegate,
-                OnKeyRepeatSettingsChanged(false, kDelta1000Ms, kDelta1000Ms));
-  }
+  auto delegate = std::make_unique<NiceMockKeyboardDelegate>();
+  auto* delegate_ptr = delegate.get();
+  // Initially load defaults.
+  EXPECT_CALL(*delegate_ptr,
+              OnKeyRepeatSettingsChanged(testing::_, testing::_, testing::_));
   Seat seat;
   Keyboard keyboard(std::move(delegate), &seat);
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
 
-  std::string email = "user0@tray";
+  // Make sure that setting prefs triggers the corresponding delegate calls.
+  const std::string email = "user0@tray";
+
+  EXPECT_CALL(*delegate_ptr,
+              OnKeyRepeatSettingsChanged(false, testing::_, testing::_));
   SetUserPref(email, ash::prefs::kXkbAutoRepeatEnabled, base::Value(false));
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
+
+  EXPECT_CALL(*delegate_ptr,
+              OnKeyRepeatSettingsChanged(false, kDelta1000Ms, testing::_));
   SetUserPref(email, ash::prefs::kXkbAutoRepeatDelay, base::Value(1000));
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
+
+  EXPECT_CALL(*delegate_ptr,
+              OnKeyRepeatSettingsChanged(false, kDelta1000Ms, kDelta1000Ms));
   SetUserPref(email, ash::prefs::kXkbAutoRepeatInterval, base::Value(1000));
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
 }
 
 TEST_F(KeyboardTest, KeyRepeatSettingsIgnoredForNonActiveUser) {
@@ -660,18 +665,23 @@ TEST_F(KeyboardTest, KeyRepeatSettingsIgnoredForNonActiveUser) {
   CreateUserSessions(2);
 
   // Key repeat settings should be sent exactly once, for the default values.
-  auto delegate = std::make_unique<MockKeyboardDelegate>();
-  EXPECT_CALL(*delegate, OnKeyRepeatSettingsChanged).Times(0);
-  EXPECT_CALL(*delegate,
+  auto delegate = std::make_unique<NiceMockKeyboardDelegate>();
+  auto* delegate_ptr = delegate.get();
+  EXPECT_CALL(*delegate_ptr,
               OnKeyRepeatSettingsChanged(true, kDelta500Ms, kDelta50Ms));
   Seat seat;
   Keyboard keyboard(std::move(delegate), &seat);
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
 
   // Set prefs for non-active user; no calls should result.
-  std::string email = "user1@tray";
+  EXPECT_CALL(*delegate_ptr,
+              OnKeyRepeatSettingsChanged(testing::_, testing::_, testing::_))
+      .Times(0);
+  const std::string email = "user1@tray";
   SetUserPref(email, ash::prefs::kXkbAutoRepeatEnabled, base::Value(true));
   SetUserPref(email, ash::prefs::kXkbAutoRepeatDelay, base::Value(1000));
   SetUserPref(email, ash::prefs::kXkbAutoRepeatInterval, base::Value(1000));
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
 }
 
 TEST_F(KeyboardTest, KeyRepeatSettingsUpdateOnProfileChange) {
@@ -684,21 +694,35 @@ TEST_F(KeyboardTest, KeyRepeatSettingsUpdateOnProfileChange) {
   SetUserPref(email, ash::prefs::kXkbAutoRepeatDelay, base::Value(1000));
   SetUserPref(email, ash::prefs::kXkbAutoRepeatInterval, base::Value(1000));
 
-  auto delegate = std::make_unique<MockKeyboardDelegate>();
-  EXPECT_CALL(*delegate, OnKeyRepeatSettingsChanged).Times(0);
-  {
-    testing::InSequence s;
-    // Initially, load default prefs for first user.
-    EXPECT_CALL(*delegate,
-                OnKeyRepeatSettingsChanged(true, kDelta500Ms, kDelta50Ms));
-    // Switching user should load new prefs.
-    EXPECT_CALL(*delegate,
-                OnKeyRepeatSettingsChanged(true, kDelta1000Ms, kDelta1000Ms));
-  }
+  auto delegate = std::make_unique<NiceMockKeyboardDelegate>();
+  auto* delegate_ptr = delegate.get();
+  // Initially, load default prefs for first user.
+  EXPECT_CALL(*delegate_ptr,
+              OnKeyRepeatSettingsChanged(true, kDelta500Ms, kDelta50Ms));
   Seat seat;
   Keyboard keyboard(std::move(delegate), &seat);
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
 
+  // Switching user should load new prefs.
+  EXPECT_CALL(*delegate_ptr,
+              OnKeyRepeatSettingsChanged(true, kDelta1000Ms, kDelta1000Ms));
   SimulateUserLogin(email, user_manager::UserType::USER_TYPE_REGULAR);
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
+}
+
+TEST_F(KeyboardTest, KeyboardLayout) {
+  auto delegate = std::make_unique<NiceMockKeyboardDelegate>();
+  auto* delegate_ptr = delegate.get();
+  // Initially, update to the current keyboard layout.
+  EXPECT_CALL(*delegate_ptr, OnKeyboardLayoutUpdated(testing::_));
+  Seat seat;
+  Keyboard keyboard(std::move(delegate), &seat);
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
+
+  // Updating the keyboard layout should trigger the delegate call.
+  EXPECT_CALL(*delegate_ptr, OnKeyboardLayoutUpdated(testing::_));
+  keyboard.OnKeyboardLayoutNameChanged("ja-jp");
+  testing::Mock::VerifyAndClearExpectations(delegate_ptr);
 }
 
 TEST_F(KeyboardTest, KeyboardObserver) {

components/exo/wayland/wayland_keyboard_delegate.cc

@@ -14,10 +14,6 @@
 #include "components/exo/xkb_tracker.h"
 #include "ui/events/keycodes/dom/dom_code.h"
 
-#if defined(OS_CHROMEOS)
-#include "ash/shell.h"
-#endif
-
 namespace exo {
 namespace wayland {
 
@@ -27,20 +23,9 @@ WaylandKeyboardDelegate::WaylandKeyboardDelegate(wl_resource* keyboard_resource,
                                                  SerialTracker* serial_tracker)
     : keyboard_resource_(keyboard_resource),
       serial_tracker_(serial_tracker),
-      xkb_tracker_(std::make_unique<XkbTracker>()) {
-#if defined(OS_CHROMEOS)
-  ash::ImeControllerImpl* ime_controller = ash::Shell::Get()->ime_controller();
-  xkb_tracker_->UpdateKeyboardLayout(ime_controller->keyboard_layout_name());
-  ime_controller->AddObserver(this);
-#endif
-  SendLayout();
-}
+      xkb_tracker_(std::make_unique<XkbTracker>()) {}
 
-WaylandKeyboardDelegate::~WaylandKeyboardDelegate() {
-#if defined(OS_CHROMEOS)
-  ash::Shell::Get()->ime_controller()->RemoveObserver(this);
-#endif
-}
+WaylandKeyboardDelegate::~WaylandKeyboardDelegate() = default;
 
 bool WaylandKeyboardDelegate::CanAcceptKeyboardEventsForSurface(
     Surface* surface) const {
@@ -107,15 +92,11 @@ void WaylandKeyboardDelegate::OnKeyboardModifiers(int modifier_flags) {
   SendKeyboardModifiers();
 }
 
-#if defined(OS_CHROMEOS)
-void WaylandKeyboardDelegate::OnCapsLockChanged(bool enabled) {}
-
-void WaylandKeyboardDelegate::OnKeyboardLayoutNameChanged(
+void WaylandKeyboardDelegate::OnKeyboardLayoutUpdated(
     const std::string& layout_name) {
   xkb_tracker_->UpdateKeyboardLayout(layout_name);
   SendLayout();
 }
-#endif
 
 uint32_t WaylandKeyboardDelegate::DomCodeToKey(ui::DomCode code) const {
   // This assumes KeycodeConverter has been built with evdev/xkb codes.

components/exo/wayland/wayland_keyboard_delegate.h

@@ -16,10 +16,6 @@
 #include "ui/base/buildflags.h"
 #include "ui/events/keycodes/dom/keycode_converter.h"
 
-#if defined(OS_CHROMEOS)
-#include "ash/ime/ime_controller_impl.h"
-#endif
-
 #if BUILDFLAG(USE_XKBCOMMON)
 #include <xkbcommon/xkbcommon.h>
 #include "ui/events/keycodes/scoped_xkb.h"  // nogncheck
@@ -38,12 +34,7 @@ class SerialTracker;
 // Keyboard delegate class that accepts events for surfaces owned by the same
 // client as a keyboard resource.
 class WaylandKeyboardDelegate : public WaylandInputDelegate,
-                                public KeyboardDelegate
-#if defined(OS_CHROMEOS)
-    ,
-                                public ash::ImeControllerImpl::Observer
-#endif
-{
+                                public KeyboardDelegate {
 #if BUILDFLAG(USE_XKBCOMMON)
  public:
   WaylandKeyboardDelegate(wl_resource* keyboard_resource,
@@ -63,12 +54,7 @@ class WaylandKeyboardDelegate : public WaylandInputDelegate,
   void OnKeyRepeatSettingsChanged(bool enabled,
                                   base::TimeDelta delay,
                                   base::TimeDelta interval) override;
-
-#if defined(OS_CHROMEOS)
-  // Overridden from ImeControllerImpl::Observer:
-  void OnCapsLockChanged(bool enabled) override;
-  void OnKeyboardLayoutNameChanged(const std::string& layout_name) override;
-#endif
+  void OnKeyboardLayoutUpdated(const std::string& layout_name) override;
 
  private:
   // Returns the corresponding key given a dom code.