Bug 133986 – Teach run-{safari, webkit-app} about iOS Simulator

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

We should make run-{safari, webkit-app} able to launch Safari on iOS or an existing iOS WebKit-based app in the iOS Simulator using the built WebKit framework.

[Daniel Bates]

Created attachment 233242 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 233242 [details]
Patch

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

[Daniel Bates]

(In reply to comment #2)
> > Tools/ChangeLog:8
> > +        Extract the logic from old-run-webkit-tests to install an launch {DumpRenderTree, WebKitTestRunnerApp}.app
> 
> Typo:  "an" => "and"
> 

Will fix.

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

Will fix.

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

I'm going to defer renaming this function and the Perl module to bug #134062 so as to keep the focus of this patch on teaching run-{safari, webkit-app} about iOS Simulator.

> > Tools/ChangeLog:34
> > +        (findOrCreateSimulatorForIPhoneDevice): Added.
> 
> Can we rename this to findOrCreateSimulatorForIOSDevice()?
> 

Will rename 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.
> 

Will extract common code into a shared subroutine called webkitdirs::runIOSWebKitApp that will be used by both this script and webkitdirs::runSafari(). Then we can simplify the code in run-webkit-app to read:

...
if (isIOSWebKit()) {
    exit exitStatus(runIOSWebKitApp($appPath));
}
exit exitStatus(runMacWebKitApp($appPath, USE_OPEN_COMMAND)); 

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

Will fix.

> > Tools/Scripts/webkitdirs.pm:102
> > +my $didLoadIPhoneSimulatorNotification;
> 
> IPhone => IOS (if we're okay renaming this before the Perl module)
> 

As aforementioned I plan to defer to renaming this function and the Perl module to bug #134062.

> > 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)?

Notice that this function works for both Mac and iOS app bundles.

> 
> > 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)?
> 

Ditto.

> > 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)?
> 

Ditto.

> > 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())
> 

Will update this function to read:

sub iosSimulatorDeviceByName($)
{
    my ($simulatorName) = @_;
    my @devices = grep {$_->{name} eq $simulatorName} iOSSimulatorDevices();
    my $deviceToUse = $devices[0];
    if (@devices > 1) {
        print "Warning: Found more than one simulator device named '$simulatorName'.\n";
        print "         Using simulator device with UDID: $deviceToUse->{UDID}.\n";
        print "         To see the list of simulator devices, run:\n";
        print "         xcrun --sdk iphonesimulator simctl list\n";
    }
    return $deviceToUse;
}

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

I'll fix this by moving the call to loadIPhoneSimulatorNotificationIfNeeded() that appears later in this function to the top of this function such that we call it before defining $makeNSDictionaryFromHash.

[Daniel Bates]

Committed r170189: <http://trac.webkit.org/changeset/170189>

[Daniel Bates]

<rdar://problem/17344733>