Support for WebSockets in HtmlUnit

[markovuksanovic]

Ok, I have added the patch here. So we can do the review...

[jat]

Thanks - this is much easier to look at.

I have only looked at it briefly -- here are my comments so far.

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 communication portion of page
retrieval/submission.
This looks like a copy/paste issue, as it doesn't seem accurate.

http://gwt-code-reviews.appspot.com/699801/diff/1/5#newcode31
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java:31: 
If this abstracts out the WebSocket connection, shouldn't it have something
about initial setup and registering events?  If I understand correctly, there is
one of these per WebClient, so it can't be something specific to a singe
connection.

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", "1 N ?|k UT0or 3o  4 I97N 5-S3O 31");
These shouldn't be fixed, right?  Should have a TODO to fix it.

http://gwt-code-reviews.appspot.com/699801/diff/1/6#newcode81
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:81: try
{
It seems like either this should set CLOSING or CLOSED state, depending on how
exactly you are handling state transitions.  Also, shouldn't there be an event
fired?

http://gwt-code-reviews.appspot.com/699801/diff/1/6#newcode125
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:125: if
(headers_ != null) {
Where does headers_ get set?

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) {
If this is intended to take arbitrary binary data (as opposed to the
String-based one below), then the framing is incorrect -- you need a frame type
and then you include a frame length in the packet and there is no terminating
0xFF.

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

http://gwt-code-reviews.appspot.com/699801/diff/1/15#newcode59
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java:59:
while ((read = isr.read()) != -1) {
SShould probably handle binary frames here too, even if you just drop them until
the JS API has support, to make sure you don't get out of sync with the server
if it sends them.

[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 communication portion of page
retrieval/submission.
you're right. Will fix.

http://gwt-code-reviews.appspot.com/699801/diff/1/5#newcode31
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnection.java:31: 
I think I will remove this as this serves no real purpose. I had in mind an
interface that would define a contract that mock connections should implement
but at the moment it is not possible to inject a connection in a js object (at
least I haven't found a way) so this should go away.

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", "1 N ?|k UT0or 3o  4 I97N 5-S3O 31");
There's no problem with having this fixed. Real clients generate this randomly
and send this to server (for security reasons I guess) which then generates
response based on this parameters. As this is a test platform it is not
necessary to mix that in as ut just adds complexity - with generating and
handling... This way you always know which response is going to be returned and
is much easier for debugging. What do you think?

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) {
line 66

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

http://gwt-code-reviews.appspot.com/699801/diff/1/15#newcode59
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java:59:
while ((read = isr.read()) != -1) {
Will improve this, including the initial 16 bytes which are used to detect
proxies.

[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 the draft status of the simulation support

http://gwt-code-reviews.appspot.com/699801/diff/1/8
File
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/EventListenersContainer2.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/1/8#newcode41
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/EventListenersContainer2.java:41:
public class EventListenersContainer2 implements Serializable {
this is mainly a copy of EventListenersContainer, isn't it. It would be better
to extract a super class to avoid code duplications

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

http://gwt-code-reviews.appspot.com/699801/diff/1/10#newcode33
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/CloseEvent.java:33:
public static final String TYPE_MESSAGE = "close";
TYPE_CLOSE?

http://gwt-code-reviews.appspot.com/699801/diff/1/10#newcode61
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/CloseEvent.java:61:
public CloseEvent(final SimpleScriptable object) {
are three constructors really needed?

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

http://gwt-code-reviews.appspot.com/699801/diff/1/13#newcode33
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/OpenEvent.java:33:
public static final String TYPE_MESSAGE = "open";
really TYPE_MESSAGE?

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);
why such a long sleep?

http://gwt-code-reviews.appspot.com/699801/diff/1/21#newcode531
src/test/java/com/gargoylesoftware/htmlunit/WebDriverTestCase.java:531: while
(actualAlerts.size() < expectedAlerts.length && System.currentTimeMillis() <
maxWait) {
max wait is quite long. Is it necessary?

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 {
test class should extend WebDriverTestCase and all test should use the WebDriver
API. This allows to easily run the tests in the real browsers as well

[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 and was left here.

http://gwt-code-reviews.appspot.com/699801/diff/1/21#newcode531
src/test/java/com/gargoylesoftware/htmlunit/WebDriverTestCase.java:531: while
(actualAlerts.size() < expectedAlerts.length && System.currentTimeMillis() <
maxWait) {
Same as above. Will revert this changes.

[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, mguillemot wrote:
> test class should extend WebDriverTestCase and all test should use the
WebDriver
> API. This allows to easily run the tests in the real browsers as well

Done.

[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 even be here. Was testing sth and was left here.

Done.

http://gwt-code-reviews.appspot.com/699801/diff/1/21#newcode531
src/test/java/com/gargoylesoftware/htmlunit/WebDriverTestCase.java:531: while
(actualAlerts.size() < expectedAlerts.length && System.currentTimeMillis() <
maxWait) {
On 2010/07/19 11:12:39, markovuksanovic wrote:
> Same as above. Will revert this changes.
> 
> 

Done.

[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", "1 N ?|k UT0or 3o  4 I97N 5-S3O 31");
On 2010/07/18 18:27:49, markovuksanovic wrote:
> There's no problem with having this fixed. Real clients generate this randomly
> and send this to server (for security reasons I guess) which then generates
> response based on this parameters. As this is a test platform it is not
> necessary to mix that in as ut just adds complexity - with generating and
> handling... This way you always know which response is going to be returned
and
> is much easier for debugging. What do you think?

HtmlUnit is used for more than debugging -- it should be as close to a real
client as is practical.  So, I think this should behave properly, though it is
perfectly fine as it is  until you get everything working, which is why I
suggested the TODO to make sure it isn't forgotten.

http://gwt-code-reviews.appspot.com/699801/diff/1/6#newcode126
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:126:
for (Entry<String, String> entry : headers_.entrySet()) {
Are you sure the headers can be in an arbitrary order?  I need to reread the
current spec, but my recollection was it required a particular order, which
means HashMap.entrySet isn't going to be sufficient.

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/18 18:27:49, markovuksanovic wrote:
> line 66

I assume this was in response to the previous comment -- what about this one
about the binary framing being incorrect?

http://gwt-code-reviews.appspot.com/699801/diff/1/8
File
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/EventListenersContainer2.java
(right):

http://gwt-code-reviews.appspot.com/699801/diff/1/8#newcode41
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/EventListenersContainer2.java:41:
public class EventListenersContainer2 implements Serializable {
On 2010/07/19 10:55:48, mguillemot wrote:
> this is mainly a copy of EventListenersContainer, isn't it. It would be better
> to extract a super class to avoid code duplications

+1

http://gwt-code-reviews.appspot.com/699801/diff/1/22
File src/test/java/com/gargoylesoftware/htmlunit/WebServerTestCase.java (right):

http://gwt-code-reviews.appspot.com/699801/diff/1/22#newcode22
src/test/java/com/gargoylesoftware/htmlunit/WebServerTestCase.java:22: import
org.eclipse.jetty.server.Handler;
Maybe the Jetty update should be done separately to avoid confusion.

[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> entry : headers_.entrySet()) {
I am 99% sure that in spec it says that the order in which headers are put must
not be important. Though I'll double check.

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) {
I'm not sure I understand the question... You think that it should be possible
to send invalid frame? 0x00 and 0xff always put the info in a valid frame.

On 2010/07/20 05:42:32, jat wrote:
> On 2010/07/18 18:27:49, markovuksanovic wrote:
> > line 66
> 
> I assume this was in response to the previous comment -- what about this one
> about the binary framing being incorrect?

[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, mguillemot wrote:
> please indicate the draft status of the simulation support

Done.

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

[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, markovuksanovic wrote:
> I'm not sure I understand the question... You think that it should be possible
> to send invalid frame? 0x00 and 0xff always put the info in a valid frame.

Yes, but 00 utf8text FF is only valid for text.  Here you accept arbitrary
binary data, which is not necessarily UTF8 text.  The wire protocol has binary
frames (frame type bit  0x80 is set), which includes a length field and no
terminator, though I am not sure it is even worth including at this point since
there is no JS API defined for it yet (and probably won't be until
ByteBuffer/etc make it into JS).

http://gwt-code-reviews.appspot.com/699801/diff/18001/19005#newcode161
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:161: 
Comment for what is happening here.

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;
The way you have the rest of the loop written here, I think you need continue
here.

I think I would structure it more like:

while (1) {
  frameType = readFrameType;
  if (frameType.isBinaryFrame()) {
    byte[] frame = readBinaryFrame();
    // do something with binary frame, possibly onerror
  } else {
    String data = readTextFrame();
    onMessage callback
  }
}

And in readTextFrame use something like Jetty's Utf8StringBuffer.

http://gwt-code-reviews.appspot.com/699801/diff/18001/19014#newcode79
src/main/java/com/gargoylesoftware/htmlunit/javascript/host/websocket/WebSocketBufferScanner.java:79:
new String(temp).trim());
Here you are relying on the JVM's default character set, which probably isn't
UTF8.  You need to specify the character set, or better yet, use
Utf8StringBuilder (or equivalent) as mentioned above.

Also, I don't believe you want to call trim, as you are changing the data that
was sent.

[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:
new String(temp).trim());
Yes, trim is a bad idea here. And you're absolutely right about character
encoding.

[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 to think of a better way to detect a binary frame... It just struck me
that using OR operator should give more precise result in this case.

[markovuksanovic]

The latest patch generates random keys for opening web socket connection.

[markovuksanovic]

The patch uploaded a few mins ago fixes the code duplication introduced by
EventListenersContainer2.java. The base logic is now moved to EventListenersBase
while logic specific to was left in EventListenersContainer.java. IMHO, it might
be worth removing the EventListenerConatiner.java completely, but in a separate
patch. This patch introduces lots of changes anyway.

[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 that these headers are in a particular order in
v76, which is what I believe you are implementing here.  Better to read all the
lines in the header and then check the values of ones you care about.

http://gwt-code-reviews.appspot.com/699801/diff/44001/45005#newcode267
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:267: }
Is this the correct style for HtmlUnit?

http://gwt-code-reviews.appspot.com/699801/diff/44001/45005#newcode281
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:281:
outputStream_.write(0x00);
This is still the wrong framing for a binary message.  I suggest either removing
it since it won't actually be in the JS API until byte arrays are added, or
correcting it.

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:
};
Can this job execute before the JS code can add callbacks?

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

http://gwt-code-reviews.appspot.com/699801/diff/44001/45025#newcode28
src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/EchoWebSocketServlet.java:28:
* This is web socket servlet that echoes the received.
Incomplete sentence.

http://gwt-code-reviews.appspot.com/699801/diff/44001/45025#newcode48
src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/EchoWebSocketServlet.java:48:
protected WebSocket doWebSocketConnect(final HttpServletRequest arg0, final
String arg1) {
Better argument names.

[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 headers is not important. I actually haven't
tried to implement this functionality here. It got totally swept of my mind.

http://gwt-code-reviews.appspot.com/699801/diff/44001/45005#newcode267
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:267: }
I think it is, cause I can't remember that checkstyle complained.... But will
double check.

http://gwt-code-reviews.appspot.com/699801/diff/44001/45005#newcode281
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:281:
outputStream_.write(0x00);
As It is not implemented in the web socket spec, I guess it would be better to
remove it? What do you think?

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:
};
You think that it would be better if we waited for the connection to be opened
(or in the other words, executed the code related to opening connection
synchronously)? I guess you  would like to make sure that the connection is
actually opened before somebody can add some other callback? Is that right?

[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 is not implemented in the web socket spec, I guess it would be better to
> remove it? What do you think?

It is in the protocol, but there is no JS API for it because JS doesn't have any
efficient way to represent byte arrays (GWT uses an array of Number, which is
horribly inefficient but better than trying to emulate it packed in a string or
something else).

By the time it is implemented in the JS API, it will be totally different in the
wire protocol (the current discussions on hybi is that there is one framing
method for both text and binary, with a different header).

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:
};
On 2010/08/29 07:38:56, markovuksanovic wrote:
> You think that it would be better if we waited for the connection to be opened
> (or in the other words, executed the code related to opening connection
> synchronously)? I guess you  would like to make sure that the connection is
> actually opened before somebody can add some other callback? Is that right?

No, I think the spec for the JS API is borderline broken, in that it should
either let you set callbacks when you create it (which would be awkward for the
API) or have a separate connect step, so you can set callbacks before
connecting.

The answer I was given is that the event loop isn't processed while creating the
WebSocket, so the callbacks can't happen until the function creating it returns,
giving time to set the callbacks.  I am just saying we need to make sure that
holds in HtmlUnit as well.

I believe a good test for that would be to create a WebSocket with a bogus
scheme, like "wxx://foo" -- that should fail immediately, and we need to make
sure the onerror callback has time to get set before it is called.

[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 scheme and it passes. I will try to get
Marc's opinion on this just to be sure it is well tested and the behavior is as
expected.

[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:
> You're right, The order of the headers is not important. I actually haven't
> tried to implement this functionality here. It got totally swept of my mind. 

Done.

http://gwt-code-reviews.appspot.com/699801/diff/44001/45005#newcode281
src/main/java/com/gargoylesoftware/htmlunit/WebSocketConnectionImpl.java:281:
outputStream_.write(0x00);
I removed this for now.

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

http://gwt-code-reviews.appspot.com/699801/diff/44001/45025#newcode48
src/test/java/com/gargoylesoftware/htmlunit/javascript/host/websockets/EchoWebSocketServlet.java:48:
protected WebSocket doWebSocketConnect(final HttpServletRequest arg0, final
String arg1) {
On 2010/08/29 04:14:08, jat wrote:
> Better argument names.

Done.

[markovuksanovic]

Most important changes in the last patch set.
1. When establishing connection http header order is not assumed. It is now
irrelevant.
2. changed some names in EchoWebSocketImpl.
3. Added test for opening a web socket using invalid scheme.

[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 with the following checkstyle problem:

Inner assignments should be avoided

Any ideas how to avoid that but preserve "the good look"?

[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 wrote:
> The above statement results with the following checkstyle problem:
> 
> Inner assignments should be avoided
> 
> Any ideas how to avoid that but preserve "the good look"?

String header = reader.readLine();
while (header.length() > 0) {
  ...
  header = reader.readLine();
}

   or

while (true) {
  String header = reader.readLine();
  if (header.length() == 0) {
    break;
  }
  ...
}

But I don't think it is necessary to write code to parse this anyway.

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");
This doesn't actually verify these headers are present, and it doesn't handle
allowable whitespace.

Surely HtmlUnit already has code to parse an HTTP header -- you should use that
instead of recreating it inaccurately.  Then you can just do something like
this:

HttpResponse resp = fooMethod(inputStream_);
String upgrade = resp.getHeader("Upgrade");
String connection = resp.getHeader("Connection");
if (!"WebSocket".equals(upgrade) || !"Upgrade".equals(connection)) {
  throw new IOException(...);
}

[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 this. I'll check with Marc tomorrow if there is a method
I could reuse. In the worst case I'll check in the exiting java source to see
how it is handled there... :) It's not that simple thing and obviously I
shouldn't reinvent the wheel :)

[markovuksanovic]

On 2010/08/29 19:09:59, markovuksanovic wrote:

Latest patch uploaded. This patch can be cleanly applied to the tip of the
(today's) trunk.

[mguillemot]

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