Bug 68662 – Cleanup: Extract code to launch a Mac WebKit application into a common function

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

We use almost identical code in webkitdirs::run{MiniBrowser, Safari, TestWebKitAPI, WebKitTestRunner}() to launch the respective application on Mac OS X. We should extract the common code into a function, say runMacWebKitApp(), so that we can remove duplicate code.

[Daniel Bates]

Created attachment 108416 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 108416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108416&action=review

It would be nice if run-webkit-app used this function, too.

> Tools/Scripts/webkitdirs.pm:2013
> +    return system $appPath, @ARGV;

While you're at it, it would be nice to switch this to the direct object form of system() so that it will work correctly even if $appPath contains spaces and @ARGV is empty.

[Daniel Bates]

(In reply to comment #2)
> (From update of attachment 108416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108416&action=review
> 
> It would be nice if run-webkit-app used this function, too.

Will do.

> 
> > Tools/Scripts/webkitdirs.pm:2013
> > +    return system $appPath, @ARGV;
> 
> While you're at it, it would be nice to switch this to the direct object form of system() so that it will work correctly even if $appPath contains spaces and @ARGV is empty.

Will change to use direct object form of system().

[Daniel Bates]

Created attachment 108504 [details]
Patch

Updated patch based on Adam Roben's suggestions.

I added an optional argument $useOpenCommand to runMacWebKitApp() that when equal to USE_OPEN_COMMAND will use the open(1) to launch the application.

[Adam Roben (:aroben)]

Comment on attachment 108504 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108504&action=review

> Tools/Scripts/webkitdirs.pm:2028
> +    if ($useOpenCommand == USE_OPEN_COMMAND) {
> +        exec("open", "-a", $appPath, "--args", @ARGV);
> +    }

It's a little surprising that USE_OPEN_COMMAND also implies using exec() instead of system(). Maybe we should have a separate parameter for that?

[Daniel Bates]

(In reply to comment #5)
> (From update of attachment 108504 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108504&action=review
> 
> > Tools/Scripts/webkitdirs.pm:2028
> > +    if ($useOpenCommand == USE_OPEN_COMMAND) {
> > +        exec("open", "-a", $appPath, "--args", @ARGV);
> > +    }
> 
> It's a little surprising that USE_OPEN_COMMAND also implies using exec() instead of system(). Maybe we should have a separate parameter for that?

For now, I'll convert this to system() and pass -W to open(1) so that it blocks until the application it opened exits. This makes the behavior of run-webkit-app consistent with the behavior of {run, debug}-safari.

Originally I thought to just use exec() so that we don't fork(2) a process for open(1), which also fork(2)s a process for the application $appPath. That is, I was looking to minimizing fork(2)ing since it seemed excessive to call fork(2) twice.

[Daniel Bates]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 108504 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=108504&action=review
> > 
> > > Tools/Scripts/webkitdirs.pm:2028
> > > +    if ($useOpenCommand == USE_OPEN_COMMAND) {
> > > +        exec("open", "-a", $appPath, "--args", @ARGV);
> > > +    }
> > 
> > It's a little surprising that USE_OPEN_COMMAND also implies using exec() instead of system(). Maybe we should have a separate parameter for that?
> 
> For now, I'll convert this to system() and pass -W to open(1) so that it blocks until the application it opened exits. This makes the behavior of run-webkit-app consistent with the behavior of {run, debug}-safari.
> 

More importantly, by using system() we can makes the usage of runMacWebKitApp() consistent. It will always return an exit status to its caller. Although the exit status of open(1) is probably not as valuable as the exit status of the application :-/.

[Daniel Bates]

Committed r96448: <http://trac.webkit.org/changeset/96448>

[Alexey Proskuryakov]

I'm now getting an error from run-safari on Lion:

Use of uninitialized value $useOpenCommand in numeric eq (==) at /Users/ap/Safari/OpenSource/Tools/Scripts/webkitdirs.pm line 2026.

[Daniel Bates]

(In reply to comment #9)
> I'm now getting an error from run-safari on Lion:
> 
> Use of uninitialized value $useOpenCommand in numeric eq (==) at /Users/ap/Safari/OpenSource/Tools/Scripts/webkitdirs.pm line 2026.

Substituted "defined($useOpenCommand) && $useOpenCommand == USE_OPEN_COMMAND" for "$useOpenCommand == USE_OPEN_COMMAND" in the if-statement condition in runMacWebKitApp() (http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitdirs.pm?rev=96448#L2026).

Committed this change in changeset 96664 <http://trac.webkit.org/changeset/96664>.