webkitpy: clean up code prior to functional changes for server startup/shutdown
Created attachment 96351 [details] Patch
Created attachment 96354 [details] ready for review
Created attachment 96357 [details] add missing ServerError
Comment on attachment 96357 [details] add missing ServerError View in context: https://bugs.webkit.org/attachment.cgi?id=96357&action=review Looks fine in general. Just a few small questions. > Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:44 > + name = 'apache' Nit: I would give this a ALL_CAPS name since it's a static constant. > Tools/Scripts/webkitpy/layout_tests/port/http_server.py:44 > + name = 'lighttpd' Same here. Maybe SERVER_NAME or something? > Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:48 > + pid_file = None Should this be in __init__? It's not clear to me what makes it different from self._process. > Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:40 > import http_server > +import http_server_base Should we switch these to full imports while we're here? > Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:201 > + if self.pid_file: Do we need this check? It looks like self.pid_file should always be set in __init__. > Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:211 > + elif self.pid_file: Same question as above.
(In reply to comment #4) > (From update of attachment 96357 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96357&action=review > > Looks fine in general. Just a few small questions. > > > Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:44 > > + name = 'apache' > > Nit: I would give this a ALL_CAPS name since it's a static constant. > It's not a static constant in all of the subclasses; in websocket_server.py it is possible for it to be overridden per-object if we're running the TLS version. The reason is it declared where it is is because it's effectively a readonly property after initialization. I can move it into the __init__ call if you think that would make things clearer. > > Tools/Scripts/webkitpy/layout_tests/port/http_server.py:44 > > + name = 'lighttpd' > > Same here. Maybe SERVER_NAME or something? > As above. > > Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:48 > > + pid_file = None > > Should this be in __init__? It's not clear to me what makes it different from self._process. > pid_file will eventually be the path of the file containing the current running process ID. It is also a readonly public property, but I can move it into init as well self._process is the actual process object returned from executive/subprocess. The pid_file object is only really used in the websocket implementation but eventually will be used everywhere, to be consistent. > > Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:40 > > import http_server > > +import http_server_base > > Should we switch these to full imports while we're here? > Done. > > Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:201 > > + if self.pid_file: > > Do we need this check? It looks like self.pid_file should always be set in __init__. > You'll notice that the default value of pidfile in the __init__ is None, so, yes, we still need it. > > Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:211 > > + elif self.pid_file: > > Same question as above. Same answer :).
Created attachment 96675 [details] update w/ review feedback
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 96357 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=96357&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:44 > > > + name = 'apache' > > > > Nit: I would give this a ALL_CAPS name since it's a static constant. > > It's not a static constant in all of the subclasses; in websocket_server.py it is possible for it to be overridden per-object if we're running the TLS version. > The reason is it declared where it is is because it's effectively a readonly property after initialization. I can move it into the __init__ call if you think that would make things clearer. It is a static constant. In websocket_server.py, you're setting self.name which is not the same as PyWebSocket.name. It sounds like it should be moved into __init__. > > > Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:48 > > > + pid_file = None > > > > Should this be in __init__? It's not clear to me what makes it different from self._process. > > > > pid_file will eventually be the path of the file containing the current running process ID. It is also a readonly public property, but I can move it into init as well It sounds like pid_file should also be moved into __init__ with an accessor method to make it read only. > self._process is the actual process object returned from executive/subprocess. The pid_file object is only really used in the websocket implementation but eventually will be used everywhere, to be consistent. Sorry, I was unclear. I meant I didn't understand why _process was in __init__, but pid_file wasn't. They have the same lifetime so it seemed like they should be initialized in the same place.
Created attachment 96840 [details] update w/ more review feedback
Comment on attachment 96840 [details] update w/ more review feedback View in context: https://bugs.webkit.org/attachment.cgi?id=96840&action=review > Tools/ChangeLog:12 > + removing unused code paths and functions, using 'name' and > + 'pid_file' as public properties on the server objects, and using Nit: Please update the part about 'name' and 'pid_file' being public properties. > Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:44 > class LayoutTestApacheHttpd(http_server_base.HttpServerBase): > + name = 'apache' This is no longer needed, right? > Tools/Scripts/webkitpy/layout_tests/port/http_server.py:91 > + raise http_server_base.ServerError('Lighttpd already running') Nit: Should this use self._name? > Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:200 > + if self.pid_file: > + self._filesystem.write_text_file(self.pid_file, "%d" % self._process.pid) This is now self._pid_file, right? > Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:210 > + elif self.pid_file: > + pid = int(self._filesystem.read_text_file(self.pid_file)) Ditto.
Created attachment 96990 [details] fix more typos found by tony
Comment on attachment 96990 [details] fix more typos found by tony clearing review flag, this fix won't work as is.
finally fixed in bug 62829. *** This bug has been marked as a duplicate of bug 62829 ***