[Remoting Host] Handle text event characters that are not presented on the keyboard

remoting/host/linux/x11_character_injector_unittest.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 "remoting/host/linux/x11_character_injector.h"
+
+#include <stdarg.h>
+
+#include <map>
+
+#include "base/memory/ptr_util.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "remoting/host/linux/x11_keyboard.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+const int kMappingExpireMs = 200;
+
+const int kSyncDelayMs = 10;
+
+constexpr base::TimeDelta kSyncDelay =
+      base::TimeDelta::FromMilliseconds(kSyncDelayMs);
+
+base::TimeDelta TimeDeltaMs(int ms) {
+  return base::TimeDelta::FromMilliseconds(ms);
+}
+
+}  // namespace
+
+namespace remoting {
+
+class FakeX11Keyboard : public X11Keyboard {

[Sergey Ulanov, 2016/09/27 18:40:49]

Add a short comment here to explain how this class works.

[Yuwei, 2016/09/28 18:45:55]

Done.

+ public:
+  struct KeyPressExpectation {
+    base::TimeDelta min_delay;  // To the last key press.
+    uint32_t keycode;
+  };
+
+  struct ChangeKeyMappingExpectation {
+    uint32_t keycode;
+    uint32_t code_point;
+  };
+
+  explicit FakeX11Keyboard(const std::vector<uint32_t>& available_keycodes);
+  ~FakeX11Keyboard() override;
+
+  std::vector<uint32_t> GetUnusedKeycodes() override;

[Sergey Ulanov, 2016/09/27 18:40:49]

// X11Keyboard interface.

[Yuwei, 2016/09/28 18:45:55]

Done.

+
+  void PressKey(uint32_t keycode, uint32_t modifiers) override;
+  bool FindKeycode(uint32_t code_point,
+                   uint32_t* keycode,
+                   uint32_t* modifiers) override;
+  bool ChangeKeyMapping(uint32_t keycode, uint32_t code_point) override;
+  void Flush() override;
+  void Sync() override;
+
+  void ExpectKeyPress(const std::vector<KeyPressExpectation>& sequence);
+  void ExpectChangeKeyMapping(
+      const std::vector<ChangeKeyMappingExpectation>& sequence);
+  void InjectorAboutToDestruct();
+
+  // Sets a callback to be called when the keypress expectation queue becomes
+  // empty.
+  void SetKeyPressFinishedCallback(const base::Closure& callback) {
+    keypress_finished_callback_ = callback;
+  }
+
+ private:
+  std::map<uint32_t, uint32_t> keycode_mapping_;

[Sergey Ulanov, 2016/09/27 18:40:49]

unordered_map
(not that performance matters here, but map<> is should be avoided almost
everywhere)

[Yuwei, 2016/09/28 18:45:55]

Done.
Okay... These names are confusing and sounds like unordered_map is a subclass of
map...

+  std::queue<KeyPressExpectation> keypress_expectations_;
+  std::queue<ChangeKeyMappingExpectation> change_key_mapping_expectations_;
+  bool injector_about_to_destruct_ = false;
+  base::TimeTicks last_keypress_time_;
+  base::Closure keypress_finished_callback_;
+};
+
+FakeX11Keyboard::FakeX11Keyboard(
+    const std::vector<uint32_t>& available_keycodes) {
+  for (uint32_t keycode : available_keycodes) {
+    keycode_mapping_.insert({keycode, 0});
+  }
+}
+
+FakeX11Keyboard::~FakeX11Keyboard() {
+  DCHECK(keypress_expectations_.empty());
+  DCHECK(change_key_mapping_expectations_.empty());
+  for (const auto& pair : keycode_mapping_) {
+    EXPECT_EQ(0u, pair.second);
+  }
+}
+
+std::vector<uint32_t> FakeX11Keyboard::GetUnusedKeycodes() {
+  std::vector<uint32_t> keycodes;
+  for (const auto& pair : keycode_mapping_) {
+    if (!pair.second) {
+      keycodes.push_back(pair.first);
+    }
+  }
+  return keycodes;
+}
+
+void FakeX11Keyboard::PressKey(uint32_t keycode, uint32_t modifiers) {
+  ASSERT_FALSE(keypress_expectations_.empty());
+  KeyPressExpectation& expectation = keypress_expectations_.front();
+  ASSERT_EQ(expectation.keycode, keycode);
+  base::TimeTicks now = base::TimeTicks::Now();
+  base::TimeDelta delay = now - last_keypress_time_;
+  ASSERT_GE(delay, expectation.min_delay);
+  last_keypress_time_ = now;
+  keypress_expectations_.pop();
+  if (keypress_expectations_.empty() && keypress_finished_callback_) {
+    keypress_finished_callback_.Run();
+  }
+}
+
+bool FakeX11Keyboard::FindKeycode(uint32_t code_point,
+                                  uint32_t* keycode,
+                                  uint32_t* modifiers) {
+  auto position = std::find_if(keycode_mapping_.begin(), keycode_mapping_.end(),
+               [code_point](const std::pair<uint32_t, uint32_t>& pair) {
+    return pair.second == code_point;
+  });
+  if (position == keycode_mapping_.end()) {
+    return false;
+  }
+  *keycode = position->first;
+  *modifiers = 0;
+  return true;
+}
+
+bool FakeX11Keyboard::ChangeKeyMapping(uint32_t keycode, uint32_t code_point) {
+  if (injector_about_to_destruct_) {
+    EXPECT_EQ(0u, code_point);
+  } else {
+    EXPECT_FALSE(change_key_mapping_expectations_.empty())
+        << "Unexpected change key request. Key Code: " << keycode
+        << " Code Point: " << code_point;
+    ChangeKeyMappingExpectation& expectation =
+        change_key_mapping_expectations_.front();
+    EXPECT_EQ(expectation.keycode, keycode);
+    EXPECT_EQ(expectation.code_point, code_point);
+    change_key_mapping_expectations_.pop();
+  }
+
+  keycode_mapping_[keycode] = code_point;
+  return true;
+}
+
+void FakeX11Keyboard::Flush() {}
+
+void FakeX11Keyboard::Sync() {
+  usleep(kSyncDelayMs * 1000);

[Sergey Ulanov, 2016/09/27 18:40:49]

Why do we need to sleep here?

[Yuwei, 2016/09/28 18:45:55]

This was a simulation of sync delay and can help for counting how many Sync is
called for entering each character.

Just removed this delay since now we don't care about details like calling order
or counts.

+}
+
+void FakeX11Keyboard::ExpectKeyPress(
+    const std::vector<KeyPressExpectation>& sequence) {
+  for (const auto& expectation : sequence) {

[Sergey Ulanov, 2016/09/27 18:40:49]

keypress_expectations_.insert(
  keypress_expectations_.begin(), sequence.begin(), sequence.end());

[Yuwei, 2016/09/28 18:45:55]

Done in ExpectEnterCodePoints(). Changed queue to deque to get the iterators.

+    keypress_expectations_.push(expectation);
+  }
+}
+
+void FakeX11Keyboard::ExpectChangeKeyMapping(
+    const std::vector<ChangeKeyMappingExpectation>& sequence) {
+  for (const auto& expectation : sequence) {

[Sergey Ulanov, 2016/09/27 18:40:49]

use std::vector::insert()

[Yuwei, 2016/09/28 18:45:55]

Obsolete.

+    change_key_mapping_expectations_.push(expectation);
+  }
+}
+
+void FakeX11Keyboard::InjectorAboutToDestruct() {
+  injector_about_to_destruct_ = true;
+}
+
+class X11CharacterInjectorTest : public testing::Test {
+ public:
+  void SetUp() override;
+  void TearDown() override;
+
+ protected:
+  void ResetRunLoop();
+  void FillUntilFull(bool immediately_run);
+
+  std::unique_ptr<X11CharacterInjector> injector_;
+  FakeX11Keyboard* keyboard_;  // Owned by |injector_|.
+
+  base::MessageLoop message_loop_;
+  std::unique_ptr<base::RunLoop> run_loop_;
+};
+
+void X11CharacterInjectorTest::SetUp() {
+  keyboard_ = new FakeX11Keyboard({55, 54, 53, 52, 51});
+  injector_.reset(new X11CharacterInjector(base::WrapUnique(keyboard_)));
+  ResetRunLoop();
+}
+
+void X11CharacterInjectorTest::TearDown() {
+  run_loop_->Run();
+  keyboard_->InjectorAboutToDestruct();
+  injector_.reset();
+}
+
+void X11CharacterInjectorTest::ResetRunLoop() {
+  run_loop_.reset(new base::RunLoop());
+  keyboard_->SetKeyPressFinishedCallback(run_loop_->QuitClosure());
+}
+
+void X11CharacterInjectorTest::FillUntilFull(bool immediately_run) {
+  injector_->Inject(1);
+  injector_->Inject(2);
+  injector_->Inject(3);
+  injector_->Inject(4);
+  injector_->Inject(5);
+
+  keyboard_->ExpectChangeKeyMapping(
+      {{55, 1}, {54, 2}, {53, 3}, {52, 4}, {51, 5}});
+
+  keyboard_->ExpectKeyPress(
+      {{kSyncDelay, 55},
+       {kSyncDelay, 54},
+       {kSyncDelay, 53},
+       {kSyncDelay, 52},
+       {kSyncDelay, 51}});
+  if (immediately_run) {
+    run_loop_->Run();
+    ResetRunLoop();
+  }
+}
+
+TEST_F(X11CharacterInjectorTest, TestNoMappingNoExpectation) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                run_loop_->QuitClosure());
+}
+
+TEST_F(X11CharacterInjectorTest, TestTypeOneCharacter) {
+  injector_->Inject(123);
+
+  keyboard_->ExpectChangeKeyMapping({{55, 123}});
+  keyboard_->ExpectKeyPress({{kSyncDelay, 55}});
+}
+
+TEST_F(X11CharacterInjectorTest, TestMapCharactersUntilFull) {
+  FillUntilFull(false);
+}
+
+TEST_F(X11CharacterInjectorTest, TestMapOneCharacterWhenFull) {
+  FillUntilFull(false);
+  injector_->Inject(6);
+
+  keyboard_->ExpectChangeKeyMapping({{55, 6}});
+
+  const int kDelayForReuseCode =
+      kMappingExpireMs - 5 * kSyncDelayMs;
+
+  keyboard_->ExpectKeyPress({{TimeDeltaMs(kDelayForReuseCode), 55}});
+}
+
+TEST_F(X11CharacterInjectorTest, TestImmediatelyReuseOldKeycodeAfterExpired) {
+  FillUntilFull(true);
+
+  usleep((kMappingExpireMs + 50) * 1000);
+
+  injector_->Inject(6);
+
+  keyboard_->ExpectChangeKeyMapping({{55, 6}});
+
+  keyboard_->ExpectKeyPress({{kSyncDelay, 55}});
+}
+
+TEST_F(X11CharacterInjectorTest, TestReusingMappedCharacterLowersPriority) {
+  FillUntilFull(true);
+
+  usleep((kMappingExpireMs + 50) * 1000);
+
+  injector_->Inject(1);
+  injector_->Inject(6);
+
+  keyboard_->ExpectChangeKeyMapping({{54, 6}});
+
+  keyboard_->ExpectKeyPress({{kSyncDelay, 55}, {kSyncDelay, 54}});
+}
+
+TEST_F(X11CharacterInjectorTest, TestOrderAfterReusingCharacters) {
+  auto inject_and_wait = [this](uint32_t keycode,

[Sergey Ulanov, 2016/09/27 18:40:49]

Make this a class method instead of a closure?

[Yuwei, 2016/09/28 18:45:55]

Made InjectAndRun function

+                                uint32_t code_point) {
+    keyboard_->ExpectKeyPress(
+        {{base::TimeDelta(), keycode}});
+    injector_->Inject(code_point);
+    run_loop_->Run();
+    ResetRunLoop();
+    usleep(kSyncDelayMs * 1000);
+  };
+
+  FillUntilFull(true);
+
+  inject_and_wait(54, 2);

[Sergey Ulanov, 2016/09/27 18:40:49]

These tests are too rigorous in the way they set expectations which makes them
brittle. E.g. they verify X11CharacterInjector uses available keycodes in a
specific order. If X11CharacterInjector is ever changed to use a different
container to store available keys then it may use them in different order. The
code would still be correct, but this test will break. All that matters for
X11CharacterInjector is that correct characters get injected. Which keycodes are
used for them is irrelevant.

[Yuwei, 2016/09/28 18:45:55]

I would say different ordering rules? Changing data structure may not
necessarily change the order.
Agreed. I have loosened the test a bit to make it check the sequence of
characters being injected instead.

+  inject_and_wait(52, 4);
+  inject_and_wait(51, 5);
+  inject_and_wait(55, 1);
+  inject_and_wait(53, 3);
+
+  usleep((kMappingExpireMs + 50) * 1000);
+
+  injector_->Inject(31);
+  injector_->Inject(32);
+  injector_->Inject(33);
+  injector_->Inject(34);
+  injector_->Inject(35);
+
+  keyboard_->ExpectChangeKeyMapping({
+    {54, 31}, {52, 32}, {51, 33}, {55, 34}, {53, 35}});
+
+  keyboard_->ExpectKeyPress(
+        {{kSyncDelay, 54},
+         {kSyncDelay, 52},
+         {kSyncDelay, 51},
+         {kSyncDelay, 55},
+         {kSyncDelay, 53}});
+}
+
+}  // namespace remoting