Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(116)

Issue 699801: Support for WebSockets in HtmlUnit

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 1 month ago by markovuksanovic
Modified:
3 years, 10 months ago
Reviewers:
mguillemot, jat
CC:
Base URL:
Visibility:
Public.

Patch Set 1

Total comments: 34

Patch Set 2 : Review by jat and mguillemot

Total comments: 6

Patch Set 3 : More refactorings.

Total comments: 1

Patch Set 4 : More improvements

Patch Set 5 : One more minor change

Patch Set 6 : Some more refactorings.

Total comments: 16

Patch Set 7 : Some more improvements.

Total comments: 4

Patch Set 8 : Some more tests and a few minor changes.

Total comments: 15
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
M pom.xml View 1 2 3 4 5 6 7 3 chunks +40 lines, -4 lines 1 comment Download
M src/main/java/com/gargoylesoftware/htmlunit/BrowserVersion.java View 1 2 3 4 5 6 7 2 chunks +10 lines, -0 lines 1 comment Download
M src/main/java/com/gargoylesoftware/htmlunit/WebClient.java View 1 2 3 4 5 6 7 1 chunk +11 lines, -0 lines 0 comments Download
A src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java View 1 2 3 4 5 6 7 1 chunk +350 lines, -0 lines 0 comments Download
M src/main/java/com/gargoylesoftware/htmlunit/javascript/host/Event.java View 1 2 3 4 5 6 7 1 chunk +14 lines, -0 lines 0 comments Download
M src/main/java/com/gargoylesoftware/htmlunit/javascript/host/EventListenersContainer.java View 6 7 2 chunks +3 lines, -282 lines 0 comments Download
A src/main/java/com/gargoylesoftware/htmlunit/javascript/host/EventListenersContainerBase.java View 6 7 1 chunk +382 lines, -0 lines 1 comment Download
M src/main/java/com/gargoylesoftware/htmlunit/javascript/host/Window.java View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
A src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/CloseEvent.java View 1 chunk +85 lines, -0 lines 1 comment Download
A src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/ErrorEvent.java View 1 chunk +64 lines, -0 lines 1 comment Download
A src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/MessageEvent.java View 1 chunk +77 lines, -0 lines 2 comments Download
A src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/OpenEvent.java View 1 chunk +64 lines, -0 lines 0 comments Download
A src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/ProtocolException.java View 1 chunk +33 lines, -0 lines 1 comment Download
A src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocket.java View 1 2 3 4 5 6 7 1 chunk +538 lines, -0 lines 1 comment Download
A src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java View 1 2 3 4 5 6 7 1 chunk +95 lines, -0 lines 0 comments Download
A src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/package.html View 1 chunk +4 lines, -0 lines 1 comment Download
A src/main/resources/com/gargoylesoftware/htmlunit/javascript/configuration/FF4.properties View 1 chunk +17 lines, -0 lines 0 comments Download
M src/main/resources/com/gargoylesoftware/htmlunit/javascript/configuration/JavaScriptConfiguration.xml View 1 2 3 4 5 6 7 1 chunk +44 lines, -0 lines 0 comments Download
M src/test/java/com/gargoylesoftware/htmlunit/BrowserRunner.java View 1 2 3 4 5 6 7 6 chunks +21 lines, -6 lines 0 comments Download
M src/test/java/com/gargoylesoftware/htmlunit/BrowserVersionClassRunner.java View 1 2 3 4 5 6 7 2 chunks +14 lines, -0 lines 0 comments Download
M src/test/java/com/gargoylesoftware/htmlunit/HttpWebConnectionInsecureSSLTest.java View 2 chunks +82 lines, -82 lines 1 comment Download
M src/test/java/com/gargoylesoftware/htmlunit/WebDriverTestCase.java View 1 2 3 4 5 6 7 6 chunks +40 lines, -11 lines 0 comments Download
M src/test/java/com/gargoylesoftware/htmlunit/WebServerTestCase.java View 1 chunk +6 lines, -6 lines 0 comments Download
M src/test/java/com/gargoylesoftware/htmlunit/WebTestCase.java View 1 2 3 4 5 6 7 2 chunks +7 lines, -0 lines 1 comment Download
A src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/BogusWebSocketServlet.java View 1 chunk +84 lines, -0 lines 1 comment Download
A src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/EchoWebSocketServlet.java View 1 2 3 4 5 6 1 chunk +82 lines, -0 lines 1 comment Download
A src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/WebSocketTest.java View 1 2 3 4 5 6 7 1 chunk +334 lines, -0 lines 1 comment Download
A src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/package.html View 1 chunk +4 lines, -0 lines 0 comments Download
A src/test/resources/webdriver.xpi View 0 chunks +-1 lines, --1 lines 0 comments Download

