Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
44a638b
Initial commit
Kurun-pan Mar 7, 2020
395df1e
remove std::optional (C++17)
Kurun-pan Mar 8, 2020
c556c7a
Fix unit test code, bugfix.
Kurun-pan Mar 8, 2020
1000496
Fixed bug that the error of EventChannel register stream handler when…
Kurun-pan Mar 8, 2020
ae81b9d
Fix coding format.
Kurun-pan Mar 9, 2020
9069e4c
Fix unit test fail.
Kurun-pan Mar 9, 2020
8e96c4f
Fix coding format
Kurun-pan Mar 9, 2020
b8fe504
Fix build break.
Kurun-pan Mar 9, 2020
251829e
Fix code format
Kurun-pan Mar 9, 2020
4f5e051
Modified unit test scenario.
Kurun-pan Mar 9, 2020
b821d51
Fix code format
Kurun-pan Mar 9, 2020
7c5e963
Fix code format
Kurun-pan Mar 9, 2020
0b9446e
Fix code format
Kurun-pan Mar 9, 2020
6168c67
Fix build error.
Kurun-pan Mar 9, 2020
6917341
Fix code format
Kurun-pan Mar 9, 2020
f870a60
Fix BUILD.gn format
Kurun-pan Mar 9, 2020
f4f8634
Fix licenses_flutter, core_wrapper_files.gni format
Kurun-pan Mar 9, 2020
02b3a3e
Fix licenses_flutter, core_wrapper_files.gni format
Kurun-pan Mar 9, 2020
59c7146
Modify comments
Kurun-pan Mar 22, 2020
5222fc0
Fix unit test code
Kurun-pan Mar 29, 2020
50576c6
Modify comment and change from class to structure
Kurun-pan Mar 29, 2020
28cc16e
Fix for code review result
Kurun-pan Mar 31, 2020
0835785
Fix code format
Kurun-pan Mar 31, 2020
cc3f913
Fix code format
Kurun-pan Mar 31, 2020
2210369
Fix build error
Kurun-pan Mar 31, 2020
6c942dc
Fix code format
Kurun-pan Mar 31, 2020
fbaaee8
Fix code format
Kurun-pan Mar 31, 2020
6c3fb5f
Fixed the points that were pointed out.
Kurun-pan Apr 11, 2020
7f0e247
Fixed the points that were pointed out.
Kurun-pan Apr 11, 2020
a4e0caf
Fix code format
Kurun-pan Apr 11, 2020
05ce3b5
- Add error handling of StreamHandler and fix unit test code.
Kurun-pan Apr 11, 2020
c830c9b
Fix code formatting
Kurun-pan Apr 11, 2020
53077da
Merge commit 'f779f09058c695f33eaee1f4b7dcd8908ac5f195' into topic-ev…
Kurun-pan Apr 11, 2020
21d9f99
Merge commit 'fb208b486ec354dc7324e4228d9c311208174b18' into topic-ev…
Kurun-pan Apr 17, 2020
3768021
Fix build error, modify the comment and test case.
Kurun-pan Apr 17, 2020
825ca23
Add StreamHandlerError to communicate errors, and fix the points that…
Kurun-pan Apr 18, 2020
3befe52
Fix code formatting.
Kurun-pan Apr 18, 2020
8a76fba
Fix code formatting.
Kurun-pan Apr 18, 2020
5f09561
Recreated StreamHandler class with reference to MethodResult.
Kurun-pan Apr 18, 2020
4896184
Fix build error.
Kurun-pan Apr 18, 2020
41eb32f
Fix code formatting.
Kurun-pan Apr 18, 2020
dc68fbd
Fix code formatting.
Kurun-pan Apr 18, 2020
0416d5b
Fix code formatting.
Kurun-pan Apr 18, 2020
b36ae30
Fix code formatting.
Kurun-pan Apr 18, 2020
dfb1451
Fix code formatting.
Kurun-pan Apr 18, 2020
61df9ba
Modified method name and other.
Kurun-pan Apr 18, 2020
090f6bf
Modify comments.
Kurun-pan Apr 18, 2020
7905db1
Fixed some comments and unit test scenario name. Furthermore, don't u…
Kurun-pan Apr 30, 2020
f71c8a0
Merge commit '590381a00a56fdab914eeddcbcf4430b0f763ec1' into topic-ev…
Kurun-pan Apr 30, 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: 4 additions & 0 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -756,10 +756,14 @@ FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/basic_message_ch
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/byte_stream_wrappers.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/encodable_value_unittests.cc
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/engine_method_result.cc
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/event_channel_unittests.cc
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/basic_message_channel.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/binary_messenger.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/encodable_value.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/engine_method_result.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/event_channel.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/event_sink.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/event_stream_handler.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/json_message_codec.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/json_method_codec.h
FILE: ../../../flutter/shell/platform/common/cpp/client_wrapper/include/flutter/json_type.h
Expand Down
1 change: 1 addition & 0 deletions shell/platform/common/cpp/client_wrapper/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ executable("client_wrapper_unittests") {
sources = [
"basic_message_channel_unittests.cc",
"encodable_value_unittests.cc",
"event_channel_unittests.cc",
"method_call_unittests.cc",
"method_channel_unittests.cc",
"plugin_registrar_unittests.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ core_cpp_client_wrapper_includes =
"include/flutter/binary_messenger.h",
"include/flutter/encodable_value.h",
"include/flutter/engine_method_result.h",
"include/flutter/event_channel.h",
"include/flutter/event_sink.h",
"include/flutter/event_stream_handler.h",
"include/flutter/json_message_codec.h",
"include/flutter/json_method_codec.h",
"include/flutter/json_type.h",
Expand Down
113 changes: 113 additions & 0 deletions shell/platform/common/cpp/client_wrapper/event_channel_unittests.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
// 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/common/cpp/client_wrapper/include/flutter/event_channel.h"

#include <memory>
#include <string>

#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/binary_messenger.h"
#include "flutter/shell/platform/common/cpp/client_wrapper/include/flutter/standard_method_codec.h"
#include "gtest/gtest.h"

namespace flutter {

namespace {

class TestBinaryMessenger : public BinaryMessenger {
public:
void Send(const std::string& channel,
const uint8_t* message,
const size_t message_size) const override {}

void Send(const std::string& channel,
const uint8_t* message,
const size_t message_size,
BinaryReply reply) const override {}

void SetMessageHandler(const std::string& channel,
BinaryMessageHandler handler) override {
last_message_handler_channel_ = channel;
last_message_handler_ = handler;
}

std::string last_message_handler_channel() {
return last_message_handler_channel_;
}

BinaryMessageHandler last_message_handler() { return last_message_handler_; }

private:
std::string last_message_handler_channel_;
BinaryMessageHandler last_message_handler_;
};

} // namespace

// Tests that SetStreamHandler sets a handler that correctly interacts with
// the binary messenger.
TEST(EventChannelTest, Registration) {
TestBinaryMessenger messenger;
const std::string channel_name("some_channel");
const StandardMethodCodec& codec = StandardMethodCodec::GetInstance();
EventChannel channel(&messenger, channel_name, &codec);

bool on_listen_called = false;
auto onListen = [&on_listen_called](
const flutter::EncodableValue* arguments,
EventSink<flutter::EncodableValue>* event_sink) {
event_sink->Success();
auto message = flutter::EncodableValue(flutter::EncodableMap{
{flutter::EncodableValue("message"),
flutter::EncodableValue("Test from Event Channel")}});
event_sink->Success(&message);
event_sink->Error("Event Channel Error Code", "Error Message", nullptr);
event_sink->EndOfStream();
on_listen_called = true;
};

bool on_cancel_called = false;
auto onCancel =
[&on_cancel_called](const flutter::EncodableValue* arguments) {
on_cancel_called = true;
};
channel.SetStreamHandler(
flutter::StreamHandler<flutter::EncodableValue>(onListen, onCancel));

EXPECT_EQ(messenger.last_message_handler_channel(), channel_name);
EXPECT_NE(messenger.last_message_handler(), nullptr);

// Send a test message to trigger the handler test assertions.
MethodCall<EncodableValue> call("listen", nullptr);
auto message = codec.EncodeMethodCall(call);
messenger.last_message_handler()(
message->data(), message->size(),
[](const uint8_t* reply, const size_t reply_size) {});
EXPECT_EQ(on_listen_called, true);

// FIXME: add onCancel test scenario
Copy link
Contributor

Choose a reason for hiding this comment

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

Please finish implementing the tests; there is commented-out code in both test methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed unit test code.

// EXPECT_EQ(on_cancel_called, true);
}

// Tests that SetStreamHandler with a null handler unregisters the handler.
TEST(EventChannelTest, Unregistration) {
TestBinaryMessenger messenger;
const std::string channel_name("some_channel");
EventChannel<flutter::EncodableValue> channel(
&messenger, channel_name, &flutter::StandardMethodCodec::GetInstance());

auto onListen = [](const flutter::EncodableValue* arguments,
EventSink<flutter::EncodableValue>* event_sink) {};
auto onCancel = [](const flutter::EncodableValue* arguments) {};
channel.SetStreamHandler(
flutter::StreamHandler<flutter::EncodableValue>(onListen, onCancel));
EXPECT_EQ(messenger.last_message_handler_channel(), channel_name);
EXPECT_NE(messenger.last_message_handler(), nullptr);

// channel.SetStreamHandler(std::nullopt);
// EXPECT_EQ(messenger.last_message_handler_channel(), channel_name);
// EXPECT_EQ(messenger.last_message_handler(), nullptr);
}

} // namespace flutter
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
// 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.

#ifndef FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_EVENT_CHANNEL_H_
#define FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_EVENT_CHANNEL_H_

#include <iostream>
#include <string>

#include "binary_messenger.h"
#include "engine_method_result.h"
#include "event_sink.h"
#include "event_stream_handler.h"

static constexpr char kOnListenMethod[] = "listen";
Copy link
Contributor

Choose a reason for hiding this comment

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

static in a header (other than in a class) is almost always a mistake; this construction can lead to ODR violations. Since we're not allowing C++17 in the wrapper yet (where you could use inline constexpr, which would be the right way to do what you are trying to do here), you need to extern these and put the values in an implementation file.

static constexpr char kOnCancelMethod[] = "cancel";

namespace flutter {

// A named channel for communicating with the Flutter application using
// asynchronous event streams. Incoming requests for event stream setup are
// decoded from binary on receipt, and C++ responses and events are encoded into
// binary before being transmitted back to Flutter. The MethodCodec used must be
// compatible with the one used by the Flutter application. This can be achieved
// by creating an EventChannel
// ("https://docs.flutter.io/flutter/services/EventChannel-class.html")
// counterpart of this channel on the Dart side. The C++ type of stream
// configuration arguments, events, and error details is Object, but only values
// supported by the specified MethodCodec can be used.
//
// The logical identity of the channel is given by its name. Identically named
// channels will interfere with each other's communication.
template <typename T>
class EventChannel {
public:
// Creates an instance that sends and receives event handler on the channel
// named |name|, encoded with |codec| and dispatched via |messenger|.
EventChannel(BinaryMessenger* messenger,
const std::string& name,
const MethodCodec<T>* codec)
: messenger_(messenger), name_(name), codec_(codec) {}
~EventChannel() = default;

// Prevent copying.
EventChannel(EventChannel const&) = delete;
EventChannel& operator=(EventChannel const&) = delete;

// Registers a stream handler on this channel.
// If no handler has been registered, any incoming stream setup requests will
// be handled silently by providing an empty stream.
void SetStreamHandler(const StreamHandler<T>& handler) const {
// TODO: The following is required when nullptr
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this comment, since the argument is a const reference, not a pointer.

// can be passed as an argument.
// if (!handler) { /* <= available for more than C++17 */
// messenger_->SetMessageHandler(name_, nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Without this code, what is the process for removing a handler?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no way. I think again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented it.

// return;
// }

const auto* codec = codec_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not use auto when the type is not immediately inferable from the line of code.

const std::string channel_name = name_;
const auto* messenger = messenger_;
EventSinkImplementation* current_sink = current_sink_;
BinaryMessageHandler binary_handler = [handler, codec, channel_name,
current_sink, messenger](
const uint8_t* message,
const size_t message_size,
BinaryReply reply) mutable {
std::unique_ptr<MethodCall<T>> method_call =
codec->DecodeMethodCall(message, message_size);
if (!method_call) {
std::cerr << "Unable to construct method call from message on channel "
<< channel_name << std::endl;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

You aren't calling reply on this codepath, which violates the API contract.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added reply.

}

const std::string& method = method_call->method_name();
if (method.compare(kOnListenMethod) == 0) {
if (current_sink) {
std::cerr << "Failed to cancel existing stream: " << channel_name
<< std::endl;
handler.onCancel(method_call->arguments());
delete current_sink;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you doing manual memory management?

current_sink = nullptr;
}

current_sink =
new EventSinkImplementation(messenger, channel_name, codec);
handler.onListen(method_call->arguments(),
static_cast<EventSink<T>*>(current_sink));

{
auto result = codec->EncodeSuccessEnvelope();
uint8_t* buffer = new uint8_t[result->size()];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you allocating a temporary buffer and copying instead of just passing data()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No copy was needed. Delete it.

std::copy(result->begin(), result->end(), buffer);
reply(buffer, result->size());
delete[] buffer;
}
} else if (method.compare(kOnCancelMethod) == 0) {
if (current_sink) {
handler.onCancel(method_call->arguments());

auto result = codec->EncodeSuccessEnvelope();
uint8_t* buffer = new uint8_t[result->size()];
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that you are leaking buffer here, demonstrating why using new/delete manually should be avoided whenever possible. (Separately from the fact that I don't think this buffer should exist in the first place.)

Copy link
Contributor Author

@Kurun-pan Kurun-pan Mar 13, 2020

Choose a reason for hiding this comment

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

I deleted these codes.

std::copy(result->begin(), result->end(), buffer);
reply(buffer, result->size());
delete current_sink;
current_sink = nullptr;
} else {
auto result = codec->EncodeErrorEnvelope(
"error", "No active stream to cancel", nullptr);
uint8_t* buffer = new uint8_t[result->size()];
std::copy(result->begin(), result->end(), buffer);
reply(buffer, result->size());
delete[] buffer;
}
} else {
std::cerr
Copy link
Contributor

Choose a reason for hiding this comment

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

Logging unhandled methods should be handled on the Flutter side already, so this shouldn't be needed.

<< "Unknown event channel method call from message on channel: "
<< channel_name << std::endl;
reply(nullptr, 0);
if (current_sink) {
delete current_sink;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does an unknown method destroy the sink? That doesn't seem to be the behavior of the ObjC implementation, for instance.

current_sink = nullptr;
}
}
};
messenger_->SetMessageHandler(name_, std::move(binary_handler));
}

private:
class EventSinkImplementation : public EventSink<T> {
public:
// Creates an instance that EventSink send event on the channel
// named |name|, encoded with |codec| and dispatched via |messenger|.
EventSinkImplementation(const BinaryMessenger* messenger,
const std::string& name,
const MethodCodec<T>* codec)
: messenger_(messenger), name_(name), codec_(codec) {}
~EventSinkImplementation() = default;

// Prevent copying.
EventSinkImplementation(EventSinkImplementation const&) = delete;
EventSinkImplementation& operator=(EventSinkImplementation const&) = delete;

private:
const BinaryMessenger* messenger_;
const std::string name_;
const MethodCodec<T>* codec_;

protected:
void SuccessInternal(T* event = nullptr) override {
auto result = codec_->EncodeSuccessEnvelope(event);
uint8_t* buffer = new uint8_t[result->size()];
std::copy(result->begin(), result->end(), buffer);
messenger_->Send(name_, buffer, result->size());
delete[] buffer;
}

void ErrorInternal(const std::string& error_code,
const std::string& error_message,
T* error_details) override {
auto result =
codec_->EncodeErrorEnvelope(error_code, error_message, error_details);
uint8_t* buffer = new uint8_t[result->size()];
std::copy(result->begin(), result->end(), buffer);
messenger_->Send(name_, buffer, result->size());
delete[] buffer;
}

void EndOfStreamInternal() override { messenger_->Send(name_, nullptr, 0); }
};

BinaryMessenger* messenger_;
std::string name_;
const MethodCodec<T>* codec_;
EventSinkImplementation* current_sink_ = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is never written to, so I'm confused as to what its purpose is.

};

} // namespace flutter

#endif // FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_EVENT_CHANNEL_H_
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// 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.

#ifndef FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_EVENT_SINK_H_
#define FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_EVENT_SINK_H_

namespace flutter {

// Event callback. Supports dual use: Producers of events to be sent to Flutter
Copy link
Contributor

Choose a reason for hiding this comment

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

Being both called and implemented isn't really "dual use"; that's what an interface is.

Copy link
Contributor Author

@Kurun-pan Kurun-pan Mar 13, 2020

Choose a reason for hiding this comment

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

I Modified comment.

// act as clients of this interface for sending events. Consumers of events sent
// from Flutter implement this interface for handling received events.
template <typename T>
class EventSink {
public:
EventSink() = default;
virtual ~EventSink() = default;

// Prevent copying.
EventSink(EventSink const&) = delete;
EventSink& operator=(EventSink const&) = delete;

// Consumes end of stream. Ensuing calls to Success(T) or
// Error(String, String, Object)}, if any, are ignored.
void EndOfStream() { EndOfStreamInternal(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: it seems like it would be clear to make this the last method, rather than the first.


// Consumes a successful event.
void Success(T* event = nullptr) { SuccessInternal(event); }

// Consumes an error event.
void Error(const std::string& error_code,
const std::string& error_message = "",
T* error_details = nullptr) {
ErrorInternal(error_code, error_message, error_details);
}

protected:
// Implementation of the public interface, to be provided by subclasses.
virtual void EndOfStreamInternal() = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.


// Implementation of the public interface, to be provided by subclasses.
virtual void SuccessInternal(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;
};

} // namespace flutter

#endif // FLUTTER_SHELL_PLATFORM_COMMON_CPP_CLIENT_WRAPPER_INCLUDE_FLUTTER_EVENT_SINK_H_
Loading