search

gimite / web-socket-js

HTML5 Web Socket implementation powered by Flash
BSD 3-Clause "New" or "Revised" License
2.73k stars 489 forks source link

Replaced FABridge with ExternalInterface, pushed communication polling into Flash #57

Closed wtritch closed 13 years ago

wtritch commented 13 years ago

Hello gimite, I hope you like something/anything about these changes, I'm afraid this is my first entry into the opensource world, so I'm a little unversed about protocol and the like. I got your comments on Friday and you were totally right about modifying another 3rd-party plugin within your project, I reverted those changes to the Base64 class. For the polling, I saw the queueing going on in WebSocket, but in the js it looked like only IE and Opera were set to poll for events? I may have missed something... A quick overview of the changes: I removed FABridge, which means there had to be a controller put in place to manage multiple WebSocket instances on both the javascript and flash side. With Flash, I just modified the main application class to keep a list of web sockets. With Javascript I created a WebSocketController class and a single wsController instance that manage the web sockets. The web socket identifier is just a simple incrementing int. I'm pushing all flash/js communications on the flash side through the bridge/JSBridge class, just as a place to keep all of the ExternalInterface communications together. I added in some setTimeouts to keep roundtrip communication problems from cropping up. ...I think that's about it.

I'd like to hear your thoughts/criticisms, wtritch

simb commented 13 years ago

I really like where this is going. I think the changes really simplify the over all layout of the library. There are a couple things I'd like to see in addition to these changes.

  1. By having the classes in the root package you are just asking for naming conflicts. Its like spamming global, and for Actionscript its bad practice.
  2. I would really like to see a looser coupling between WebSocket.as and WebSocketMain.as. I think that websocket should dispatch events for log error and fatal, and we can pass in callerURL. Which would make it much easier to write some unit tests for WebSocket.as.

I am not sure where else to address these questions so since I like the movement wtritch has made I figure this will do.

gimite commented 13 years ago

Thanks for the patch. Overall it looks good. I pulled the change with some further cleanup.

I added in some setTimeouts to keep roundtrip communication problems from cropping up.

I removed some of them which looks unnecessary. Please let me know if some of them are actually required.