Bug 59761 - fix pretty_patch_available and wdiff_available
Summary: fix pretty_patch_available and wdiff_available
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-28 16:37 PDT by Ojan Vafai
Modified: 2011-04-29 12:17 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.67 KB, patch)
2011-04-28 16:39 PDT, Ojan Vafai
no flags Details | Formatted Diff | Diff
Patch (9.79 KB, patch)
2011-04-29 11:11 PDT, Ojan Vafai
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-04-28 16:37:45 PDT
fix pretty_patch_available and wdiff_available
Comment 1 Ojan Vafai 2011-04-28 16:39:41 PDT
Created attachment 91588 [details]
Patch
Comment 2 Tony Chang 2011-04-28 16:47:43 PDT
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.
Comment 3 Dirk Pranke 2011-04-28 16:52:42 PDT
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().
Comment 4 Ojan Vafai 2011-04-29 11:11:27 PDT
Created attachment 91705 [details]
Patch
Comment 5 Dirk Pranke 2011-04-29 12:13:26 PDT
Comment on attachment 91705 [details]
Patch

looks great! Thanks for cleaning this up.
Comment 6 Ojan Vafai 2011-04-29 12:17:46 PDT
Committed r85347: <http://trac.webkit.org/changeset/85347>