From 949ff4025242ad08dd1e526f7398d99c5657560a Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 30 Dec 2021 01:36:16 -0800 Subject: [PATCH 01/10] Refactor to MockKeyResponseController (ATP) --- ...ibility_bridge_delegate_win32_unittests.cc | 6 +-- .../windows/flutter_window_win32_unittests.cc | 10 +++-- .../windows/flutter_windows_view_unittests.cc | 19 +++++--- .../windows/keyboard_win32_unittests.cc | 15 +++++-- .../platform/windows/testing/test_keyboard.cc | 41 +++++++++-------- .../platform/windows/testing/test_keyboard.h | 44 ++++++++++++++++--- 6 files changed, 92 insertions(+), 43 deletions(-) diff --git a/shell/platform/windows/accessibility_bridge_delegate_win32_unittests.cc b/shell/platform/windows/accessibility_bridge_delegate_win32_unittests.cc index 78a4a528f522c..82d43d9637bf8 100644 --- a/shell/platform/windows/accessibility_bridge_delegate_win32_unittests.cc +++ b/shell/platform/windows/accessibility_bridge_delegate_win32_unittests.cc @@ -82,9 +82,9 @@ std::unique_ptr GetTestEngine() { [](FLUTTER_API_SYMBOL(FlutterEngine) engine, bool enabled) { return kSuccess; }; - MockEmbedderApiForKeyboard( - modifier, [] { return false; }, - [](const FlutterKeyEvent* event) { return false; }); + + MockEmbedderApiForKeyboard(modifier, + std::make_shared()); engine->RunWithEntrypoint(nullptr); return engine; diff --git a/shell/platform/windows/flutter_window_win32_unittests.cc b/shell/platform/windows/flutter_window_win32_unittests.cc index 1b4a77f13649a..d7210103e022c 100644 --- a/shell/platform/windows/flutter_window_win32_unittests.cc +++ b/shell/platform/windows/flutter_window_win32_unittests.cc @@ -315,9 +315,13 @@ std::unique_ptr GetTestEngine() { auto engine = std::make_unique(project); EngineModifier modifier(engine.get()); - MockEmbedderApiForKeyboard( - modifier, [] { return test_response; }, - [](const FlutterKeyEvent* event) { return false; }); + auto key_response_controller = + std::make_shared(); + key_response_controller->SetChannelResponse( + [](MockKeyResponseController::ResponseCallback callback) { + callback(test_response); + }); + MockEmbedderApiForKeyboard(modifier, key_response_controller); return engine; } diff --git a/shell/platform/windows/flutter_windows_view_unittests.cc b/shell/platform/windows/flutter_windows_view_unittests.cc index 1df7b193aae1f..fdd3dc2150b64 100644 --- a/shell/platform/windows/flutter_windows_view_unittests.cc +++ b/shell/platform/windows/flutter_windows_view_unittests.cc @@ -65,17 +65,22 @@ std::unique_ptr GetTestEngine() { auto engine = std::make_unique(project); EngineModifier modifier(engine.get()); - MockEmbedderApiForKeyboard( - modifier, - [] { + + auto key_response_controller = std::make_shared(); + key_response_controller->SetChannelResponse( + [](MockKeyResponseController::ResponseCallback callback) { key_event_logs.push_back(kKeyEventFromChannel); - return test_response; - }, - [](const FlutterKeyEvent* event) { + callback(test_response); + }); + key_response_controller->SetEmbedderResponse( + [](const FlutterKeyEvent* event, + MockKeyResponseController::ResponseCallback callback) { key_event_logs.push_back(kKeyEventFromEmbedder); - return test_response; + callback(test_response); }); + MockEmbedderApiForKeyboard(modifier, key_response_controller); + engine->RunWithEntrypoint(nullptr); return engine; } diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc index 2be6885057810..c7e0cb1264732 100644 --- a/shell/platform/windows/keyboard_win32_unittests.cc +++ b/shell/platform/windows/keyboard_win32_unittests.cc @@ -242,6 +242,8 @@ std::unique_ptr GetTestEngine(); class KeyboardTester { public: + using KeyEventHandler = std::function; + explicit KeyboardTester() { view_ = std::make_unique( [](const std::u16string& text) { @@ -260,6 +262,8 @@ class KeyboardTester { void Responding(bool response) { test_response = response; } + void LateRespond(bool response) { test_response = response; } + void SetLayout(MapVkToCharHandler layout) { window_->SetLayout(layout); } void InjectMessages(int count, Win32Message message1, ...) { @@ -307,9 +311,10 @@ std::unique_ptr GetTestEngine() { EngineModifier modifier(engine.get()); - MockEmbedderApiForKeyboard( - modifier, [] { return KeyboardTester::test_response; }, - [](const FlutterKeyEvent* event) { + auto key_response_controller = std::make_shared(); + key_response_controller->SetEmbedderResponse( + [](const FlutterKeyEvent* event, + MockKeyResponseController::ResponseCallback callback) { FlutterKeyEvent clone_event = *event; clone_event.character = event->character == nullptr ? nullptr @@ -318,9 +323,11 @@ std::unique_ptr GetTestEngine() { .type = kKeyCallOnKey, .key_event = clone_event, }); - return KeyboardTester::test_response; + callback(KeyboardTester::test_response); }); + MockEmbedderApiForKeyboard(modifier, key_response_controller); + engine->RunWithEntrypoint(nullptr); return engine; } diff --git a/shell/platform/windows/testing/test_keyboard.cc b/shell/platform/windows/testing/test_keyboard.cc index 01971f936bca4..9d1aea8e7013d 100644 --- a/shell/platform/windows/testing/test_keyboard.cc +++ b/shell/platform/windows/testing/test_keyboard.cc @@ -85,8 +85,7 @@ LPARAM CreateKeyEventLparam(USHORT scancode, (LPARAM(scancode) << 16) | LPARAM(repeat_count)); } -static MockKeyEventChannelHandler stored_channel_handler; -static MockKeyEventEmbedderHandler stored_embedder_handler; +static std::shared_ptr stored_response_controller; // Set EngineModifier, listen to event messages that go through the channel and // the embedder API, while disabling other methods so that the engine can be @@ -95,10 +94,8 @@ static MockKeyEventEmbedderHandler stored_embedder_handler; // The |channel_handler| and |embedder_handler| should return a boolean // indicating whether the framework decides to handle the event. void MockEmbedderApiForKeyboard(EngineModifier& modifier, - MockKeyEventChannelHandler channel_handler, - MockKeyEventEmbedderHandler embedder_handler) { - stored_channel_handler = channel_handler; - stored_embedder_handler = embedder_handler; + std::shared_ptr response_controller) { + stored_response_controller = response_controller; // This mock handles channel messages. modifier.embedder_api().SendPlatformMessage = [](FLUTTER_API_SYMBOL(FlutterEngine) engine, @@ -107,29 +104,31 @@ void MockEmbedderApiForKeyboard(EngineModifier& modifier, return kSuccess; } if (std::string(message->channel) == std::string("flutter/keyevent")) { - bool result = stored_channel_handler(); - auto response = _keyHandlingResponse(result); - const TestResponseHandle* response_handle = - reinterpret_cast( - message->response_handle); - if (response_handle->callback != nullptr) { - response_handle->callback(response->data(), response->size(), - response_handle->user_data); - } + stored_response_controller->HandleChannelMessage([message](bool handled) { + auto response = _keyHandlingResponse(handled); + const TestResponseHandle* response_handle = + reinterpret_cast( + message->response_handle); + if (response_handle->callback != nullptr) { + response_handle->callback(response->data(), response->size(), + response_handle->user_data); + } + }); return kSuccess; } return kSuccess; }; - // This mock handles key events sent through the embedder API, - // and records it in `key_calls`. + // This mock handles key events sent through the embedder API. modifier.embedder_api().SendKeyEvent = [](FLUTTER_API_SYMBOL(FlutterEngine) engine, const FlutterKeyEvent* event, FlutterKeyEventCallback callback, void* user_data) { - bool result = stored_embedder_handler(event); - if (callback != nullptr) { - callback(result, user_data); - } + stored_response_controller->HandleEmbedderMessage( + event, [callback, user_data](bool handled) { + if (callback != nullptr) { + callback(handled, user_data); + } + }); return kSuccess; }; diff --git a/shell/platform/windows/testing/test_keyboard.h b/shell/platform/windows/testing/test_keyboard.h index 39d13c1dcd35e..41aff364b527f 100644 --- a/shell/platform/windows/testing/test_keyboard.h +++ b/shell/platform/windows/testing/test_keyboard.h @@ -42,13 +42,47 @@ LPARAM CreateKeyEventLparam(USHORT scancode, bool context_code = 0, bool transition_state = 1); -typedef std::function MockKeyEventChannelHandler; -typedef std::function - MockKeyEventEmbedderHandler; +class MockKeyResponseController { + public: + using ResponseCallback = std::function; + using EmbedderCallbackHandler = std::function; + using ChannelCallbackHandler = std::function; + + MockKeyResponseController() + : channel_response_(ChannelRespondFalse), + embedder_response_(EmbedderRespondFalse) {} + + void SetChannelResponse(ChannelCallbackHandler handler) { + channel_response_ = std::move(handler); + } + + void SetEmbedderResponse(EmbedderCallbackHandler handler) { + embedder_response_ = std::move(handler); + } + + void HandleChannelMessage(ResponseCallback callback) { + channel_response_(callback); + } + + void HandleEmbedderMessage(const FlutterKeyEvent* event, ResponseCallback callback) { + embedder_response_(event, std::move(callback)); + } + + private: + EmbedderCallbackHandler embedder_response_; + ChannelCallbackHandler channel_response_; + + static void ChannelRespondFalse(ResponseCallback callback) { + callback(false); + } + + static void EmbedderRespondFalse(const FlutterKeyEvent* event, ResponseCallback callback) { + callback(false); + } +}; void MockEmbedderApiForKeyboard(EngineModifier& modifier, - MockKeyEventChannelHandler channel_handler, - MockKeyEventEmbedderHandler embedder_handler); + std::shared_ptr response_controller); // Simulate a message queue for WM messages. // From ecd545e0f6466b703de0e40c60b93645db64bb8e Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 30 Dec 2021 18:45:38 -0800 Subject: [PATCH 02/10] Implement LateResponding and move GetEngine in (ATP) --- .../windows/keyboard_win32_unittests.cc | 98 +++++++++++-------- 1 file changed, 55 insertions(+), 43 deletions(-) diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc index c7e0cb1264732..02eea3c3129eb 100644 --- a/shell/platform/windows/keyboard_win32_unittests.cc +++ b/shell/platform/windows/keyboard_win32_unittests.cc @@ -238,13 +238,16 @@ void clear_key_calls() { key_calls.clear(); } -std::unique_ptr GetTestEngine(); - class KeyboardTester { public: - using KeyEventHandler = std::function; + using ResponseHandler = std::function; explicit KeyboardTester() { + callback_handler_ = + [](const FlutterKeyEvent* event, + MockKeyResponseController::ResponseCallback callback) { + callback(false); + }; view_ = std::make_unique( [](const std::u16string& text) { key_calls.push_back(KeyCall{ @@ -252,7 +255,20 @@ class KeyboardTester { .text = text, }); }); - view_->SetEngine(std::move(GetTestEngine())); + view_->SetEngine(GetTestEngine( + [&callback_handler = callback_handler_]( + const FlutterKeyEvent* event, + MockKeyResponseController::ResponseCallback callback) { + FlutterKeyEvent clone_event = *event; + clone_event.character = event->character == nullptr + ? nullptr + : clone_string(event->character); + key_calls.push_back(KeyCall{ + .type = kKeyCallOnKey, + .key_event = clone_event, + }); + callback_handler(event, callback); + })); window_ = std::make_unique(view_.get()); } @@ -260,9 +276,17 @@ class KeyboardTester { view_->SetKeyState(key, pressed, toggled_on); } - void Responding(bool response) { test_response = response; } + void Responding(bool response) { + callback_handler_ = + [response](const FlutterKeyEvent* event, + MockKeyResponseController::ResponseCallback callback) { + callback(response); + }; + } - void LateRespond(bool response) { test_response = response; } + void LateResponding(MockKeyResponseController::EmbedderCallbackHandler handler) { + callback_handler_ = std::move(handler); + } void SetLayout(MapVkToCharHandler layout) { window_->SetLayout(layout); } @@ -289,48 +313,36 @@ class KeyboardTester { return view_->InjectPendingEvents(window_.get(), redispatch_char); } - static bool test_response; - private: std::unique_ptr view_; std::unique_ptr window_; + MockKeyResponseController::EmbedderCallbackHandler callback_handler_; + + // Returns an engine instance configured with dummy project path values, and + // overridden methods for sending platform messages, so that the engine can + // respond as if the framework were connected. + static std::unique_ptr GetTestEngine( + MockKeyResponseController::EmbedderCallbackHandler + embedder_callback_handler) { + FlutterDesktopEngineProperties properties = {}; + properties.assets_path = L"C:\\foo\\flutter_assets"; + properties.icu_data_path = L"C:\\foo\\icudtl.dat"; + properties.aot_library_path = L"C:\\foo\\aot.so"; + FlutterProjectBundle project(properties); + auto engine = std::make_unique(project); + + EngineModifier modifier(engine.get()); + + auto key_response_controller = std::make_shared(); + key_response_controller->SetEmbedderResponse(std::move(embedder_callback_handler)); + + MockEmbedderApiForKeyboard(modifier, key_response_controller); + + engine->RunWithEntrypoint(nullptr); + return engine; + } }; -bool KeyboardTester::test_response = false; - -// Returns an engine instance configured with dummy project path values, and -// overridden methods for sending platform messages, so that the engine can -// respond as if the framework were connected. -std::unique_ptr GetTestEngine() { - FlutterDesktopEngineProperties properties = {}; - properties.assets_path = L"C:\\foo\\flutter_assets"; - properties.icu_data_path = L"C:\\foo\\icudtl.dat"; - properties.aot_library_path = L"C:\\foo\\aot.so"; - FlutterProjectBundle project(properties); - auto engine = std::make_unique(project); - - EngineModifier modifier(engine.get()); - - auto key_response_controller = std::make_shared(); - key_response_controller->SetEmbedderResponse( - [](const FlutterKeyEvent* event, - MockKeyResponseController::ResponseCallback callback) { - FlutterKeyEvent clone_event = *event; - clone_event.character = event->character == nullptr - ? nullptr - : clone_string(event->character); - key_calls.push_back(KeyCall{ - .type = kKeyCallOnKey, - .key_event = clone_event, - }); - callback(KeyboardTester::test_response); - }); - - MockEmbedderApiForKeyboard(modifier, key_response_controller); - - engine->RunWithEntrypoint(nullptr); - return engine; -} constexpr uint64_t kScanCodeKeyA = 0x1e; constexpr uint64_t kScanCodeKeyE = 0x12; From 9bb871243cfe9a6c5585a13d7e13cd495382ca47 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 30 Dec 2021 18:48:53 -0800 Subject: [PATCH 03/10] RespondValue --- .../windows/keyboard_win32_unittests.cc | 22 ++++++++----------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc index 02eea3c3129eb..8494a7ff3d481 100644 --- a/shell/platform/windows/keyboard_win32_unittests.cc +++ b/shell/platform/windows/keyboard_win32_unittests.cc @@ -242,12 +242,7 @@ class KeyboardTester { public: using ResponseHandler = std::function; - explicit KeyboardTester() { - callback_handler_ = - [](const FlutterKeyEvent* event, - MockKeyResponseController::ResponseCallback callback) { - callback(false); - }; + explicit KeyboardTester() : callback_handler_(RespondValue(false)) { view_ = std::make_unique( [](const std::u16string& text) { key_calls.push_back(KeyCall{ @@ -276,13 +271,7 @@ class KeyboardTester { view_->SetKeyState(key, pressed, toggled_on); } - void Responding(bool response) { - callback_handler_ = - [response](const FlutterKeyEvent* event, - MockKeyResponseController::ResponseCallback callback) { - callback(response); - }; - } + void Responding(bool response) { callback_handler_ = RespondValue(response); } void LateResponding(MockKeyResponseController::EmbedderCallbackHandler handler) { callback_handler_ = std::move(handler); @@ -341,6 +330,13 @@ class KeyboardTester { engine->RunWithEntrypoint(nullptr); return engine; } + + static MockKeyResponseController::EmbedderCallbackHandler RespondValue(bool value) { + return [value](const FlutterKeyEvent* event, + MockKeyResponseController::ResponseCallback callback) { + callback(value); + }; + } }; From 8f96b43f7990d6b5db66b461d87bc73ff2615f21 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 30 Dec 2021 19:31:18 -0800 Subject: [PATCH 04/10] DisorderlyRepliedEvents --- .../windows/keyboard_win32_unittests.cc | 52 +++++++++++++++++++ .../platform/windows/testing/test_keyboard.h | 2 +- 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc index 8494a7ff3d481..89c807120e381 100644 --- a/shell/platform/windows/keyboard_win32_unittests.cc +++ b/shell/platform/windows/keyboard_win32_unittests.cc @@ -341,6 +341,7 @@ class KeyboardTester { constexpr uint64_t kScanCodeKeyA = 0x1e; +constexpr uint64_t kScanCodeKeyB = 0x30; constexpr uint64_t kScanCodeKeyE = 0x12; constexpr uint64_t kScanCodeKeyQ = 0x10; constexpr uint64_t kScanCodeKeyW = 0x11; @@ -356,6 +357,7 @@ constexpr uint64_t kScanCodeBracketLeft = 0x1a; constexpr uint64_t kVirtualDigit1 = 0x31; constexpr uint64_t kVirtualKeyA = 0x41; +constexpr uint64_t kVirtualKeyB = 0x42; constexpr uint64_t kVirtualKeyE = 0x45; constexpr uint64_t kVirtualKeyQ = 0x51; constexpr uint64_t kVirtualKeyW = 0x57; @@ -1092,5 +1094,55 @@ TEST(KeyboardTest, MultibyteCharacter) { clear_key_calls(); } +TEST(KeyboardTest, DisorderlyRepliedEvents) { + KeyboardTester tester; + + std::vector recorded_callbacks; + + // Store callbacks to manually call them. + tester.LateResponding( + [&recorded_callbacks]( + const FlutterKeyEvent* event, + MockKeyResponseController::ResponseCallback callback) { + recorded_callbacks.push_back(callback); + }); + + // Press A + tester.InjectMessages( + 2, + WmKeyDownInfo{kVirtualKeyA, kScanCodeKeyA, kNotExtended, kWasUp}.Build( + kWmResultZero), + WmCharInfo{'a', kScanCodeKeyA, kNotExtended, kWasUp}.Build( + kWmResultZero)); + + // Press B + tester.InjectMessages( + 2, + WmKeyDownInfo{kVirtualKeyB, kScanCodeKeyB, kNotExtended, kWasUp}.Build( + kWmResultZero), + WmCharInfo{'b', kScanCodeKeyB, kNotExtended, kWasUp}.Build( + kWmResultZero)); + + EXPECT_EQ(key_calls.size(), 2); + EXPECT_EQ(recorded_callbacks.size(), 2); + clear_key_calls(); + + // Resolve the second event first to test disordered responses. + recorded_callbacks.back()(false); + + tester.InjectPendingEvents('b'); + EXPECT_EQ(key_calls.size(), 1); + EXPECT_CALL_IS_TEXT(key_calls[0], u"b"); + clear_key_calls(); + + // Resolve the first event. + recorded_callbacks.front()(false); + + tester.InjectPendingEvents('a'); + EXPECT_EQ(key_calls.size(), 1); + EXPECT_CALL_IS_TEXT(key_calls[0], u"a"); + clear_key_calls(); +} + } // namespace testing } // namespace flutter diff --git a/shell/platform/windows/testing/test_keyboard.h b/shell/platform/windows/testing/test_keyboard.h index 41aff364b527f..1f2aca593d9fc 100644 --- a/shell/platform/windows/testing/test_keyboard.h +++ b/shell/platform/windows/testing/test_keyboard.h @@ -45,7 +45,7 @@ LPARAM CreateKeyEventLparam(USHORT scancode, class MockKeyResponseController { public: using ResponseCallback = std::function; - using EmbedderCallbackHandler = std::function; + using EmbedderCallbackHandler = std::function; using ChannelCallbackHandler = std::function; MockKeyResponseController() From f5c2286177542999d62f4a227614ab0c6db1bb27 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 30 Dec 2021 19:50:11 -0800 Subject: [PATCH 05/10] SlowFrameworkResponse --- .../windows/keyboard_win32_unittests.cc | 58 ++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc index 89c807120e381..fbb41d4b9b144 100644 --- a/shell/platform/windows/keyboard_win32_unittests.cc +++ b/shell/platform/windows/keyboard_win32_unittests.cc @@ -1094,7 +1094,7 @@ TEST(KeyboardTest, MultibyteCharacter) { clear_key_calls(); } -TEST(KeyboardTest, DisorderlyRepliedEvents) { +TEST(KeyboardTest, DisorderlyRespondedEvents) { KeyboardTester tester; std::vector recorded_callbacks; @@ -1144,5 +1144,61 @@ TEST(KeyboardTest, DisorderlyRepliedEvents) { clear_key_calls(); } +// Regression test for a crash in an earlier implementation. +// +// In real life, the framework responds slowly. The next real event might +// arrive earlier than the framework response, and if the 2nd event has an +// identical hash as the one waiting for response, an earlier implementation +// will crash upon the response. +TEST(KeyboardTest, SlowFrameworkResponse) { + KeyboardTester tester; + + std::vector recorded_callbacks; + + // Store callbacks to manually call them. + tester.LateResponding( + [&recorded_callbacks]( + const FlutterKeyEvent* event, + MockKeyResponseController::ResponseCallback callback) { + recorded_callbacks.push_back(callback); + }); + + // Press A + tester.InjectMessages( + 2, + WmKeyDownInfo{kVirtualKeyA, kScanCodeKeyA, kNotExtended, kWasUp}.Build( + kWmResultZero), + WmCharInfo{'a', kScanCodeKeyA, kNotExtended, kWasUp}.Build( + kWmResultZero)); + + // Hold A + tester.InjectMessages( + 2, + WmKeyDownInfo{kVirtualKeyA, kScanCodeKeyA, kNotExtended, kWasDown}.Build( + kWmResultZero), + WmCharInfo{'a', kScanCodeKeyA, kNotExtended, kWasDown}.Build( + kWmResultZero)); + + EXPECT_EQ(key_calls.size(), 2); + EXPECT_EQ(recorded_callbacks.size(), 2); + clear_key_calls(); + + // The first response. + recorded_callbacks.front()(false); + + tester.InjectPendingEvents('a'); + EXPECT_EQ(key_calls.size(), 1); + EXPECT_CALL_IS_TEXT(key_calls[0], u"a"); + clear_key_calls(); + + // The second response. + recorded_callbacks.back()(false); + + tester.InjectPendingEvents('a'); + EXPECT_EQ(key_calls.size(), 1); + EXPECT_CALL_IS_TEXT(key_calls[0], u"a"); + clear_key_calls(); +} + } // namespace testing } // namespace flutter From 1275615601f944a35e14c6a61db60424843cecf8 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 30 Dec 2021 19:54:52 -0800 Subject: [PATCH 06/10] NeverRedispatchShiftRightKeyDown --- .../windows/keyboard_win32_unittests.cc | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc index fbb41d4b9b144..ea03eb46af678 100644 --- a/shell/platform/windows/keyboard_win32_unittests.cc +++ b/shell/platform/windows/keyboard_win32_unittests.cc @@ -352,7 +352,7 @@ constexpr uint64_t kScanCodeDigit6 = 0x07; constexpr uint64_t kScanCodeControl = 0x1d; constexpr uint64_t kScanCodeAlt = 0x38; constexpr uint64_t kScanCodeShiftLeft = 0x2a; -// constexpr uint64_t kScanCodeShiftRight = 0x36; +constexpr uint64_t kScanCodeShiftRight = 0x36; constexpr uint64_t kScanCodeBracketLeft = 0x1a; constexpr uint64_t kVirtualDigit1 = 0x31; @@ -1094,6 +1094,30 @@ TEST(KeyboardTest, MultibyteCharacter) { clear_key_calls(); } +// A key down event for shift right must not be redispatched even if +// the framework returns unhandled. +// +// The reason for this test is documented in |IsKeyDownShiftRight|. +TEST(KeyboardTest, NeverRedispatchShiftRightKeyDown) { + KeyboardTester tester; + tester.Responding(false); + + // Press ShiftRight and the delegate responds false. + tester.SetKeyState(VK_RSHIFT, true, true); + tester.InjectMessages( + 1, + WmKeyDownInfo{VK_SHIFT, kScanCodeShiftRight, kNotExtended, kWasUp}.Build( + kWmResultZero)); + + EXPECT_EQ(key_calls.size(), 1); + clear_key_calls(); + + // Try to dispatch events. There should be nothing. + tester.InjectPendingEvents(); + EXPECT_EQ(key_calls.size(), 0); +} + + TEST(KeyboardTest, DisorderlyRespondedEvents) { KeyboardTester tester; From d6ff9d9756f96e987962d8309bfe2253a250d865 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Wed, 5 Jan 2022 03:37:28 -0800 Subject: [PATCH 07/10] Migrate AltGr event. (ATP) --- .../windows/flutter_window_win32_unittests.cc | 7 +- .../windows/keyboard_key_embedder_handler.cc | 4 +- .../platform/windows/keyboard_key_handler.cc | 13 +- .../windows/keyboard_win32_unittests.cc | 183 +++++++++++++++--- 4 files changed, 169 insertions(+), 38 deletions(-) diff --git a/shell/platform/windows/flutter_window_win32_unittests.cc b/shell/platform/windows/flutter_window_win32_unittests.cc index d7210103e022c..42378fb6c3286 100644 --- a/shell/platform/windows/flutter_window_win32_unittests.cc +++ b/shell/platform/windows/flutter_window_win32_unittests.cc @@ -269,8 +269,7 @@ class TestFlutterWindowsView : public FlutterWindowsView { // "SendInput" directly to the window. const KEYBDINPUT kbdinput = pInputs->ki; const bool is_key_up = kbdinput.dwFlags & KEYEVENTF_KEYUP; - const UINT message = is_key_up ? (is_syskey_ ? WM_SYSKEYUP : WM_KEYUP) - : (is_syskey_ ? WM_SYSKEYDOWN : WM_KEYDOWN); + const UINT message = is_key_up ? WM_KEYUP : WM_KEYDOWN; const LPARAM lparam = CreateKeyEventLparam( kbdinput.wScan, kbdinput.dwFlags & KEYEVENTF_EXTENDEDKEY, is_key_up); @@ -410,9 +409,11 @@ TEST(FlutterWindowWin32Test, SystemKeyDownPropagation) { false /* extended */, _)) .Times(2) .RetiresOnSaturation(); + // Syskey events are not redispatched, so TextInputPlugin, which relies on + // them to receive events, no longer works. EXPECT_CALL(*flutter_windows_view.text_input_plugin, KeyboardHook(_, _, _, _, _, _)) - .Times(1) + .Times(0) .RetiresOnSaturation(); EXPECT_CALL(*flutter_windows_view.key_event_handler, TextHook(_)).Times(0); EXPECT_CALL(*flutter_windows_view.text_input_plugin, TextHook(_)).Times(0); diff --git a/shell/platform/windows/keyboard_key_embedder_handler.cc b/shell/platform/windows/keyboard_key_embedder_handler.cc index d28ee0a38bdf5..e9617464ccb6a 100644 --- a/shell/platform/windows/keyboard_key_embedder_handler.cc +++ b/shell/platform/windows/keyboard_key_embedder_handler.cc @@ -243,7 +243,9 @@ void KeyboardKeyEmbedderHandler::KeyboardHookImpl( if (eventual_logical_record != 0) { pressingRecords_[physical_key] = eventual_logical_record; } else { - pressingRecords_.erase(last_logical_record_iter); + auto record_iter = pressingRecords_.find(physical_key); + assert(record_iter != pressingRecords_.end()); + pressingRecords_.erase(record_iter); } if (result_logical_key == VK_PROCESSKEY) { diff --git a/shell/platform/windows/keyboard_key_handler.cc b/shell/platform/windows/keyboard_key_handler.cc index bb47b21736bb5..72b330b34aab8 100644 --- a/shell/platform/windows/keyboard_key_handler.cc +++ b/shell/platform/windows/keyboard_key_handler.cc @@ -129,9 +129,9 @@ void KeyboardKeyHandler::DispatchEvent(const PendingEvent& event) { #else char32_t character = event.character; - if (event.action == WM_SYSKEYDOWN || event.action == WM_SYSKEYUP) { - return; - } + // Sys key events can't be synthesized. Their dispatching should be prevented + // in earlier code. + assert(event.action != WM_SYSKEYDOWN && event.action != WM_SYSKEYUP); INPUT input_event{ .type = INPUT_KEYBOARD, @@ -277,8 +277,11 @@ void KeyboardKeyHandler::ResolvePendingEvent(uint64_t sequence_id, // Redispatching dead keys events makes Win32 ignore the dead key state // and redispatches a normal character without combining it with the // next letter key. - const bool should_redispatch = - !event_ptr->any_handled && !_IsDeadKey(event_ptr->character); + const bool is_syskey = + event.action == WM_SYSKEYDOWN || event.action == WM_SYSKEYUP; + const bool should_redispatch = !event_ptr->any_handled && + !_IsDeadKey(event_ptr->character) && + !is_syskey; if (should_redispatch) { RedispatchEvent(std::move(event_ptr)); } diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc index ea03eb46af678..00ea76e20db7f 100644 --- a/shell/platform/windows/keyboard_win32_unittests.cc +++ b/shell/platform/windows/keyboard_win32_unittests.cc @@ -150,18 +150,14 @@ class TestFlutterWindowsView : public FlutterWindowsView { // affect keyboard. : FlutterWindowsView( std::make_unique<::testing::NiceMock>()), - on_text_(std::move(on_text)), - redispatch_char(0) {} - - uint32_t redispatch_char; + on_text_(std::move(on_text)) {} void OnText(const std::u16string& text) override { on_text_(text); } int InjectPendingEvents(MockMessageQueue* queue, uint32_t redispatch_char) { std::vector messages; int num_pending_responds = pending_responds_.size(); - for (const SendInputInfo& input : pending_responds_) { - const KEYBDINPUT kbdinput = input.kbdinput; + for (const KEYBDINPUT& kbdinput : pending_responds_) { const UINT message = (kbdinput.dwFlags & KEYEVENTF_KEYUP) ? WM_KEYUP : WM_KEYDOWN; const bool is_key_up = kbdinput.dwFlags & KEYEVENTF_KEYUP; @@ -180,8 +176,8 @@ class TestFlutterWindowsView : public FlutterWindowsView { } } - queue->InjectMessageList(messages.size(), messages.data()); pending_responds_.clear(); + queue->InjectMessageList(messages.size(), messages.data()); return num_pending_responds; } @@ -204,12 +200,14 @@ class TestFlutterWindowsView : public FlutterWindowsView { private: UINT SendInput(UINT cInputs, LPINPUT pInputs, int cbSize) { - pending_responds_.push_back({cInputs, pInputs->ki, cbSize}); + for (UINT input_idx = 0; input_idx < cInputs; input_idx += 1) { + pending_responds_.push_back(pInputs[input_idx].ki); + } return 1; } U16StringHandler on_text_; - std::vector pending_responds_; + std::vector pending_responds_; TestKeystate key_state_; }; @@ -240,7 +238,8 @@ void clear_key_calls() { class KeyboardTester { public: - using ResponseHandler = std::function; + using ResponseHandler = + std::function; explicit KeyboardTester() : callback_handler_(RespondValue(false)) { view_ = std::make_unique( @@ -273,7 +272,14 @@ class KeyboardTester { void Responding(bool response) { callback_handler_ = RespondValue(response); } - void LateResponding(MockKeyResponseController::EmbedderCallbackHandler handler) { + // Manually handle event callback of the onKeyData embedder API. + // + // On every onKeyData call, the |handler| will be invoked with the target + // key data and the result callback. Immediately calling the callback with + // a boolean is equivalent to setting |Responding| with the boolean. However, + // |LateResponding| allows storing the callback to call later. + void LateResponding( + MockKeyResponseController::EmbedderCallbackHandler handler) { callback_handler_ = std::move(handler); } @@ -322,8 +328,10 @@ class KeyboardTester { EngineModifier modifier(engine.get()); - auto key_response_controller = std::make_shared(); - key_response_controller->SetEmbedderResponse(std::move(embedder_callback_handler)); + auto key_response_controller = + std::make_shared(); + key_response_controller->SetEmbedderResponse( + std::move(embedder_callback_handler)); MockEmbedderApiForKeyboard(modifier, key_response_controller); @@ -331,9 +339,10 @@ class KeyboardTester { return engine; } - static MockKeyResponseController::EmbedderCallbackHandler RespondValue(bool value) { + static MockKeyResponseController::EmbedderCallbackHandler RespondValue( + bool value) { return [value](const FlutterKeyEvent* event, - MockKeyResponseController::ResponseCallback callback) { + MockKeyResponseController::ResponseCallback callback) { callback(value); }; } @@ -729,7 +738,7 @@ TEST(KeyboardTest, AltGrModifiedKey) { kNotSynthesized); clear_key_calls(); - tester.InjectPendingEvents(); + EXPECT_EQ(tester.InjectPendingEvents(), 2); EXPECT_EQ(key_calls.size(), 0); clear_key_calls(); @@ -746,7 +755,7 @@ TEST(KeyboardTest, AltGrModifiedKey) { kLogicalKeyQ, "@", kNotSynthesized); clear_key_calls(); - tester.InjectPendingEvents('@'); + EXPECT_EQ(tester.InjectPendingEvents('@'), 1); EXPECT_EQ(key_calls.size(), 1); EXPECT_CALL_IS_TEXT(key_calls[0], u"@"); clear_key_calls(); @@ -761,12 +770,12 @@ TEST(KeyboardTest, AltGrModifiedKey) { kLogicalKeyQ, "", kNotSynthesized); clear_key_calls(); - tester.InjectPendingEvents(); + EXPECT_EQ(tester.InjectPendingEvents(), 1); EXPECT_EQ(key_calls.size(), 0); // Release AltGr. Win32 doesn't dispatch ControlLeft up. Instead Flutter will - // dispatch one. The AltGr is a system key, so will be handled by Win32's - // default WndProc. + // dispatch one. The AltGr is a system key, therefore will be handled by + // Win32's default WndProc. tester.InjectMessages( 1, WmSysKeyUpInfo{VK_MENU, kScanCodeAlt, kExtended}.Build(kWmResultDefault)); @@ -776,13 +785,131 @@ TEST(KeyboardTest, AltGrModifiedKey) { kLogicalAltRight, "", kNotSynthesized); clear_key_calls(); + // Dispatch the ControlLeft up event appended by Flutter. + tester.SetKeyState(VK_LCONTROL, false, true); + EXPECT_EQ(tester.InjectPendingEvents(), 1); + EXPECT_EQ(key_calls.size(), 1); + EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeUp, + kPhysicalControlLeft, kLogicalControlLeft, "", + kNotSynthesized); + clear_key_calls(); +} + +// Test the following two key sequences at the same time: +// +// 1. Tap AltGr, then tap AltGr. +// 2. Tap AltGr, hold CtrlLeft, tap AltGr, release CtrlLeft. +// +// The two sequences are indistinguishable until the very end when a CtrlLeft +// up event might or might not follow. +// +// Sequence 1: CtrlLeft down, AltRight down, AltRight up +// Sequence 2: CtrlLeft down, AltRight down, AltRight up, CtrlLeft up +// +// This is because pressing AltGr alone causes Win32 to send a fake "CtrlLeft +// down" event first (see |IsKeyDownAltRight| for detailed explanation). +TEST(KeyboardTest, AltGrTwice) { + KeyboardTester tester; + tester.Responding(false); + + // 1. AltGr down. + + // The key down event causes a ControlLeft down and a AltRight (extended + // AltLeft) down. + tester.SetKeyState(VK_LCONTROL, true, true); + tester.InjectMessages( + 2, + WmKeyDownInfo{VK_LCONTROL, kScanCodeControl, kNotExtended, kWasUp}.Build( + kWmResultZero), + WmKeyDownInfo{VK_MENU, kScanCodeAlt, kExtended, kWasUp}.Build( + kWmResultZero)); + + EXPECT_EQ(key_calls.size(), 2); + EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeDown, + kPhysicalControlLeft, kLogicalControlLeft, "", + kNotSynthesized); + EXPECT_CALL_IS_EVENT(key_calls[1], kFlutterKeyEventTypeDown, + kPhysicalAltRight, kLogicalAltRight, "", + kNotSynthesized); + clear_key_calls(); + + EXPECT_EQ(tester.InjectPendingEvents(), 2); + EXPECT_EQ(key_calls.size(), 0); + + // 2. AltGr up. + + // The key up event only causes a AltRight (extended AltLeft) up. + tester.SetKeyState(VK_RMENU, false, true); + tester.InjectMessages( + 1, WmSysKeyUpInfo{VK_MENU, kScanCodeAlt, kExtended}.Build(kWmResultDefault)); + EXPECT_EQ(key_calls.size(), 1); + EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeUp, + kPhysicalAltRight, kLogicalAltRight, "", + kNotSynthesized); + clear_key_calls(); + + // Dispatch the ControlLeft up event appended by Flutter. + tester.SetKeyState(VK_LCONTROL, false, true); + EXPECT_EQ(tester.InjectPendingEvents(), 1); + EXPECT_EQ(key_calls.size(), 1); + EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeUp, + kPhysicalControlLeft, kLogicalControlLeft, "", + kNotSynthesized); + clear_key_calls(); + + // Redispatch the ControlLeft up event. + EXPECT_EQ(tester.InjectPendingEvents(), 1); + EXPECT_EQ(key_calls.size(), 0); + clear_key_calls(); + + // 3. AltGr down (or: ControlLeft down then AltRight down.) + + tester.SetKeyState(VK_LCONTROL, true, false); + tester.InjectMessages( + 2, + WmKeyDownInfo{VK_LCONTROL, kScanCodeControl, kNotExtended, kWasUp}.Build( + kWmResultZero), + WmKeyDownInfo{VK_MENU, kScanCodeAlt, kExtended, kWasUp}.Build( + kWmResultZero)); + + EXPECT_EQ(key_calls.size(), 2); + EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeDown, + kPhysicalControlLeft, kLogicalControlLeft, "", + kNotSynthesized); + EXPECT_CALL_IS_EVENT(key_calls[1], kFlutterKeyEventTypeDown, + kPhysicalAltRight, kLogicalAltRight, "", + kNotSynthesized); + clear_key_calls(); + + EXPECT_EQ(tester.InjectPendingEvents(), 2); + EXPECT_EQ(key_calls.size(), 0); + + // 4. AltGr up. + + // The key up event only causes a AltRight (extended AltLeft) up. + tester.SetKeyState(VK_RMENU, false, false); + tester.InjectMessages( + 1, WmSysKeyUpInfo{VK_MENU, kScanCodeAlt, kExtended}.Build(kWmResultDefault)); + EXPECT_EQ(key_calls.size(), 1); + EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeUp, + kPhysicalAltRight, kLogicalAltRight, "", + kNotSynthesized); + clear_key_calls(); + + // Dispatch a ControlLeft up event from Flutter. tester.SetKeyState(VK_LCONTROL, false, false); - tester.InjectPendingEvents(); + EXPECT_EQ(tester.InjectPendingEvents(), 1); EXPECT_EQ(key_calls.size(), 1); EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeUp, kPhysicalControlLeft, kLogicalControlLeft, "", kNotSynthesized); clear_key_calls(); + + // 5. For key sequence 2: a real ControlLeft up. + tester.InjectMessages( + 1, WmKeyUpInfo{VK_LCONTROL, kScanCodeControl, kNotExtended}.Build(kWmResultDefault)); + EXPECT_EQ(key_calls.size(), 0); + clear_key_calls(); } // This tests dead key ^ then E on a French keyboard, which should be combined @@ -1113,17 +1240,15 @@ TEST(KeyboardTest, NeverRedispatchShiftRightKeyDown) { clear_key_calls(); // Try to dispatch events. There should be nothing. - tester.InjectPendingEvents(); + EXPECT_EQ(tester.InjectPendingEvents(), 0); EXPECT_EQ(key_calls.size(), 0); } - TEST(KeyboardTest, DisorderlyRespondedEvents) { KeyboardTester tester; - std::vector recorded_callbacks; - // Store callbacks to manually call them. + std::vector recorded_callbacks; tester.LateResponding( [&recorded_callbacks]( const FlutterKeyEvent* event, @@ -1154,7 +1279,7 @@ TEST(KeyboardTest, DisorderlyRespondedEvents) { // Resolve the second event first to test disordered responses. recorded_callbacks.back()(false); - tester.InjectPendingEvents('b'); + EXPECT_EQ(tester.InjectPendingEvents('b'), 1); EXPECT_EQ(key_calls.size(), 1); EXPECT_CALL_IS_TEXT(key_calls[0], u"b"); clear_key_calls(); @@ -1162,7 +1287,7 @@ TEST(KeyboardTest, DisorderlyRespondedEvents) { // Resolve the first event. recorded_callbacks.front()(false); - tester.InjectPendingEvents('a'); + EXPECT_EQ(tester.InjectPendingEvents('a'), 1); EXPECT_EQ(key_calls.size(), 1); EXPECT_CALL_IS_TEXT(key_calls[0], u"a"); clear_key_calls(); @@ -1210,7 +1335,7 @@ TEST(KeyboardTest, SlowFrameworkResponse) { // The first response. recorded_callbacks.front()(false); - tester.InjectPendingEvents('a'); + EXPECT_EQ(tester.InjectPendingEvents('a'), 1); EXPECT_EQ(key_calls.size(), 1); EXPECT_CALL_IS_TEXT(key_calls[0], u"a"); clear_key_calls(); @@ -1218,7 +1343,7 @@ TEST(KeyboardTest, SlowFrameworkResponse) { // The second response. recorded_callbacks.back()(false); - tester.InjectPendingEvents('a'); + EXPECT_EQ(tester.InjectPendingEvents('a'), 1); EXPECT_EQ(key_calls.size(), 1); EXPECT_CALL_IS_TEXT(key_calls[0], u"a"); clear_key_calls(); From 38babdb147a53a472faeec458225f8e9c6f6e9a5 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Wed, 5 Jan 2022 04:00:43 -0800 Subject: [PATCH 08/10] Correctly handle sys messages. --- .../platform/windows/flutter_window_win32_unittests.cc | 10 +++------- shell/platform/windows/keyboard_key_handler.cc | 6 ++++-- shell/platform/windows/keyboard_manager_win32.cc | 5 +---- 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/shell/platform/windows/flutter_window_win32_unittests.cc b/shell/platform/windows/flutter_window_win32_unittests.cc index 42378fb6c3286..d6f44d905727e 100644 --- a/shell/platform/windows/flutter_window_win32_unittests.cc +++ b/shell/platform/windows/flutter_window_win32_unittests.cc @@ -229,12 +229,10 @@ class TestFlutterWindowsView : public FlutterWindowsView { public: TestFlutterWindowsView(std::unique_ptr window_binding, WPARAM virtual_key, - bool is_printable = true, - bool is_syskey = false) + bool is_printable = true) : FlutterWindowsView(std::move(window_binding)), virtual_key_(virtual_key), - is_printable_(is_printable), - is_syskey_(is_syskey) {} + is_printable_(is_printable) {} SpyKeyboardKeyHandler* key_event_handler; SpyTextInputPlugin* text_input_plugin; @@ -294,7 +292,6 @@ class TestFlutterWindowsView : public FlutterWindowsView { std::vector pending_responds_; WPARAM virtual_key_; bool is_printable_; - bool is_syskey_; }; // The static value to return as the "handled" value from the framework for key @@ -395,8 +392,7 @@ TEST(FlutterWindowWin32Test, SystemKeyDownPropagation) { auto window_binding_handler = std::make_unique<::testing::NiceMock>(); TestFlutterWindowsView flutter_windows_view( - std::move(window_binding_handler), virtual_key, false /* is_printable */, - true /* is_syskey */); + std::move(window_binding_handler), virtual_key, false /* is_printable */); win32window.SetView(&flutter_windows_view); LPARAM lparam = CreateKeyEventLparam(scan_code, false, false); diff --git a/shell/platform/windows/keyboard_key_handler.cc b/shell/platform/windows/keyboard_key_handler.cc index 72b330b34aab8..cf4653095c474 100644 --- a/shell/platform/windows/keyboard_key_handler.cc +++ b/shell/platform/windows/keyboard_key_handler.cc @@ -272,11 +272,13 @@ void KeyboardKeyHandler::ResolvePendingEvent(uint64_t sequence_id, if (event.unreplied == 0) { std::unique_ptr event_ptr = std::move(*iter); pending_responds_.erase(iter); - // Don't dispatch handled events or dead key events. + // Don't dispatch handled events, and also ignore dead key events and + // sys events. // // Redispatching dead keys events makes Win32 ignore the dead key state // and redispatches a normal character without combining it with the - // next letter key. + // next letter key. Redispatching sys events is impossible due to + // the limitation of |SendInput|. const bool is_syskey = event.action == WM_SYSKEYDOWN || event.action == WM_SYSKEYUP; const bool should_redispatch = !event_ptr->any_handled && diff --git a/shell/platform/windows/keyboard_manager_win32.cc b/shell/platform/windows/keyboard_manager_win32.cc index aadca7ec69754..4bb9e522bdb63 100644 --- a/shell/platform/windows/keyboard_manager_win32.cc +++ b/shell/platform/windows/keyboard_manager_win32.cc @@ -174,10 +174,7 @@ bool KeyboardManagerWin32::HandleMessage(UINT const message, keyCode = ResolveKeyCode(keyCode, extended, scancode); const bool was_down = lparam & 0x40000000; bool is_syskey = message == WM_SYSKEYDOWN || message == WM_SYSKEYUP; - const int action = is_keydown_message - ? (is_syskey ? WM_SYSKEYDOWN : WM_KEYDOWN) - : (is_syskey ? WM_SYSKEYUP : WM_KEYUP); - if (window_delegate_->OnKey(keyCode, scancode, action, 0, extended, + if (window_delegate_->OnKey(keyCode, scancode, message, 0, extended, was_down)) { // For system keys, always pass them to the default WndProc so that keys // like the ALT-TAB or Kanji switches are processed correctly. From 55a1642e0044d67b6e80dc3d77c0e189c92280a6 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Wed, 5 Jan 2022 04:04:06 -0800 Subject: [PATCH 09/10] Remove SendInputInfo --- shell/platform/windows/keyboard_win32_unittests.cc | 6 ------ 1 file changed, 6 deletions(-) diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc index 00ea76e20db7f..29137ea0f4f57 100644 --- a/shell/platform/windows/keyboard_win32_unittests.cc +++ b/shell/platform/windows/keyboard_win32_unittests.cc @@ -133,12 +133,6 @@ class TestKeystate { std::map state_; }; -typedef struct { - UINT cInputs; - KEYBDINPUT kbdinput; - int cbSize; -} SendInputInfo; - // A FlutterWindowsView that overrides the RegisterKeyboardHandlers function // to register the keyboard hook handlers that can be spied upon. class TestFlutterWindowsView : public FlutterWindowsView { From d0bb813699ba4158d33356199f1279a21635da6d Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Wed, 5 Jan 2022 15:28:35 -0800 Subject: [PATCH 10/10] Comment and format --- .../windows/flutter_window_win32_unittests.cc | 3 +-- .../platform/windows/keyboard_key_handler.cc | 6 ++--- .../windows/keyboard_win32_unittests.cc | 24 ++++++++--------- .../platform/windows/testing/test_keyboard.cc | 26 ++++++++++--------- .../platform/windows/testing/test_keyboard.h | 14 ++++++---- 5 files changed, 39 insertions(+), 34 deletions(-) diff --git a/shell/platform/windows/flutter_window_win32_unittests.cc b/shell/platform/windows/flutter_window_win32_unittests.cc index d6f44d905727e..ccfe35cdd45f5 100644 --- a/shell/platform/windows/flutter_window_win32_unittests.cc +++ b/shell/platform/windows/flutter_window_win32_unittests.cc @@ -311,8 +311,7 @@ std::unique_ptr GetTestEngine() { auto engine = std::make_unique(project); EngineModifier modifier(engine.get()); - auto key_response_controller = - std::make_shared(); + auto key_response_controller = std::make_shared(); key_response_controller->SetChannelResponse( [](MockKeyResponseController::ResponseCallback callback) { callback(test_response); diff --git a/shell/platform/windows/keyboard_key_handler.cc b/shell/platform/windows/keyboard_key_handler.cc index cf4653095c474..e43dfd182ac51 100644 --- a/shell/platform/windows/keyboard_key_handler.cc +++ b/shell/platform/windows/keyboard_key_handler.cc @@ -129,9 +129,9 @@ void KeyboardKeyHandler::DispatchEvent(const PendingEvent& event) { #else char32_t character = event.character; - // Sys key events can't be synthesized. Their dispatching should be prevented - // in earlier code. - assert(event.action != WM_SYSKEYDOWN && event.action != WM_SYSKEYUP); + assert(event.action != WM_SYSKEYDOWN && event.action != WM_SYSKEYUP && + "Unexpectedly dispatching a SYS event. SYS events can't be dispatched " + "and should have been prevented in earlier code."); INPUT input_event{ .type = INPUT_KEYBOARD, diff --git a/shell/platform/windows/keyboard_win32_unittests.cc b/shell/platform/windows/keyboard_win32_unittests.cc index 29137ea0f4f57..0ef5a1908071d 100644 --- a/shell/platform/windows/keyboard_win32_unittests.cc +++ b/shell/platform/windows/keyboard_win32_unittests.cc @@ -342,7 +342,6 @@ class KeyboardTester { } }; - constexpr uint64_t kScanCodeKeyA = 0x1e; constexpr uint64_t kScanCodeKeyB = 0x30; constexpr uint64_t kScanCodeKeyE = 0x12; @@ -816,7 +815,7 @@ TEST(KeyboardTest, AltGrTwice) { WmKeyDownInfo{VK_LCONTROL, kScanCodeControl, kNotExtended, kWasUp}.Build( kWmResultZero), WmKeyDownInfo{VK_MENU, kScanCodeAlt, kExtended, kWasUp}.Build( - kWmResultZero)); + kWmResultZero)); EXPECT_EQ(key_calls.size(), 2); EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeDown, @@ -835,11 +834,11 @@ TEST(KeyboardTest, AltGrTwice) { // The key up event only causes a AltRight (extended AltLeft) up. tester.SetKeyState(VK_RMENU, false, true); tester.InjectMessages( - 1, WmSysKeyUpInfo{VK_MENU, kScanCodeAlt, kExtended}.Build(kWmResultDefault)); + 1, + WmSysKeyUpInfo{VK_MENU, kScanCodeAlt, kExtended}.Build(kWmResultDefault)); EXPECT_EQ(key_calls.size(), 1); - EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeUp, - kPhysicalAltRight, kLogicalAltRight, "", - kNotSynthesized); + EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeUp, kPhysicalAltRight, + kLogicalAltRight, "", kNotSynthesized); clear_key_calls(); // Dispatch the ControlLeft up event appended by Flutter. @@ -864,7 +863,7 @@ TEST(KeyboardTest, AltGrTwice) { WmKeyDownInfo{VK_LCONTROL, kScanCodeControl, kNotExtended, kWasUp}.Build( kWmResultZero), WmKeyDownInfo{VK_MENU, kScanCodeAlt, kExtended, kWasUp}.Build( - kWmResultZero)); + kWmResultZero)); EXPECT_EQ(key_calls.size(), 2); EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeDown, @@ -883,11 +882,11 @@ TEST(KeyboardTest, AltGrTwice) { // The key up event only causes a AltRight (extended AltLeft) up. tester.SetKeyState(VK_RMENU, false, false); tester.InjectMessages( - 1, WmSysKeyUpInfo{VK_MENU, kScanCodeAlt, kExtended}.Build(kWmResultDefault)); + 1, + WmSysKeyUpInfo{VK_MENU, kScanCodeAlt, kExtended}.Build(kWmResultDefault)); EXPECT_EQ(key_calls.size(), 1); - EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeUp, - kPhysicalAltRight, kLogicalAltRight, "", - kNotSynthesized); + EXPECT_CALL_IS_EVENT(key_calls[0], kFlutterKeyEventTypeUp, kPhysicalAltRight, + kLogicalAltRight, "", kNotSynthesized); clear_key_calls(); // Dispatch a ControlLeft up event from Flutter. @@ -901,7 +900,8 @@ TEST(KeyboardTest, AltGrTwice) { // 5. For key sequence 2: a real ControlLeft up. tester.InjectMessages( - 1, WmKeyUpInfo{VK_LCONTROL, kScanCodeControl, kNotExtended}.Build(kWmResultDefault)); + 1, WmKeyUpInfo{VK_LCONTROL, kScanCodeControl, kNotExtended}.Build( + kWmResultDefault)); EXPECT_EQ(key_calls.size(), 0); clear_key_calls(); } diff --git a/shell/platform/windows/testing/test_keyboard.cc b/shell/platform/windows/testing/test_keyboard.cc index 9d1aea8e7013d..b93308d7cc92d 100644 --- a/shell/platform/windows/testing/test_keyboard.cc +++ b/shell/platform/windows/testing/test_keyboard.cc @@ -93,8 +93,9 @@ static std::shared_ptr stored_response_controller; // // The |channel_handler| and |embedder_handler| should return a boolean // indicating whether the framework decides to handle the event. -void MockEmbedderApiForKeyboard(EngineModifier& modifier, - std::shared_ptr response_controller) { +void MockEmbedderApiForKeyboard( + EngineModifier& modifier, + std::shared_ptr response_controller) { stored_response_controller = response_controller; // This mock handles channel messages. modifier.embedder_api().SendPlatformMessage = @@ -104,16 +105,17 @@ void MockEmbedderApiForKeyboard(EngineModifier& modifier, return kSuccess; } if (std::string(message->channel) == std::string("flutter/keyevent")) { - stored_response_controller->HandleChannelMessage([message](bool handled) { - auto response = _keyHandlingResponse(handled); - const TestResponseHandle* response_handle = - reinterpret_cast( - message->response_handle); - if (response_handle->callback != nullptr) { - response_handle->callback(response->data(), response->size(), - response_handle->user_data); - } - }); + stored_response_controller->HandleChannelMessage( + [message](bool handled) { + auto response = _keyHandlingResponse(handled); + const TestResponseHandle* response_handle = + reinterpret_cast( + message->response_handle); + if (response_handle->callback != nullptr) { + response_handle->callback(response->data(), response->size(), + response_handle->user_data); + } + }); return kSuccess; } return kSuccess; diff --git a/shell/platform/windows/testing/test_keyboard.h b/shell/platform/windows/testing/test_keyboard.h index 1f2aca593d9fc..1d0b8f02c5763 100644 --- a/shell/platform/windows/testing/test_keyboard.h +++ b/shell/platform/windows/testing/test_keyboard.h @@ -45,7 +45,8 @@ LPARAM CreateKeyEventLparam(USHORT scancode, class MockKeyResponseController { public: using ResponseCallback = std::function; - using EmbedderCallbackHandler = std::function; + using EmbedderCallbackHandler = + std::function; using ChannelCallbackHandler = std::function; MockKeyResponseController() @@ -64,7 +65,8 @@ class MockKeyResponseController { channel_response_(callback); } - void HandleEmbedderMessage(const FlutterKeyEvent* event, ResponseCallback callback) { + void HandleEmbedderMessage(const FlutterKeyEvent* event, + ResponseCallback callback) { embedder_response_(event, std::move(callback)); } @@ -76,13 +78,15 @@ class MockKeyResponseController { callback(false); } - static void EmbedderRespondFalse(const FlutterKeyEvent* event, ResponseCallback callback) { + static void EmbedderRespondFalse(const FlutterKeyEvent* event, + ResponseCallback callback) { callback(false); } }; -void MockEmbedderApiForKeyboard(EngineModifier& modifier, - std::shared_ptr response_controller); +void MockEmbedderApiForKeyboard( + EngineModifier& modifier, + std::shared_ptr response_controller); // Simulate a message queue for WM messages. //