> On 2011-10-22 02:32:13, Thomas Broyer wrote: > > 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).
New patch with your comments. I've also updated socket-io to the git version (I couldn't patched correctly these days). I'm using this repo if someone want to check my changes: https://github.com/vjrj/Socket.IO-Java So, I've update the new binaries again: http://homes.ourproject.org/~vjrj/otros/patch-socketio.tgz (please remove previous socketio-java jars from third_party/runtime/socketio/) I did have time to deal with the sign in redirection issue. I think it's because of the new jetty version (I can only reproduce this in chrome). > On 2011-10-22 02:32:13, Thomas Broyer wrote: > > /.classpath, lines 33-44 > > <https://reviews.apache.org/r/2533/diff/1/?file=52345#file52345line33> > > > > 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. XHR doesn't work for me (some logs [1][2][3]) with 8.0.3 (neither using the socket.io of code.google [4], neither using the more maintained git version [5]). [1] Disconnected[ERROR]: XHR Connection dropped unexpectedly [2] Disconnected[TIMEOUT]: null [3] 2011-10-22 14:18:22,327 WARN [qtp1762721320-17] [socketio.server.DefaultSession] - Session[tFbCSlch_gedPssWQ9TN]: Exception thrown by SocketIOInbound.onConnect() java.lang.NullPointerException at org.eclipse.jetty.io.BufferUtil.putHexInt(BufferUtil.java:149) at org.eclipse.jetty.http.HttpGenerator.prepareBuffers(HttpGenerator.java:994) at org.eclipse.jetty.http.HttpGenerator.flushBuffer(HttpGenerator.java:824) at org.eclipse.jetty.server.HttpConnection.flushResponse(HttpConnection.java:617) at org.eclipse.jetty.server.Response.flushBuffer(Response.java:1004) at com.glines.socketio.server.transport.XHRMultipartDataHandler.onWriteData(XHRMultipartDataHandler.java:94) [4] https://code.google.com/p/socketio-java/ [5] https://github.com/Ovea/Socket.IO-Java > On 2011-10-22 02:32:13, Thomas Broyer wrote: > > /build.xml, line 605 > > <https://reviews.apache.org/r/2533/diff/1/?file=52346#file52346line605> > > > > 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. same here > On 2011-10-22 02:32:13, Thomas Broyer wrote: > > /build.xml, line 609 > > <https://reviews.apache.org/r/2533/diff/1/?file=52346#file52346line609> > > > > Same here. same here... already fixed in trunk. > On 2011-10-22 02:32:13, Thomas Broyer wrote: > > /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java, line 31 > > <https://reviews.apache.org/r/2533/diff/1/?file=52349#file52349line31> > > > > Rename to 'connection'? Done > On 2011-10-22 02:32:13, Thomas Broyer wrote: > > /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java, line 45 > > <https://reviews.apache.org/r/2533/diff/1/?file=52349#file52349line45> > > > > The javadoc should be updated. Done > On 2011-10-22 02:32:13, Thomas Broyer wrote: > > /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java, line 95 > > <https://reviews.apache.org/r/2533/diff/1/?file=52349#file52349line95> > > > > Not sure this comment still applies. No... deleted > On 2011-10-22 02:32:13, Thomas Broyer wrote: > > /src/org/waveprotocol/box/webclient/client/WebClient.java, line 274 > > <https://reviews.apache.org/r/2533/diff/1/?file=52350#file52350line274> > > > > 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). Done > On 2011-10-22 02:32:13, Thomas Broyer wrote: > > /build.xml, line 515 > > <https://reviews.apache.org/r/2533/diff/1/?file=52346#file52346line515> > > > > Why is this needed? Does not seem to be related to the proposed changes > > (and works the same if omitted). forget that. This is related with the previous attachments patch from Yuri that missed that (now it's commited) > On 2011-10-22 02:32:13, Thomas Broyer wrote: > > /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java, line 75 > > <https://reviews.apache.org/r/2533/diff/1/?file=52349#file52349line75> > > > > 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. Done (although jetty doesn't document more this) > On 2011-10-22 02:32:13, Thomas Broyer wrote: > > /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java, line 76 > > <https://reviews.apache.org/r/2533/diff/1/?file=52349#file52349line76> > > > > Should we log the closeCode and message? (do you know what the message > > represents?) I let the log with some more info (the close code and message). Useful for debugging. - vjrj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/2533/#review2769 ----------------------------------------------------------- On 2011-10-22 16:13:42, vjrj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/2533/ > ----------------------------------------------------------- > > (Updated 2011-10-22 16:13:42) > > > 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 1187587 > /src/org/waveprotocol/box/server/robots/agent/passwd/PasswordRobot.java > 1187587 > /src/org/waveprotocol/box/server/rpc/AbstractWaveSocketIOServlet.java > PRE-CREATION > /src/org/waveprotocol/box/server/rpc/ServerRpcProvider.java 1187587 > /src/org/waveprotocol/box/server/rpc/SocketIOServerChannel.java 1187587 > /src/org/waveprotocol/box/server/rpc/WebSocketServerChannel.java 1187587 > /src/org/waveprotocol/box/webclient/client/WaveSocketFactory.java 1187587 > /src/org/waveprotocol/box/webclient/client/WebClient.java 1187587 > /test/org/waveprotocol/wave/crypto/CertpathTest.java 1187587 > /third_party/codegen/socketio/COPYING PRE-CREATION > /third_party/codegen/socketio/README.google PRE-CREATION > /third_party/codegen/socketio/socketio-gwt-0.1-SNAPSHOT-sources.jar UNKNOWN > /third_party/codegen/socketio/socketio-gwt-0.1-SNAPSHOT.jar UNKNOWN > /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/README.google 1187587 > /third_party/runtime/socketio/socketio-core-0.1-SNAPSHOT-sources.jar > UNKNOWN > /third_party/runtime/socketio/socketio-core-0.1-SNAPSHOT.jar UNKNOWN > /third_party/runtime/socketio/socketio-java-src.jar UNKNOWN > /third_party/runtime/socketio/socketio-java.jar UNKNOWN > /third_party/runtime/socketio/socketio-jetty-0.1-SNAPSHOT-sources.jar > UNKNOWN > /third_party/runtime/socketio/socketio-jetty-0.1-SNAPSHOT.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 > >
