[webkit-reviews] review granted: [Bug 168531] Build ImageDiff with host SDK : [Attachment 306840] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Apr 13 10:28:09 PDT 2017
David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 168531: Build ImageDiff with host SDK
https://bugs.webkit.org/show_bug.cgi?id=168531
Attachment 306840: Patch
https://bugs.webkit.org/attachment.cgi?id=306840&action=review
--- Comment #33 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 306840
--> https://bugs.webkit.org/attachment.cgi?id=306840
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=306840&action=review
r=me
> Tools/ChangeLog:8
> + ImageDiff should be built and ran with the host SDK, not the target
SDK.
Typo: ran => run
> Tools/Scripts/build-webkit:302
> + my @command = (getcwd() . "/Tools/Scripts/build-imagediff");
I prefer to use File::Spec->catfile(getcwd(), "Tools/Scripts/build-imagediff")
in these cases, but making this change doesn't block landing.
> Tools/Scripts/webkitdirs.pm:459
> + my @extract = ('--device', '--efl', '--gtk', '--ios', '--platform',
'--sdk', '--simulator', '--wincairo', 'SDKROOT', 'ARCHS', "'ARCHS", '"ARCHS');
Do the single/double quotes around ARCHS really survive here after being
processed by the shell? Just wondering if the last two items can be removed.
> Tools/Scripts/webkitdirs.pm:465
> + if (length($line) >= length($_) && substr($line, 0, length($_))
eq $_
> + && index($line, 'i386') == -1 && index($line, 'x86_64') ==
-1) {
This will break if devices ever existed that used Intel chips but didn't run
macOS.
Maybe make the subroutine name more Mac-specific:
extractNonMacOSHostConfiguration()?
> Tools/Scripts/webkitperl/webkitdirs_unittest/extractNonHostConfiguration.pl:1
> +#!/usr/bin/perl -w
Nice! Love the unit tests!
More information about the webkit-reviews
mailing list