[Webkit-unassigned] [Bug 37388] Factor WebKitPort out of MacPort to allow for WinPort

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37388





--- Comment #2 from Dirk Pranke <dpranke at chromium.org>  2010-04-10 11:25:24 PST ---
(From update of attachment 53042)
>  
> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/apache_http_server.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/apache_http_server.py
> index 3cf0f5b..45dce8c 100644
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/apache_http_server.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/apache_http_server.py
> @@ -79,6 +79,7 @@ class LayoutTestApacheHttpd(http_server_base.HttpServerBase):
>          error_log = self._cygwin_safe_join(output_dir, "error_log.txt")
>          document_root = self._cygwin_safe_join(test_dir, "http", "tests")
>  
> +        # FIXME: We shouldn't be calling a protected method of _port_obj!
>          executable = self._port_obj._path_to_apache()

I'm not terribly troubled by this. I consider these routines to have
package-level scope, so that callers shouldn't call them but other members of
the package can. Unfortunately Python doesn't give us a good way to indicate
this. We can make them public and document them correctly if necessary.

> +    # FIXME: This doesn't have anything to do with WebKit.
>      def _kill_process(self, pid):
>          """Forcefully kill the process.
>          os.kill(pid, signal.SIGKILL)
>  
> +    # FIXME: This doesn't have anything to do with WebKit.
>      def _kill_all_process(self, process_name):
>          # On Mac OS X 10.6, killall has a new constraint: -SIGNALNAME or
>          # -SIGNALNUMBER must come first.  Example problem:

For routines like this, it's true that these are platform-specific routines
that aren't strictly tied to a WebKit implementation, but we need to be careful
to ensure that "generic" code (like run_webkit_tests and the layout_package
code) do not call the platform-specific equivalents directly.  The original
design intent (hopefully still preserved, although I may have bungled things at
some point and allowed exceptions to creep in) allows for implementations like
the "test" port that don't ever start or stop processes. More realistic
examples would be something like an iPhone or Android implementation where the
actual testing is done on an embedded device and no direct process controlling
is done by the upper layers (everything needs to be mediated through the Port
abstractions).

That said, it's true that these routines are here because the interface mixes
"port-specific" and "platform-specific" logic, and while code like
http_server.py still needs some platform abstraction to call. Maybe Executive
or some other class is a better fit for this?

Otherwise, this looks like about right (although I am not a reviewer).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.