chromeos: Simplify chromeos::input_method::XKeyboard interface.

Notable changes:
 * Replace the num-lock related methods with a single DisableNumLock(), since
   num-lock LED is always turned off in chromeos (more details at
   crbug.com/124189)
 * Remove SetLockedModifiers(), since DisableNumLock() and SetCapsLockEnabled()
   are sufficient.
 * Remove GetLockedModifiers(), since only CapsLockIsEnabled() is useful.

BUG=none
R=derat@chromium.org, yukishiino@chromium.org

Review URL: https://codereview.chromium.org/189663009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@255943 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

@@ -298,8 +298,7 @@ class DBusServices {
 
     if (base::SysInfo::IsRunningOnChromeOS()) {
       // Disable Num Lock on X start up for http://crosbug.com/29169.
-      input_method::InputMethodManager::Get()->GetXKeyboard()->
-          SetNumLockEnabled(false);
+      input_method::InputMethodManager::Get()->GetXKeyboard()->DisableNumLock();
     }
 
     // Initialize the device settings service so that we'll take actions per

chrome/browser/chromeos/events/system_key_event_listener.cc

@@ -43,12 +43,10 @@ SystemKeyEventListener* SystemKeyEventListener::GetInstance() {
 
 SystemKeyEventListener::SystemKeyEventListener()
     : stopped_(false),
-      num_lock_mask_(0),
       xkb_event_base_(0) {
   input_method::XKeyboard* xkeyboard =
       input_method::InputMethodManager::Get()->GetXKeyboard();
-  num_lock_mask_ = xkeyboard->GetNumLockMask();
-  xkeyboard->GetLockedModifiers(&caps_lock_is_on_, NULL);
+  caps_lock_is_on_ = xkeyboard->CapsLockIsEnabled();
 
   XDisplay* display = gfx::GetXDisplay();
   int xkb_major_version = XkbMajorVersion;
@@ -121,9 +119,7 @@ bool SystemKeyEventListener::ProcessedXEvent(XEvent* xevent) {
       if (xkey_event->state.mods) {
         // TODO(yusukes,adlr): Let the user know that num lock is unsupported.
         // Force turning off Num Lock (crosbug.com/29169)
-        input_method_manager->GetXKeyboard()->SetLockedModifiers(
-            input_method::kDontChange  /* caps lock */,
-            input_method::kDisableLock  /* num lock */);
+        input_method_manager->GetXKeyboard()->DisableNumLock();
       }
       return true;
     }

chrome/browser/chromeos/events/system_key_event_listener.h

@@ -58,7 +58,6 @@ class SystemKeyEventListener : public base::MessageLoopForUI::Observer {
 
   bool stopped_;
 
-  unsigned int num_lock_mask_;
   bool caps_lock_is_on_;
   ObserverList<CapsLockObserver> caps_lock_observers_;
 

chromeos/ime/fake_xkeyboard.cc

@@ -10,7 +10,6 @@ namespace input_method {
 FakeXKeyboard::FakeXKeyboard()
     : set_current_keyboard_layout_by_name_count_(0),
       caps_lock_is_enabled_(false),
-      num_lock_is_enabled_(false),
       auto_repeat_is_enabled_(false) {
 }
 
@@ -28,42 +27,17 @@ bool FakeXKeyboard::ReapplyCurrentKeyboardLayout() {
 void FakeXKeyboard::ReapplyCurrentModifierLockStatus() {
 }
 
-void FakeXKeyboard::SetLockedModifiers(ModifierLockStatus new_caps_lock_status,
-                                       ModifierLockStatus new_num_lock_status) {
-  if (new_caps_lock_status != kDontChange) {
-    caps_lock_is_enabled_ =
-        (new_caps_lock_status == kEnableLock) ? true : false;
-  }
-  if (new_num_lock_status != kDontChange)
-    num_lock_is_enabled_ = (new_num_lock_status == kEnableLock) ? true : false;
-}
-
-void FakeXKeyboard::SetNumLockEnabled(bool enable_num_lock) {
-  num_lock_is_enabled_ = enable_num_lock;
+void FakeXKeyboard::DisableNumLock() {
 }
 
 void FakeXKeyboard::SetCapsLockEnabled(bool enable_caps_lock) {
   caps_lock_is_enabled_ = enable_caps_lock;
 }
 
-bool FakeXKeyboard::NumLockIsEnabled() {
-  return num_lock_is_enabled_;
-}
-
 bool FakeXKeyboard::CapsLockIsEnabled() {
   return caps_lock_is_enabled_;
 }
 
-unsigned int FakeXKeyboard::GetNumLockMask() {
-  return 1;
-}
-
-void FakeXKeyboard::GetLockedModifiers(bool* out_caps_lock_enabled,
-                                       bool* out_num_lock_enabled) {
-  *out_caps_lock_enabled = caps_lock_is_enabled_;
-  *out_num_lock_enabled = num_lock_is_enabled_;
-}
-
 bool FakeXKeyboard::SetAutoRepeatEnabled(bool enabled) {
   auto_repeat_is_enabled_ = enabled;
   return true;

chromeos/ime/fake_xkeyboard.h

@@ -24,23 +24,15 @@ class CHROMEOS_EXPORT FakeXKeyboard : public XKeyboard {
       OVERRIDE;
   virtual bool ReapplyCurrentKeyboardLayout() OVERRIDE;
   virtual void ReapplyCurrentModifierLockStatus() OVERRIDE;
-  virtual void SetLockedModifiers(ModifierLockStatus new_caps_lock_status,
-                                  ModifierLockStatus new_num_lock_status)
-      OVERRIDE;
-  virtual void SetNumLockEnabled(bool enable_num_lock) OVERRIDE;
+  virtual void DisableNumLock() OVERRIDE;
   virtual void SetCapsLockEnabled(bool enable_caps_lock) OVERRIDE;
-  virtual bool NumLockIsEnabled() OVERRIDE;
   virtual bool CapsLockIsEnabled() OVERRIDE;
-  virtual unsigned int GetNumLockMask() OVERRIDE;
-  virtual void GetLockedModifiers(bool* out_caps_lock_enabled,
-                                  bool* out_num_lock_enabled) OVERRIDE;
   virtual bool SetAutoRepeatEnabled(bool enabled) OVERRIDE;
   virtual bool SetAutoRepeatRate(const AutoRepeatRate& rate) OVERRIDE;
 
   int set_current_keyboard_layout_by_name_count_;
   std::string last_layout_;
   bool caps_lock_is_enabled_;
-  bool num_lock_is_enabled_;
   bool auto_repeat_is_enabled_;
   AutoRepeatRate last_auto_repeat_rate_;
   // TODO(yusukes): Add more variables for counting the numbers of the API calls

chromeos/ime/xkeyboard.cc

@@ -74,20 +74,20 @@ class XKeyboardImpl : public XKeyboard {
       const std::string& layout_name) OVERRIDE;
   virtual bool ReapplyCurrentKeyboardLayout() OVERRIDE;
   virtual void ReapplyCurrentModifierLockStatus() OVERRIDE;
-  virtual void SetLockedModifiers(
-      ModifierLockStatus new_caps_lock_status,
-      ModifierLockStatus new_num_lock_status) OVERRIDE;
-  virtual void SetNumLockEnabled(bool enable_num_lock) OVERRIDE;
+  virtual void DisableNumLock() OVERRIDE;
   virtual void SetCapsLockEnabled(bool enable_caps_lock) OVERRIDE;
-  virtual bool NumLockIsEnabled() OVERRIDE;
   virtual bool CapsLockIsEnabled() OVERRIDE;
-  virtual unsigned int GetNumLockMask() OVERRIDE;
-  virtual void GetLockedModifiers(bool* out_caps_lock_enabled,
-                                  bool* out_num_lock_enabled) OVERRIDE;
   virtual bool SetAutoRepeatEnabled(bool enabled) OVERRIDE;
   virtual bool SetAutoRepeatRate(const AutoRepeatRate& rate) OVERRIDE;
 
  private:
+  // Returns a mask for Num Lock (e.g. 1U << 4). Returns 0 on error.
+  unsigned int GetNumLockMask();
+
+  // Sets the caps-lock status. Note that calling this function always disables
+  // the num-lock.
+  void SetLockedModifiers(bool caps_lock_enabled);
+
   // This function is used by SetLayout() and RemapModifierKeys(). Calls
   // setxkbmap command if needed, and updates the last_full_layout_name_ cache.
   bool SetLayoutInternal(const std::string& layout_name, bool force);
@@ -106,9 +106,9 @@ class XKeyboardImpl : public XKeyboard {
   const bool is_running_on_chrome_os_;
   unsigned int num_lock_mask_;
 
-  // The current Num Lock and Caps Lock status. If true, enabled.
-  bool current_num_lock_status_;
+  // The current Caps Lock status. If true, enabled.
   bool current_caps_lock_status_;
+
   // The XKB layout name which we set last time like "us" and "us(dvorak)".
   std::string current_layout_name_;
 
@@ -139,7 +139,56 @@ XKeyboardImpl::XKeyboardImpl()
     LOG_IF(ERROR, num_lock_mask_ != Mod2Mask)
         << "NumLock is not assigned to Mod2Mask.  : " << num_lock_mask_;
   }
-  GetLockedModifiers(&current_caps_lock_status_, &current_num_lock_status_);
+
+  current_caps_lock_status_ = CapsLockIsEnabled();
+}
+
+unsigned int XKeyboardImpl::GetNumLockMask() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  static const unsigned int kBadMask = 0;
+
+  unsigned int real_mask = kBadMask;
+  XkbDescPtr xkb_desc =
+      XkbGetKeyboard(GetXDisplay(), XkbAllComponentsMask, XkbUseCoreKbd);
+  if (!xkb_desc)
+    return kBadMask;
+
+  if (xkb_desc->dpy && xkb_desc->names && xkb_desc->names->vmods) {
+    const std::string string_to_find(kNumLockVirtualModifierString);
+    for (size_t i = 0; i < XkbNumVirtualMods; ++i) {
+      const unsigned int virtual_mod_mask = 1U << i;
+      char* virtual_mod_str_raw_ptr =
+          XGetAtomName(xkb_desc->dpy, xkb_desc->names->vmods[i]);
+      if (!virtual_mod_str_raw_ptr)
+        continue;
+      const std::string virtual_mod_str = virtual_mod_str_raw_ptr;
+      XFree(virtual_mod_str_raw_ptr);
+
+      if (string_to_find == virtual_mod_str) {
+        if (!XkbVirtualModsToReal(xkb_desc, virtual_mod_mask, &real_mask)) {
+          DVLOG(1) << "XkbVirtualModsToReal failed";
+          real_mask = kBadMask;  // reset the return value, just in case.
+        }
+        break;
+      }
+    }
+  }
+  XkbFreeKeyboard(xkb_desc, 0, True /* free all components */);
+  return real_mask;
+}
+
+void XKeyboardImpl::SetLockedModifiers(bool caps_lock_enabled) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // Always turn off num lock.
+  unsigned int affect_mask = num_lock_mask_;
+  unsigned int value_mask = 0;
+
+  affect_mask |= LockMask;
+  value_mask |= (caps_lock_enabled ? LockMask : 0);
+  current_caps_lock_status_ = caps_lock_enabled;
+
+  XkbLockModifiers(GetXDisplay(), XkbUseCoreKbd, affect_mask, value_mask);
 }
 
 bool XKeyboardImpl::SetLayoutInternal(const std::string& layout_name,
@@ -232,71 +281,11 @@ void XKeyboardImpl::PollUntilChildFinish(const base::ProcessHandle handle) {
   }
 }
 
-bool XKeyboardImpl::NumLockIsEnabled() {
-  bool num_lock_enabled = false;
-  GetLockedModifiers(NULL /* Caps Lock */, &num_lock_enabled);
-  return num_lock_enabled;
-}
-
 bool XKeyboardImpl::CapsLockIsEnabled() {
-  bool caps_lock_enabled = false;
-  GetLockedModifiers(&caps_lock_enabled, NULL /* Num Lock */);
-  return caps_lock_enabled;
-}
-
-unsigned int XKeyboardImpl::GetNumLockMask() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  static const unsigned int kBadMask = 0;
-
-  unsigned int real_mask = kBadMask;
-  XkbDescPtr xkb_desc =
-      XkbGetKeyboard(GetXDisplay(), XkbAllComponentsMask, XkbUseCoreKbd);
-  if (!xkb_desc)
-    return kBadMask;
-
-  if (xkb_desc->dpy && xkb_desc->names && xkb_desc->names->vmods) {
-    const std::string string_to_find(kNumLockVirtualModifierString);
-    for (size_t i = 0; i < XkbNumVirtualMods; ++i) {
-      const unsigned int virtual_mod_mask = 1U << i;
-      char* virtual_mod_str_raw_ptr =
-          XGetAtomName(xkb_desc->dpy, xkb_desc->names->vmods[i]);
-      if (!virtual_mod_str_raw_ptr)
-        continue;
-      const std::string virtual_mod_str = virtual_mod_str_raw_ptr;
-      XFree(virtual_mod_str_raw_ptr);
-
-      if (string_to_find == virtual_mod_str) {
-        if (!XkbVirtualModsToReal(xkb_desc, virtual_mod_mask, &real_mask)) {
-          DVLOG(1) << "XkbVirtualModsToReal failed";
-          real_mask = kBadMask;  // reset the return value, just in case.
-        }
-        break;
-      }
-    }
-  }
-  XkbFreeKeyboard(xkb_desc, 0, True /* free all components */);
-  return real_mask;
-}
-
-void XKeyboardImpl::GetLockedModifiers(bool* out_caps_lock_enabled,
-                                       bool* out_num_lock_enabled) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  if (out_num_lock_enabled && !num_lock_mask_) {
-    DVLOG(1) << "Cannot get locked modifiers. Num Lock mask unknown.";
-    if (out_caps_lock_enabled)
-      *out_caps_lock_enabled = false;
-    if (out_num_lock_enabled)
-      *out_num_lock_enabled = false;
-    return;
-  }
-
   XkbStateRec status;
   XkbGetState(GetXDisplay(), XkbUseCoreKbd, &status);
-  if (out_caps_lock_enabled)
-    *out_caps_lock_enabled = status.locked_mods & LockMask;
-  if (out_num_lock_enabled)
-    *out_num_lock_enabled = status.locked_mods & num_lock_mask_;
+  return (status.locked_mods & LockMask);
 }
 
 bool XKeyboardImpl::SetAutoRepeatEnabled(bool enabled) {
@@ -321,39 +310,8 @@ bool XKeyboardImpl::SetAutoRepeatRate(const AutoRepeatRate& rate) {
   return true;
 }
 
-void XKeyboardImpl::SetLockedModifiers(ModifierLockStatus new_caps_lock_status,
-                                       ModifierLockStatus new_num_lock_status) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (!num_lock_mask_) {
-    DVLOG(1) << "Cannot set locked modifiers. Num Lock mask unknown.";
-    return;
-  }
-
-  unsigned int affect_mask = 0;
-  unsigned int value_mask = 0;
-  if (new_caps_lock_status != kDontChange) {
-    affect_mask |= LockMask;
-    value_mask |= ((new_caps_lock_status == kEnableLock) ? LockMask : 0);
-    current_caps_lock_status_ = (new_caps_lock_status == kEnableLock);
-  }
-  if (new_num_lock_status != kDontChange) {
-    affect_mask |= num_lock_mask_;
-    value_mask |= ((new_num_lock_status == kEnableLock) ? num_lock_mask_ : 0);
-    current_num_lock_status_ = (new_num_lock_status == kEnableLock);
-  }
-
-  if (affect_mask)
-    XkbLockModifiers(GetXDisplay(), XkbUseCoreKbd, affect_mask, value_mask);
-}
-
-void XKeyboardImpl::SetNumLockEnabled(bool enable_num_lock) {
-  SetLockedModifiers(
-      kDontChange, enable_num_lock ? kEnableLock : kDisableLock);
-}
-
 void XKeyboardImpl::SetCapsLockEnabled(bool enable_caps_lock) {
-  SetLockedModifiers(
-      enable_caps_lock ? kEnableLock : kDisableLock, kDontChange);
+  SetLockedModifiers(enable_caps_lock);
 }
 
 bool XKeyboardImpl::SetCurrentKeyboardLayoutByName(
@@ -374,8 +332,11 @@ bool XKeyboardImpl::ReapplyCurrentKeyboardLayout() {
 }
 
 void XKeyboardImpl::ReapplyCurrentModifierLockStatus() {
-  SetLockedModifiers(current_caps_lock_status_ ? kEnableLock : kDisableLock,
-                     current_num_lock_status_ ? kEnableLock : kDisableLock);
+  SetLockedModifiers(current_caps_lock_status_);
+}
+
+void XKeyboardImpl::DisableNumLock() {
+  SetCapsLockEnabled(current_caps_lock_status_);
 }
 
 void XKeyboardImpl::OnSetLayoutFinish() {

chromeos/ime/xkeyboard.h

@@ -20,12 +20,6 @@ struct AutoRepeatRate {
   unsigned int repeat_interval_in_ms;
 };
 
-enum ModifierLockStatus {
-  kDisableLock = 0,
-  kEnableLock,
-  kDontChange,
-};
-
 enum ModifierKey {
   kSearchKey = 0,  // Customizable.
   kControlKey,  // Customizable.
@@ -65,40 +59,17 @@ class CHROMEOS_EXPORT XKeyboard {
   // core/master keyboard.
   virtual void ReapplyCurrentModifierLockStatus() = 0;
 
-  // Sets the Caps Lock and Num Lock status. Do not call the function from
-  // non-UI threads.
-  virtual void SetLockedModifiers(ModifierLockStatus new_caps_lock_status,
-                                  ModifierLockStatus new_num_lock_status) = 0;
-
-  // Sets the num lock status to |enable_num_lock|. Do not call the function
-  // from non-UI threads.
-  virtual void SetNumLockEnabled(bool enable_num_lock) = 0;
+  // Disables the num lock.
+  virtual void DisableNumLock() = 0;
 
   // Sets the caps lock status to |enable_caps_lock|. Do not call the function
   // from non-UI threads.
   virtual void SetCapsLockEnabled(bool enable_caps_lock) = 0;
 
-  // Returns true if num lock is enabled. Do not call the function from non-UI
-  // threads.
-  virtual bool NumLockIsEnabled() = 0;
-
   // Returns true if caps lock is enabled. Do not call the function from non-UI
   // threads.
   virtual bool CapsLockIsEnabled() = 0;
 
-  // Returns a mask (e.g. 1U<<4) for Num Lock. On error, returns 0. Do not call
-  // the function from non-UI threads.
-  // TODO(yusukes): Move this and webdriver::GetXModifierMask() functions in
-  // chrome/test/webdriver/keycode_text_conversion_x.cc to ui/base/x/x11_util.
-  // The two functions are almost the same.
-  virtual unsigned int GetNumLockMask() = 0;
-
-  // Set true on |out_caps_lock_enabled| if Caps Lock is enabled. Set true on
-  // |out_num_lock_enabled| if Num Lock is enabled. Both out parameters can be
-  // NULL. Do not call the function from non-UI threads.
-  virtual void GetLockedModifiers(bool* out_caps_lock_enabled,
-                                  bool* out_num_lock_enabled) = 0;
-
   // Turns on and off the auto-repeat of the keyboard. Returns true on success.
   // Do not call the function from non-UI threads.
   virtual bool SetAutoRepeatEnabled(bool enabled) = 0;

chromeos/ime/xkeyboard_unittest.cc

@@ -91,85 +91,6 @@ TEST_F(XKeyboardTest, TestSetCapsLockEnabled) {
   xkey_->SetCapsLockEnabled(initial_lock_state);
 }
 
-TEST_F(XKeyboardTest, TestSetNumLockEnabled) {
-  if (!DisplayAvailable()) {
-    DVLOG(1) << "X server is not available. Skip the test.";
-    return;
-  }
-  const unsigned int num_lock_mask = xkey_->GetNumLockMask();
-  ASSERT_NE(0U, num_lock_mask);
-
-  const bool initial_lock_state = xkey_->NumLockIsEnabled();
-  xkey_->SetNumLockEnabled(true);
-  EXPECT_TRUE(xkey_->NumLockIsEnabled());
-  xkey_->SetNumLockEnabled(false);
-  EXPECT_FALSE(xkey_->NumLockIsEnabled());
-  xkey_->SetNumLockEnabled(true);
-  EXPECT_TRUE(xkey_->NumLockIsEnabled());
-  xkey_->SetNumLockEnabled(false);
-  EXPECT_FALSE(xkey_->NumLockIsEnabled());
-  xkey_->SetNumLockEnabled(initial_lock_state);
-}
-
-TEST_F(XKeyboardTest, TestSetCapsLockAndNumLockAtTheSameTime) {
-  if (!DisplayAvailable()) {
-    DVLOG(1) << "X server is not available. Skip the test.";
-    return;
-  }
-  const unsigned int num_lock_mask = xkey_->GetNumLockMask();
-  ASSERT_NE(0U, num_lock_mask);
-
-  const bool initial_caps_lock_state = xkey_->CapsLockIsEnabled();
-  const bool initial_num_lock_state = xkey_->NumLockIsEnabled();
-
-  // Flip both.
-  xkey_->SetLockedModifiers(
-      initial_caps_lock_state ? kDisableLock : kEnableLock,
-      initial_num_lock_state ? kDisableLock : kEnableLock);
-  EXPECT_EQ(!initial_caps_lock_state, xkey_->CapsLockIsEnabled());
-  EXPECT_EQ(!initial_num_lock_state, xkey_->NumLockIsEnabled());
-
-  // Flip Caps Lock.
-  xkey_->SetLockedModifiers(
-      initial_caps_lock_state ? kEnableLock : kDisableLock,
-      kDontChange);
-  // Use GetLockedModifiers() for verifying the result.
-  bool c, n;
-  xkey_->GetLockedModifiers(&c, &n);
-  EXPECT_EQ(initial_caps_lock_state, c);
-  EXPECT_EQ(!initial_num_lock_state, n);
-
-  // Flip both.
-  xkey_->SetLockedModifiers(
-      initial_caps_lock_state ? kDisableLock : kEnableLock,
-      initial_num_lock_state ? kEnableLock : kDisableLock);
-  EXPECT_EQ(!initial_caps_lock_state, xkey_->CapsLockIsEnabled());
-  EXPECT_EQ(initial_num_lock_state, xkey_->NumLockIsEnabled());
-
-  // Flip Num Lock.
-  xkey_->SetLockedModifiers(
-      kDontChange,
-      initial_num_lock_state ? kDisableLock : kEnableLock);
-  xkey_->GetLockedModifiers(&c, &n);
-  EXPECT_EQ(!initial_caps_lock_state, c);
-  EXPECT_EQ(!initial_num_lock_state, n);
-
-  // Flip both to restore the initial state.
-  xkey_->SetLockedModifiers(
-      initial_caps_lock_state ? kEnableLock : kDisableLock,
-      initial_num_lock_state ? kEnableLock : kDisableLock);
-  EXPECT_EQ(initial_caps_lock_state, xkey_->CapsLockIsEnabled());
-  EXPECT_EQ(initial_num_lock_state, xkey_->NumLockIsEnabled());
-
-  // No-op SetLockedModifiers call.
-  xkey_->SetLockedModifiers(kDontChange, kDontChange);
-  EXPECT_EQ(initial_caps_lock_state, xkey_->CapsLockIsEnabled());
-  EXPECT_EQ(initial_num_lock_state, xkey_->NumLockIsEnabled());
-
-  // No-op GetLockedModifiers call. Confirm it does not crash.
-  xkey_->GetLockedModifiers(NULL, NULL);
-}
-
 TEST_F(XKeyboardTest, TestSetAutoRepeatEnabled) {
   if (!DisplayAvailable()) {
     DVLOG(1) << "X server is not available. Skip the test.";