Relay data between WebSocket and WebRTC connection
closes #3 (closed)
Merge request reports
Activity
added Needs Review label
added 1 commit
- a05bf22b - Created new package Seriazation and moved SDPSerializer int to it, created...
Woohoo! I also got it to bootstrap to 100% :D
This all looks really good! My only piece of feedback is that the
MyPersistentService
class is getting very large.Also, the way this is written, we'll only be able to accommodate one client at a time, since there is only one websocket connection for the entire persistent service (another limiting factor is the
isConnectionAlive
boolean we introduced earlier). I think this is okay. I'm not sure a cell connection is something that can handle more than one client anyway (we have the option for supporting more than one client on the browser-based proxies right now but just choose not to). Given the fact that we already have some limiting constraints, I don't want to make the change now. We can leave it for a refactor ticket later.Other than that I think we're in really good shape with the overall structure of the code.
This all looks really good! My only piece of feedback is that the
MyPersistentService
class is getting very large.This was on my mind too; I'll look into this.
Also, the way this is written, we'll only be able to accommodate one client at a time, since there is only one WebSocket connection for the entire persistent service (another limiting factor is the
isConnectionAlive
boolean we introduced earlier). I think this is okay. I'm not sure a cell connection is something that can handle more than one client anyway (we have the option for supporting more than one client on the browser-based proxies right now but just choose not to). Given the fact that we already have some limiting constraints, I don't want to make the change now. We can leave it for a refactor ticket later.That's really good input Cecylia! I haven't thought of cellular connection earlier. While starting, the plan was to accommodate multiple clients later on in the project. But, if I may remind, I promptly addressed this earlier, the batter life is a much major issue on mobile phones. But, I agree, let's keep this option open and discuss it as a future improvement of the project.
We can still manage multiple clients' code; there certainly are some variables that are in global scope rather than a thread. It shouldn't be a problem, we can move them into if we want to handle multiple clients.
Edited by HashikD