From f4c9d3d85cd981dede82b5278e79dc9269cd7edc Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Tue, 15 Mar 2022 11:14:11 -0700 Subject: [PATCH 01/16] Work around background clipboard error --- shell/platform/windows/platform_handler_win32.cc | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index 7f3b885748d88..b0f27905b546a 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -14,6 +14,7 @@ #include "flutter/shell/platform/windows/flutter_windows_view.h" static constexpr char kValueKey[] = "value"; +static constexpr int kAccessDeniedErrorCode = 5; namespace flutter { @@ -235,18 +236,27 @@ void PlatformHandlerWin32::GetPlainText( void PlatformHandlerWin32::GetHasStrings( std::unique_ptr> result) { ScopedClipboard clipboard; + bool hasStrings; if (!clipboard.Open(std::get(*view_->GetRenderTarget()))) { rapidjson::Document error_code; error_code.SetInt(::GetLastError()); - result->Error(kClipboardError, "Unable to open clipboard", error_code); - return; + // Swallow errors of type ERROR_ACCESS_DENIED. These happen when the app is + // not in the foreground and HasStrings is irrelevant. + // See https://github.com/flutter/flutter/issues/95817. + if (error_code != kAccessDeniedErrorCode) { + result->Error(kClipboardError, "Unable to open clipboard", error_code); + return; + } + hasStrings = false; + } else { + hasStrings = clipboard.HasString(); } rapidjson::Document document; document.SetObject(); rapidjson::Document::AllocatorType& allocator = document.GetAllocator(); document.AddMember(rapidjson::Value(kValueKey, allocator), - rapidjson::Value(clipboard.HasString()), allocator); + rapidjson::Value(hasStrings), allocator); result->Success(document); } From b92feedf276f484af6aa7400338a70eda68cc2f5 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Fri, 25 Mar 2022 11:45:55 -0700 Subject: [PATCH 02/16] ScopeClipboard returns error codes and internalizes the error swallowing logic from this PR --- .../windows/platform_handler_win32.cc | 83 +++++++++++-------- 1 file changed, 50 insertions(+), 33 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index b0f27905b546a..7cf245c253f72 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -105,27 +105,25 @@ class ScopedClipboard { ScopedClipboard(ScopedClipboard const&) = delete; ScopedClipboard& operator=(ScopedClipboard const&) = delete; - // Attempts to open the clipboard for the given window, returning true if - // successful. - bool Open(HWND window); + // Attempts to open the clipboard for the given window, returning the error + // code in the case of failure and -1 otherwise. + int Open(HWND window); // Returns true if there is string data available to get. bool HasString(); // Returns string data from the clipboard. // - // If getting a string fails, returns no value. Get error information with - // ::GetLastError(). + // If getting a string fails, returns the error code. // // Open(...) must have succeeded to call this method. - std::optional GetString(); + std::variant GetString(); - // Sets the string content of the clipboard, returning true on success. - // - // On failure, get error information with ::GetLastError(). + // Sets the string content of the clipboard, returning the error code on + // failure and -1 otherwise. // // Open(...) must have succeeded to call this method. - bool SetString(const std::wstring string); + int SetString(const std::wstring string); private: bool opened_ = false; @@ -139,9 +137,20 @@ ScopedClipboard::~ScopedClipboard() { } } -bool ScopedClipboard::Open(HWND window) { +int ScopedClipboard::Open(HWND window) { opened_ = ::OpenClipboard(window); - return opened_; + + if (!opened_) { + int error_code = ::GetLastError(); + // Swallow errors of type ERROR_ACCESS_DENIED. These happen when the app is + // not in the foreground and HasStrings is irrelevant. + // See https://github.com/flutter/flutter/issues/95817. + if (error_code != kAccessDeniedErrorCode) { + return error_code; + } + } + + return -1; } bool ScopedClipboard::HasString() { @@ -150,24 +159,28 @@ bool ScopedClipboard::HasString() { ::IsClipboardFormatAvailable(CF_TEXT); } -std::optional ScopedClipboard::GetString() { +std::variant ScopedClipboard::GetString() { assert(opened_); HANDLE data = ::GetClipboardData(CF_UNICODETEXT); if (data == nullptr) { - return std::nullopt; + return ::GetLastError(); } ScopedGlobalLock locked_data(data); if (!locked_data.get()) { - return std::nullopt; + return ::GetLastError(); + } + std::optional string = static_cast(locked_data.get()); + if (!string) { + return ::GetLastError(); } - return std::optional(static_cast(locked_data.get())); + return string.value(); } -bool ScopedClipboard::SetString(const std::wstring string) { +int ScopedClipboard::SetString(const std::wstring string) { assert(opened_); if (!::EmptyClipboard()) { - return false; + return ::GetLastError(); } size_t null_terminated_byte_count = sizeof(decltype(string)::traits_type::char_type) * (string.size() + 1); @@ -175,15 +188,15 @@ bool ScopedClipboard::SetString(const std::wstring string) { null_terminated_byte_count); ScopedGlobalLock locked_memory(destination_memory.get()); if (!locked_memory.get()) { - return false; + return ::GetLastError(); } memcpy(locked_memory.get(), string.c_str(), null_terminated_byte_count); if (!::SetClipboardData(CF_UNICODETEXT, locked_memory.get())) { - return false; + return ::GetLastError(); } // The clipboard now owns the global memory. destination_memory.release(); - return true; + return -1; } } // namespace @@ -205,9 +218,10 @@ void PlatformHandlerWin32::GetPlainText( std::unique_ptr> result, std::string_view key) { ScopedClipboard clipboard; - if (!clipboard.Open(std::get(*view_->GetRenderTarget()))) { + int open_result = clipboard.Open(std::get(*view_->GetRenderTarget())); + if (open_result != -1) { rapidjson::Document error_code; - error_code.SetInt(::GetLastError()); + error_code.SetInt(open_result); result->Error(kClipboardError, "Unable to open clipboard", error_code); return; } @@ -215,10 +229,10 @@ void PlatformHandlerWin32::GetPlainText( result->Success(rapidjson::Document()); return; } - std::optional clipboard_string = clipboard.GetString(); - if (!clipboard_string) { + std::variant get_string_result = clipboard.GetString(); + if (std::holds_alternative(get_string_result)) { rapidjson::Document error_code; - error_code.SetInt(::GetLastError()); + error_code.SetInt(std::get(get_string_result)); result->Error(kClipboardError, "Unable to get clipboard data", error_code); return; } @@ -228,7 +242,7 @@ void PlatformHandlerWin32::GetPlainText( rapidjson::Document::AllocatorType& allocator = document.GetAllocator(); document.AddMember( rapidjson::Value(key.data(), allocator), - rapidjson::Value(fml::WideStringToUtf8(*clipboard_string), allocator), + rapidjson::Value(fml::WideStringToUtf8(std::get(get_string_result)), allocator), allocator); result->Success(document); } @@ -237,9 +251,10 @@ void PlatformHandlerWin32::GetHasStrings( std::unique_ptr> result) { ScopedClipboard clipboard; bool hasStrings; - if (!clipboard.Open(std::get(*view_->GetRenderTarget()))) { + int open_result = clipboard.Open(std::get(*view_->GetRenderTarget())); + if (open_result != -1) { rapidjson::Document error_code; - error_code.SetInt(::GetLastError()); + error_code.SetInt(open_result); // Swallow errors of type ERROR_ACCESS_DENIED. These happen when the app is // not in the foreground and HasStrings is irrelevant. // See https://github.com/flutter/flutter/issues/95817. @@ -264,15 +279,17 @@ void PlatformHandlerWin32::SetPlainText( const std::string& text, std::unique_ptr> result) { ScopedClipboard clipboard; - if (!clipboard.Open(std::get(*view_->GetRenderTarget()))) { + int open_result = clipboard.Open(std::get(*view_->GetRenderTarget())); + if (open_result != -1) { rapidjson::Document error_code; - error_code.SetInt(::GetLastError()); + error_code.SetInt(open_result); result->Error(kClipboardError, "Unable to open clipboard", error_code); return; } - if (!clipboard.SetString(fml::Utf8ToWideString(text))) { + int set_result = clipboard.SetString(fml::Utf8ToWideString(text)); + if (set_result != -1) { rapidjson::Document error_code; - error_code.SetInt(::GetLastError()); + error_code.SetInt(set_result); result->Error(kClipboardError, "Unable to set clipboard data", error_code); return; } From fcd81d11fc8f815ddacdddc0f0c02667873bce86 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Fri, 25 Mar 2022 16:16:03 -0700 Subject: [PATCH 03/16] Can pass in scopedclipboard --- .../windows/platform_handler_win32.cc | 29 ++++++++++--------- .../platform/windows/platform_handler_win32.h | 17 ++++++++++- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index 7cf245c253f72..95f065c5ccff6 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -96,7 +96,7 @@ class ScopedGlobalLock { // A Clipboard wrapper that automatically closes the clipboard when it goes out // of scope. -class ScopedClipboard { +class ScopedClipboard : public ScopedClipboardInterface { public: ScopedClipboard(); ~ScopedClipboard(); @@ -209,27 +209,32 @@ std::unique_ptr PlatformHandler::Create( } PlatformHandlerWin32::PlatformHandlerWin32(BinaryMessenger* messenger, - FlutterWindowsView* view) - : PlatformHandler(messenger), view_(view) {} + FlutterWindowsView* view, std::optional clipboard_reference = nullptr) + : PlatformHandler(messenger), view_(view) { + if (clipboard_reference == nullptr) { + ScopedClipboard clipboard; + clipboard_reference = &clipboard; + } + clipboard_ = clipboard_reference.value(); + } PlatformHandlerWin32::~PlatformHandlerWin32() = default; void PlatformHandlerWin32::GetPlainText( std::unique_ptr> result, std::string_view key) { - ScopedClipboard clipboard; - int open_result = clipboard.Open(std::get(*view_->GetRenderTarget())); + int open_result = clipboard_->Open(std::get(*view_->GetRenderTarget())); if (open_result != -1) { rapidjson::Document error_code; error_code.SetInt(open_result); result->Error(kClipboardError, "Unable to open clipboard", error_code); return; } - if (!clipboard.HasString()) { + if (!clipboard_->HasString()) { result->Success(rapidjson::Document()); return; } - std::variant get_string_result = clipboard.GetString(); + std::variant get_string_result = clipboard_->GetString(); if (std::holds_alternative(get_string_result)) { rapidjson::Document error_code; error_code.SetInt(std::get(get_string_result)); @@ -249,9 +254,8 @@ void PlatformHandlerWin32::GetPlainText( void PlatformHandlerWin32::GetHasStrings( std::unique_ptr> result) { - ScopedClipboard clipboard; bool hasStrings; - int open_result = clipboard.Open(std::get(*view_->GetRenderTarget())); + int open_result = clipboard_->Open(std::get(*view_->GetRenderTarget())); if (open_result != -1) { rapidjson::Document error_code; error_code.SetInt(open_result); @@ -264,7 +268,7 @@ void PlatformHandlerWin32::GetHasStrings( } hasStrings = false; } else { - hasStrings = clipboard.HasString(); + hasStrings = clipboard_->HasString(); } rapidjson::Document document; @@ -278,15 +282,14 @@ void PlatformHandlerWin32::GetHasStrings( void PlatformHandlerWin32::SetPlainText( const std::string& text, std::unique_ptr> result) { - ScopedClipboard clipboard; - int open_result = clipboard.Open(std::get(*view_->GetRenderTarget())); + int open_result = clipboard_->Open(std::get(*view_->GetRenderTarget())); if (open_result != -1) { rapidjson::Document error_code; error_code.SetInt(open_result); result->Error(kClipboardError, "Unable to open clipboard", error_code); return; } - int set_result = clipboard.SetString(fml::Utf8ToWideString(text)); + int set_result = clipboard_->SetString(fml::Utf8ToWideString(text)); if (set_result != -1) { rapidjson::Document error_code; error_code.SetInt(set_result); diff --git a/shell/platform/windows/platform_handler_win32.h b/shell/platform/windows/platform_handler_win32.h index d69915d787c39..61101c06868c2 100644 --- a/shell/platform/windows/platform_handler_win32.h +++ b/shell/platform/windows/platform_handler_win32.h @@ -15,11 +15,24 @@ namespace flutter { class FlutterWindowsView; +// A public interface for ScopedClipboard, so that it can be injected into +// PlatformHandlerWin32. +class ScopedClipboardInterface { + public: + virtual int Open(HWND window) = 0; + + virtual bool HasString() = 0; + + virtual std::variant GetString() = 0; + + virtual int SetString(const std::wstring string) = 0; +}; + // Win32 implementation of PlatformHandler. class PlatformHandlerWin32 : public PlatformHandler { public: explicit PlatformHandlerWin32(BinaryMessenger* messenger, - FlutterWindowsView* view); + FlutterWindowsView* view, std::optional clipboard_reference); virtual ~PlatformHandlerWin32(); @@ -45,6 +58,8 @@ class PlatformHandlerWin32 : public PlatformHandler { private: // A reference to the Flutter view. FlutterWindowsView* view_; + // A clipboard instance that can be passed in. + ScopedClipboardInterface* clipboard_; }; } // namespace flutter From e4f31bab222ea98181095a0158e645935371a58c Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Mon, 28 Mar 2022 16:11:51 -0700 Subject: [PATCH 04/16] Test by injecting a fake ScopedClipboard --- shell/platform/windows/BUILD.gn | 1 + .../windows/platform_handler_win32.cc | 10 +- .../platform_handler_win32_unittests.cc | 124 ++++++++++++++++++ 3 files changed, 127 insertions(+), 8 deletions(-) create mode 100644 shell/platform/windows/platform_handler_win32_unittests.cc diff --git a/shell/platform/windows/BUILD.gn b/shell/platform/windows/BUILD.gn index 1ac8628583c06..51157138fdd69 100644 --- a/shell/platform/windows/BUILD.gn +++ b/shell/platform/windows/BUILD.gn @@ -183,6 +183,7 @@ executable("flutter_windows_unittests") { "keyboard_key_handler_unittests.cc", "keyboard_win32_unittests.cc", "platform_handler_unittests.cc", + "platform_handler_win32_unittests.cc", "sequential_id_generator_unittests.cc", "settings_plugin_unittests.cc", "system_utils_unittests.cc", diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index 95f065c5ccff6..2cc936985de10 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -141,13 +141,7 @@ int ScopedClipboard::Open(HWND window) { opened_ = ::OpenClipboard(window); if (!opened_) { - int error_code = ::GetLastError(); - // Swallow errors of type ERROR_ACCESS_DENIED. These happen when the app is - // not in the foreground and HasStrings is irrelevant. - // See https://github.com/flutter/flutter/issues/95817. - if (error_code != kAccessDeniedErrorCode) { - return error_code; - } + return ::GetLastError(); } return -1; @@ -260,7 +254,7 @@ void PlatformHandlerWin32::GetHasStrings( rapidjson::Document error_code; error_code.SetInt(open_result); // Swallow errors of type ERROR_ACCESS_DENIED. These happen when the app is - // not in the foreground and HasStrings is irrelevant. + // not in the foreground and GetHasStrings is irrelevant. // See https://github.com/flutter/flutter/issues/95817. if (error_code != kAccessDeniedErrorCode) { result->Error(kClipboardError, "Unable to open clipboard", error_code); diff --git a/shell/platform/windows/platform_handler_win32_unittests.cc b/shell/platform/windows/platform_handler_win32_unittests.cc new file mode 100644 index 0000000000000..cb3382319a7c0 --- /dev/null +++ b/shell/platform/windows/platform_handler_win32_unittests.cc @@ -0,0 +1,124 @@ +// Copyright 2013 The Flutter 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 "flutter/shell/platform/windows/platform_handler_win32.h" + +#include + +#include "flutter/shell/platform/common/json_method_codec.h" +#include "flutter/shell/platform/windows/testing/mock_window_binding_handler.h" +#include "flutter/shell/platform/windows/testing/test_binary_messenger.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include "rapidjson/document.h" + +namespace flutter { +namespace testing { + +namespace { +using ::testing::_; + +static constexpr char kChannelName[] = "flutter/platform"; + +static constexpr char kHasStringsClipboardMethod[] = "Clipboard.hasStrings"; + +static constexpr char kTextPlainFormat[] = "text/plain"; + +static constexpr char kValueKey[] = "value"; +static constexpr int kAccessDeniedErrorCode = 5; + +} // namespace + +// A test version of the private ScopedClipboard. +class TestScopedClipboard : public ScopedClipboardInterface { + public: + TestScopedClipboard(); + ~TestScopedClipboard(); + + // Prevent copying. + TestScopedClipboard(TestScopedClipboard const&) = delete; + TestScopedClipboard& operator=(TestScopedClipboard const&) = delete; + + int Open(HWND window); + + bool HasString(); + + std::variant GetString(); + + int SetString(const std::wstring string); + + private: + bool opened_ = false; +}; + +TestScopedClipboard::TestScopedClipboard() {} + +TestScopedClipboard::~TestScopedClipboard() { + if (opened_) { + ::CloseClipboard(); + } +} + +// Always fail to open with kAccessDeniedErrorCode. +int TestScopedClipboard::Open(HWND window) { + return kAccessDeniedErrorCode; +} + +bool TestScopedClipboard::HasString() { + return true; +} + +std::variant TestScopedClipboard::GetString() { + return -1; +} + +int TestScopedClipboard::SetString(const std::wstring string) { + return -1; +} + +class MockMethodResult : public MethodResult { + public: + MOCK_METHOD1(SuccessInternal, void(const rapidjson::Document*)); + MOCK_METHOD3(ErrorInternal, + void(const std::string&, + const std::string&, + const rapidjson::Document*)); + MOCK_METHOD0(NotImplementedInternal, void()); +}; + +TEST(PlatformHandlerWin32, HasStringsAccessDeniedReturnsFalseWithoutError) { + TestBinaryMessenger messenger; + TestScopedClipboard clipboard_reference; + FlutterWindowsView view( + std::make_unique<::testing::NiceMock>()); + PlatformHandlerWin32 platform_handler(&messenger, &view, &clipboard_reference); + + auto args = std::make_unique(rapidjson::kStringType); + auto& allocator = args->GetAllocator(); + args->SetString(kTextPlainFormat); + auto encoded = JsonMethodCodec::GetInstance().EncodeMethodCall( + MethodCall(kHasStringsClipboardMethod, + std::move(args))); + + MockMethodResult result; + rapidjson::Document document; + document.SetObject(); + rapidjson::Document::AllocatorType& document_allocator = document.GetAllocator(); + document.AddMember(rapidjson::Value(kValueKey, document_allocator), + rapidjson::Value(false), document_allocator); + + EXPECT_CALL(result, SuccessInternal(_)) + .WillOnce([](const rapidjson::Document* document) { + ASSERT_FALSE((*document)[kValueKey].GetBool()); + }); + EXPECT_TRUE(messenger.SimulateEngineMessage( + kChannelName, encoded->data(), encoded->size(), + [&](const uint8_t* reply, size_t reply_size) { + JsonMethodCodec::GetInstance().DecodeAndProcessResponseEnvelope( + reply, reply_size, &result); + })); +} + +} // namespace testing +} // namespace flutter From e97172e96488f50a7b19cbbd77bab1679b7d7421 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Tue, 29 Mar 2022 14:30:22 -0700 Subject: [PATCH 05/16] Formatting --- .../windows/platform_handler_win32.cc | 22 +++++++++++-------- .../platform/windows/platform_handler_win32.h | 6 +++-- .../platform_handler_win32_unittests.cc | 8 ++++--- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index 2cc936985de10..9c0a2e7bedbbc 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -202,15 +202,17 @@ std::unique_ptr PlatformHandler::Create( return std::make_unique(messenger, view); } -PlatformHandlerWin32::PlatformHandlerWin32(BinaryMessenger* messenger, - FlutterWindowsView* view, std::optional clipboard_reference = nullptr) +PlatformHandlerWin32::PlatformHandlerWin32( + BinaryMessenger* messenger, + FlutterWindowsView* view, + std::optional clipboard_reference = nullptr) : PlatformHandler(messenger), view_(view) { - if (clipboard_reference == nullptr) { - ScopedClipboard clipboard; - clipboard_reference = &clipboard; - } - clipboard_ = clipboard_reference.value(); - } + if (clipboard_reference == nullptr) { + ScopedClipboard clipboard; + clipboard_reference = &clipboard; + } + clipboard_ = clipboard_reference.value(); +} PlatformHandlerWin32::~PlatformHandlerWin32() = default; @@ -241,7 +243,9 @@ void PlatformHandlerWin32::GetPlainText( rapidjson::Document::AllocatorType& allocator = document.GetAllocator(); document.AddMember( rapidjson::Value(key.data(), allocator), - rapidjson::Value(fml::WideStringToUtf8(std::get(get_string_result)), allocator), + rapidjson::Value( + fml::WideStringToUtf8(std::get(get_string_result)), + allocator), allocator); result->Success(document); } diff --git a/shell/platform/windows/platform_handler_win32.h b/shell/platform/windows/platform_handler_win32.h index 61101c06868c2..f5a758a4d79a9 100644 --- a/shell/platform/windows/platform_handler_win32.h +++ b/shell/platform/windows/platform_handler_win32.h @@ -31,8 +31,10 @@ class ScopedClipboardInterface { // Win32 implementation of PlatformHandler. class PlatformHandlerWin32 : public PlatformHandler { public: - explicit PlatformHandlerWin32(BinaryMessenger* messenger, - FlutterWindowsView* view, std::optional clipboard_reference); + explicit PlatformHandlerWin32( + BinaryMessenger* messenger, + FlutterWindowsView* view, + std::optional clipboard_reference); virtual ~PlatformHandlerWin32(); diff --git a/shell/platform/windows/platform_handler_win32_unittests.cc b/shell/platform/windows/platform_handler_win32_unittests.cc index cb3382319a7c0..84c8b77cb6065 100644 --- a/shell/platform/windows/platform_handler_win32_unittests.cc +++ b/shell/platform/windows/platform_handler_win32_unittests.cc @@ -91,8 +91,9 @@ TEST(PlatformHandlerWin32, HasStringsAccessDeniedReturnsFalseWithoutError) { TestBinaryMessenger messenger; TestScopedClipboard clipboard_reference; FlutterWindowsView view( - std::make_unique<::testing::NiceMock>()); - PlatformHandlerWin32 platform_handler(&messenger, &view, &clipboard_reference); + std::make_unique<::testing::NiceMock>()); + PlatformHandlerWin32 platform_handler(&messenger, &view, + &clipboard_reference); auto args = std::make_unique(rapidjson::kStringType); auto& allocator = args->GetAllocator(); @@ -104,7 +105,8 @@ TEST(PlatformHandlerWin32, HasStringsAccessDeniedReturnsFalseWithoutError) { MockMethodResult result; rapidjson::Document document; document.SetObject(); - rapidjson::Document::AllocatorType& document_allocator = document.GetAllocator(); + rapidjson::Document::AllocatorType& document_allocator = + document.GetAllocator(); document.AddMember(rapidjson::Value(kValueKey, document_allocator), rapidjson::Value(false), document_allocator); From 1b764e6144fcaa44f765f0549394131cf57b658b Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Tue, 29 Mar 2022 15:19:13 -0700 Subject: [PATCH 06/16] License --- ci/licenses_golden/licenses_flutter | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index cc529b958e279..77997513f80b8 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -2271,6 +2271,7 @@ FILE: ../../../flutter/shell/platform/windows/platform_handler.h FILE: ../../../flutter/shell/platform/windows/platform_handler_unittests.cc FILE: ../../../flutter/shell/platform/windows/platform_handler_win32.cc FILE: ../../../flutter/shell/platform/windows/platform_handler_win32.h +FILE: ../../../flutter/shell/platform/windows/platform_handler_win32_unittests.cc FILE: ../../../flutter/shell/platform/windows/public/flutter_windows.h FILE: ../../../flutter/shell/platform/windows/sequential_id_generator.cc FILE: ../../../flutter/shell/platform/windows/sequential_id_generator.h From 173da5d8e97a5952315258eaf90b867304205b89 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Fri, 8 Apr 2022 12:34:38 -0700 Subject: [PATCH 07/16] Code review: 0 success error code --- .../platform/windows/platform_handler_win32.cc | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index 9c0a2e7bedbbc..ebcf4ec093830 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -15,6 +15,7 @@ static constexpr char kValueKey[] = "value"; static constexpr int kAccessDeniedErrorCode = 5; +static constexpr int kErrorSuccess = 0; namespace flutter { @@ -106,7 +107,7 @@ class ScopedClipboard : public ScopedClipboardInterface { ScopedClipboard& operator=(ScopedClipboard const&) = delete; // Attempts to open the clipboard for the given window, returning the error - // code in the case of failure and -1 otherwise. + // code in the case of failure and 0 otherwise. int Open(HWND window); // Returns true if there is string data available to get. @@ -120,7 +121,7 @@ class ScopedClipboard : public ScopedClipboardInterface { std::variant GetString(); // Sets the string content of the clipboard, returning the error code on - // failure and -1 otherwise. + // failure and 0 otherwise. // // Open(...) must have succeeded to call this method. int SetString(const std::wstring string); @@ -144,7 +145,7 @@ int ScopedClipboard::Open(HWND window) { return ::GetLastError(); } - return -1; + return kErrorSuccess; } bool ScopedClipboard::HasString() { @@ -190,7 +191,7 @@ int ScopedClipboard::SetString(const std::wstring string) { } // The clipboard now owns the global memory. destination_memory.release(); - return -1; + return kErrorSuccess; } } // namespace @@ -220,7 +221,7 @@ void PlatformHandlerWin32::GetPlainText( std::unique_ptr> result, std::string_view key) { int open_result = clipboard_->Open(std::get(*view_->GetRenderTarget())); - if (open_result != -1) { + if (open_result != kErrorSuccess) { rapidjson::Document error_code; error_code.SetInt(open_result); result->Error(kClipboardError, "Unable to open clipboard", error_code); @@ -254,7 +255,7 @@ void PlatformHandlerWin32::GetHasStrings( std::unique_ptr> result) { bool hasStrings; int open_result = clipboard_->Open(std::get(*view_->GetRenderTarget())); - if (open_result != -1) { + if (open_result != kErrorSuccess) { rapidjson::Document error_code; error_code.SetInt(open_result); // Swallow errors of type ERROR_ACCESS_DENIED. These happen when the app is @@ -281,14 +282,14 @@ void PlatformHandlerWin32::SetPlainText( const std::string& text, std::unique_ptr> result) { int open_result = clipboard_->Open(std::get(*view_->GetRenderTarget())); - if (open_result != -1) { + if (open_result != kErrorSuccess) { rapidjson::Document error_code; error_code.SetInt(open_result); result->Error(kClipboardError, "Unable to open clipboard", error_code); return; } int set_result = clipboard_->SetString(fml::Utf8ToWideString(text)); - if (set_result != -1) { + if (set_result != kErrorSuccess) { rapidjson::Document error_code; error_code.SetInt(set_result); result->Error(kClipboardError, "Unable to set clipboard data", error_code); From 519bc4fd767a52efea32376155ddd23e83236d7d Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Fri, 8 Apr 2022 13:49:39 -0700 Subject: [PATCH 08/16] Code review redundant GetStrings --- shell/platform/windows/platform_handler_win32.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index ebcf4ec093830..a2a25cc4aac8e 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -162,9 +162,6 @@ std::variant ScopedClipboard::GetString() { return ::GetLastError(); } ScopedGlobalLock locked_data(data); - if (!locked_data.get()) { - return ::GetLastError(); - } std::optional string = static_cast(locked_data.get()); if (!string) { return ::GetLastError(); From 047bad154cb656af82b390775da610bb62349518 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Fri, 8 Apr 2022 15:16:21 -0700 Subject: [PATCH 09/16] Code review:virtual destructor and moved comments to header and overrides --- shell/platform/windows/platform_handler_win32.cc | 14 +------------- shell/platform/windows/platform_handler_win32.h | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index a2a25cc4aac8e..d69fae722f0ca 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -100,30 +100,18 @@ class ScopedGlobalLock { class ScopedClipboard : public ScopedClipboardInterface { public: ScopedClipboard(); - ~ScopedClipboard(); + virtual ~ScopedClipboard(); // Prevent copying. ScopedClipboard(ScopedClipboard const&) = delete; ScopedClipboard& operator=(ScopedClipboard const&) = delete; - // Attempts to open the clipboard for the given window, returning the error - // code in the case of failure and 0 otherwise. int Open(HWND window); - // Returns true if there is string data available to get. bool HasString(); - // Returns string data from the clipboard. - // - // If getting a string fails, returns the error code. - // - // Open(...) must have succeeded to call this method. std::variant GetString(); - // Sets the string content of the clipboard, returning the error code on - // failure and 0 otherwise. - // - // Open(...) must have succeeded to call this method. int SetString(const std::wstring string); private: diff --git a/shell/platform/windows/platform_handler_win32.h b/shell/platform/windows/platform_handler_win32.h index f5a758a4d79a9..8a7c684ba6fb9 100644 --- a/shell/platform/windows/platform_handler_win32.h +++ b/shell/platform/windows/platform_handler_win32.h @@ -19,12 +19,26 @@ class FlutterWindowsView; // PlatformHandlerWin32. class ScopedClipboardInterface { public: + virtual ~ScopedClipboardInterface() {}; + + // Attempts to open the clipboard for the given window, returning the error + // code in the case of failure and 0 otherwise. virtual int Open(HWND window) = 0; + // Returns true if there is string data available to get. virtual bool HasString() = 0; + // Returns string data from the clipboard. + // + // If getting a string fails, returns the error code. + // + // Open(...) must have succeeded to call this method. virtual std::variant GetString() = 0; + // Sets the string content of the clipboard, returning the error code on + // failure and 0 otherwise. + // + // Open(...) must have succeeded to call this method. virtual int SetString(const std::wstring string) = 0; }; From 479b568a356321867a2ae55e0c17e44053c2bfe5 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Fri, 8 Apr 2022 15:30:28 -0700 Subject: [PATCH 10/16] Code review:overrides and clipboard_reference naming and remove optional --- .../platform/windows/platform_handler_win32.cc | 18 +++++++++--------- .../platform/windows/platform_handler_win32.h | 2 +- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index d69fae722f0ca..1838091768297 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -106,13 +106,13 @@ class ScopedClipboard : public ScopedClipboardInterface { ScopedClipboard(ScopedClipboard const&) = delete; ScopedClipboard& operator=(ScopedClipboard const&) = delete; - int Open(HWND window); + int Open(HWND window) override; - bool HasString(); + bool HasString() override; - std::variant GetString(); + std::variant GetString() override; - int SetString(const std::wstring string); + int SetString(const std::wstring string) override; private: bool opened_ = false; @@ -191,13 +191,13 @@ std::unique_ptr PlatformHandler::Create( PlatformHandlerWin32::PlatformHandlerWin32( BinaryMessenger* messenger, FlutterWindowsView* view, - std::optional clipboard_reference = nullptr) + ScopedClipboardInterface* clipboard = nullptr) : PlatformHandler(messenger), view_(view) { - if (clipboard_reference == nullptr) { - ScopedClipboard clipboard; - clipboard_reference = &clipboard; + if (clipboard == nullptr) { + ScopedClipboard scoped_clipboard; + clipboard = &scoped_clipboard; } - clipboard_ = clipboard_reference.value(); + clipboard_ = clipboard; } PlatformHandlerWin32::~PlatformHandlerWin32() = default; diff --git a/shell/platform/windows/platform_handler_win32.h b/shell/platform/windows/platform_handler_win32.h index 8a7c684ba6fb9..c9055c437f436 100644 --- a/shell/platform/windows/platform_handler_win32.h +++ b/shell/platform/windows/platform_handler_win32.h @@ -48,7 +48,7 @@ class PlatformHandlerWin32 : public PlatformHandler { explicit PlatformHandlerWin32( BinaryMessenger* messenger, FlutterWindowsView* view, - std::optional clipboard_reference); + ScopedClipboardInterface* clipboard); virtual ~PlatformHandlerWin32(); From 437ab103f82bf14a6ffabbdac5bf03e60efcb13b Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Fri, 8 Apr 2022 15:34:03 -0700 Subject: [PATCH 11/16] Code review:clipboard default parameter --- shell/platform/windows/platform_handler_win32.cc | 2 +- shell/platform/windows/platform_handler_win32.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index 1838091768297..226f0c136c264 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -191,7 +191,7 @@ std::unique_ptr PlatformHandler::Create( PlatformHandlerWin32::PlatformHandlerWin32( BinaryMessenger* messenger, FlutterWindowsView* view, - ScopedClipboardInterface* clipboard = nullptr) + ScopedClipboardInterface* clipboard) : PlatformHandler(messenger), view_(view) { if (clipboard == nullptr) { ScopedClipboard scoped_clipboard; diff --git a/shell/platform/windows/platform_handler_win32.h b/shell/platform/windows/platform_handler_win32.h index c9055c437f436..d68bf195f7ff0 100644 --- a/shell/platform/windows/platform_handler_win32.h +++ b/shell/platform/windows/platform_handler_win32.h @@ -48,7 +48,7 @@ class PlatformHandlerWin32 : public PlatformHandler { explicit PlatformHandlerWin32( BinaryMessenger* messenger, FlutterWindowsView* view, - ScopedClipboardInterface* clipboard); + ScopedClipboardInterface* clipboard = nullptr); virtual ~PlatformHandlerWin32(); @@ -74,7 +74,7 @@ class PlatformHandlerWin32 : public PlatformHandler { private: // A reference to the Flutter view. FlutterWindowsView* view_; - // A clipboard instance that can be passed in. + // A clipboard instance that can be passed in for mocking in tests. ScopedClipboardInterface* clipboard_; }; From dd97459ebdd0264f6a0dda4d9c91e24e353c0898 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Mon, 11 Apr 2022 15:30:03 -0700 Subject: [PATCH 12/16] Code review: unique_ptr --- shell/platform/windows/platform_handler_win32.cc | 8 ++++---- shell/platform/windows/platform_handler_win32.h | 4 ++-- .../platform/windows/platform_handler_win32_unittests.cc | 3 +-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index 226f0c136c264..038f269fbbde1 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -191,13 +191,13 @@ std::unique_ptr PlatformHandler::Create( PlatformHandlerWin32::PlatformHandlerWin32( BinaryMessenger* messenger, FlutterWindowsView* view, - ScopedClipboardInterface* clipboard) + std::unique_ptr clipboard) : PlatformHandler(messenger), view_(view) { if (clipboard == nullptr) { - ScopedClipboard scoped_clipboard; - clipboard = &scoped_clipboard; + clipboard_ = std::make_unique(); + } else { + clipboard_ = std::move(clipboard); } - clipboard_ = clipboard; } PlatformHandlerWin32::~PlatformHandlerWin32() = default; diff --git a/shell/platform/windows/platform_handler_win32.h b/shell/platform/windows/platform_handler_win32.h index d68bf195f7ff0..e9771c7565fbe 100644 --- a/shell/platform/windows/platform_handler_win32.h +++ b/shell/platform/windows/platform_handler_win32.h @@ -48,7 +48,7 @@ class PlatformHandlerWin32 : public PlatformHandler { explicit PlatformHandlerWin32( BinaryMessenger* messenger, FlutterWindowsView* view, - ScopedClipboardInterface* clipboard = nullptr); + std::unique_ptr clipboard = nullptr); virtual ~PlatformHandlerWin32(); @@ -75,7 +75,7 @@ class PlatformHandlerWin32 : public PlatformHandler { // A reference to the Flutter view. FlutterWindowsView* view_; // A clipboard instance that can be passed in for mocking in tests. - ScopedClipboardInterface* clipboard_; + std::unique_ptr clipboard_; }; } // namespace flutter diff --git a/shell/platform/windows/platform_handler_win32_unittests.cc b/shell/platform/windows/platform_handler_win32_unittests.cc index 84c8b77cb6065..fc910b4fc9d9a 100644 --- a/shell/platform/windows/platform_handler_win32_unittests.cc +++ b/shell/platform/windows/platform_handler_win32_unittests.cc @@ -89,11 +89,10 @@ class MockMethodResult : public MethodResult { TEST(PlatformHandlerWin32, HasStringsAccessDeniedReturnsFalseWithoutError) { TestBinaryMessenger messenger; - TestScopedClipboard clipboard_reference; FlutterWindowsView view( std::make_unique<::testing::NiceMock>()); PlatformHandlerWin32 platform_handler(&messenger, &view, - &clipboard_reference); + std::make_unique()); auto args = std::make_unique(rapidjson::kStringType); auto& allocator = args->GetAllocator(); From 33a98e37ac6f9804f5644d2a844ab20077bf580a Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Tue, 12 Apr 2022 09:47:31 -0700 Subject: [PATCH 13/16] Code review: Full tests for hasstrings --- .../platform/windows/platform_handler_win32.h | 2 +- .../platform_handler_win32_unittests.cc | 364 ++++++++++++------ 2 files changed, 240 insertions(+), 126 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.h b/shell/platform/windows/platform_handler_win32.h index e9771c7565fbe..62d6444158462 100644 --- a/shell/platform/windows/platform_handler_win32.h +++ b/shell/platform/windows/platform_handler_win32.h @@ -19,7 +19,7 @@ class FlutterWindowsView; // PlatformHandlerWin32. class ScopedClipboardInterface { public: - virtual ~ScopedClipboardInterface() {}; + virtual ~ScopedClipboardInterface(){}; // Attempts to open the clipboard for the given window, returning the error // code in the case of failure and 0 otherwise. diff --git a/shell/platform/windows/platform_handler_win32_unittests.cc b/shell/platform/windows/platform_handler_win32_unittests.cc index fc910b4fc9d9a..be503014f36f6 100644 --- a/shell/platform/windows/platform_handler_win32_unittests.cc +++ b/shell/platform/windows/platform_handler_win32_unittests.cc @@ -1,125 +1,239 @@ -// Copyright 2013 The Flutter 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 "flutter/shell/platform/windows/platform_handler_win32.h" - -#include - -#include "flutter/shell/platform/common/json_method_codec.h" -#include "flutter/shell/platform/windows/testing/mock_window_binding_handler.h" -#include "flutter/shell/platform/windows/testing/test_binary_messenger.h" -#include "gmock/gmock.h" -#include "gtest/gtest.h" -#include "rapidjson/document.h" - -namespace flutter { -namespace testing { - -namespace { -using ::testing::_; - -static constexpr char kChannelName[] = "flutter/platform"; - -static constexpr char kHasStringsClipboardMethod[] = "Clipboard.hasStrings"; - -static constexpr char kTextPlainFormat[] = "text/plain"; - -static constexpr char kValueKey[] = "value"; -static constexpr int kAccessDeniedErrorCode = 5; - -} // namespace - -// A test version of the private ScopedClipboard. -class TestScopedClipboard : public ScopedClipboardInterface { - public: - TestScopedClipboard(); - ~TestScopedClipboard(); - - // Prevent copying. - TestScopedClipboard(TestScopedClipboard const&) = delete; - TestScopedClipboard& operator=(TestScopedClipboard const&) = delete; - - int Open(HWND window); - - bool HasString(); - - std::variant GetString(); - - int SetString(const std::wstring string); - - private: - bool opened_ = false; -}; - -TestScopedClipboard::TestScopedClipboard() {} - -TestScopedClipboard::~TestScopedClipboard() { - if (opened_) { - ::CloseClipboard(); - } -} - -// Always fail to open with kAccessDeniedErrorCode. -int TestScopedClipboard::Open(HWND window) { - return kAccessDeniedErrorCode; -} - -bool TestScopedClipboard::HasString() { - return true; -} - -std::variant TestScopedClipboard::GetString() { - return -1; -} - -int TestScopedClipboard::SetString(const std::wstring string) { - return -1; -} - -class MockMethodResult : public MethodResult { - public: - MOCK_METHOD1(SuccessInternal, void(const rapidjson::Document*)); - MOCK_METHOD3(ErrorInternal, - void(const std::string&, - const std::string&, - const rapidjson::Document*)); - MOCK_METHOD0(NotImplementedInternal, void()); -}; - -TEST(PlatformHandlerWin32, HasStringsAccessDeniedReturnsFalseWithoutError) { - TestBinaryMessenger messenger; - FlutterWindowsView view( - std::make_unique<::testing::NiceMock>()); - PlatformHandlerWin32 platform_handler(&messenger, &view, - std::make_unique()); - - auto args = std::make_unique(rapidjson::kStringType); - auto& allocator = args->GetAllocator(); - args->SetString(kTextPlainFormat); - auto encoded = JsonMethodCodec::GetInstance().EncodeMethodCall( - MethodCall(kHasStringsClipboardMethod, - std::move(args))); - - MockMethodResult result; - rapidjson::Document document; - document.SetObject(); - rapidjson::Document::AllocatorType& document_allocator = - document.GetAllocator(); - document.AddMember(rapidjson::Value(kValueKey, document_allocator), - rapidjson::Value(false), document_allocator); - - EXPECT_CALL(result, SuccessInternal(_)) - .WillOnce([](const rapidjson::Document* document) { - ASSERT_FALSE((*document)[kValueKey].GetBool()); - }); - EXPECT_TRUE(messenger.SimulateEngineMessage( - kChannelName, encoded->data(), encoded->size(), - [&](const uint8_t* reply, size_t reply_size) { - JsonMethodCodec::GetInstance().DecodeAndProcessResponseEnvelope( - reply, reply_size, &result); - })); -} - -} // namespace testing -} // namespace flutter +// Copyright 2013 The Flutter 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 "flutter/shell/platform/windows/platform_handler_win32.h" + +#include + +#include "flutter/shell/platform/common/json_method_codec.h" +#include "flutter/shell/platform/windows/testing/mock_window_binding_handler.h" +#include "flutter/shell/platform/windows/testing/test_binary_messenger.h" +#include "gmock/gmock.h" +#include "gtest/gtest.h" +#include "rapidjson/document.h" + +namespace flutter { +namespace testing { + +namespace { +using ::testing::_; + +static constexpr char kChannelName[] = "flutter/platform"; + +static constexpr char kHasStringsClipboardMethod[] = "Clipboard.hasStrings"; + +static constexpr char kTextPlainFormat[] = "text/plain"; + +static constexpr char kValueKey[] = "value"; +static constexpr int kAccessDeniedErrorCode = 5; +static constexpr int kErrorSuccess = 0; + +} // namespace + +// A test version of the private ScopedClipboard. +class TestScopedClipboard : public ScopedClipboardInterface { + public: + TestScopedClipboard(int open_error, bool has_strings); + ~TestScopedClipboard(); + + // Prevent copying. + TestScopedClipboard(TestScopedClipboard const&) = delete; + TestScopedClipboard& operator=(TestScopedClipboard const&) = delete; + + int Open(HWND window); + + bool HasString(); + + std::variant GetString(); + + int SetString(const std::wstring string); + + private: + bool opened_ = false; + bool has_strings_; + int open_error_; +}; + +TestScopedClipboard::TestScopedClipboard(int open_error, bool has_strings) { + open_error_ = open_error; + has_strings_ = has_strings; +}; + +TestScopedClipboard::~TestScopedClipboard() { + if (opened_) { + ::CloseClipboard(); + } +} + +int TestScopedClipboard::Open(HWND window) { + return open_error_; +} + +bool TestScopedClipboard::HasString() { + return has_strings_; +} + +std::variant TestScopedClipboard::GetString() { + return -1; +} + +int TestScopedClipboard::SetString(const std::wstring string) { + return -1; +} + +class MockMethodResult : public MethodResult { + public: + MOCK_METHOD1(SuccessInternal, void(const rapidjson::Document*)); + MOCK_METHOD3(ErrorInternal, + void(const std::string&, + const std::string&, + const rapidjson::Document*)); + MOCK_METHOD0(NotImplementedInternal, void()); +}; + +// Regression test for https://github.com/flutter/flutter/issues/95817. +TEST(PlatformHandlerWin32, HasStringsAccessDeniedReturnsFalseWithoutError) { + TestBinaryMessenger messenger; + FlutterWindowsView view( + std::make_unique<::testing::NiceMock>()); + // HasStrings will receive access denied on the clipboard, but will return + // false without error. + PlatformHandlerWin32 platform_handler( + &messenger, &view, + std::make_unique(kAccessDeniedErrorCode, true)); + + auto args = std::make_unique(rapidjson::kStringType); + auto& allocator = args->GetAllocator(); + args->SetString(kTextPlainFormat); + auto encoded = JsonMethodCodec::GetInstance().EncodeMethodCall( + MethodCall(kHasStringsClipboardMethod, + std::move(args))); + + MockMethodResult result; + rapidjson::Document document; + document.SetObject(); + rapidjson::Document::AllocatorType& document_allocator = + document.GetAllocator(); + document.AddMember(rapidjson::Value(kValueKey, document_allocator), + rapidjson::Value(false), document_allocator); + + EXPECT_CALL(result, SuccessInternal(_)) + .WillOnce([](const rapidjson::Document* document) { + ASSERT_FALSE((*document)[kValueKey].GetBool()); + }); + EXPECT_TRUE(messenger.SimulateEngineMessage( + kChannelName, encoded->data(), encoded->size(), + [&](const uint8_t* reply, size_t reply_size) { + JsonMethodCodec::GetInstance().DecodeAndProcessResponseEnvelope( + reply, reply_size, &result); + })); +} + +TEST(PlatformHandlerWin32, HasStringsSuccessWithStrings) { + TestBinaryMessenger messenger; + FlutterWindowsView view( + std::make_unique<::testing::NiceMock>()); + // HasStrings will succeed and return true. + PlatformHandlerWin32 platform_handler( + &messenger, &view, + std::make_unique(kErrorSuccess, true)); + + auto args = std::make_unique(rapidjson::kStringType); + auto& allocator = args->GetAllocator(); + args->SetString(kTextPlainFormat); + auto encoded = JsonMethodCodec::GetInstance().EncodeMethodCall( + MethodCall(kHasStringsClipboardMethod, + std::move(args))); + + MockMethodResult result; + rapidjson::Document document; + document.SetObject(); + rapidjson::Document::AllocatorType& document_allocator = + document.GetAllocator(); + document.AddMember(rapidjson::Value(kValueKey, document_allocator), + rapidjson::Value(false), document_allocator); + + EXPECT_CALL(result, SuccessInternal(_)) + .WillOnce([](const rapidjson::Document* document) { + ASSERT_TRUE((*document)[kValueKey].GetBool()); + }); + EXPECT_TRUE(messenger.SimulateEngineMessage( + kChannelName, encoded->data(), encoded->size(), + [&](const uint8_t* reply, size_t reply_size) { + JsonMethodCodec::GetInstance().DecodeAndProcessResponseEnvelope( + reply, reply_size, &result); + })); +} + +TEST(PlatformHandlerWin32, HasStringsSuccessWithoutStrings) { + TestBinaryMessenger messenger; + FlutterWindowsView view( + std::make_unique<::testing::NiceMock>()); + // HasStrings will succeed and return false. + PlatformHandlerWin32 platform_handler( + &messenger, &view, + std::make_unique(kErrorSuccess, false)); + + auto args = std::make_unique(rapidjson::kStringType); + auto& allocator = args->GetAllocator(); + args->SetString(kTextPlainFormat); + auto encoded = JsonMethodCodec::GetInstance().EncodeMethodCall( + MethodCall(kHasStringsClipboardMethod, + std::move(args))); + + MockMethodResult result; + rapidjson::Document document; + document.SetObject(); + rapidjson::Document::AllocatorType& document_allocator = + document.GetAllocator(); + document.AddMember(rapidjson::Value(kValueKey, document_allocator), + rapidjson::Value(false), document_allocator); + + EXPECT_CALL(result, SuccessInternal(_)) + .WillOnce([](const rapidjson::Document* document) { + ASSERT_FALSE((*document)[kValueKey].GetBool()); + }); + EXPECT_TRUE(messenger.SimulateEngineMessage( + kChannelName, encoded->data(), encoded->size(), + [&](const uint8_t* reply, size_t reply_size) { + JsonMethodCodec::GetInstance().DecodeAndProcessResponseEnvelope( + reply, reply_size, &result); + })); +} + +TEST(PlatformHandlerWin32, HasStringsError) { + TestBinaryMessenger messenger; + FlutterWindowsView view( + std::make_unique<::testing::NiceMock>()); + // HasStrings will fail. + PlatformHandlerWin32 platform_handler( + &messenger, &view, std::make_unique(1, true)); + + auto args = std::make_unique(rapidjson::kStringType); + auto& allocator = args->GetAllocator(); + args->SetString(kTextPlainFormat); + auto encoded = JsonMethodCodec::GetInstance().EncodeMethodCall( + MethodCall(kHasStringsClipboardMethod, + std::move(args))); + + MockMethodResult result; + rapidjson::Document document; + document.SetObject(); + rapidjson::Document::AllocatorType& document_allocator = + document.GetAllocator(); + document.AddMember(rapidjson::Value(kValueKey, document_allocator), + rapidjson::Value(false), document_allocator); + + EXPECT_CALL(result, SuccessInternal(_)).Times(0); + EXPECT_CALL(result, ErrorInternal(_, _, _)).Times(1); + EXPECT_TRUE(messenger.SimulateEngineMessage( + kChannelName, encoded->data(), encoded->size(), + [&](const uint8_t* reply, size_t reply_size) { + JsonMethodCodec::GetInstance().DecodeAndProcessResponseEnvelope( + reply, reply_size, &result); + })); +} + +} // namespace testing +} // namespace flutter From bc6d7d8de03b755c3492724435b291fd190653d8 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Mon, 18 Apr 2022 13:56:25 -0700 Subject: [PATCH 14/16] Code review: No optional needed --- shell/platform/windows/platform_handler_win32.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32.cc b/shell/platform/windows/platform_handler_win32.cc index 038f269fbbde1..dac80a0e5a9b5 100644 --- a/shell/platform/windows/platform_handler_win32.cc +++ b/shell/platform/windows/platform_handler_win32.cc @@ -150,11 +150,11 @@ std::variant ScopedClipboard::GetString() { return ::GetLastError(); } ScopedGlobalLock locked_data(data); - std::optional string = static_cast(locked_data.get()); - if (!string) { + + if (!locked_data.get()) { return ::GetLastError(); } - return string.value(); + return static_cast(locked_data.get()); } int ScopedClipboard::SetString(const std::wstring string) { From 58f2da5e2ef8006fde2067c13b50006b6e95ecc0 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Mon, 2 May 2022 14:56:57 -0700 Subject: [PATCH 15/16] Review: override annotations --- .../platform/windows/platform_handler_win32_unittests.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/shell/platform/windows/platform_handler_win32_unittests.cc b/shell/platform/windows/platform_handler_win32_unittests.cc index be503014f36f6..662950c8f270c 100644 --- a/shell/platform/windows/platform_handler_win32_unittests.cc +++ b/shell/platform/windows/platform_handler_win32_unittests.cc @@ -41,13 +41,13 @@ class TestScopedClipboard : public ScopedClipboardInterface { TestScopedClipboard(TestScopedClipboard const&) = delete; TestScopedClipboard& operator=(TestScopedClipboard const&) = delete; - int Open(HWND window); + int Open(HWND window) override; - bool HasString(); + bool HasString() override; - std::variant GetString(); + std::variant GetString() override; - int SetString(const std::wstring string); + int SetString(const std::wstring string) override; private: bool opened_ = false; From 853c6ee63fde1fb08ba9d47487f823a9672e8ef9 Mon Sep 17 00:00:00 2001 From: Justin McCandless Date: Mon, 2 May 2022 14:58:41 -0700 Subject: [PATCH 16/16] Review: error code constant --- shell/platform/windows/platform_handler_win32_unittests.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/shell/platform/windows/platform_handler_win32_unittests.cc b/shell/platform/windows/platform_handler_win32_unittests.cc index 662950c8f270c..9d350857640f9 100644 --- a/shell/platform/windows/platform_handler_win32_unittests.cc +++ b/shell/platform/windows/platform_handler_win32_unittests.cc @@ -28,6 +28,7 @@ static constexpr char kTextPlainFormat[] = "text/plain"; static constexpr char kValueKey[] = "value"; static constexpr int kAccessDeniedErrorCode = 5; static constexpr int kErrorSuccess = 0; +static constexpr int kArbitraryErrorCode = 1; } // namespace @@ -208,7 +209,8 @@ TEST(PlatformHandlerWin32, HasStringsError) { std::make_unique<::testing::NiceMock>()); // HasStrings will fail. PlatformHandlerWin32 platform_handler( - &messenger, &view, std::make_unique(1, true)); + &messenger, &view, + std::make_unique(kArbitraryErrorCode, true)); auto args = std::make_unique(rapidjson::kStringType); auto& allocator = args->GetAllocator();