[webkit-reviews] review granted: [Bug 41776] Need to have a way to specify different results for Windows XP and 7 : [Attachment 60753] [PATCH] Fix - Take 2

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 41776: Need to have a way to specify different results for Windows XP and 7
https://bugs.webkit.org/show_bug.cgi?id=41776

Attachment 60753: [PATCH] Fix - Take 2
https://bugs.webkit.org/attachment.cgi?id=60753&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +my @winPlatforms = ("win-xp", "win-vista", "win-7", "win");

If you add "mac-snowleopard" and "mac" as the last two items in this list, you
can get rid of the code that appends them to the list in
expectedDirectoryForTest.

> @@ -1852,13 +1861,14 @@ sub buildPlatformResultHierarchy()
>      mkpath($platformTestDirectory) if ($platform eq "undefined" && !-d
"$platformTestDirectory");
>  
>      my @platforms;
> -    if ($platform =~ /^mac-/) {
> +    if ($platform =~ /^mac-/ || $platform =~ /^win-/) {
> +	   my @platformList = $platform =~  /^mac-/ ? @macPlatforms :
@winPlatforms;

You have an extra space after =~ here.

I think it would be better to do something like this:

my $isMac = $platform =~ /^mac-/;
my $isWin = $platform =~ /^win-/;

if ($isMac || $isWin) {
    my @platformList = $isMac ? @macPlatforms : @winPlatforms;

> +sub isWindows7()
> +{
> +    return isCygwin() && winVersion() eq "6.1";
> +}
> +
> +sub isWindowsVista()
> +{
> +    return isCygwin() && winVersion() eq "6.0";
> +}
> +
> +sub isWindowsXP()
> +{
> +    return isCygwin() && winVersion() eq "5.1";
> +}

No need for the isCygwin() checks here, given the way that winVersion() works.