Bug 37388 – Factor WebKitPort out of MacPort to allow for WinPort

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Factor WebKitPort out of MacPort to allow for WinPort

[Adam Barth]

Created attachment 53042 [details]
Patch

[Dirk Pranke]

Comment on attachment 53042 [details]
Patch

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

[Adam Barth]

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

Python has a notion of public and non-public:

[[
      Use one leading underscore only for non-public methods and instance
      variables.
]]

Technically, each file is its own package, so these are inter-package uses.

> Unfortunately Python doesn't give us a good way to indicate
> this. We can make them public and document them correctly if necessary.

I think its simpler just to use non-underscored names.  The main point of the underscores is to limit the amount of other code you need to think about when reasoning about them.

> > +    # 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.

Yes, that's a good goal.  In fact, that's exactly the problem that Executive is intended to solve.  The main idea is that you don't want to interact with process creation / destruction using statics, e.g.:

+         self._process = subprocess.Popen(start_cmd,

because executing that line of code always creates a process.  Instead, you want to indirect through a non-static:

+         self._process = self._executive.make_me_a_process(...)

During testing, you can replaces self._executive with a mock object that doesn't actually create a process.  We seem to have a mock port object already, but that doesn't mean we need to hang all of our mocks off of Port.

In any case, the current containment of subprocess is incomplete because lots of code calls subprocess.Popen (or an equivalent) directly.  If we want to be able to test the code without creating processes, then we'll want to direct all those instances through a non-static that can be replaced with a mock.

At a higher level, the code (as written) puts kill_process on the Port object but not create_process.  The reason for that appears to be that Python has a nice cross-platform API for create_process but not a great API for kill_process.  That's somehow an implementation detail of the Python library and shouldn't really be driving the architecture of our code.

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

Well, it needs to be mediated by some abstraction.  It's unclear why port should be the end-all, be-all abstraction.  There's already a many-to-many relationship between ports and platforms (e.g., Chromium, Qt, and Gtk all run on Linux, but Apple, Chromium, and Qt also run on Mac).  If we take the current approach, we'll have to copy/paste all the Linux and Mac specific bits into all the port classes, which doesn't really scale.  Instead, we need some kind of architecture that can represent a many-to-many relationship.  We can do it with inheritance, but it will be messy because it require multiple inheritance.   It's much easier to do it with composition, which is what I'm trying to get at with these FIXMEs.

> 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?

Executive is an abstraction for interacting with processes.  We've mostly used it for process creation, but it should also handle process destruction.  Other system-specific bits should probably be represent by other abstractions.

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

Thanks for the review.  I'm glad that we're getting this opportunity to cross-pollinate ideas.  Hopefully we can make webkitpy into a more cohesive package instead of two or three independent silos.

[Eric Seidel (no email)]

Comment on attachment 53042 [details]
Patch

Yay!  Here comes Windows/Qt/Gtk support. :)

Still more lovin needed, but this is a great start.

[Adam Barth]

Comment on attachment 53042 [details]
Patch

Clearing flags on attachment: 53042

Committed r57423: <http://trac.webkit.org/changeset/57423>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.