[webkit-reviews] review granted: [Bug 133663] old-run-webkit-tests: Create CoreSimulator device on demand and find it by name : [Attachment 232802] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted David Farler
<dfarler at apple.com>'s request for review:
Bug 133663: old-run-webkit-tests: Create CoreSimulator device on demand and
find it by name
https://bugs.webkit.org/show_bug.cgi?id=133663

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

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=232802&action=review


r=me

> Tools/Scripts/old-run-webkit-tests:1390
> -    my $deviceName = architecture() eq 'i386' ? "iPhone 5" : "iPhone 5s";
> +    my $deviceName = architecture() eq 'i386' ? "iPhone 5 WebKit Tester" :
"iPhone 5s WebKit Tester";
> +
> +    my $deviceType = "com.apple.CoreSimulator.SimDeviceType.iPhone-5";
> +    $deviceType .= "s" if architecture() eq "x86_64";

Nit: It might be clearer to just make this an if/else statement:

    my $deviceName = "iPhone 5 WebKit Tester";
    my $deviceType = "com.apple.CoreSimulator.SimDeviceType.iPhone-5";
    if (architecture() eq 'x86_64') {
	$deviceName = "iPhone 5s WebKit Tester";
	$deviceType = "com.apple.CoreSimulator.SimDeviceType.iPhone-5s";
    }

> Tools/Scripts/webkitdirs.pm:1122
> +    return "$ENV{HOME}/Library/Developer/CoreSimulator/Devices";

Should this be $ENV{CFFIXED_USER_HOME} instead?

> Tools/Scripts/webkitdirs.pm:1127
> +    use Foundation;

I'm hoping this won't be evaluated until the method is called, since this could
break non-Mac users of the script.

> Tools/Scripts/webkitdirs.pm:1166
> +    my $tries = 5;
> +    while (1) {
> +	   my @devices = iOSSimulatorDevices();
> +	   foreach my $device (@devices) {
> +	       return $device if $device->{name} eq $name and
$device->{deviceType} eq $deviceTypeId and $device->{runtime} eq $runtimeId;
> +	   }
> +	   sleep 5;
> +	   $tries -= 1;
> +	   if ($tries <= 0) {
> +	       die "Device $name $deviceTypeId $runtimeId wasn't found in " .
iOSSimulatorDevicesPath();
> +	   }
> +    }

This would be simpler with a for() loop that just falls through:

    for (my $tries = 5; $tries > 0; $tries--) {
	[...]
	sleep 5;
    }

    die "Device $name $deviceTypeId $runtimeId wasn't found in " .
iOSSimulatorDevicesPath();