Bug 27490 – Web Sockets Test Infrastructure Part 1/3: Server

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Yuzo Fujishima]

Add test infrastructure for Web Sockets (http://www.w3.org/TR/websockets/).

This bug is Part 1/3: Server.

Other parts are:
Part 2/3: Patch to run-webkit-tests
Part 3/3: Example test
Parts must be submitted in the order Part1, Part2, then Part3.

[Yuzo Fujishima]

Links:

Part 1/3: Server: https://bugs.webkit.org/show_bug.cgi?id=27490
Part 2/3: Patch to run-webkit-tests: https://bugs.webkit.org/show_bug.cgi?id=27491
Part 3/3: Example test: https://bugs.webkit.org/show_bug.cgi?id=27492

[Yuzo Fujishima]

Created attachment 33157 [details]
Web Socket Server for LayoutTests.

[Alexey Proskuryakov]

What would it take to run WebSockets server from within Apache, to test how Upgrade works? I think this setup should be closer to what real sites will do.

[Yuzo Fujishima]

Hi, Alexey,

Thank you for the review.

Honestly, I'm not familiar with Apache. But I guess that adding non-HTTP
handler to it should be non-trivial.

I believe writing a simple standalone server like the one in the patch has better
reward/cost ratio, at least until popular web servers, e.g., Apache, support
Web Sockets. (The cost would include that for learning how to extend Apache as such.)

I do agree that we should switch to Apache (or other popular web server)-based
testing when Apache supports Web Sockets.

Yuzo

[Yuzo Fujishima]

Created attachment 34053 [details]
Web Socket Server for LayoutTests, derived from Python's SimpleHTTPServer

To enable both same-origin (http://) and cross-origin (file://) testing,
and also to make testing a little bit more realistic, I've rewritten the
test Web Socket server to derive from Python's SimpleHTTPServer.

[Mark Rowe (bdash)]

Comment on attachment 34053 [details]
Web Socket Server for LayoutTests, derived from Python's SimpleHTTPServer

Two big issues jump out at me:
1) Coding style.  Python code in WebKit typically follows the standard Python style guidelines as documented in PEP 8 at http://www.python.org/dev/peps/pep-0008/.  This includes using four spaces for indentation, lower_case_and_underscores for function and method names, and so forth.  I think we're more than happy to ignore the 80 character line length limit as we do elsewhere in WebKit source.
2) The script have a hardcoded #! line that refers to Python 2.4.  Tiger only ships with Python 2.3, so a test harness will need to be compatible with that version.  I've not studied the code closely enough to determine whether there are 2.4 features used in the code or if it's just the shebang line that is the problem.

The "websocket_" prefix on the file names also seems redundant given that this all lives inside a websocket directory.

I'll try and take a closer look at the substance of the patch soon, but it's clear at this point that it needs revision.

[Alexey Proskuryakov]

+DEFAULT_WS_PORT = 81

This doesn't look like a great default, given that the server would need root privileges to take this port.

+  WebSocketHTTPRequestHandler.protocol_version = 'HTTP/1.0'

Why 1.0?

[Yuzo Fujishima]

Created attachment 34124 [details]
Web Socket Server, in desired coding style, with shorter file names.

Thank you for your reviews.

- I've changed the coding style to conform with PEP 8.

