[webkit-reviews] review denied: [Bug 135589]=?UTF-8?Q?=20Move=20run=2Dsafari=20=E2=80=94simulator=20over=20to=20simctl=20launch=20?=: [Attachment 236071] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied David Farler <dfarler at apple.com>'s
request for review:
Bug 135589: Move run-safari —simulator over to simctl launch
https://bugs.webkit.org/show_bug.cgi?id=135589

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=236071&action=review


I'm r-'ing this patch since it seems like we're missing logic to install the
built MobileSafari. This patch also adds a function called
installIOSWebKitAppInSimulator(), but doesn't use it.

> Tools/ChangeLog:3
> +	   Move run-safari —simulator over to simctl launch

Nit: "—" => "--" (notice these are two dash characters)

> Tools/ChangeLog:14
> +	   (runIOSWebKitAppInSimulator):
> +	   Use simctl launch.

It seems sufficient to collapse these lines into a single line that reads:

(runIOSWebKitAppInSimulator): Use simctl launch.

> Tools/ChangeLog:16
> +	   (installIOSWebKitAppInSimulator):
> +	   Use simctl install.

Similarly, it seems sufficient to write this line as:

(installIOSWebKitAppInSimulator): Use simctl install.

> Tools/Scripts/webkitdirs.pm:2200
> +	   sleep 2;

This is OK as-is. I wish we could wait for some kind of exit status code or
stdout output message instead of polling the file system every 2 seconds.

> Tools/Scripts/webkitdirs.pm:2201
> +    } while ($device->{state} ne '3');

Nit: ' => "

Unless you explicitly want to prevent string interpolation, please use double
quotes instead of single quotes for string literals.

> Tools/Scripts/webkitdirs.pm:2202
> +    sleep 5;

How did you come to pick 5 seconds to sleep? I take it you picked it from
experimenting. Regardless, I suggest we add an inline comment to this line that
explains why we are sleeping as well as how this time was chosen. Maybe
something like: Wait for services to start; 5 seconds is sufficient in
practice.

> Tools/Scripts/webkitdirs.pm:2248
> +sub runIOSWebKitAppInSimulator

Did you intend to remove the prototype for this function? Do you intend to make
the second argument required? If so, then we should remove line 2258 (below).

> Tools/Scripts/webkitdirs.pm:2269
> +    foreach my $key (keys(%simulatorENV)) {

Nit: We tend to omit the parentheses around keys() in our Perl code, and hence
I would write this line as:

foreach my $key (keys %simulatorENV) {

> Tools/Scripts/webkitdirs.pm:2274
> +    system('xcrun', '-sdk', 'iphonesimulator', 'simctl', 'launch',
$simulatedDevice->{UDID}, $appIdentifier, @$applicationArguments);

Nit: ' => "

Unless you explicitly want to prevent string interpolation, please use double
quotes instead of single quotes for string literals.

> Tools/Scripts/webkitdirs.pm:2275
> +    return $?;

Instead of explicitly querying the special variable $? for the exit status I
suggest that we inline the previous line into this line and use exitStatus() to
retrieve the actual exit status, such that this line reads:

return exitStatus(system("xcrun", "-sdk", "iphonesimulator", "simctl",
"launch", $simulatedDevice->{UDID}, $appIdentifier, @$applicationArguments));

The benefit of this approach is that it improves the readability of the code by
making use of the return value of system() instead of indirectly querying for
the exit status.

> Tools/Scripts/webkitdirs.pm:2292
> +sub installIOSWebKitAppInSimulator($$;$)

Where is this function used?

Also, the third argument of this function isn't used in the proposed changes.

> Tools/Scripts/webkitdirs.pm:2300
> +    system('xcrun', '-sdk', 'iphonesimulator', 'simctl', 'install',
$simulatedDevice->{UDID}, $appBundle);
> +    return $?;

Similarly, I would write this as:

return exitStatus(system("xcrun", "-sdk", "iphonesimulator", "simctl",
"install", $simulatedDevice->{UDID}, $appBundle);