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

#692 Add the possibility to specify "Sec-WebSocket-Protocol" #693

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

KaSSaaaa
Copy link

I took the liberty to add some documentation as well. Feel free if you have any feedback :)

include/crow/websocket.h Outdated Show resolved Hide resolved
@MichaelSB
Copy link
Contributor

MichaelSB commented Jul 29, 2023

I could use your split function in my websocket compression PR, if yours is merged first :D

@KaSSaaaa
Copy link
Author

I rewrote find_first_of algorithm and put it in utility.h :)

@MichaelSB
Copy link
Contributor

If I don't miss it, I think a getter is needed for the selected subprotocol in the connection. Otherwise looks good to me.

@KaSSaaaa
Copy link
Author

Currently, the "subprotocol_" is only a private variable and is only used in the Connection::start but I can add a getter in the interface and implementation if it's needed.

@MichaelSB
Copy link
Contributor

I think that's it :) I asked for the getter, because users must be able to know which subprotocol was selected and probably switch code paths accordingly.

include/crow/routing.h Outdated Show resolved Hide resolved
include/crow/websocket.h Outdated Show resolved Hide resolved
@gittiver
Copy link
Member

build fails on ubuntu at least, other os will run later.

@KaSSaaaa
Copy link
Author

Apparently missed the include of for utility::find_first_of ...

@gittiver gittiver added the feature Code based project improvement label Jan 30, 2024
@@ -721,6 +746,7 @@ namespace crow
uint16_t remaining_length16_{0};
uint64_t remaining_length_{0};
uint64_t max_payload_bytes_{UINT64_MAX};
std::string subprotocol_;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe more specific name as it is only connected with one header?

@mrozigor
Copy link
Member

mrozigor commented Mar 9, 2024

May I ask you also to add unit tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Code based project improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants