----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2533/#review2769 -----------------------------------------------------------
Ship it! The third_party/runtime/socketio/README.google should be updated (either to state that this is a modified version –and maybe put the patch in the same dir– or if socketio-java integrates the patch to update the "Version" field). The third_party/runtime/jetty/README.google should be updated too with the new version of Jetty. I also see an (easy to fix) warning when using "ant run-server": > System Property [org.eclipse.jetty.util.log.DEBUG] has been deprecated! (Use > org.eclipse.jetty.LEVEL=DEBUG instead) Finally, it does not seem to work with IE (tried with IE8, IE9-in-IE8-mode, and IE9-in-IE7-mode –didn't work at all in IE9 mode–), while it works fine in Chrome 16-dev and Firefox 7.0.1. There's also an issue at sign-in: when hitting "sign in", you're redirected to .../;jsession=XXX which displays a directory listing for the root (i.e. static/ and webclient/), removing the ";jsession=XXX" makes it work, but I don't know if it's due to the move to 8.0.0 or not (didn't try before applying the patch). /.classpath <https://reviews.apache.org/r/2533/#comment6212> Any reason you didn't upgrade to 8.0.3.v20111011? It implements WebSocket draft-14, but it's backwards compatible with draft-13 (and Web-Socket-Version is still 13). I checked later drafts (15, 16 and 17) and they only contain rewordings and small fixes; including a change saying that that Web-Socket-Version: 13 corresponds to "RFCXXXX" (rather than draft-13); so it's safe to upgrade to a newer "draft", even if Chrome only implements draft-13. I tried with 8.0.3 binaries and had the exact same behavior as with 8.0.0. /build.xml <https://reviews.apache.org/r/2533/#comment6204> Why is this needed? Does not seem to be related to the proposed changes (and works the same if omitted). /build.xml <https://reviews.apache.org/r/2533/#comment6205> I don't understand why it's needed, but it seems to be required for changes to appear live on other browsers connected to the same Wave. /build.xml <https://reviews.apache.org/r/2533/#comment6206> Same here. /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java <https://reviews.apache.org/r/2533/#comment6211> Rename to 'connection'? /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java <https://reviews.apache.org/r/2533/#comment6207> The javadoc should be updated. /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java <https://reviews.apache.org/r/2533/#comment6208> Other parameters were javadoc'd, I don't know enough about the Wave coding styles to tell whether these should be added to the javadoc or not. /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java <https://reviews.apache.org/r/2533/#comment6210> Should we log the closeCode and message? (do you know what the message represents?) /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java <https://reviews.apache.org/r/2533/#comment6209> Not sure this comment still applies. /src/org/waveprotocol/box/webclient/client/WebClient.java <https://reviews.apache.org/r/2533/#comment6213> Shouldn't we use window.WebSocket instead? Doesn't a bare "WebSocket" generate an error if it doesn't exist? (note: because com.google.gwt.websockets.client.WebSocket uses "new WebSocket", the code here should use window.WebSocket, not $wnd.WebSocket; just in case $wnd contains a polyfill; FYI, GWT had issues with XMLHttpRequest and the Sarissa polyfill) BTW, socket.io's test seems much more robust, and in recent versions account for MozWebSocket (which maybe we should support in com.google.gwt.websockets.*, but that's another issue). - Thomas On 2011-10-21 22:57:37, vjrj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2533/ > ----------------------------------------------------------- > > (Updated 2011-10-21 22:57:37) > > > Review request for wave, Yuri Zelikov, Tad Glines, soren, and Nelson Silva. > > > Summary > ------- > > New chrome (14, 15) use the new websocket specification that is not support > by socketio-java (that uses socket-io.js 0.6). > > As a temporal solution (while the websocket stuff is more stable) this patch: > - Upgrades WIAB to use a 8 jetty version > - Upgrades socketio-java to use the same jetty version > - Enables both socket and socket-io in client side then if client supports > websocket it will connect via jetty and if not, it will use socket-io-java. > > For now, I did not removed use_socketio server parameter to make this patch > more clear. > > Yuri, can you add more reviewers? because I don't know the new usernames here. > > The binaries: > http://homes.ourproject.org/~vjrj/otros/patch-socketio.tgz > > > Diffs > ----- > > /.classpath 1187538 > /build.xml 1187538 > /src/org/waveprotocol/box/server/robots/agent/passwd/PasswordRobot.java > 1187538 > /src/org/waveprotocol/box/server/rpc/ServerRpcProvider.java 1187538 > /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java 1187538 > /src/org/waveprotocol/box/webclient/client/WebClient.java 1187538 > /third_party/runtime/jetty/jetty-client-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-client-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-continuation-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-continuation-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-http-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-http-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-io-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-io-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-security-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-security-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-server-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-server-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-servlet-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-servlet-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-servlets-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-servlets-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-util-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-util-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-webapp-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-webapp-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-websocket-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-websocket-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/jetty-xml-7.1.6.v20100715.jar UNKNOWN > /third_party/runtime/jetty/jetty-xml-8.0.0.v20110901.jar UNKNOWN > /third_party/runtime/jetty/servlet-api-2.5.jar UNKNOWN > /third_party/runtime/jetty/servlet-api-3.0.jar UNKNOWN > /third_party/runtime/socketio/socketio-java-src.jar UNKNOWN > /third_party/runtime/socketio/socketio-java.jar UNKNOWN > > Diff: https://reviews.apache.org/r/2533/diff > > > Testing > ------- > > To connect to WIAB correctly using firefox 7.0.1 and chormmium (15). > > > Thanks, > > vjrj > >
