Bug 41776 - Need to have a way to specify different results for Windows XP and 7
Summary: Need to have a way to specify different results for Windows XP and 7
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-07 10:04 PDT by Brian Weinstein
Modified: 2010-07-07 19:27 PDT (History)
2 users (show)

See Also:


Attachments
[PATCH] Fix (3.21 KB, patch)
2010-07-07 10:13 PDT, Brian Weinstein
aroben: review-
bweinstein: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Fix - Take 2 (4.02 KB, patch)
2010-07-07 10:52 PDT, Brian Weinstein
aroben: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Fix (one last pass) (4.94 KB, patch)
2010-07-07 14:43 PDT, Brian Weinstein
aroben: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Small nits picked, so I can set cq+ (4.67 KB, patch)
2010-07-07 14:55 PDT, Brian Weinstein
aroben: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Weinstein 2010-07-07 10:04:34 PDT
There are some international text tests that are failing on Windows 7, while passing on Windows XP due to differences in international fonts.

We need some way to have different results for Window XP and Windows 7, like we have for Tiger, Leopard, and Snow Leopard.
Comment 1 Brian Weinstein 2010-07-07 10:13:10 PDT
Created attachment 60750 [details]
[PATCH] Fix
Comment 2 Adam Roben (:aroben) 2010-07-07 10:27:04 PDT
Comment on attachment 60750 [details]
[PATCH] Fix

It looks like you've made it so XP will look in win-xp and win, and Vista will look in win-vista and win, and 7 will look in win.

I think it would be better and less confusing to match Mac by having it be:

XP: win-xp, win-vista, win-7, win
Vista: win-vista, win-7, win
7: win-7, win

You could basically adapt the code that uses @macPlatforms in old-run-webkit-tests.

I think "isWindowsXP" and "isWindowsVista" and "isWindows7" are better than using the "Win" nickname in the function names.

> +sub determineWinVersion()
> +{
> +    return if $winVersion;
> +
> +    if (!isCygwin()) {
> +        $winVersion = -1;
> +        return;
> +    }
> +
> +    my $versionString = `uname -s`;
> +    $versionString =~ /(\d.\d)/;

This will allow any character between the two digits. You should escape the . with a backslash.

> +
> +    my @splitVersion = split(/\./, $1);
> +    @splitVersion >= 2 or die "Invalid version $versionString";
> +    $winVersion = {
> +            "major" => $splitVersion[0],
> +            "minor" => $splitVersion[1]
> +    };
> +}

I don't think there's any reason to split the major and minor version. Just keep it as a string and do things like (winVersion() eq "6.1") in the is* functions.
Comment 3 Brian Weinstein 2010-07-07 10:51:44 PDT
(In reply to comment #2)
> (From update of attachment 60750 [details])
> It looks like you've made it so XP will look in win-xp and win, and Vista will look in win-vista and win, and 7 will look in win.
> 
> I think it would be better and less confusing to match Mac by having it be:
> 
> XP: win-xp, win-vista, win-7, win
> Vista: win-vista, win-7, win
> 7: win-7, win

Fixed, and updated the ChangeLog to describe this.

> 
> You could basically adapt the code that uses @macPlatforms in old-run-webkit-tests.

Done, refactored that code a little bit.

> 
> I think "isWindowsXP" and "isWindowsVista" and "isWindows7" are better than using the "Win" nickname in the function names.

Fixed.

> 
> > +sub determineWinVersion()
> > +{
> > +    return if $winVersion;
> > +
> > +    if (!isCygwin()) {
> > +        $winVersion = -1;
> > +        return;
> > +    }
> > +
> > +    my $versionString = `uname -s`;
> > +    $versionString =~ /(\d.\d)/;
> 
> This will allow any character between the two digits. You should escape the . with a backslash.

Done.

> 
> > +
> > +    my @splitVersion = split(/\./, $1);
> > +    @splitVersion >= 2 or die "Invalid version $versionString";
> > +    $winVersion = {
> > +            "major" => $splitVersion[0],
> > +            "minor" => $splitVersion[1]
> > +    };
> > +}
> 
> I don't think there's any reason to split the major and minor version. Just keep it as a string and do things like (winVersion() eq "6.1") in the is* functions.

Done. Thanks!
Comment 4 Brian Weinstein 2010-07-07 10:52:22 PDT
Created attachment 60753 [details]
[PATCH] Fix - Take 2
Comment 5 Adam Roben (:aroben) 2010-07-07 11:04:04 PDT
Comment on attachment 60753 [details]
[PATCH] Fix - Take 2

> +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.
Comment 6 Brian Weinstein 2010-07-07 14:31:50 PDT
(In reply to comment #5)
> (From update of attachment 60753 [details])
> > +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.
> 

Fixed.

> > @@ -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.

Refactored, so not an issue anymore.

> 
> 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;
> 

Fixed.

> > +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.

Fixed. Thanks!
Comment 7 Brian Weinstein 2010-07-07 14:43:49 PDT
Created attachment 60780 [details]
[PATCH] Fix (one last pass)
Comment 8 Adam Roben (:aroben) 2010-07-07 14:46:54 PDT
Comment on attachment 60780 [details]
[PATCH] Fix (one last pass)

> -    if ($platform =~ /^mac-/) {
> +    
> +    my $isSpecificMac = $platform =~ /^mac-/;
> +    my $isWin = $platform =~ /^win/;

Doing:

my $isMac = $platform =~ /^mac/;

would have the same behavior, I believe (only "mac" will get added to @platforms). It might be slightly easier to understand, too, since it would make Mac and Win the same.

r=me
Comment 9 Brian Weinstein 2010-07-07 14:55:02 PDT
Created attachment 60783 [details]
[PATCH] Small nits picked, so I can set cq+
Comment 10 Adam Roben (:aroben) 2010-07-07 14:55:50 PDT
Comment on attachment 60783 [details]
[PATCH] Small nits picked, so I can set cq+

r=me
Comment 11 Brian Weinstein 2010-07-07 19:27:15 PDT
Landed in r62742.