[webkit-reviews] review denied: [Bug 55535] NRWT - implement Linux Hardy 64-bit port : [Attachment 84357] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 55535: NRWT - implement Linux Hardy 64-bit port
https://bugs.webkit.org/show_bug.cgi?id=55535

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84357&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:48
> +    FALLBACK_PATHS = {
> +	   'x86_64': ['chromium-linux-x86_64', 'chromium-linux',
'chromium-win', 'chromium', 'win', 'mac'],
> +	   'x86': ['chromium-linux', 'chromium-win', 'chromium', 'win', 'mac'],

> +    }

Nit: I would just move this inline and do something like:
fallback = ['chromium-linux', 'chromium-win', 'chromium', 'win', 'mac']
if self._architecture == 'x64_64':
    fallback.insert(0, 'chromium-linux-x86_64')

To avoid duplication of the fallback list.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:60
> +	       self._architecture = port_name[port_name.index('-linux-') + 7:]

Nit: I would use .split('-')[-1] to avoid the 7 constant needing to keep in
sync with -linux-.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:64
> +    def _get_architecture(self):

This names makes it seem like this will return the value of _architecture. 
Maybe name this _determine_architecture or something.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:67
> +	   if not self._filesystem.exists(driver_path):
> +	       return 'x86'

I think we should log a message to the user if we get here.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:68
> +	   file_output = self._executive.run_command(['file', driver_path])

What happens if 'file' doesn't exist or for some reason driver_path is not
readable?  It would be nice if we could print the captured stdout+stderr.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:73
> +	   raise AssertionError('unrecognized or unsupported architecture')

I think we should still fallback to x86 and just log a message to the user.  If
people want to run this on say FreeBSD or SunOS, we should still run.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux_unittest.py:67
> +	   self.assert_architecture(file_output='ELF 32-bit LSB executable',
> +				    expected_architecture='x86')

Should we have negative tests for stuff like chromium-gpu-linux-x86 or
chromium-x86-linux?