8000 Change datachannel message type identifier from a string to an enum, and remove it altogether from the send method and message listener. by FrobtheBuilder · Pull Request #88 · flutter-webrtc/flutter-webrtc · GitHub
[go: up one dir, main page]
Skip to content

Change datachannel message type identifier from a string to an enum, and remove it altogether from the send method and message listener. #88

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

Jump to bottom
Merged
merged 4 commits into from
May 14, 2019
Merged

Change datachannel message type identifier from a string to an enum, and remove it altogether from the send method and message listener. #88

merged 4 commits into from
May 14, 2019

Conversation

FrobtheBuilder
Copy link
Contributor
@FrobtheBuilder FrobtheBuilder commented May 11, 2019 •
edited
Loading

Map lookups should be faster than string comparisons, so this will likely improve performance.
This will change the API (again) but it will also make it safer, since the compiler will notify you if you mistype the identifier as an enum.

I also did some slight reformatting elsewhere in the file.

@FrobtheBuilder
Change message type identifier from a string to an enum. Map lookups …
c7011ae 
…should be faster than string comparisons, so this will likely improve performance.
@cloudwebrtc
Copy link
Member

@FrobtheBuilder @rostopira @Codeusa
I am wondering if we can remove the enumeration type from the interface because the RTCDataChannelMessage class has internally defined the type to binary or text.
Then the send method only uses the unique parameter message, and the native sdk provides only text and binary types, so I only need to determine the type when constructing the DataBuffer.

Future<void> send(RTCDataChannelMessage message) async {
    ...
}

For these we can refer to WebRTC native:
https://chromium.googlesource.com/external/webrtc/+/branch-heads/m74/sdk/objc/api/peerconnection/RTCDataChannel.h#126

https://chromium.googlesource.com/external/webrtc/+/branch-heads/m74/sdk/android/api/org/webrtc/DataChannel.java#160

Another change is that the base64 type I think can be removed, because base64 is essentially a string, just encode before the transfer, decode it after receiving it, now we have the binary type, base64 is unnecessary.

@andrewmd5
Copy link
andrewmd5 commented May 11, 2019 •
edited
Loading

It does seem redundant to put the message type on the interface itself. So long as the type is set via a map internally on the DataBuffer/RTCDataChannelMessage it should be efficient. String comparisons are indeed slow though so the idea of this change is good.

The usefulness of the base64 type IMO is that data channels do support arbitrary strings, and base64 is a work around for the binary data on Android for now. So there needs to be a way to tell the two apart until the Android side is brought to spec and just has binary data and normal strings.

@FrobtheBuilder
Copy link
Contributor Author

@Codeusa and @cloudwebrtc, I normalized the API between Android and iOS to encode and decode transparently already. That was in the last PR. But yes, it would be possible to just determine which type of message is being sent from the message itself. Would you rather I reconfigure it to do it that way instead?

@FrobtheBuilder
Copy link
Contributor Author

Also, the reason I have those base64 constructors and properties on RTCDataChannelMessage is to assist migration from the old api, which required base64 encoding and decoding on the client side, as well as to make the logic cleaner for doing that transparent encoding and decoding in the first place. I would prefer to keep those in place for the time being.

@FrobtheBuilder
Go ahead and remove the MessageType from the send method and the onMe…
95250d6 
…ssage handler. You can determine what kind of message is being sent and received from the RTCDataChannelMessage's type.
@FrobtheBuilder
Copy link
Contributor Author

I just went ahead and made the change. MessageType is still exposed because the RTCDataChannel's type property returns it. I think that's reasonable.

@FrobtheBuilder FrobtheBuilder changed the title Change message type identifier from a string to an enum. Change message type identifier from a string to an enum, and remove it altogether from the send method and message listener. May 13, 2019
@FrobtheBuilder FrobtheBuilder changed the title Change message type identifier from a string to an enum, and remove it altogether from the send method and message listener. Change datachannel message type identifier from a string to an enum, and remove it altogether from the send method and message listener. May 13, 2019
@cloudwebrtc cloudwebrtc merged commit 6672e47 into flutter-webrtc:master May 14, 2019
cloudwebrtc added a commit that referenced this pull request Jul 4, 2020
@cloudwebrtc
Merge pull request #88 from RainwayApp/enum
370601d 
Change datachannel message type identifier from a string to an enum, and remove it altogether from the send method and message listener.
evdokimovs pushed a commit to evdokimovs/flutter-webrtc that referenced this pull request Jun 6, 2024
@rogurotus @alexlapa @tyranron
Bootstrap getStats() for desktop platforms (flutter-webrtc#88)
346b233 
Co-authored-by: alexlapa <lapa.alex@ex.ua>
Co-authored-by: Kai Ren <tyranron@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@FrobtheBuilder @cloudwebrtc @andrewmd5
0