Bug 71403 - META: strip the Port class of its godlike powers
Summary: META: strip the Port class of its godlike powers
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: NRWT
Depends on: 103827 75037 78301
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-02 14:17 PDT by Dirk Pranke
Modified: 2012-12-01 17:56 PST (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2011-11-02 14:17:06 PDT
the Port class in webkitpy.layout_tests.port predates much of the other infrastructure in webkitpy and now does too many things that should probably be done elsewhere. This bug is intended to track that cleanup.

Some possible improvements:

* the 'filesystem', 'executive', and 'config' arguments should be passed into the constructors, either explicitly or via a host object. They should not be public properties and other parts of the code should not retrieve references to filesystem, executive, or config objects from the Port class

* at least the following routines should probably be moved elsewhere or reworked to push more of the logic out of the Port directory:
  * wdiff_available(), check_image_diff(), check_pretty_patch(), check_wdiff(), check_httpd()
  * webkit_base(), path_from_webkit_base(), layout_tests_dir(), webkit_base(), path_from_webkit_base()
  * maybe_make_directory(), split_test()
  * operating_system(), version(), graphics_type(), architecture(), test_configuration(), all_test_configurations(), configuration_specifier_macros(), all_baseline_variants() - these should somehow be merged into the test_configuration() objects and split out somehow
  * get_option(), set_option() - these should be better integrated into the options parameter that is passed to the constructor and we need a better way to initialize port-specific default options.
  * results_directory() and default_results_directory()
  * show_results_html_file()

This list is merely meant as a first draft, and not intended to be exhaustive or definitive.

Here's the rationale I have in mind for what should stay in the port package. These principles are open for debate.

1) "Ports" in the webkitpy sense capture the combination of an implementation of a "webkit port" in the Apple/Chromium/Qt/Gtk sense and the "port" in the operating system sense (mac/win/linux/etc. plus architecture like arm/x86/x86_64). A single concrete port specifies a single combination of those attributes, but of course mixes in behaviour that is shared (e.g., WebKitPort, ChromiumPort are really mix-ins). Most if not all of the operating-system-type functionality should actually be implemented in webkit.common.system, however.

1) Anything needed to be able to implement the Test Port properly belongs on the Port interface, so that unit and integration tests can run quickly, reliably, and portably. The primary reason for the Test port to exist is to demonstrate the routines that may need to be overridden to implement a port that works radically differently than the "real" ports.

2) Anything that manages how tests are found and named (this allows for handling filesystem path differences like unix/win separators, and also allows for in-memory constructs like those used in the Test port. They ensure that more esoteric ports that don't necessarily have a full suite of tests embedded under a single LayoutTests directory  in the filesystem can be supported without having to change the rest of NRWT.

3) Anything that is actually overridden in one of the Port subclasses.
Comment 1 Eric Seidel (no email) 2011-11-02 14:46:56 PDT
I think this is a great start.  This will be a great meta-bug for doing this work.  Thanks.
Comment 2 Dirk Pranke 2011-11-02 15:01:16 PDT
I will note that cleaning up the way that port objects are constructed (all the name parsing and handling of defaults and so on) is probably tangled up in this somehow but I would like to leave this out of scope as much as possible.
Comment 3 Dirk Pranke 2012-07-13 19:30:20 PDT
resetting the owner in case someone else wants to take a look, as these bugs aren't on my immediate to-do list.
Comment 4 Dirk Pranke 2012-12-01 17:56:28 PST
I now think the stuff in 2) should probably be a separate object, mostly to make the Port conceptually smaller and make it clearer what tends to vary between ports. I've filed bug 103827 for that effort.