Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Use references for C++ MethodResult and EventSink #20651

Merged
merged 2 commits into from
Aug 20, 2020
Merged

Use references for C++ MethodResult and EventSink #20651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2131841
Use references for C++ MethodResult and EventSink
stuartmorgan-g Aug 20, 2020
b3d42fa
Typo fix
stuartmorgan-g Aug 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions shell/platform/common/cpp/client_wrapper/include/flutter/event_channel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,14 @@ class EventChannel {
const MethodCodec<T>* 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());
Expand Down
33 changes: 27 additions & 6 deletions shell/platform/common/cpp/client_wrapper/include/flutter/event_sink.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@gspencergoog gspencergoog Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we do the C++14 deprecated annotation thing? Probably not, since warnings-are-errors, but it seems useful.

i.e.:

  [[deprecated("Use the reference version instead.")]]
  void Error(const std::string& error_code,
             const std::string& error_message,
             const T* error_details) {
    ErrorInternal(error_code, error_message, error_details);
  }

Copy link
Contributor Author

@stuartmorgan-g stuartmorgan-g Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, since warnings-are-errors

Exactly. I tried that the last time I did one of these and realized it would just make it an immediate breaking change, which is unfortunate.

In practice, since Windows is still subject to change without warning, it's not a big deal (I'm doing it in stages so I can not break FDE and google3 without having a chance to pre-fix them), but long term it would be nice if we could use them to do soft breaking changes. It's a potential argument for relaxing the template to not treat warnings as errors.

// 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;
Expand Down
46 changes: 37 additions & 9 deletions shell/platform/common/cpp/client_wrapper/include/flutter/method_result.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(); }
Expand Down
7 changes: 3 additions & 4 deletions shell/platform/common/cpp/client_wrapper/method_result_functions_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 <functional>
#include <string>

#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/method_result_functions.h"
Copy link
Contributor

@gspencergoog gspencergoog Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that the header corresponding to the cc file always came first... Am I just wrong? Is it supposed to come after the system headers like this? Or is that only in the non-test files?

Copy link
Contributor Author

@stuartmorgan-g stuartmorgan-g Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should; the style guide is explicit about this. But for reasons that aren't clear to me, clang-format in the engine repo keeps moving the header down in test files.

I keep meaning to look into it, then forgetting. I'll file a bug. Maybe it only understands _test, and not _unittests? But I swear it used to leave it alone.

#include "gtest/gtest.h"

namespace flutter {
Expand All @@ -29,7 +28,7 @@ TEST(MethodChannelTest, Success) {
EXPECT_EQ(*i, value);
},
nullptr, nullptr);
result.Success(&value);
result.Success(value);
EXPECT_TRUE(called);
}

Expand All @@ -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);
}

Expand Down
26 changes: 19 additions & 7 deletions shell/platform/common/cpp/client_wrapper/standard_codec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,21 +522,33 @@ 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: {
EncodableValue code = serializer_->ReadValue(&stream);
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<std::string>(code),
message.IsNull() ? "" : std::get<std::string>(message),
details.IsNull() ? nullptr : &details);
const std::string& message_string =
message.IsNull() ? "" : std::get<std::string>(message);
if (details.IsNull()) {
result->Error(std::get<std::string>(code), message_string);
} else {
result->Error(std::get<std::string>(code), message_string, details);
}
#endif
return true;
}
Expand Down
12 changes: 10 additions & 2 deletions shell/platform/common/cpp/json_method_codec.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,15 +131,23 @@ bool JsonMethodCodec::DecodeAndProcessResponseEnvelopeInternal(
case 1: {
std::unique_ptr<rapidjson::Document> 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: {
std::string code = (*json_response)[0].GetString();
std::string message = (*json_response)[1].GetString();
std::unique_ptr<rapidjson::Document> 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:
Expand Down
13 changes: 6 additions & 7 deletions shell/platform/windows/win32_platform_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,20 +228,19 @@ void PlatformHandler::HandleMethodCall(
if (!clipboard.Open(std::get<HWND>(*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<std::wstring> clipboard_string = clipboard.GetString();
if (!clipboard_string) {
rapidjson::Document error_code;
error_code.SetInt(::GetLastError());
result->Error(kClipboardError, "Unable to get clipboard data",
&error_code);
error_code);
return;
}

Expand All @@ -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);
Expand All @@ -266,14 +265,14 @@ void PlatformHandler::HandleMethodCall(
if (!clipboard.Open(std::get<HWND>(*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();
Expand Down