Currently, we set "-x", "/websocket/tests/cookies" for pywebsocket invocation to specify /websocket/tests/cookies is cgi directory, so contents will be served as CGI instead of static file. Ideally, we should be able to set his to "/websocket/tests". That way, no one will get surprised by trying to add a .pl test to another subdirectory. Of course, pywebsocket would need to learn how to distinguish .html and .pl files.
http://code.google.com/p/pywebsocket/issues/detail?id=43 in upstream issue tracker.
Created attachment 48879 [details] Patch
+ specify --server_host 127.0.0.1 -x /websocket/tests What is --server-host, and why is it good to specify it? I think I know the answer, but please explain it in ChangeLog. Seems untidy to have a mix of short and long option names. > if '..' in self.path: > return False Do functions that we use for path manipulation perform tilde expansion (for home directory)? Writing a truly secure server is out of scope for the test tool, it's just too similar to existing check for "..", so I wanted to ask. + scriptfile = self.translate_path(self.path.split('?', 2)[0]) I don't understand what this line does. Someone who is better familiar with Python might understand it, and give their r+.
(In reply to comment #3) > + specify --server_host 127.0.0.1 -x /websocket/tests > > What is --server-host, and why is it good to specify it? I think I know the > answer, but please explain it in ChangeLog. Ok. > Seems untidy to have a mix of short and long option names. Changed to use long option names. > > if '..' in self.path: > > return False > > Do functions that we use for path manipulation perform tilde expansion (for > home directory)? Writing a truly secure server is out of scope for the test > tool, it's just too similar to existing check for "..", so I wanted to ask. I believe it doesn't perform tilde expansion. > + scriptfile = self.translate_path(self.path.split('?', 2)[0]) > > I don't understand what this line does. Someone who is better familiar with > Python might understand it, and give their r+.
Created attachment 48963 [details] Patch
Comment on attachment 48963 [details] Patch > + "--server_host", "127.0.0.1", > + "--port", "$webSocketPort", > + "--document_root", "$webSocketHandlerDir", > + "--scan_dir", "$webSocketHandlerScanDir", > + "--websock_handlers_map_file", "$webSocketHandlerMapFile", > + "--cgi_paths", "/websocket/tests", > + "--log_file", "$logFile", I think that the usual Unix style for long options is to use dashes, not underscores.
(In reply to comment #6) > (From update of attachment 48963 [details]) > > + "--server_host", "127.0.0.1", > > + "--port", "$webSocketPort", > > + "--document_root", "$webSocketHandlerDir", > > + "--scan_dir", "$webSocketHandlerScanDir", > > + "--websock_handlers_map_file", "$webSocketHandlerMapFile", > > + "--cgi_paths", "/websocket/tests", > > + "--log_file", "$logFile", > > I think that the usual Unix style for long options is to use dashes, not > underscores. We've used userscores... Since this options is in pywebsocket, and no WebKitTools/Scripts uses underscores, should I fix it to use dashes?
Comment on attachment 48963 [details] Patch Underscores are a strange Google convention. WebKit-land uses dashes. Also, you patch does not appear to apply cleanly to top of tree.
(In reply to comment #8) > (From update of attachment 48963 [details]) > Underscores are a strange Google convention. WebKit-land uses dashes. I think pywebsocket is third party code, so don't need to flow WebKit-style. > Also, > you patch does not appear to apply cleanly to top of tree. Hmm, this is webkit-patch (with git) bug, when moving files and directory becomes empty? webkit-patch apply-attachment says $ ./WebKitTools/Scripts/webkit-patch apply-attachment 48963 [details] Cleaning working directory Updating working directory Reading Keychain for bugs.webkit.org account and password. Click "Allow" to continue... Logging in as ukai@chromium.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=48963&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=34879&ctype=xml Processing 1 patch from 1 bug. Processing patch 48963 from bug 34879. Failed to run "['/Users/ukai/src/chromium-webkit2/src/third_party/WebKit/WebKitTools/Scripts/svn-apply']" exit_code: 2 Last 500 characters of output: h fuzz 3. patching file WebKitTools/Scripts/run-webkit-tests patching file WebKitTools/Scripts/run-webkit-websocketserver patching file WebKitTools/Scripts/webkitpy/layout_tests/port/websocket_server.py patching file WebKitTools/pywebsocket/mod_pywebsocket/standalone.py patching file WebKitTools/pywebsocket/setup.py Could not open 'LayoutTests/websocket/tests/cookies' to list files: 0 at /Users/ukai/src/chromium-webkit2/src/third_party/WebKit/WebKitTools/Scripts/svn-apply line 314, <> line 417. Failed to run "['/Users/ukai/src/chromium-webkit2/src/third_party/WebKit/WebKitTools/Scripts/svn-apply']" exit_code: 2 ukai-macpro:WebKit ukai$ git reset --hard
Created attachment 48992 [details] Patch
Created attachment 49188 [details] Patch
FYI, we now have a README.webkit file in pywebsocket's directory that can be updated whenever local changes are made to pywebsocket: https://bugs.webkit.org/show_bug.cgi?id=35198
Created attachment 49714 [details] Patch
Created attachment 49885 [details] Patch
I think I could review this patch if you explained what this line did: + scriptfile = self.translate_path(self.path.split('?', 2)[0]) It seems worth both a short comment, and a longer explanation in the bug.
Created attachment 49969 [details] Patch
(In reply to comment #15) > I think I could review this patch if you explained what this line did: > > + scriptfile = self.translate_path(self.path.split('?', 2)[0]) it gets real pathname from request path. self.path contains request path in HTTP request header self.path.split('?', 2)[0] strips query parameter if any self.translate_path() converts resource name into pathname on filesystem. > It seems worth both a short comment, and a longer explanation in the bug. added comments
I'm not sure what the svn-apply bug which this is tripping over is: Could not open 'LayoutTests/websocket/tests/cookies' to list files: 0 at /mnt/git/webkit-chromium-ews/WebKitTools/Scripts/svn-apply line 314, <> line 715.
(In reply to comment #18) > I'm not sure what the svn-apply bug which this is tripping over is: > Could not open 'LayoutTests/websocket/tests/cookies' to list files: 0 at > /mnt/git/webkit-chromium-ews/WebKitTools/Scripts/svn-apply line 314, <> line > 715. Hmm, this is because the patch is trying to remove the directory?
Sounds like bug 34871. Bug 14590 could also possibly be related.
Comment on attachment 49969 [details] Patch r=me. Please land this with svn - as far as I know, git wrappers won't preserve history of file copies correctly.
Committed r55719: <http://trac.webkit.org/changeset/55719>