| Summary: | Web Sockets Test Infrastructure Part 1/3: Server | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Yuzo Fujishima <yuzo> |
| Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | ap, commit-queue, jorlow, levin, mike, mrowe |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: | |||
|
Description
Yuzo Fujishima
2009-07-21 01:34:25 PDT
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 Created attachment 33157 [details]
Web Socket Server for LayoutTests.
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. 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 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.
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. +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? 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
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?
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 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 Comment on attachment 34124 [details]
Web Socket Server, in desired coding style, with shorter file names.
Clearing review flag per comment above.
(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. (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 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 > Whom should I talk to?
Please e-mail webkit-dev.
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(-)
Created attachment 40353 [details]
add pywebsocket to test Web Socket
Comment on attachment 40353 [details]
add pywebsocket to test Web Socket
We do not allow code under the Apache license at this time. r-.
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. 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. (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.) According to bdash, dual licensing would be an acceptable solution. Created attachment 40493 [details]
add mod_pywebsocket to test Web Sockets
Now mod_pywebsocket is licensed under BSD. Can you take another look? Yuzo Created attachment 40705 [details]
add mod_pywebsocket to test Web Sockets
I'll try to look at this tonight. (Feel free to ping me to remind me if you see no action.) Hi, David, Can you take a look? The files are being imported from http://code.google.com/p/pywebsocket/ Yuzo I'm looking. This is complicated by the fact that the patch is huge. 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 > +2009-10-01 Yuzo Fujishima <yuzo@google.com> > + > + 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". Created attachment 41268 [details]
add mod_pywebsocket to test Web Sockets
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 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.
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> All reviewed patches have been landed. Closing bug. |