Bug 62256 – webkitpy: clean up code prior to functional changes for server startup/shutdown

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Dirk Pranke]

webkitpy: clean up code prior to functional changes for server startup/shutdown

[Dirk Pranke]

Created attachment 96351 [details]
Patch

[Dirk Pranke]

Created attachment 96354 [details]
ready for review

[Dirk Pranke]

Created attachment 96357 [details]
add missing ServerError

[Tony Chang]

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.

[Dirk Pranke]

(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 :).

[Dirk Pranke]

Created attachment 96675 [details]
update w/ review feedback

[Tony Chang]

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

[Dirk Pranke]

Created attachment 96840 [details]
update w/ more review feedback

[Tony Chang]

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.

[Dirk Pranke]

Created attachment 96990 [details]
fix more typos found by tony

[Dirk Pranke]

Comment on attachment 96990 [details]
fix more typos found by tony

clearing review flag, this fix won't work as is.

[Dirk Pranke]

finally fixed in bug 62829.

*** This bug has been marked as a duplicate of bug 62829 ***