Messages

Total messages: 28
markovuksanovic
Ok, I have added the patch here. So we can do the review...
4 years, 1 month ago #1
jat
Thanks - this is much easier to look at. I have only looked at it ...
4 years, 1 month ago #2
markovuksanovic
My comments... http://gwt-code-reviews.appspot.com/699801/diff/1/5 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java (right): http://gwt-code-reviews.appspot.com/699801/diff/1/5#newcode19 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java:19: * An object which handles the actual ...
4 years, 1 month ago #3
mguillemot
A few comments http://gwt-code-reviews.appspot.com/699801/diff/1/3 File src/main/java/com/gargoylesoftware/htmlunit/BrowserVersion.java (right): http://gwt-code-reviews.appspot.com/699801/diff/1/3#newcode130 src/main/java/com/gargoylesoftware/htmlunit/BrowserVersion.java:130: /** Firefox 4.0. */ please indicate ...
4 years, 1 month ago #4
markovuksanovic
http://gwt-code-reviews.appspot.com/699801/diff/1/21 File src/test/java/com/gargoylesoftware/htmlunit/WebDriverTestCase.java (right): http://gwt-code-reviews.appspot.com/699801/diff/1/21#newcode499 src/test/java/com/gargoylesoftware/htmlunit/WebDriverTestCase.java:499: Thread.sleep(10000); This shouldn't even be here. Was testing sth ...
4 years, 1 month ago #5
markovuksanovic
http://gwt-code-reviews.appspot.com/699801/diff/1/26 File src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/WebSocketTest.java (right): http://gwt-code-reviews.appspot.com/699801/diff/1/26#newcode40 src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/WebSocketTest.java:40: public class WebSocketTest extends WebServerTestCase { On 2010/07/19 10:55:48, ...
4 years, 1 month ago #6
markovuksanovic
http://gwt-code-reviews.appspot.com/699801/diff/1/21 File src/test/java/com/gargoylesoftware/htmlunit/WebDriverTestCase.java (right): http://gwt-code-reviews.appspot.com/699801/diff/1/21#newcode499 src/test/java/com/gargoylesoftware/htmlunit/WebDriverTestCase.java:499: Thread.sleep(10000); On 2010/07/19 11:12:39, markovuksanovic wrote: > This shouldn't ...
4 years, 1 month ago #7
jat
Please upload an updated version of your patch. http://gwt-code-reviews.appspot.com/699801/diff/1/6 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java (right): http://gwt-code-reviews.appspot.com/699801/diff/1/6#newcode68 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:68: headers_.put("Sec-WebSocket-Key2", ...
4 years, 1 month ago #8
markovuksanovic
Will post the updated patch shortly. http://gwt-code-reviews.appspot.com/699801/diff/1/6 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java (right): http://gwt-code-reviews.appspot.com/699801/diff/1/6#newcode126 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:126: for (Entry<String, String> ...
4 years, 1 month ago #9
markovuksanovic
Updated patch. http://gwt-code-reviews.appspot.com/699801/diff/1/3 File src/main/java/com/gargoylesoftware/htmlunit/BrowserVersion.java (right): http://gwt-code-reviews.appspot.com/699801/diff/1/3#newcode130 src/main/java/com/gargoylesoftware/htmlunit/BrowserVersion.java:130: /** Firefox 4.0. */ On 2010/07/19 10:55:48, ...
4 years, 1 month ago #10
markovuksanovic
http://gwt-code-reviews.appspot.com/699801/diff/18001/19014 File src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java (right): http://gwt-code-reviews.appspot.com/699801/diff/18001/19014#newcode62 src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java:62: } This might be a mistake.
4 years, 1 month ago #11
jat
http://gwt-code-reviews.appspot.com/699801/diff/1/6 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java (right): http://gwt-code-reviews.appspot.com/699801/diff/1/6#newcode208 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:208: public void send(final byte[] data) { On 2010/07/20 06:07:21, ...
4 years, 1 month ago #12
markovuksanovic
Comments. http://gwt-code-reviews.appspot.com/699801/diff/18001/19014 File src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java (right): http://gwt-code-reviews.appspot.com/699801/diff/18001/19014#newcode61 src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java:61: started = true; Did some refactoring. http://gwt-code-reviews.appspot.com/699801/diff/18001/19014#newcode79 src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java:79: ...
4 years, 1 month ago #13
markovuksanovic
4 years, 1 month ago #14
markovuksanovic
http://gwt-code-reviews.appspot.com/699801/diff/37001/16015 File src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java (right): http://gwt-code-reviews.appspot.com/699801/diff/37001/16015#newcode76 src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java:76: if ((frameType & 0x80) == 0x80) { Will try ...
4 years, 1 month ago #15
markovuksanovic
The latest patch generates random keys for opening web socket connection.
4 years ago #16
markovuksanovic
The patch uploaded a few mins ago fixes the code duplication introduced by EventListenersContainer2.java. The ...
4 years ago #17
jat
http://gwt-code-reviews.appspot.com/699801/diff/44001/45005 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java (right): http://gwt-code-reviews.appspot.com/699801/diff/44001/45005#newcode213 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:213: header = reader.readLine(); I don't believe it is required ...
4 years ago #18
markovuksanovic
http://gwt-code-reviews.appspot.com/699801/diff/44001/45005 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java (right): http://gwt-code-reviews.appspot.com/699801/diff/44001/45005#newcode213 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:213: header = reader.readLine(); You're right, The order of the ...
4 years ago #19
jat
http://gwt-code-reviews.appspot.com/699801/diff/44001/45005 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java (right): http://gwt-code-reviews.appspot.com/699801/diff/44001/45005#newcode281 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:281: outputStream_.write(0x00); On 2010/08/29 07:38:56, markovuksanovic wrote: > As It ...
4 years ago #20
markovuksanovic
http://gwt-code-reviews.appspot.com/699801/diff/44001/45014 File src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocket.java (right): http://gwt-code-reviews.appspot.com/699801/diff/44001/45014#newcode124 src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocket.java:124: }; Ok, I've added the test with the invalid ...
4 years ago #21
markovuksanovic
http://gwt-code-reviews.appspot.com/699801/diff/44001/45005 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java (right): http://gwt-code-reviews.appspot.com/699801/diff/44001/45005#newcode213 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:213: header = reader.readLine(); On 2010/08/29 07:38:56, markovuksanovic wrote: > ...
4 years ago #22
markovuksanovic
Most important changes in the last patch set. 1. When establishing connection http header order ...
4 years ago #23
markovuksanovic
http://gwt-code-reviews.appspot.com/699801/diff/61001/62004 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java (right): http://gwt-code-reviews.appspot.com/699801/diff/61001/62004#newcode209 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java:209: while (!(header = reader.readLine()).equals("")) { The above statement results ...
4 years ago #24
jat
http://gwt-code-reviews.appspot.com/699801/diff/61001/62004 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java (right): http://gwt-code-reviews.appspot.com/699801/diff/61001/62004#newcode209 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java:209: while (!(header = reader.readLine()).equals("")) { On 2010/08/29 17:25:16, markovuksanovic ...
4 years ago #25
markovuksanovic
http://gwt-code-reviews.appspot.com/699801/diff/61001/62004 File src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java (right): http://gwt-code-reviews.appspot.com/699801/diff/61001/62004#newcode213 src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java:213: throw new IOException("Invalid handshake response"); Ok, you're right about ...
4 years ago #26
markovuksanovic
On 2010/08/29 19:09:59, markovuksanovic wrote: Latest patch uploaded. This patch can be cleanly applied to ...
3 years, 11 months ago #27
mguillemot
3 years, 10 months ago #28
sorry, it took too long but I finally reviewed the patch again.

