Bug 59761 – fix pretty_patch_available and wdiff_available

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

fix pretty_patch_available and wdiff_available

[Ojan Vafai]

Created attachment 91588 [details]
Patch

[Tony Chang]

Comment on attachment 91588 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/base.py:211
> +    def check_wdiff(self, logging=True):
> +        return False

What is the logging param for?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:110
> -        result = self._check_wdiff_install() and result
> +        result = self.check_wdiff() and result

I think mac can use wdiff too.

[Dirk Pranke]

Patch looks like it's along the lines we discussed and so generally looks fine, but it needs to be implemented more broadly or I'm worried about the inconsistencies that might arise (shouldn't be much additional work). See the specific comments.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:211
> +        return False

wdiff actually exists (or can) on nearly every port, so we need to provide a better default implementation. There's also a _check_wdiff in chromium_mac that should be renamed, and you should probably add a check_wdiff() call to the chromium implementation of check_build().

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux_unittest.py:84
> +

This should probably be a generic test in port/port_testcase.py instead.

Tony - the logging parameter controls whether the routine will log any messages if it finds wdiff isn't available; it is called with a value of True by port.check_build().

[Ojan Vafai]

Created attachment 91705 [details]
Patch

[Dirk Pranke]

Comment on attachment 91705 [details]
Patch

looks great! Thanks for cleaning this up.

[Ojan Vafai]

Committed r85347: <http://trac.webkit.org/changeset/85347>