Bug 133986 - Teach run-{safari, webkit-app} about iOS Simulator
Summary: Teach run-{safari, webkit-app} about iOS Simulator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-06-17 10:34 PDT by Daniel Bates
Modified: 2014-06-20 10:23 PDT (History)
6 users (show)

See Also:


Attachments
Patch (19.74 KB, patch)
2014-06-17 10:36 PDT, Daniel Bates
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2014-06-17 10:34:16 PDT
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.
Comment 1 Daniel Bates 2014-06-17 10:36:54 PDT
Created attachment 233242 [details]
Patch
Comment 2 David Kilzer (:ddkilzer) 2014-06-18 15:52:09 PDT
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.)
Comment 3 Daniel Bates 2014-06-19 09:47:48 PDT
(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.
Comment 4 Daniel Bates 2014-06-20 10:21:20 PDT
Committed r170189: <http://trac.webkit.org/changeset/170189>
Comment 5 Daniel Bates 2014-06-20 10:23:28 PDT
<rdar://problem/17344733>