[webkit-reviews] review granted: [Bug 133986] Teach run-{safari, webkit-app} about iOS Simulator : [Attachment 233242] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 133986: Teach run-{safari, webkit-app} about iOS Simulator
https://bugs.webkit.org/show_bug.cgi?id=133986

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

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


r=me

> Tools/ChangeLog:8
> +	   Extract the logic from old-run-webkit-tests to install an launch
{DumpRenderTree, WebKitTestRunnerApp}.app

Typo:  "an" => "and"

> Tools/ChangeLog:25
> +	   (mobilesafariBundle): Added.

Nit:  mobileSafariBundle()?

> Tools/ChangeLog:29
> +	   (loadIPhoneSimulatorNotificationIfNeeded): Added.

Can we rename this to loadIOSSimulatorNotificationIfNeeded()?

I guess this would require the Perl module to be renamed, too, unless we wanted
to update this code first, then change the Perl module at a later time.

I'm okay either way (not renaming it now, or renaming the webkitdirs.pm code in
anticipation of renaming the module later).

> Tools/ChangeLog:34
> +	   (findOrCreateSimulatorForIPhoneDevice): Added.

Can we rename this to findOrCreateSimulatorForIOSDevice()?

> Tools/Scripts/run-webkit-app:53
> +if (willUseIOSDeviceSDKWhenBuilding()) {
> +    die "Only running app in iOS Simulator is supported now.";
> +}
> +if (willUseIOSSimulatorSDKWhenBuilding()) {
> +    # FIXME: We should ensure that $appPath is a path to an app bundle as
opposed to the actual executable file.
> +    exit exitStatus(runIOSWebKitApp($appPath));
> +}
>  exit exitStatus(runMacWebKitApp($appPath, USE_OPEN_COMMAND));

It seems like this 'switch' code could be extracted into a method in
webkitdirs.pm, since there is similar code in runSafari().

Not necessary to land this patch, though.

> Tools/Scripts/webkitdirs.pm:63
> +	  &findOrCreateSimulatorForIPhoneDevice

IPhone => IOS

> Tools/Scripts/webkitdirs.pm:102
> +my $didLoadIPhoneSimulatorNotification;

IPhone => IOS (if we're okay renaming this before the Perl module)

> Tools/Scripts/webkitdirs.pm:2156
> +    # Use MobileSafari.app in product directory if present (good for Safari
development team).

Nit: Parenthetical text in comment probably isn't necessary here.

> Tools/Scripts/webkitdirs.pm:2163
> +sub plistPathFromBundle($)

Should this be named plistPathFromIOSBundle() since Mac OS X bundles are
usually deeper (with the interesting stuff under Contents)?

> Tools/Scripts/webkitdirs.pm:2171
> +sub appIdentiferFromBundle($)

Should this be named appIdentiferFromIOSBundle() since Mac OS X bundles are
usually deeper (with the interesting stuff under Contents)?

> Tools/Scripts/webkitdirs.pm:2179
> +sub appDisplayNameFromBundle($)

Should this be named appDisplayNameFromIOSBundle() since Mac OS X bundles are
usually deeper (with the interesting stuff under Contents)?

> Tools/Scripts/webkitdirs.pm:2221
> +    return (grep {$_->{name} eq $simulatorName} iOSSimulatorDevices())[0];

Might be useful to warn() if more than one item is returned here (which would
indicate we're creating duplicate devices):
(grep {$_->{name} eq $simulatorName} iOSSimulatorDevices())

> Tools/Scripts/webkitdirs.pm:2284
> +    my ($appBundle, $simulatedDevice, $simulatorOptions) = @_;

This needs a "require Foundation;" line in the method in case it's called
before "require IPhoneSimulatorNotification;" (which is what make this code
work in old-run-webkit-tests).

This code is all PerlObjC bridge code, and will fail if the Foundation module
is not included before it's run.  (And we don't want "use Foundation;" here
since that's a compile-time directive for Perl and will break everyone that
doesn't have the Foundation module.)