- I've changed the #! line and other parts of the code
  such that the scripts will run with Python 2.3 and later.
  (I've tested with Python 2.3, 2.4, and 2.5)

- I've removed 'websocket_' from the file names except for
  websocket_http_server.py, for which the prefix is meaningful, IMHO.

- I've changed the protocol_version to HTTP/1.1.

- I've defined two kinds of "default port". One is as defined by the spec.
  The other is for testing.

Yuzo

[Eric Seidel (no email)]

Comment on attachment 34124 [details]
Web Socket Server, in desired coding style, with shorter file names.

This is too large for me to review.  Unless you're just looking for a rubber stamp?

[Yuzo Fujishima]

Hi, reviewers,

Thank you for your comments.

I've written a Web Socket server for Apache HTTP server using mod_python.
This is expected to be merged into LayoutTests/http.

Because of its size, I've uploaded it to http://codereview.appspot.com,
after splitting it into 8 parts (part 0 through 7), in dependency order.
(The most depended on comes first.)

Please let me know if you like it uploaded to this bug instead.

Web Socket Server for Apache HTTP Server with mod_python: 
http://codereview.appspot.com/105107 Part 0: mp_websock module 
http://codereview.appspot.com/105108 Part 1: script to run all tests
http://codereview.appspot.com/105109 Part 2: testing helpers
http://codereview.appspot.com/105110 Part 3: utilities 
http://codereview.appspot.com/104108 Part 4: message utilities
http://codereview.appspot.com/105111 Part 5: handshake
http://codereview.appspot.com/105112 Part 6: dispatch
http://codereview.appspot.com/104109 Part 7: PythonConnectionHandler

I haven't created ChangeLog yet.

Yuzo

[Yuzo Fujishima]

Hi, reviewers,

Sorry, but on second thought it seems to me that directly adding a Web Socket server
to the WebKit repository may not be a good idea, because
- it is not part of WebKit per se. It is only for testing.
- it is not small enough to ignore its review/maintenance cost.

Accordingly, I'd like to at least temporarily withdraw the patches.
(codereview.appspot.com links should be now inaccessible.)

Yuzo

[Adam Barth]

Comment on attachment 34124 [details]
Web Socket Server, in desired coding style, with shorter file names.

Clearing review flag per comment above.

[Alexey Proskuryakov]

(In reply to comment #11)
> Sorry, but on second thought it seems to me that directly adding a Web Socket
> server to the WebKit repository may not be a good idea

Is there any other (existing or future) way to test Web Socket functionality? It can't go untested.

[Yuzo Fujishima]

(In reply to comment #13)
> (In reply to comment #11)
> > Sorry, but on second thought it seems to me that directly adding a Web Socket
> > server to the WebKit repository may not be a good idea
> 
> Is there any other (existing or future) way to test Web Socket functionality?
> It can't go untested.

Hi,

I'm preparing an open source project for an experimental Web Socket server
extension for Apache. The plan is I'll make it public this week.

Then I'll propose using it for testing WebKit.

Yuzo

[Yuzo Fujishima]

I'd propose using mod_pywebsocket http://code.google.com/p/pywebsocket/
to test Web Socket implementation of WebKit.

I probably need help from some WebKit experts in adding mod_python and
mod_pywebsocket to the WebKit development environment. Whom should I
talk to?

Yuzo

[Alexey Proskuryakov]

> Whom should I talk to?

Please e-mail webkit-dev.

[Yuzo Fujishima]

Created attachment 40351 [details]
add pywebsocket to test Web Socket

 WebKitTools/ChangeLog                              |   38 +++
 WebKitTools/pywebsocket/COPYING                    |  202 +++++++++++++
 WebKitTools/pywebsocket/MANIFEST.in                |    6 +
 WebKitTools/pywebsocket/README                     |    6 +
 WebKitTools/pywebsocket/example/echo_client.py     |  168 +++++++++++
 WebKitTools/pywebsocket/example/echo_wsh.py        |   31 ++
 .../pywebsocket/mod_pywebsocket/__init__.py        |   79 +++++
 .../pywebsocket/mod_pywebsocket/dispatch.py        |  160 +++++++++++
 .../pywebsocket/mod_pywebsocket/handshake.py       |  162 +++++++++++
 .../mod_pywebsocket/headerparserhandler.py         |   77 +++++
 WebKitTools/pywebsocket/mod_pywebsocket/msgutil.py |  184 ++++++++++++
 .../pywebsocket/mod_pywebsocket/standalone.py      |  156 ++++++++++
 WebKitTools/pywebsocket/mod_pywebsocket/util.py    |   37 +++
 WebKitTools/pywebsocket/setup.py                   |   48 +++
 WebKitTools/pywebsocket/test/config.py             |   30 ++
 WebKitTools/pywebsocket/test/mock.py               |  188 ++++++++++++
 WebKitTools/pywebsocket/test/run_all.py            |   49 ++++
 WebKitTools/pywebsocket/test/test_dispatch.py      |  156 ++++++++++
 WebKitTools/pywebsocket/test/test_handshake.py     |  301 ++++++++++++++++++++
 WebKitTools/pywebsocket/test/test_mock.py          |  111 +++++++
 WebKitTools/pywebsocket/test/test_msgutil.py       |  134 +++++++++
 WebKitTools/pywebsocket/test/test_util.py          |   42 +++
 .../pywebsocket/test/testdata/handlers/a_wsh.py    |   27 ++
 .../pywebsocket/test/testdata/handlers/b_wsh.py    |   16 +
 .../test/testdata/handlers/sub/c_wsh.py            |   24 ++
 .../pywebsocket/test/testdata/handlers/sub/d.py    |   29 ++
 .../test/testdata/handlers/sub/e_wsh.py            |   25 ++
 .../test/testdata/handlers/sub/f_wsh.py            |   29 ++
 .../test/testdata/handlers/sub/g_wsh.py            |   29 ++
 .../test/testdata/handlers/sub/h_wsh.py            |   29 ++
 30 files changed, 2573 insertions(+), 0 deletions(-)

[Yuzo Fujishima]

Created attachment 40353 [details]
add pywebsocket to test Web Socket

[Sam Weinig]

Comment on attachment 40353 [details]
add pywebsocket to test Web Socket

We do not allow code under the Apache license at this time. r-.

[Jeremy Orlow]

Interesting.  This is even true when the code is not linked with anything else?

Note that most if not all of what's in  http://trac.webkit.org/browser/trunk/LayoutTests/svg/batik is under the Apache 2 license.

[Mark Rowe (bdash)]

When attaching a patch to Bugzilla, you click through text on the "Add an attachment" page that mentions:

By clicking below you agree that your file is licensed under either the BSD license or LGPL 2.1, as indicated in your file.

That's clearly not the case here.

[Jeremy Orlow]

(In reply to comment #21)
> When attaching a patch to Bugzilla, you click through text on the "Add an
> attachment" page that mentions:
> 
> By clicking below you agree that your file is licensed under either the BSD
> license or LGPL 2.1, as indicated in your file.
> 
> That's clearly not the case here.

(In reply to comment #21)
> When attaching a patch to Bugzilla, you click through text on the "Add an
> attachment" page that mentions:
> 
> By clicking below you agree that your file is licensed under either the BSD
> license or LGPL 2.1, as indicated in your file.
> 
> That's clearly not the case here.

Talked to bdash on IRC.  Realized the example I posted was put in a long time ago.  (Presumably before all code had to be LGPL or BSD.)

[Jeremy Orlow]

According to bdash, dual licensing would be an acceptable solution.

[Yuzo Fujishima]

Created attachment 40493 [details]
add mod_pywebsocket to test Web Sockets

[Yuzo Fujishima]

Now mod_pywebsocket is licensed under BSD.
Can you take another look?

Yuzo

[Yuzo Fujishima]

Created attachment 40705 [details]
add mod_pywebsocket to test Web Sockets

[David Levin]

I'll try to look at this tonight. (Feel free to ping me to remind me if you see no action.)

[Yuzo Fujishima]

Hi, David,

Can you take a look?

The files are being imported from 
http://code.google.com/p/pywebsocket/

Yuzo

[David Levin]

I'm looking. This is complicated by the fact that the patch is huge.

[David Levin]

Comment on attachment 40705 [details]
add mod_pywebsocket to test Web Sockets

General notes:

Ideally the copyright would be of this format:
    Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
    Copyright (C) 2009 Somebody <email>

Also make sure that variable name and ideally file names too use full words (not abbreviations).

I think web_socket_shake_hands is misnamed. It isn't a handler that is suppose to do any handshaking. It seems it is only there to allow handlers to perform custom checks before the handshake is sent from the server.  I haven't thought a lot about a better name... Here's some brief name brainstorming: web_socket_handshake_check, web_socket_allow_handshake, etc.

Here's a quote from the websocket spec:
   The first field consists of three tokens separated by space
   characters (byte 0x20).  The middle token is the path being opened.
   If the server supports multiple paths, then the server should echo
   the value of this field in the initial handshake, as part of the URL
   given on the |WebSocket-Location| line (after the appropriate scheme
   and host).

   If the first field does not have three tokens, the server should
   abort the connection as it probably represents an errorneous client.

Where is this done?


> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> [phone]  Yuzo Fujishima  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Add mod_pywebsocket to test Web Sockets.
> +        http://code.google.com/p/pywebsocket/
> +        https://bugs.webkit.org/show_bug.cgi?id=27490
> +
> +        * pywebsocket/COPYING: Added.

Usually you'd say something about the files where they are listed.  This changelog is a bit sparse.


It would be nice if it referenced the protocol spec since that is what is being implemented: http://tools.ietf.org/html/draft-hixie-thewebsocketprotocol


> diff --git a/WebKitTools/pywebsocket/COPYING b/WebKitTools/pywebsocket/COPYING
> +++ b/WebKitTools/pywebsocket/COPYING

Is it really needed to check in this file?


> diff --git a/WebKitTools/pywebsocket/README b/WebKitTools/pywebsocket/README
> new file mode 100644
> index 0000000..1f9f05f
> --- /dev/null
> +++ b/WebKitTools/pywebsocket/README
> @@ -0,0 +1,6 @@
> +Install this package by:
> +$ python setup.py build
> +$ sudo python setup.py install
> +
> +Then read document by:
> +$ pydoc mod_pywebsocket

Does this need to be installed by every webkit developer before running layout tests?
There needs to be another way to handle this (and then delete this file :) ).



> diff --git a/WebKitTools/pywebsocket/example/echo_client.py b/WebKitTools/pywebsocket/example/echo_client.py
> +class EchoClient(object):

> +    def _handshake(self):
...
> +        for expected_char in (
> +                'HTTP/1.1 101 Web Socket Protocol Handshake\r\n'
> +                'Upgrade: WebSocket\r\n'
> +                'Connection: Upgrade\r\n'):

Why not put this in a string constant?

Also you repeat several strings here and in the "self._socket.send" above. If these will always be the same, it may be better to have only one instance of them for maintainence reasons.


> +    def _skip_headers(self):
> +        terminator = '\r\n\r\n'
> +        pos = 0
> +        while pos < len(terminator):
> +            received = self._socket.recv(1)[0]
> +            if received == terminator[pos]:
> +                pos += 1
> +            else:
> +                pos = 0

This isn't correct. It should be 

           if received == terminator[pos]:
               pos += 1
           elif received == terminator[0]:
               pos = 1
           else:
               pos = 0

> +    def _format_host_header(self):
> +        host = 'Host: ' + self._options.server_host
> +        if ((not self._options.use_tls and
> +             self._options.server_port != _DEFAULT_PORT) or
> +            (self._options.use_tls and
> +             self._options.server_port != _DEFAULT_SECURE_PORT)):

Please put the "and" "or" at the beginning of lines instead of the end following the WebKit style guide of putting &&/|| at the beginning of lines when there are continuations.
Also, this would be easier to read if there weren't as many wrapped lines. I would suggest not warping after the "and"s. WebKit doesn't have an 80 column limit.


> +    (options, _) = parser.parse_args()

"_" is a valid variable name but non-descript and looks like a magic variable from perl ($_).

Why not just put "arguments" or "unused"?

> +    # Default port number depends on whether TLS is used.
> +    if options.server_port == _UNDEFINED_PORT:
> +        if options.use_tls:
> +            options.server_port = _DEFAULT_SECURE_PORT
> +        else:
> +            options.server_port = _DEFAULT_PORT

It would be nice to make this a function "def get_default_port(use_tls):" which would be used here and in the "if" in _format_host_header.

> +# vi:sts=4 sw=4 et

No other files in WebKit have this "vi" formatting note. Please get rid of it everywhere.


> diff --git a/WebKitTools/pywebsocket/example/echo_wsh.py b/WebKitTools/pywebsocket/example/echo_wsh.py

> +from mod_pywebsocket import msgutil
> +
> +
> +def web_socket_shake_hands(request):
> +    pass  # always accept

Since python leaves it unspecified, follow the typical WebKit style and only put one space before end of line comments.
Also, whole sentences that start with a capital and end with a period are preferred.

> diff --git a/WebKitTools/pywebsocket/mod_pywebsocket/dispatch.py b/WebKitTools/pywebsocket/mod_pywebsocket/dispatch.py

> +import os
> +import re
> +

Why is there a blank line here?

> +_SHAKE_HANDS_HANDLER_NAME = 'web_socket_shake_hands'
I can't find any references to "shaking hands" when talking about a "handshake" in a protocol

I'd just call it 'web_socket_do_handshake' or 'web_socket_perform_handshake'

> +_TRANSFER_DATA_HANDLER_NAME = 'web_socket_transfer_data'


> +def _source(source_str):
> +    """Source a handler definition string."""
> +
> +    global_dic = {}

Use full words "global_dic"

> +    try:
> +        exec source_str in global_dic
> +    except Exception:
> +        raise DispatchError('Error in sourcing handler:' +
> +                            util.get_stack_trace())

No need to line wrap this.

> +def _extract_handler(dic, name):

Use full words "dic"

> +class Dispatcher(object):
> +    def shake_hands(self, request):
> +        """Hook into Web Socket handshake.
> +
> +        Select a handler based on request.uri and call its
> +        web_socket_shake_hands function.
> +
> +        Args:
> +            request: mod_python request.
> +        """
> +
> +        shake_hands_, _ = self._handler(request)

"_" unfortunate variable name.

> +        try:
> +            shake_hands_(request)
> +        except Exception:
> +            raise DispatchError('shake_hands() raised exception: ' +

"shake_hands" isn't the current api name. Why not just use _SHAKE_HANDS_HANDLER_NAME here?


> +    def transfer_data(self, request):
...
> +        _, transfer_data_ = self._handler(request)

"_" unfortunate variable name.

> +        except Exception:
> +            raise DispatchError('transfer_data() raised exception: ' +

Why not use _TRANSFER_DATA_HANDLER_NAME for the function name?

> +                                util.get_stack_trace())


> diff --git a/WebKitTools/pywebsocket/mod_pywebsocket/handshake.py b/WebKitTools/pywebsocket/mod_pywebsocket/handshake.py


> +class Handshaker(object):
> +    def _set_resource(self):
> +        self._request.ws_resource = self._request.uri

Avoid abbreviations "ws_resource"

> +    def _set_location(self):
> +        location_parts = []
> +        if self._request.is_https():
> +            location_parts.append(_WEB_SOCKET_SECURE_SCHEME)
> +        else:
> +            location_parts.append(_WEB_SOCKET_SCHEME)
> +        location_parts.append('://')
> +        host, port = self._parse_host_header()
> +        conn_port = self._request.connection.local_addr[1]

Avoid abbreviations "conn_port"

> +        if port != conn_port:
> +            raise HandshakeError('Header/connection port mismatch: %d/%d' %
> +                                 (port, conn_port))
> +        location_parts.append(host)
> +        if ((not self._request.is_https() and
> +             port != _DEFAULT_WEB_SOCKET_PORT) or
> +            (self._request.is_https() and
> +             port != _DEFAULT_WEB_SOCKET_SECURE_PORT)):

Ideally there would be a method to return the default port based on is_https (perhaps re-use "def get_default_port(use_tls):").

> +    def _parse_host_header(self):
> +        fields = self._request.headers_in['Host'].split(':', 1)
> +        if len(fields) == 1:
> +            port = _DEFAULT_WEB_SOCKET_PORT
> +            if self._request.is_https():
> +                port = _DEFAULT_WEB_SOCKET_SECURE_PORT

Use function to return the port (as above).

> +    def _check_header_lines(self):
...
> +                raise HandshakeError('Header %s not defined' % key)

How about
                raise HandshakeError('Header %s is not defined.' % key)
?


> diff --git a/WebKitTools/pywebsocket/mod_pywebsocket/headerparserhandler.py b/WebKitTools/pywebsocket/mod_pywebsocket/headerparserhandler.py

> +def headerparserhandler(request):
...
> +        request.log_error('mod_pywebsocket: resource:%r' % request.ws_resource,

Did you want a space before the %r?

> diff --git a/WebKitTools/pywebsocket/mod_pywebsocket/msgutil.py b/WebKitTools/pywebsocket/mod_pywebsocket/msgutil.py

> +import Queue
> +import StringIO
> +import traceback
> +import threading

out of order (threading before traceback).


> +
> +def _receive_bytes(request, length):
> +    bytes = ''
> +    while length > 0:
> +        new_bytes = request.connection.read(length)
> +        bytes += new_bytes
> +        length -= len(new_bytes)
> +    return bytes

Why not use array concatenation followed by a ''.join() like you've done elsewhere (to avoid slow string concatenation)?

> +def _read_until(request, delim_char):
> +    bytes = ''
> +    while True:
> +        ch = request.connection.read(1)
> +        if ch == delim_char:
> +            break
> +        bytes += ch
> +    return bytes

Why not use array concatenation followed by a ''.join() like you've done elsewhere (to avoid slow string concatenation)?


> +class MessageReceiver(threading.Thread):
> +    def __init__(self, request, onmessage=None):
> +        """Construct an instance.
> +
> +        Args:
> +            request: mod_python request.
> +            onmessage: a function to be called when a message is received.
> +                       Can be None.

s/Can/May/

It may be valuable to mention two things:
1. The onmessage will be called on another thread.
2. If you provide an on message handler, then the received functions are not useful.

I find #2 strange. It seems like it would be better if there were another class that used this one to provive receive/receive_nowait, and then this class would require onmessageto be set.


> +    def run(self):
> +        while True:
> +            message = receive_message(self._request)
> +            if self._onmessage:
> +                self._onmessage(message)
> +            else:
> +                self._queue.put(message)

How does this exit?  (Does receive_message throw an exception and then the thread exits?)

> +class MessageSender(threading.Thread):
> +    def send(self, message):
> +        """Send a message, blocking."""
> +
> +        send_message(self._request, message)

So the ordering of this with respect to other messages in the queue is undefined?

If seems like it should come after all messages that are already in the queue. I guess this could be explained with some comment in the code about why this functionality is the way it is.


> diff --git a/WebKitTools/pywebsocket/mod_pywebsocket/standalone.py b/WebKitTools/pywebsocket/mod_pywebsocket/standalone.py
> +"""Standalone Web Socket server.
> +
> +Use this server to run mod_pywebsocket without Apache HTTP Server.
> +"""

Is this going to be used? It would be nice to have only one way to run these in apache or not in apache.



> diff --git a/WebKitTools/pywebsocket/setup.py b/WebKitTools/pywebsocket/setup.py

We need to get rid of set-up. There needs to be a checked in solution that runs without it.

> diff --git a/WebKitTools/pywebsocket/test/mock.py b/WebKitTools/pywebsocket/test/mock.py

> +class MockConn(_MockConnBase):
...
> +
> +    def readline(self):
> +        """Override mod_python.apache.mp_conn.readline."""
> +
> +        if self._read_pos >= len(self._read_data):
> +            return ''
> +        end_index = self._read_data.find('\n', self._read_pos) + 1
> +        if end_index == 0:

avoid comparisons to 0.
   if not end_index:

> +            end_index = len(self._read_data)
> +        return self._read_up_to(end_index)

> +class MockBlockingConn(_MockConnBase):
...
> +    def readline(self):
...
> +        while True:
> +            ch = self._queue.get()

If you believe that this variable is worth more than one character, use whole words. 'c' may be sufficient here.


> +    def read(self, length):
> +        """Override mod_python.apache.mp_conn.read."""
> +
> +        data = ''
> +        for _ in range(length):

"_" looks like perl. Why not just use i?


> +class MockTable(dict):
> +    """Mock table.
> +
> +    This mimicks mod_python mp_table. Note that only the methods used by

s/mimicks/mimics/

> +class MockRequest(object):

> +    def __init__(self, uri=None, headers_in={}, connection=None,
> +                 is_https=False):
> +        self.uri = uri
> +        self.connection = connection
> +        self.headers_in = MockTable(headers_in)
> +        self.is_https_ = is_https

Why does is_https_ have a trailing underscore? Did you mean to use a leading underscore?



> diff --git a/WebKitTools/pywebsocket/test/test_dispatch.py b/WebKitTools/pywebsocket/test/test_dispatch.py

> +class DispatcherTest(unittest.TestCase):
> +    def test_shake_hand(self):
> +        dispatcher = dispatch.Dispatcher(_TEST_HANDLERS_DIR)
> +        request = mock.MockRequest()
> +        request.ws_resource = '/a'
> +        request.ws_origin = 'http://example.com'
> +        dispatcher.shake_hands(request)  # Must not raise exception.
> +
> +        request.ws_origin = 'http://bad.example.com'
> +        self.assertRaises(dispatch.DispatchError,
> +                          dispatcher.shake_hands, request)
> +
> +    def test_transfer_data(self):
> +        dispatcher = dispatch.Dispatcher(_TEST_HANDLERS_DIR)
> +        request = mock.MockRequest(connection=mock.MockConn(''))
> +        request.ws_resource = '/a'
> +        request.ws_protocol = 'p1'
> +
> +        dispatcher.transfer_data(request)
> +        self.assertEqual('a_wsh.py is called for /a, p1',
> +                         request.connection.written_data())
> +
> +        request = mock.MockRequest(connection=mock.MockConn(''))
> +        request.ws_resource = '/sub/e'
> +        request.ws_protocol = None
> +        dispatcher.transfer_data(request)
> +        self.assertEqual('sub/e_wsh.py is called for /sub/e, None',
> +                         request.connection.written_data())
> +
> +    def test_transfer_data_no_handler(self):
> +        dispatcher = dispatch.Dispatcher(_TEST_HANDLERS_DIR)
> +        for resource in ['/b', '/sub/c', '/sub/d', '/does/not/exist']:
> +            request = mock.MockRequest(connection=mock.MockConn(''))
> +            request.ws_resource = resource
> +            request.ws_protocol = 'p2'
> +            try:
> +                dispatcher.transfer_data(request)
> +                self.fail()
> +            except dispatch.DispatchError, e:
> +                self.failUnless(str(e).find('No handler') != -1)
> +            except Exception:
> +                self.fail()
> +
> +    def test_transfer_data_handler_exception(self):
> +        dispatcher = dispatch.Dispatcher(_TEST_HANDLERS_DIR)
> +        request = mock.MockRequest(connection=mock.MockConn(''))
> +        request.ws_resource = '/sub/f'
> +        request.ws_protocol = 'p3'
> +        try:
> +            dispatcher.transfer_data(request)
> +            self.fail()
> +        except dispatch.DispatchError, e:
> +            self.failUnless(str(e).find('Intentional') != -1)
> +        except Exception:
> +            self.fail()


> diff --git a/WebKitTools/pywebsocket/test/test_handshake.py b/WebKitTools/pywebsocket/test/test_handshake.py
> +_GOOD_REQUEST = (
> +    80,
> +    '/demo',
> +    {
> +        'Upgrade':'WebSocket',
> +        'Connection':'Upgrade',
> +        'Host':'example.com',
> +        'Origin':'http://example.com',
> +        'WebSocket-Protocol':'sample',
> +    }
> +)

> +_GOOD_RESPONSE_SECURE_NONDEF = (

Avoid abbreviations "_GOOD_RESPONSE_SECURE_NONDEF"

> +def _create_request(request_def):
> +    conn = mock.MockConn('')

Use whole words "conn"

> diff --git a/WebKitTools/pywebsocket/test/test_util.py b/WebKitTools/pywebsocket/test/test_util.py
> +class UtilTest(unittest.TestCase):
> +    def test_get_stack_trace(self):
> +        self.assertEqual('None\n', util.get_stack_trace())
> +        try:
> +            a = 1 / 0  # Intentionally raise exception

Single space before # and make the comment end with a period.

> diff --git a/WebKitTools/pywebsocket/test/testdata/handlers/a_wsh.py b/WebKitTools/pywebsocket/test/testdata/handlers/a_wsh.py

Something more descriptive than "a" (ditto for "b_wsh.py" .. "g_wsh.py) would be nice. Name them according to what they do.

Also "wsh" is an abbreviation. (I think it stands for web socket handler...) It would be nice to make that more descriptive as well.


> diff --git a/WebKitTools/pywebsocket/test/testdata/handlers/sub/c_wsh.py b/WebKitTools/pywebsocket/test/testdata/handlers/sub/c_wsh.py

Why is this in a "sub" but a and b aren't?


> diff --git a/WebKitTools/pywebsocket/test/testdata/handlers/sub/d.py b/WebKitTools/pywebsocket/test/testdata/handlers/sub/d.py

Why doesn't d.py have the _wsh.py postfix like the others? I guess this is about testing the handler.


> diff --git a/WebKitTools/pywebsocket/test/testdata/handlers/sub/e_wsh.py b/WebKitTools/pywebsocket/test/testdata/handlers/sub/e_wsh.py
> +def web_socket_shake_hands(request):
> +    return True

Why return True here? In other places this just does "pass".

[Yuzo Fujishima]

Created attachment 41268 [details]
add mod_pywebsocket to test Web Sockets

[Yuzo Fujishima]

Hi, can anyone review the patch?

This patch tries to import code from http://code.google.com/p/pywebsocket/ .
The review decision should be either accept it or decline it as a whole.

- pywebsocket is licensed under BSD. It should be compatible with WebKit.

- pywebsocket is used also by Chromium project. It should be maintained
  reasonably well. (Actually, I'm one of the owners of the project.)

I've addressed David's review comments to the previous patch except
stylistic ones.

Yuzo

[David Levin]

Comment on attachment 41268 [details]
add mod_pywebsocket to test Web Sockets

Thanks for addressing my comments even though you are basically importing a 3rd party project into WebKit.

I think there are only a few things relevant to this review:
1. Is the license compatible with WebKit?
Yes, it is BSD.

2. Does this module implement something useful to WebKit?
After reviewing it, it appears to correctly implement the server side of Web Sockets which is useful for testing. (And there have been other reviews in the external project.)

3. Is it possible to integrate this into the layout test run without having every developer/machine install something?
Yes. It will use standalone.py for this.

Given the affirmative answers on these items. It seems like this should go in.

[WebKit Commit Bot]

Comment on attachment 41268 [details]
add mod_pywebsocket to test Web Sockets

Clearing flags on attachment: 41268

Committed r49672: <http://trac.webkit.org/changeset/49672>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.