Bug 76360 – webkitpy: make PortFactory.get() fully data-driven

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dirk Pranke]

break out from other bug.

[Dirk Pranke]

Created attachment 122588 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 122588 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122588&action=review

> Tools/Scripts/webkitpy/layout_tests/port/factory.py:63
> +    PORT_CLASSES = (
> +        'chromium_gpu.ChromiumGpuLinuxPort',
> +        'chromium_gpu.ChromiumGpuMacPort',
> +        'chromium_gpu.ChromiumGpuWinPort',
> +        'chromium_linux.ChromiumLinuxPort',
> +        'chromium_mac.ChromiumMacPort',
> +        'chromium_win.ChromiumWinPort',
> +        'dryrun.DryRunPort',
> +        'efl.EflPort',
> +        'google_chrome.GoogleChromeLinux32Port',
> +        'google_chrome.GoogleChromeLinux64Port',
> +        'google_chrome.GoogleChromeMacPort',
> +        'google_chrome.GoogleChromeWinPort',
> +        'gtk.GtkPort',
> +        'mac.MacPort',
> +        'mock_drt.MockDRTPort',
> +        'qt.QtPort',
> +        'test.TestPort',
> +        'win.WinPort',
> +        )

I wonder if the class-lookup model which Command/MultiCommandTool uses would be a better fit than this hard-coded list in factory.py.  Your goal here is to get rid of the big if statement w/o having to import everythign up-front I assume?  Is importing every port from factory.py a problem in practice?

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 122588 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122588&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/factory.py:63
> > +    PORT_CLASSES = (
> > +        'chromium_gpu.ChromiumGpuLinuxPort',
> > +        'chromium_gpu.ChromiumGpuMacPort',
> > +        'chromium_gpu.ChromiumGpuWinPort',
> > +        'chromium_linux.ChromiumLinuxPort',
> > +        'chromium_mac.ChromiumMacPort',
> > +        'chromium_win.ChromiumWinPort',
> > +        'dryrun.DryRunPort',
> > +        'efl.EflPort',
> > +        'google_chrome.GoogleChromeLinux32Port',
> > +        'google_chrome.GoogleChromeLinux64Port',
> > +        'google_chrome.GoogleChromeMacPort',
> > +        'google_chrome.GoogleChromeWinPort',
> > +        'gtk.GtkPort',
> > +        'mac.MacPort',
> > +        'mock_drt.MockDRTPort',
> > +        'qt.QtPort',
> > +        'test.TestPort',
> > +        'win.WinPort',
> > +        )
> 
> I wonder if the class-lookup model which Command/MultiCommandTool uses would be a better fit than this hard-coded list in factory.py.  Your goal here is to get rid of the big if statement w/o having to import everythign up-front I assume?  Is importing every port from factory.py a problem in practice?

At one point in time I was worried that Ports would not necessarily work on all platforms (e.g., you couldn't import "win" on a Mac) but we eventually decided to make that required to work.

So, I'm not worried about importing everything up front, but I did/do want to get rid of the big if statement, and this approach let me keep everything in one place. It does have the downside that typos might be slightly harder to catch and debug.

An alternative would be to have two lists ... a list of import statements and then the list of class names in PORT_CLASSES. Slightly more redundant and more lines of code, but maybe a bit clearer.

I took a quick look at multicommandtool and see that you're using something called the __subclasses__ method, which I wasn't aware of. I'll look into that some more. I'm a bit leery of accidentally pulling in classes I don't want in the list (e.g., I'll have to figure out how to strip out the ChromiumPort and WebKitPort) and/or making things a little less explicit but I'll definitely try it out to see how it works.

[Eric Seidel (no email)]

Comment on attachment 122588 [details]
Patch

I think this is OK.

[Eric Seidel (no email)]

You can see that the MultiCommandTool architecture also filters out commands based on a self.hidden bool (at least for display in the main --help).  We could use something similar for filtering out ports we wanted to hide.

I'm not certain it's better than an explicit list, but it does allow the explicit list to be a list of imports in one place.

[Dirk Pranke]

Committed r105184: <http://trac.webkit.org/changeset/105184>