[webkit-reviews] review granted: [Bug 48103] new-run-webkit-tests: clean up duplicate, ugly code in port/chromium_gpu*.py : [Attachment 100545] Try again - fold chromium_gpu into other chromium* ports.

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 48103: new-run-webkit-tests: clean up duplicate, ugly code in
port/chromium_gpu*.py
https://bugs.webkit.org/show_bug.cgi?id=48103

Attachment 100545: Try again - fold chromium_gpu into other chromium* ports.
https://bugs.webkit.org/attachment.cgi?id=100545&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100545&action=review


This is *way* better than waht we have, but could still be better.  Please fix
the things mentioned in review before landing.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:89
> +	   if self.get_option('accelerated_compositing') is None:
> +	       self._options.accelerated_compositing = True

We have a function set_default_option which does exactly this.	(I think you
even wrote the function originally.)

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:96
> +	   # on the bots.
> +	   if self.get_option('builder_name') is not None and not ' - GPU' in
self._options.builder_name:
> +	       self._options.builder_name += ' - GPU'

This is bad.  We should generate the builder names from component parts on
demand, not store the whole thing and modify it blindly.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:122
> +	   fallback_paths =
(self.GRAPHICS_TYPE_FALLBACK_PATHS[self._graphics_type] +
> +			    
self.ARCHITECTURE_FALLBACK_PATHS[self._architecture] +
> +			     self.VERSION_FALLBACK_PATHS[self._version] +
> +			     self.BASE_FALLBACK_PATHS)

See the implementation of this function in webkit.py.  This is OK too... we'll
have to see how it looks in the end.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:178
> +	   if self._graphics_type == 'gpu':
> +	       return 1

the gpu port seems super lame.