From 213184190d05282d18cc43a81dc2355ef9b5f667 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 19 Aug 2020 18:19:30 -0700 Subject: [PATCH 1/2] Use references for C++ MethodResult and EventSink The reponse APIs for method channels and event channels used pointers for optional parameters; this kept the API surface simple, but meant that they couldn't take rvalues. As a result, returning success values or error details often took an extra line, declaring a variable for the result just to have something to pass the address of. This converts them to using references, with function overloading to allow for optional parameters, so that values can be inlined. For now the pointer versions are still present, so that conversion can be done before it becomes a breaking change; they will be removed soon. Part of https://github.com/flutter/flutter/issues/63975 --- .../include/flutter/event_channel.h | 4 +- .../include/flutter/event_sink.h | 33 ++++++++++--- .../include/flutter/method_result.h | 46 +++++++++++++++---- .../method_result_functions_unittests.cc | 7 ++- .../cpp/client_wrapper/standard_codec.cc | 26 ++++++++--- .../platform/common/cpp/json_method_codec.cc | 12 ++++- .../windows/win32_platform_handler.cc | 13 +++--- 7 files changed, 104 insertions(+), 37 deletions(-) diff --git a/shell/platform/common/cpp/client_wrapper/include/flutter/event_channel.h b/shell/platform/common/cpp/client_wrapper/include/flutter/event_channel.h index 301c73a418195..23fd7940752db 100644 --- a/shell/platform/common/cpp/client_wrapper/include/flutter/event_channel.h +++ b/shell/platform/common/cpp/client_wrapper/include/flutter/event_channel.h @@ -144,14 +144,14 @@ class EventChannel { const MethodCodec* codec_; protected: - void SuccessInternal(T* event = nullptr) override { + void SuccessInternal(const T* event = nullptr) override { auto result = codec_->EncodeSuccessEnvelope(event); messenger_->Send(name_, result->data(), result->size()); } void ErrorInternal(const std::string& error_code, const std::string& error_message, - T* error_details) override { + const T* error_details) override { auto result = codec_->EncodeErrorEnvelope(error_code, error_message, error_details); messenger_->Send(name_, result->data(), result->size()); diff --git a/shell/platform/common/cpp/client_wrapper/include/flutter/event_sink.h b/shell/platform/common/cpp/client_wrapper/include/flutter/event_sink.h index 6223ad7b6a572..7b2115f1c4359 100644 --- a/shell/platform/common/cpp/client_wrapper/include/flutter/event_sink.h +++ b/shell/platform/common/cpp/client_wrapper/include/flutter/event_sink.h @@ -19,28 +19,49 @@ class EventSink { EventSink(EventSink const&) = delete; EventSink& operator=(EventSink const&) = delete; + // DEPRECATED. Use the reference version below. This will be removed in the + // near future. + void Success(const T* event) { SuccessInternal(event); } + + // Consumes a successful event + void Success(const T& event) { SuccessInternal&(event); } + // Consumes a successful event. - void Success(T* event = nullptr) { SuccessInternal(event); } + void Success() { SuccessInternal(nullptr); } - // Consumes an error event. + // DEPRECATED. Use the reference version below. This will be removed in the + // near future. void Error(const std::string& error_code, - const std::string& error_message = "", - T* error_details = nullptr) { + const std::string& error_message, + const T* error_details) { ErrorInternal(error_code, error_message, error_details); } + // Consumes an error event. + void Error(const std::string& error_code, + const std::string& error_message, + const T& error_details) { + ErrorInternal(error_code, error_message, &error_details); + } + + // Consumes an error event. + void Error(const std::string& error_code, + const std::string& error_message = "") { + ErrorInternal(error_code, error_message, nullptr); + } + // Consumes end of stream. Ensuing calls to Success() or // Error(), if any, are ignored. void EndOfStream() { EndOfStreamInternal(); } protected: // Implementation of the public interface, to be provided by subclasses. - virtual void SuccessInternal(T* event = nullptr) = 0; + virtual void SuccessInternal(const T* event = nullptr) = 0; // Implementation of the public interface, to be provided by subclasses. virtual void ErrorInternal(const std::string& error_code, const std::string& error_message, - T* error_details) = 0; + const T* error_details) = 0; // Implementation of the public interface, to be provided by subclasses. virtual void EndOfStreamInternal() = 0; diff --git a/shell/platform/common/cpp/client_wrapper/include/flutter/method_result.h b/shell/platform/common/cpp/client_wrapper/include/flutter/method_result.h index 1e9c822e253e8..c2b2df5791058 100644 --- a/shell/platform/common/cpp/client_wrapper/include/flutter/method_result.h +++ b/shell/platform/common/cpp/client_wrapper/include/flutter/method_result.h @@ -22,20 +22,48 @@ class MethodResult { MethodResult(MethodResult const&) = delete; MethodResult& operator=(MethodResult const&) = delete; - // Sends a success response, indicating that the call completed successfully. - // An optional value can be provided as part of the success message. - void Success(const T* result = nullptr) { SuccessInternal(result); } + // DEPRECATED. Use the reference versions below. This will be removed in the + // near future. + void Success(const T* result) { SuccessInternal(result); } - // Sends an error response, indicating that the call was understood but - // handling failed in some way. A string error code must be provided, and in - // addition an optional user-readable error_message and/or details object can - // be included. + // Sends a success response, indicating that the call completed successfully + // with the given result. + void Success(const T& result) { SuccessInternal(&result); } + + // Sends a success response, indicating that the call completed successfully + // with no result. + void Success() { SuccessInternal(nullptr); } + + // DEPRECATED. Use the reference versions below. This will be removed in the + // near future. void Error(const std::string& error_code, - const std::string& error_message = "", - const T* error_details = nullptr) { + const std::string& error_message, + const T* error_details) { ErrorInternal(error_code, error_message, error_details); } + // Sends an error response, indicating that the call was understood but + // handling failed in some way. + // + // error_code: A string error code describing the error. + // error_message: A user-readable error message. + // error_details: Arbitrary extra details about the error. + void Error(const std::string& error_code, + const std::string& error_message, + const T& error_details) { + ErrorInternal(error_code, error_message, &error_details); + } + + // Sends an error response, indicating that the call was understood but + // handling failed in some way. + // + // error_code: A string error code describing the error. + // error_message: A user-readable error message (optional). + void Error(const std::string& error_code, + const std::string& error_message = "") { + ErrorInternal(error_code, error_message, nullptr); + } + // Sends a not-implemented response, indicating that the method either was not // recognized, or has not been implemented. void NotImplemented() { NotImplementedInternal(); } diff --git a/shell/platform/common/cpp/client_wrapper/method_result_functions_unittests.cc b/shell/platform/common/cpp/client_wrapper/method_result_functions_unittests.cc index 98750dbba244b..9c32da9774a7c 100644 --- a/shell/platform/common/cpp/client_wrapper/method_result_functions_unittests.cc +++ b/shell/platform/common/cpp/client_wrapper/method_result_functions_unittests.cc @@ -2,11 +2,10 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/method_result_functions.h" - #include #include +#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/method_result_functions.h" #include "gtest/gtest.h" namespace flutter { @@ -29,7 +28,7 @@ TEST(MethodChannelTest, Success) { EXPECT_EQ(*i, value); }, nullptr, nullptr); - result.Success(&value); + result.Success(value); EXPECT_TRUE(called); } @@ -50,7 +49,7 @@ TEST(MethodChannelTest, Error) { EXPECT_EQ(*details, error_details); }, nullptr); - result.Error(error_code, error_message, &error_details); + result.Error(error_code, error_message, error_details); EXPECT_TRUE(called); } diff --git a/shell/platform/common/cpp/client_wrapper/standard_codec.cc b/shell/platform/common/cpp/client_wrapper/standard_codec.cc index a17845750d8ca..bb6309844fb9f 100644 --- a/shell/platform/common/cpp/client_wrapper/standard_codec.cc +++ b/shell/platform/common/cpp/client_wrapper/standard_codec.cc @@ -522,7 +522,11 @@ bool StandardMethodCodec::DecodeAndProcessResponseEnvelopeInternal( switch (flag) { case 0: { EncodableValue value = serializer_->ReadValue(&stream); - result->Success(value.IsNull() ? nullptr : &value); + if (value.IsNull()) { + result->Success(); + } else { + result->Success(value); + } return true; } case 1: { @@ -530,13 +534,21 @@ bool StandardMethodCodec::DecodeAndProcessResponseEnvelopeInternal( EncodableValue message = serializer_->ReadValue(&stream); EncodableValue details = serializer_->ReadValue(&stream); #ifdef USE_LEGACY_ENCODABLE_VALUE - result->Error(code.StringValue(), - message.IsNull() ? "" : message.StringValue(), - details.IsNull() ? nullptr : &details); + if (details.IsNull()) { + result->Error(code.StringValue(), + message.IsNull() ? "" : message.StringValue()); + } else { + result->Error(code.StringValue(), + message.IsNull() ? "" : message.StringValue(), details); + } #else - result->Error(std::get(code), - message.IsNull() ? "" : std::get(message), - details.IsNull() ? nullptr : &details); + const std::string& message_string = + message.IsNull() ? "" : std::get(message); + if (details.IsNull()) { + result->Error(std::get(code), message_string); + } else { + result->Error(std::get(code), message_string, details); + } #endif return true; } diff --git a/shell/platform/common/cpp/json_method_codec.cc b/shell/platform/common/cpp/json_method_codec.cc index 8fae8b7ed9b82..f026061ad235f 100644 --- a/shell/platform/common/cpp/json_method_codec.cc +++ b/shell/platform/common/cpp/json_method_codec.cc @@ -131,7 +131,11 @@ bool JsonMethodCodec::DecodeAndProcessResponseEnvelopeInternal( case 1: { std::unique_ptr value = ExtractElement(json_response.get(), &((*json_response)[0])); - result->Success(value->IsNull() ? nullptr : value.get()); + if (value->IsNull()) { + result->Success(); + } else { + result->Success(*value); + } return true; } case 3: { @@ -139,7 +143,11 @@ bool JsonMethodCodec::DecodeAndProcessResponseEnvelopeInternal( std::string message = (*json_response)[1].GetString(); std::unique_ptr details = ExtractElement(json_response.get(), &((*json_response)[2])); - result->Error(code, message, details->IsNull() ? nullptr : details.get()); + if (details->IsNull()) { + result->Error(code, message); + } else { + result->Error(code, message, *details); + } return true; } default: diff --git a/shell/platform/windows/win32_platform_handler.cc b/shell/platform/windows/win32_platform_handler.cc index a9efc31f9a2d7..02a3d9c37faa2 100644 --- a/shell/platform/windows/win32_platform_handler.cc +++ b/shell/platform/windows/win32_platform_handler.cc @@ -228,12 +228,11 @@ void PlatformHandler::HandleMethodCall( if (!clipboard.Open(std::get(*view_->GetRenderTarget()))) { rapidjson::Document error_code; error_code.SetInt(::GetLastError()); - result->Error(kClipboardError, "Unable to open clipboard", &error_code); + result->Error(kClipboardError, "Unable to open clipboard", error_code); return; } if (!clipboard.HasString()) { - rapidjson::Document null; - result->Success(&null); + result->Success(rapidjson::Document()); return; } std::optional clipboard_string = clipboard.GetString(); @@ -241,7 +240,7 @@ void PlatformHandler::HandleMethodCall( rapidjson::Document error_code; error_code.SetInt(::GetLastError()); result->Error(kClipboardError, "Unable to get clipboard data", - &error_code); + error_code); return; } @@ -252,7 +251,7 @@ void PlatformHandler::HandleMethodCall( rapidjson::Value(kTextKey, allocator), rapidjson::Value(Utf8FromUtf16(*clipboard_string), allocator), allocator); - result->Success(&document); + result->Success(document); } else if (method.compare(kSetClipboardDataMethod) == 0) { const rapidjson::Value& document = *method_call.arguments(); rapidjson::Value::ConstMemberIterator itr = document.FindMember(kTextKey); @@ -266,14 +265,14 @@ void PlatformHandler::HandleMethodCall( if (!clipboard.Open(std::get(*view_->GetRenderTarget()))) { rapidjson::Document error_code; error_code.SetInt(::GetLastError()); - result->Error(kClipboardError, "Unable to open clipboard", &error_code); + result->Error(kClipboardError, "Unable to open clipboard", error_code); return; } if (!clipboard.SetString(Utf16FromUtf8(itr->value.GetString()))) { rapidjson::Document error_code; error_code.SetInt(::GetLastError()); result->Error(kClipboardError, "Unable to set clipboard data", - &error_code); + error_code); return; } result->Success(); From b3d42fa711ba0694349cfa4afcc531c94505c6a5 Mon Sep 17 00:00:00 2001 From: Stuart Morgan Date: Wed, 19 Aug 2020 21:30:02 -0700 Subject: [PATCH 2/2] Typo fix --- .../common/cpp/client_wrapper/include/flutter/event_sink.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/platform/common/cpp/client_wrapper/include/flutter/event_sink.h b/shell/platform/common/cpp/client_wrapper/include/flutter/event_sink.h index 7b2115f1c4359..764ea9d9f1fb8 100644 --- a/shell/platform/common/cpp/client_wrapper/include/flutter/event_sink.h +++ b/shell/platform/common/cpp/client_wrapper/include/flutter/event_sink.h @@ -24,7 +24,7 @@ class EventSink { void Success(const T* event) { SuccessInternal(event); } // Consumes a successful event - void Success(const T& event) { SuccessInternal&(event); } + void Success(const T& event) { SuccessInternal(&event); } // Consumes a successful event. void Success() { SuccessInternal(nullptr); }