Don't store the keyboard info when reading the layout property fails

Reading the keyboard device layout property from udev might fail. When
this happens, we used to store the keyboard info with a default layout
value = Layout1.

This CL avoid storing the keyboard info when a failure in udev is detected,
we will still fallback to the default layout for the current key event, but
we will attempt to re-read the layout property from udev on subsequent keys
until a valid layout is returned. This eliminates the need to reboot the
device to exit the bad state.

BUG=783166
TEST=added new test.

Change-Id: I94d6771ef955e20cee831ad0a93e31dcaaf8f610
Reviewed-on: https://chromium-review.googlesource.com/764444Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516063}

chrome/browser/chromeos/events/event_rewriter_unittest.cc

@@ -1953,6 +1953,67 @@ TEST_F(EventRewriterTest, TestRewriteFunctionKeysLayout2) {
     CheckKeyTestCase(rewriter_, test);
 }
 
+TEST_F(EventRewriterTest, TestRewriteFunctionKeysInvalidLayout) {
+  chromeos::Preferences::RegisterProfilePrefs(prefs()->registry());
+
+  // Not adding a keyboard simulates a failure in getting top row layout, which
+  // will fallback to Layout1 in which case keys are rewritten to their default
+  // values.
+  KeyTestCase invalid_layout_tests[] = {
+      // F2 -> Forward
+      {ui::ET_KEY_PRESSED,
+       {ui::VKEY_F2, ui::DomCode::F2, ui::EF_NONE, ui::DomKey::F2},
+       {ui::VKEY_BROWSER_FORWARD, ui::DomCode::BROWSER_FORWARD, ui::EF_NONE,
+        ui::DomKey::BROWSER_FORWARD}},
+      // F3 -> Refresh
+      {ui::ET_KEY_PRESSED,
+       {ui::VKEY_F3, ui::DomCode::F3, ui::EF_NONE, ui::DomKey::F3},
+       {ui::VKEY_BROWSER_REFRESH, ui::DomCode::BROWSER_REFRESH, ui::EF_NONE,
+        ui::DomKey::BROWSER_REFRESH}},
+      // F4 -> Launch App 2
+      {ui::ET_KEY_PRESSED,
+       {ui::VKEY_F4, ui::DomCode::F4, ui::EF_NONE, ui::DomKey::F4},
+       {ui::VKEY_MEDIA_LAUNCH_APP2, ui::DomCode::ZOOM_TOGGLE, ui::EF_NONE,
+        ui::DomKey::ZOOM_TOGGLE}},
+      // F7 -> Brightness up
+      {ui::ET_KEY_PRESSED,
+       {ui::VKEY_F7, ui::DomCode::F7, ui::EF_NONE, ui::DomKey::F7},
+       {ui::VKEY_BRIGHTNESS_UP, ui::DomCode::BRIGHTNESS_UP, ui::EF_NONE,
+        ui::DomKey::BRIGHTNESS_UP}}};
+
+  for (const auto& test : invalid_layout_tests)
+    CheckKeyTestCase(rewriter_, test);
+
+  // Adding a keyboard with a valid layout will take effect.
+  rewriter_->KeyboardDeviceAddedForTesting(
+      kKeyboardDeviceId, "PC Keyboard",
+      ui::EventRewriterChromeOS::kKbdTopRowLayout2);
+  KeyTestCase layout2_tests[] = {
+      // F2 -> Refresh
+      {ui::ET_KEY_PRESSED,
+       {ui::VKEY_F2, ui::DomCode::F2, ui::EF_NONE, ui::DomKey::F2},
+       {ui::VKEY_BROWSER_REFRESH, ui::DomCode::BROWSER_REFRESH, ui::EF_NONE,
+        ui::DomKey::BROWSER_REFRESH}},
+      // F3 -> Launch App 2
+      {ui::ET_KEY_PRESSED,
+       {ui::VKEY_F3, ui::DomCode::F3, ui::EF_NONE, ui::DomKey::F3},
+       {ui::VKEY_MEDIA_LAUNCH_APP2, ui::DomCode::ZOOM_TOGGLE, ui::EF_NONE,
+        ui::DomKey::ZOOM_TOGGLE}},
+      // F4 -> Launch App 1
+      {ui::ET_KEY_PRESSED,
+       {ui::VKEY_F4, ui::DomCode::F4, ui::EF_NONE, ui::DomKey::F4},
+       {ui::VKEY_MEDIA_LAUNCH_APP1, ui::DomCode::SELECT_TASK, ui::EF_NONE,
+        ui::DomKey::LAUNCH_MY_COMPUTER}},
+      // F7 -> Media Play/Pause
+      {ui::ET_KEY_PRESSED,
+       {ui::VKEY_F7, ui::DomCode::F7, ui::EF_NONE, ui::DomKey::F7},
+       {ui::VKEY_MEDIA_PLAY_PAUSE, ui::DomCode::MEDIA_PLAY_PAUSE, ui::EF_NONE,
+        ui::DomKey::MEDIA_PLAY_PAUSE}}};
+
+  for (const auto& test : layout2_tests)
+    CheckKeyTestCase(rewriter_, test);
+}
+
 TEST_F(EventRewriterTest, TestRewriteExtendedKeysWithSearchRemapped) {
   // Remap Search to Control.
   chromeos::Preferences::RegisterProfilePrefs(prefs()->registry());

ui/chromeos/events/event_rewriter_chromeos.cc

@@ -274,15 +274,15 @@ EventRewriterChromeOS::EventRewriterChromeOS(
 
 EventRewriterChromeOS::~EventRewriterChromeOS() {}
 
-EventRewriterChromeOS::DeviceType
-EventRewriterChromeOS::KeyboardDeviceAddedForTesting(
+void EventRewriterChromeOS::KeyboardDeviceAddedForTesting(
     int device_id,
     const std::string& device_name,
     KeyboardTopRowLayout layout) {
   // Tests must avoid XI2 reserved device IDs.
   DCHECK((device_id < 0) || (device_id > 1));
-  return KeyboardDeviceAddedInternal(device_id, device_name, kUnknownVendorId,
-                                     kUnknownProductId, layout);
+  KeyboardDeviceAddedInternal(
+      device_id,
+      GetDeviceType(device_name, kUnknownVendorId, kUnknownProductId), layout);
 }
 
 void EventRewriterChromeOS::RewriteMouseButtonEventForTesting(
@@ -346,36 +346,40 @@ void EventRewriterChromeOS::BuildRewrittenKeyEvent(
 }
 
 // static
-EventRewriterChromeOS::KeyboardTopRowLayout
-EventRewriterChromeOS::GetKeyboardTopRowLayout(
-    const base::FilePath& device_path) {
+bool EventRewriterChromeOS::GetKeyboardTopRowLayout(
+    const base::FilePath& device_path,
+    KeyboardTopRowLayout* out_layout) {
   device::ScopedUdevPtr udev(device::udev_new());
   if (!udev.get())
-    return EventRewriterChromeOS::kKbdTopRowLayoutDefault;
+    return false;
 
   device::ScopedUdevDevicePtr device(device::udev_device_new_from_syspath(
       udev.get(), device_path.value().c_str()));
   if (!device.get())
-    return EventRewriterChromeOS::kKbdTopRowLayoutDefault;
+    return false;
 
   const char kLayoutProperty[] = "CROS_KEYBOARD_TOP_ROW_LAYOUT";
   std::string layout =
       device::UdevDeviceGetPropertyValue(device.get(), kLayoutProperty);
-  if (layout.empty())
-    return EventRewriterChromeOS::kKbdTopRowLayoutDefault;
+  if (layout.empty()) {
+    *out_layout = EventRewriterChromeOS::kKbdTopRowLayoutDefault;
+    return true;
+  }
 
   int layout_id;
   if (!base::StringToInt(layout, &layout_id)) {
     LOG(WARNING) << "Failed to parse " << kLayoutProperty << " value '"
                  << layout << "'";
-    return EventRewriterChromeOS::kKbdTopRowLayoutDefault;
+    return false;
   }
   if (layout_id < EventRewriterChromeOS::kKbdTopRowLayoutMin ||
       layout_id > EventRewriterChromeOS::kKbdTopRowLayoutMax) {
     LOG(WARNING) << "Invalid " << kLayoutProperty << " '" << layout << "'";
-    return EventRewriterChromeOS::kKbdTopRowLayoutDefault;
+    return false;
   }
-  return static_cast<EventRewriterChromeOS::KeyboardTopRowLayout>(layout_id);
+  *out_layout =
+      static_cast<EventRewriterChromeOS::KeyboardTopRowLayout>(layout_id);
+  return true;
 }
 
 void EventRewriterChromeOS::DeviceKeyPressedOrReleased(int device_id) {
@@ -1165,32 +1169,13 @@ int EventRewriterChromeOS::RewriteModifierClick(
   return ui::EF_NONE;
 }
 
-EventRewriterChromeOS::DeviceType
-EventRewriterChromeOS::KeyboardDeviceAddedInternal(
+void EventRewriterChromeOS::KeyboardDeviceAddedInternal(
     int device_id,
-    const std::string& device_name,
-    int vendor_id,
-    int product_id,
+    DeviceType type,
     KeyboardTopRowLayout layout) {
-  const DeviceType type = GetDeviceType(device_name, vendor_id, product_id);
-  if (type == kDeviceAppleKeyboard) {
-    VLOG(1) << "Apple keyboard '" << device_name << "' connected: "
-            << "id=" << device_id;
-  } else if (type == kDeviceHotrodRemote) {
-    VLOG(1) << "Hotrod remote '" << device_name << "' connected: "
-            << "id=" << device_id;
-  } else if (type == kDeviceVirtualCoreKeyboard) {
-    VLOG(1) << "Xorg virtual '" << device_name << "' connected: "
-            << "id=" << device_id;
-  } else {
-    VLOG(1) << "Unknown keyboard '" << device_name << "' connected: "
-            << "id=" << device_id;
-  }
-
   // Always overwrite the existing device_id since the X server may reuse a
   // device id for an unattached device.
   device_id_to_info_[device_id] = {type, layout};
-  return type;
 }
 
 EventRewriterChromeOS::DeviceType EventRewriterChromeOS::KeyboardDeviceAdded(
@@ -1201,9 +1186,32 @@ EventRewriterChromeOS::DeviceType EventRewriterChromeOS::KeyboardDeviceAdded(
       ui::InputDeviceManager::GetInstance()->GetKeyboardDevices();
   for (const auto& keyboard : keyboard_devices) {
     if (keyboard.id == device_id) {
-      return KeyboardDeviceAddedInternal(
-          keyboard.id, keyboard.name, keyboard.vendor_id, keyboard.product_id,
-          GetKeyboardTopRowLayout(keyboard.sys_path));
+      const DeviceType type =
+          GetDeviceType(keyboard.name, keyboard.vendor_id, keyboard.product_id);
+      if (type == kDeviceAppleKeyboard) {
+        VLOG(1) << "Apple keyboard '" << keyboard.name << "' connected: "
+                << "id=" << device_id;
+      } else if (type == kDeviceHotrodRemote) {
+        VLOG(1) << "Hotrod remote '" << keyboard.name << "' connected: "
+                << "id=" << device_id;
+      } else if (type == kDeviceVirtualCoreKeyboard) {
+        VLOG(1) << "Xorg virtual '" << keyboard.name << "' connected: "
+                << "id=" << device_id;
+      } else {
+        VLOG(1) << "Unknown keyboard '" << keyboard.name << "' connected: "
+                << "id=" << device_id;
+      }
+
+      KeyboardTopRowLayout layout;
+      if (GetKeyboardTopRowLayout(keyboard.sys_path, &layout)) {
+        // Don't store a device info when an error occurred while reading from
+        // udev. This gives a chance to reattempt reading from udev on
+        // subsequent key events, rather than being stuck in a bad state until
+        // next reboot. crbug.com/783166.
+        KeyboardDeviceAddedInternal(keyboard.id, type, layout);
+      }
+
+      return type;
     }
   }
   return kDeviceUnknown;

ui/chromeos/events/event_rewriter_chromeos.h

@@ -103,7 +103,7 @@ class EventRewriterChromeOS : public ui::EventRewriter {
   ~EventRewriterChromeOS() override;
 
   // Calls KeyboardDeviceAddedInternal.
-  DeviceType KeyboardDeviceAddedForTesting(
+  void KeyboardDeviceAddedForTesting(
       int device_id,
       const std::string& device_name,
       KeyboardTopRowLayout layout = kKbdTopRowLayoutDefault);
@@ -138,8 +138,9 @@ class EventRewriterChromeOS : public ui::EventRewriter {
 
   // Given the file path of a keyboard device, returns the layout type of the
   // top row keys.
-  static KeyboardTopRowLayout GetKeyboardTopRowLayout(
-      const base::FilePath& device_path);
+  static bool GetKeyboardTopRowLayout(const base::FilePath& device_path,
+                                      KeyboardTopRowLayout* out_layout)
+      WARN_UNUSED_RESULT;
 
  private:
   struct DeviceInfo {
@@ -149,15 +150,14 @@ class EventRewriterChromeOS : public ui::EventRewriter {
 
   void DeviceKeyPressedOrReleased(int device_id);
 
-  // Adds a device to |device_id_to_type_|.
+  // Adds a device to |device_id_to_info_| only if no failure occurs in
+  // retrieving the top row layout from udev, and returns the device type of
+  // this keyboard even if it wasn't stored in |device_id_to_info_|.
   DeviceType KeyboardDeviceAdded(int device_id);
 
-  // Checks the type of the |device_name|, |vendor_id| and |product_id|, and
-  // inserts a new entry to |device_id_to_type_|.
-  DeviceType KeyboardDeviceAddedInternal(int device_id,
-                                         const std::string& device_name,
-                                         int vendor_id,
-                                         int product_id,
+  // Inserts a new entry to |device_id_to_info_|.
+  void KeyboardDeviceAddedInternal(int device_id,
+                                   DeviceType type,
                                    KeyboardTopRowLayout layout);
 
   // Returns true if |last_keyboard_device_id_| is Apple's.

ui/chromeos/events/keyboard_layout_util.cc

@@ -12,10 +12,11 @@ namespace ui {
 bool DeviceUsesKeyboardLayout2() {
   for (const InputDevice& keyboard :
        InputDeviceManager::GetInstance()->GetKeyboardDevices()) {
+    EventRewriterChromeOS::KeyboardTopRowLayout layout;
     if (keyboard.type == InputDeviceType::INPUT_DEVICE_INTERNAL &&
-        EventRewriterChromeOS::GetKeyboardTopRowLayout(keyboard.sys_path) ==
-            EventRewriterChromeOS::kKbdTopRowLayout2) {
-      return true;
+        EventRewriterChromeOS::GetKeyboardTopRowLayout(keyboard.sys_path,
+                                                       &layout)) {
+      return layout == EventRewriterChromeOS::kKbdTopRowLayout2;
     }
   }