[webkit-reviews] review granted: [Bug 35062] check sys deps better on chromium ports : [Attachment 48943] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 35062: check sys deps better on chromium ports
https://bugs.webkit.org/show_bug.cgi?id=35062

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
I plan to add PrettyPatch support (a ruby version of wdiff which is inluded in
webkit.org's repository) to the base.Port at some point.  Then ChromiumMac can
use that instead of wdiff if they so desire.

We eventually need to rename "target" to "configuration" to match webkit/mac
terminology:
 188	     return self._build_path(self._options.target, 'image_diff')

I'm surprised that the _build_path function doesn't automatically include the
target already:
 182	     return self._build_path(self._options.target, 'test_shell')

seems silly to have to include it everywhere.  Maybe you just want a
_current_build_path or _build_path when passed "none" for the "target" option
should default to the current?

Why are both debug and release required?
 106	 def _check_build_up_to_date(self, target):

We tend to avoid space-based indent alignment in webkit, because you always end
up having to change it later:
 59	    result = self._check_lighttpd_install() and result
 60	    result = self._check_apache_install()   and result
 61	    result = self._check_wdiff_install()    and result


Shouldn't this just be on the base port?
 45 def check_file_exists(path_to_file, str):

Otherwise looks OK.