Cheers,
Marc.

http://gwt-code-reviews.appspot.com/699801/diff/75001/76001
File pom.xml (right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76001#newcode764
pom.xml:764: <groupId>org.eclipse.jetty</groupId>
is there now a version of Jetty available in Maven central repo that works for
WebSocket tests without introducing anywhere else?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76002
File src/main/java/com/gargoylesoftware/htmlunit/BrowserVersion.java (right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76002#newcode128
src/main/java/com/gargoylesoftware/htmlunit/BrowserVersion.java:128:
"Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:2.0b1) Gecko/2010630
Firefox/4.0b1",
"Gecko/2010630" -> the date format looks strange, is a "0" missing?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76007
File
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/EventListenersContainerBase.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76007#newcode36
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/EventListenersContainerBase.java:36:
* Alternative container for event listener.
"Alternative"?
I will rename current EventContainer to EventContainerBase to avoid having this
change in your WebSocket patch

http://gwt-code-reviews.appspot.com/699801/diff/75001/76009
File
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/CloseEvent.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76009#newcode21
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/CloseEvent.java:21:
* This class models "close" event.
any link to a spec?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76010
File
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/ErrorEvent.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76010#newcode21
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/ErrorEvent.java:21:
* This class models "error" event.
link to spec here too?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76011
File
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/MessageEvent.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76011#newcode21
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/MessageEvent.java:21:
* This class models "message" event.
link to spec?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76011#newcode28
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/MessageEvent.java:28:
private static final long serialVersionUID = 4542119950659582338L;
in fact since you started working on this, we decided to remove all
serialVersionUID as they don't make sense: the ones generated at compile time
are better in our case

http://gwt-code-reviews.appspot.com/699801/diff/75001/76013
File
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/ProtocolException.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76013#newcode23
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/ProtocolException.java:23:
public class ProtocolException extends Exception {
shouldn't this be a JS exception?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76014
File
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocket.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76014#newcode379
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocket.java:379:
private RhinoException asJavaScriptException(final DOMException exception) {
hmm, should I move Node.asJavaScriptException to make it reachable from here
rather than to duplicate code?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76016
File
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/package.html
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76016#newcode3
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/package.html:3:
Implementations of the WebSocket JavaScript host objects.
link to spec?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76021
File
src/test/java/com/gargoylesoftware/htmlunit/HttpWebConnectionInsecureSSLTest.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76021#newcode42
src/test/java/com/gargoylesoftware/htmlunit/HttpWebConnectionInsecureSSLTest.java:42:
//
is it the test that has troubles with the Jetty version? Is it still the case?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76024
File src/test/java/com/gargoylesoftware/htmlunit/WebTestCase.java (right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76024#newcode126
src/test/java/com/gargoylesoftware/htmlunit/WebTestCase.java:126:
MOCK_WEB_SOCKET_URL = new URL("http://foo.bar:" + PORT + "/");
what about moving this url to something specific to web socket tests?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76025
File
src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/BogusWebSocketServlet.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76025#newcode53
src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/BogusWebSocketServlet.java:53:
e.printStackTrace();
better to throw an error

http://gwt-code-reviews.appspot.com/699801/diff/75001/76026
File
src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/EchoWebSocketServlet.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76026#newcode53
src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/EchoWebSocketServlet.java:53:
e.printStackTrace();
better to throw an error
or can this be refactored with BogusWebSocketServlet?

http://gwt-code-reviews.appspot.com/699801/diff/75001/76027
File
src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/WebSocketTest.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/75001/76027#newcode48
src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/WebSocketTest.java:48:
+ "    <title>XMLHttpRequest Test</title>\n"
drop wrong title
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld revision f51cb906c4ad+