Skip to content
Snippets Groups Projects

Relay data between WebSocket and WebRTC connection

Merged HashikD requested to merge HashikD/snowflake-mobile:relay-data into master

closes #3 (closed)

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • added Needs Review label

  • HashikD added 1 commit

    added 1 commit

    • a05bf22b - Created new package Seriazation and moved SDPSerializer int to it, created...

    Compare with previous version

  • 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.

  • Author Maintainer

    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
Please register or sign in to reply
Loading