Skip to content
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.

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

OnWsClose を SoraSignalingObserver に追加する #130

Merged
merged 3 commits into from
Sep 27, 2024

Conversation

tnoho
Copy link
Contributor

@tnoho tnoho commented Sep 25, 2024

WebSocket が閉じられた時の code と reason を取得できるよう OnWsClose を SoraSignalingObserver に追加します

@tnoho tnoho requested review from voluntas and melpon September 25, 2024 15:45
@tnoho tnoho self-assigned this Sep 25, 2024
@tnoho tnoho changed the title WebSocket の Close を取得できるよう OnWsClose を SoraSignalingObserver に追加する OnWsClose を SoraSignalingObserver に追加する Sep 25, 2024
@@ -226,6 +227,7 @@ class SoraSignaling : public std::enable_shared_from_this<SoraSignaling>,
void SendOnSignalingMessage(SoraSignalingType type,
SoraSignalingDirection direction,
std::string message);
void SendOnWsClose(boost::beast::websocket::close_reason& reason);
Copy link
Contributor

Choose a reason for hiding this comment

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

パラメータに const あった方が良さそう

@@ -705,6 +707,7 @@ void SoraSignaling::DoInternalDisconnect(
bool ec_error = ec != boost::beast::websocket::error::closed;
if (ec_error) {
auto reason = self->ws_->reason();
self->SendOnWsClose(reason);
Copy link
Contributor

Choose a reason for hiding this comment

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

ここだと非エラーの時に SendOnWsClose が呼ばれなさそうな気がします

void SoraSignaling::SendOnWsClose(
boost::beast::websocket::close_reason& reason) {
if (auto ob = config_.observer.lock(); ob) {
ob->OnWsClose(reason.code, std::string(reason.reason.c_str()));
Copy link
Contributor

Choose a reason for hiding this comment

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

std::string で囲む必要は無さそう

@tnoho
Copy link
Contributor Author

tnoho commented Sep 27, 2024

@melpon ありがとうございます。いただいたレビューをもとに修正しました。再度ご確認いただければと思います。

Copy link
Contributor

@melpon melpon left a comment

Choose a reason for hiding this comment

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

よさそう

@tnoho
Copy link
Contributor Author

tnoho commented Sep 27, 2024

ありがとうございます!

@tnoho tnoho merged commit fa80769 into develop Sep 27, 2024
10 checks passed
@tnoho tnoho deleted the feature/add-on-ws-close branch September 27, 2024 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants