Skip to content

Remove the app_control reply channel #183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 15, 2021
Merged

Remove the app_control reply channel #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3543f93
Remove the app_control reply channel
swift-kim Sep 10, 2021
cad4577
Convert shared_ptr to unique_ptr
swift-kim Sep 12, 2021
f3011db
Fix memory leaks
swift-kim Sep 12, 2021
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
1 change: 1 addition & 0 deletions shell/platform/tizen/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ template("embedder") {
"//flutter/shell/platform/common:common_cpp_input",
"//flutter/shell/platform/common:common_cpp_library_headers",
"//flutter/shell/platform/common/client_wrapper:client_wrapper",
"//third_party/dart/runtime:dart_api",
"//third_party/rapidjson",
]
}
Expand Down
119 changes: 77 additions & 42 deletions shell/platform/tizen/channels/app_control.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,69 @@
#include "app_control.h"

#include "flutter/shell/platform/tizen/channels/app_control_channel.h"
#include "flutter/shell/platform/tizen/logger.h"

intptr_t NativeInitializeDartApi(void* data) {
return Dart_InitializeApiDL(data);
}

int32_t NativeCreateAppControl(Dart_Handle handle) {
auto app_control = std::make_unique<flutter::AppControl>();
if (!app_control->handle()) {
return -1;
}
auto id = app_control->id();
Dart_NewFinalizableHandle_DL(
handle, app_control.get(), 64,
Copy link
Member

@bwikbs bwikbs Sep 13, 2021

Choose a reason for hiding this comment

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

Why is it 64? (Just curious.. 😄 )

Copy link
Member Author

@swift-kim swift-kim Sep 13, 2021

Choose a reason for hiding this comment

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

The value is a hint for the garbage collector as far as the documentation says. Increasing the size may increase the chance of finalizable objects to be collected.

As a side note, the general finalizer support will be added to the Dart language in the near future so this (finalization through FFI) is a temporary solution until then.

[](void* isolate_callback_data, void* peer) {
auto app_control = reinterpret_cast<flutter::AppControl*>(peer);
flutter::AppControlManager::GetInstance().Remove(app_control->id());
});
flutter::AppControlManager::GetInstance().Insert(std::move(app_control));
return id;
}

bool NativeAttachAppControl(int32_t id, Dart_Handle handle) {
auto app_control = flutter::AppControlManager::GetInstance().FindById(id);
if (!app_control || !app_control->handle()) {
return false;
}
Dart_NewFinalizableHandle_DL(
handle, app_control, 64, [](void* isolate_callback_data, void* peer) {
auto app_control = reinterpret_cast<flutter::AppControl*>(peer);
flutter::AppControlManager::GetInstance().Remove(app_control->id());
});
return true;
}

