Bug 68662 - Cleanup: Extract code to launch a Mac WebKit application into a common function
Summary: Cleanup: Extract code to launch a Mac WebKit application into a common function
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-22 16:10 PDT by Daniel Bates
Modified: 2011-10-04 16:55 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.71 KB, patch)
2011-09-22 16:14 PDT, Daniel Bates
aroben: review+
Details | Formatted Diff | Diff
Patch (6.64 KB, patch)
2011-09-23 11:54 PDT, Daniel Bates
aroben: 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 2011-09-22 16:10:21 PDT
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.
Comment 1 Daniel Bates 2011-09-22 16:14:25 PDT
Created attachment 108416 [details]
Patch
Comment 2 Adam Roben (:aroben) 2011-09-23 03:29:48 PDT
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.
Comment 3 Daniel Bates 2011-09-23 11:29:25 PDT
(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().
Comment 4 Daniel Bates 2011-09-23 11:54:25 PDT
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.
Comment 5 Adam Roben (:aroben) 2011-09-26 03:59:56 PDT
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?
Comment 6 Daniel Bates 2011-09-30 18:27:57 PDT
(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.
Comment 7 Daniel Bates 2011-09-30 18:35:46 PDT
(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 :-/.
Comment 8 Daniel Bates 2011-09-30 18:39:50 PDT
Committed r96448: <http://trac.webkit.org/changeset/96448>
Comment 9 Alexey Proskuryakov 2011-10-04 16:09:45 PDT
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.
Comment 10 Daniel Bates 2011-10-04 16:55:49 PDT
(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>.