[webkit-reviews] review granted: [Bug 76357] webkitpy: add determine_full_port_name(), clean up port.__init__() : [Attachment 122587] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 76357: webkitpy: add determine_full_port_name(), clean up port.__init__()
https://bugs.webkit.org/show_bug.cgi?id=76357

Attachment 122587: Patch
https://bugs.webkit.org/attachment.cgi?id=122587&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=122587&action=review


It's slightly hard to read this patch because it has the earlier patch mixed
in.  Generally, this looks like a reasonable patch.  My main concern is that
the mac and win ports seem to have some very similar looking code that probably
should be shared.

> Tools/ChangeLog:101
> +2012-01-15  Dirk Pranke  <dpranke at chromium.org>

Looks like you've got two ChangeLogs here.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:116
> +	   assert port_name in ('chromium-linux', 'chromium-gpu-linux',
> +				'chromium-linux-x86', 'chromium-linux-x86_64',
> +				'chromium-gpu-linux-x86_64')

Can we avoid listing information about all the subclasses here?

> Tools/Scripts/webkitpy/layout_tests/port/mac.py:50
> +	       # FIXME: This may be wrong, since options is a global property,
and the port_name is specific to this object?
> +	       if options and not hasattr(options, 'webkit_test_runner'):
> +		   options.webkit_test_runner = True

Yuck.  options is usually immutable.

> Tools/Scripts/webkitpy/layout_tests/port/win.py:59
> +    @classmethod
> +    def determine_full_port_name(cls, host, options, port_name):
> +	   if port_name.endswith('-wk2'):
> +	       # FIXME: This may be wrong, since options is a global property,
and the port_name is specific to this object?
> +	       if options and not hasattr(options, 'webkit_test_runner'):
> +		   options.webkit_test_runner = True
> +	       port_name = port_name.replace('-wk2', '')
> +	   if port_name.endswith('win'):
> +	       if host.platform.os_name != 'win':
> +		   return 'win'
> +	       return port_name + '-' + host.platform.os_version
> +	   return port_name

Can we share some of this code with MacPort?