[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 #3 from Adam Barth <abarth at webkit.org>  2010-04-10 12:38:00 PST ---
> 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.

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