namespace flutter {

int AppControl::next_id_ = 0;
int32_t AppControl::next_id_ = 0;

AppControl::AppControl(app_control_h app_control) : id_(next_id_++) {
handle_ = app_control;
AppControl::AppControl() : id_(next_id_++) {
app_control_h handle = nullptr;
AppControlResult ret = app_control_create(&handle);
if (!ret) {
FT_LOG(Error) << "app_control_create() failed: " << ret.message();
return;
}
handle_ = handle;
}

AppControl::AppControl(app_control_h handle) : id_(next_id_++) {
app_control_h clone = nullptr;
AppControlResult ret = app_control_clone(&clone, handle);
if (!ret) {
FT_LOG(Error) << "app_control_clone() failed: " << ret.message();
return;
}
handle_ = clone;
}

AppControl::~AppControl() {
app_control_destroy(handle_);
if (handle_) {
app_control_destroy(handle_);
}
}

AppControlResult AppControl::GetString(std::string& str,
Expand Down Expand Up @@ -109,14 +161,6 @@ AppControlResult AppControl::SetExtraData(const EncodableMap& map) {
return AppControlResult();
}

void AppControl::SetManager(AppControlChannel* manager) {
manager_ = manager;
}

AppControlChannel* AppControl::GetManager() {
return manager_;
}

AppControlResult AppControl::GetOperation(std::string& operation) {
return GetString(operation, app_control_get_operation);
}
Expand Down Expand Up @@ -183,6 +227,12 @@ AppControlResult AppControl::SetLaunchMode(const std::string& launch_mode) {
return AppControlResult(ret);
}

bool AppControl::IsReplyRequested() {
bool requested = false;
app_control_is_reply_requested(handle_, &requested);
return requested;
}

EncodableValue AppControl::SerializeAppControlToMap() {
std::string app_id, operation, mime, category, uri, caller_id, launch_mode;
AppControlResult results[7];
Expand All @@ -202,43 +252,31 @@ EncodableValue AppControl::SerializeAppControlToMap() {
}
}
EncodableMap map;
map[EncodableValue("id")] = EncodableValue(GetId());
map[EncodableValue("id")] = EncodableValue(id());
map[EncodableValue("appId")] = EncodableValue(app_id);
map[EncodableValue("operation")] = EncodableValue(operation);
map[EncodableValue("mime")] = EncodableValue(mime);
map[EncodableValue("category")] = EncodableValue(category);
map[EncodableValue("uri")] = EncodableValue(uri);
map[EncodableValue("callerId")] = EncodableValue(caller_id);
map[EncodableValue("callerAppId")] = EncodableValue(caller_id);
map[EncodableValue("launchMode")] = EncodableValue(launch_mode);
map[EncodableValue("extraData")] = EncodableValue(extra_data);
map[EncodableValue("shouldReply")] = EncodableValue(IsReplyRequested());

return EncodableValue(map);
}

AppControlResult AppControl::SendLaunchRequest() {
AppControlResult ret =
app_control_send_launch_request(handle_, nullptr, nullptr);
return ret;
return app_control_send_launch_request(handle_, nullptr, nullptr);
}

AppControlResult AppControl::SendLaunchRequestWithReply(
std::shared_ptr<EventSink<EncodableValue>> reply_sink,
AppControlChannel* manager) {
SetManager(manager);
auto on_reply = [](app_control_h request, app_control_h reply,
app_control_result_e result, void* user_data) {
ReplyCallback on_reply) {
auto reply_callback = [](app_control_h request, app_control_h reply,
app_control_result_e result, void* user_data) {
AppControl* app_control = static_cast<AppControl*>(user_data);
app_control_h clone = nullptr;
AppControlResult ret = app_control_clone(&clone, reply);
if (!ret) {
FT_LOG(Error) << "Could not clone app_control: " << ret.message();
return;
}

std::shared_ptr<AppControl> app_control_reply =
std::make_shared<AppControl>(clone);
auto app_control_reply = std::make_unique<AppControl>(reply);
EncodableMap map;
map[EncodableValue("id")] = EncodableValue(app_control->GetId());
map[EncodableValue("reply")] =
app_control_reply->SerializeAppControlToMap();
if (result == APP_CONTROL_RESULT_APP_STARTED) {
Expand All @@ -250,23 +288,20 @@ AppControlResult AppControl::SendLaunchRequestWithReply(
} else if (result == APP_CONTROL_RESULT_CANCELED) {
map[EncodableValue("result")] = EncodableValue("canceled");
}

app_control->reply_sink_->Success(EncodableValue(map));
app_control->GetManager()->AddExistingAppControl(
std::move(app_control_reply));
app_control->on_reply_(EncodableValue(map));
app_control->on_reply_ = nullptr;
AppControlManager::GetInstance().Insert(std::move(app_control_reply));
};
reply_sink_ = std::move(reply_sink);
AppControlResult ret =
app_control_send_launch_request(handle_, on_reply, this);
return ret;
on_reply_ = on_reply;
return app_control_send_launch_request(handle_, reply_callback, this);
}

AppControlResult AppControl::SendTerminateRequest() {
AppControlResult ret = app_control_send_terminate_request(handle_);
return ret;
}

AppControlResult AppControl::Reply(std::shared_ptr<AppControl> reply,
AppControlResult AppControl::Reply(AppControl* reply,
const std::string& result) {
app_control_result_e result_e;
if (result == "appStarted") {
Expand All @@ -281,7 +316,7 @@ AppControlResult AppControl::Reply(std::shared_ptr<AppControl> reply,
return AppControlResult(APP_CONTROL_ERROR_INVALID_PARAMETER);
}
AppControlResult ret = app_control_reply_to_launch_request(
reply->Handle(), this->handle_, result_e);
reply->handle(), this->handle_, result_e);
Copy link

@WonyoungChoi WonyoungChoi Sep 13, 2021

Choose a reason for hiding this comment

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

Is there no need the null checking for AppControl*? other functions have same issues.

Copy link
Member Author

@swift-kim swift-kim Sep 13, 2021

Choose a reason for hiding this comment

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

https://github.com/swift-kim/plugins/blob/ffe8d2536693eb1d222cb354b5b958813662f654/packages/tizen_app_control/lib/app_control.dart#L64-L93

An exception will be thrown in the Dart side constructors in case of instantiation failure so it cannot be nullptr as far as I can see (although it can be "invalid" if the destructor already has been called. The platform will return error if the app_control handle is invalid.)

Also I'll closely review the overall implementation in a follow-up PR. I tried not to make many changes to the existing code in this PR.

Copy link

@WonyoungChoi WonyoungChoi Sep 13, 2021

Choose a reason for hiding this comment

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

ok I understand.

return ret;
}

Expand Down
87 changes: 67 additions & 20 deletions shell/platform/tizen/channels/app_control.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,34 @@

#include <app.h>

#include <unordered_map>

#include "flutter/shell/platform/common/client_wrapper/include/flutter/encodable_value.h"
#include "flutter/shell/platform/common/client_wrapper/include/flutter/event_channel.h"
#include "flutter/shell/platform/tizen/logger.h"
#include "flutter/shell/platform/common/public/flutter_export.h"
#include "third_party/dart/runtime/include/dart_api_dl.h"

// Called by Dart code through FFI to initialize dart_api_dl.h.
DART_EXPORT FLUTTER_EXPORT intptr_t NativeInitializeDartApi(void* data);

// Creates an internally managed instance of AppControl and associates with
// |handle|.
//
// A finalizer is attached to the created instance and invoked when the
// associated |handle| is disposed by GC.
//
// Returns a unique AppControl ID on success, otherwise -1.
DART_EXPORT FLUTTER_EXPORT int32_t NativeCreateAppControl(Dart_Handle handle);

// Finds an instance of AppControl with |id| and associates with |handle|.
//
// A finalizer is attached to the instance and invoked when the associated
// |handle| is disposed by GC.
//
// Returns false if an instance of AppControl with the given |id| could not
// be found, otherwise true.
DART_EXPORT FLUTTER_EXPORT bool NativeAttachAppControl(int32_t id,
Dart_Handle handle);

namespace flutter {

Expand All @@ -25,15 +50,16 @@ struct AppControlResult {
int error_code;
};

class AppControlChannel;

class AppControl {
public:
AppControl(app_control_h app_control);
~AppControl();
using ReplyCallback = std::function<void(const EncodableValue& response)>;

explicit AppControl();
explicit AppControl(app_control_h handle);
virtual ~AppControl();

int GetId() { return id_; }
app_control_h Handle() { return handle_; }
int32_t id() { return id_; }
app_control_h handle() { return handle_; }

AppControlResult GetOperation(std::string& operation);
AppControlResult SetOperation(const std::string& operation);
Expand All @@ -48,24 +74,19 @@ class AppControl {
AppControlResult GetCaller(std::string& caller);
AppControlResult GetLaunchMode(std::string& launch_mode);
AppControlResult SetLaunchMode(const std::string& launch_mode);
bool IsReplyRequested();

EncodableValue SerializeAppControlToMap();

AppControlResult SendLaunchRequest();
AppControlResult SendLaunchRequestWithReply(
std::shared_ptr<EventSink<EncodableValue>> reply_sink,
AppControlChannel* manager);
AppControlResult SendLaunchRequestWithReply(ReplyCallback on_reply);
AppControlResult SendTerminateRequest();

AppControlResult Reply(std::shared_ptr<AppControl> reply,
const std::string& result);
AppControlResult Reply(AppControl* reply, const std::string& result);

AppControlResult GetExtraData(EncodableMap& value);
AppControlResult SetExtraData(const EncodableMap& value);

void SetManager(AppControlChannel* manager);
AppControlChannel* GetManager();

private:
AppControlResult GetString(std::string& str, int func(app_control_h, char**));
AppControlResult SetString(const std::string& str,
Expand All @@ -74,12 +95,38 @@ class AppControl {
AppControlResult AddExtraData(std::string key, EncodableValue value);
AppControlResult AddExtraDataList(std::string& key, EncodableList& list);

app_control_h handle_;
int id_;
static int next_id_;
std::shared_ptr<EventSink<EncodableValue>> reply_sink_;
app_control_h handle_ = nullptr;
int32_t id_;
static int32_t next_id_;
ReplyCallback on_reply_ = nullptr;
};

class AppControlManager {
public:
// Returns an instance of this class.
static AppControlManager& GetInstance() {
static AppControlManager instance;
return instance;
}

void Insert(std::unique_ptr<AppControl> app_control) {
map_.insert({app_control->id(), std::move(app_control)});
}

void Remove(int32_t id) { map_.erase(id); }

AppControl* FindById(const int32_t id) {
if (map_.find(id) == map_.end()) {
return nullptr;
}
return map_[id].get();
}

private:
explicit AppControlManager() {}
~AppControlManager() {}

AppControlChannel* manager_;
std::unordered_map<int32_t, std::unique_ptr<AppControl>> map_;
};

} // namespace flutter
Expand Down
Loading