Bug 68499 – Extract common gdb code into its own function; Remove script gdb-safari

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

[Daniel Bates]

Duplicate code is used in webkitdirs::{debugMiniBrowser, debugWebKitTestRunner, runSafari}() and in the script gdb-safari to launch gdb to debug an application. Instead of duplicating code we should extract the common code into a function.

Moreover, I propose removing the script gdb-safari since it's functionality can be folded into the functionality afforded by debug-safari.

For completeness, gdb-safari was added in changeset 9395 <http://trac.webkit.org/changeset/9395> and debug-safari was added in changeset 27878 <http://trac.webkit.org/changeset/27878>.

[Daniel Bates]

Created attachment 108105 [details]
Patch

[Mark Rowe (bdash)]

Please don’t delete gdb-safari without first giving webkit-dev advance notice. I’m sure that many people use it and would like to be aware of the change before it happens.

[Daniel Bates]

(In reply to comment #2)
> Please don’t delete gdb-safari without first giving webkit-dev advance notice. I’m sure that many people use it and would like to be aware of the change before it happens.

<https://lists.webkit.org/pipermail/webkit-dev/2011-September/018021.html>

[Daniel Bates]

Comment on attachment 108105 [details]
Patch

Clearing review flag as this patch no longer applies. Will rebase patch.

[Daniel Bates]

Created attachment 110321 [details]
Patch

I also changed in runSafari():

my $webKitLauncherPath = File::Spec->catfile(productDir(), "WebKit.exe");
$result = system { $webKitLauncherPath } $webKitLauncherPath, @ARGV;
return $result if $result;

To:

my $webKitLauncherPath = File::Spec->catfile(productDir(), "WebKit.exe");
return system { $webKitLauncherPath } $webKitLauncherPath, @ARGV;

since the former would implicitly return undef if $result was 0 and undef has a truth value that is equivalent to 0.

[David Kilzer (:ddkilzer)]

Comment on attachment 110321 [details]
Patch

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

r=me

> Tools/Scripts/webkitdirs.pm:2067
> +        return system { $vcBuildPath } $vcBuildPath, "/debugexe", "\"$safariPath\"", @ARGV;

This returns a different value on success (e.g., when system() returns 0) than runSafari() does, but I think that's okay since it's wrapped by exitStatus() in its new use.

> Tools/Scripts/webkitdirs.pm:2084
> +        return system { $webKitLauncherPath } $webKitLauncherPath, @ARGV;

This change causes the method to return 0 instead 1 when system() returns 0 on Windows.  This also seems fine since runSafari() is only used (from a quick grep) in the run-safari script, and that is also wrapped by exitStatus().

> Tools/Scripts/webkitdirs.pm:2102
> +        debugMacWebKitApp(File::Spec->catfile(productDir(), "MiniBrowser.app", "Contents", "MacOS", "MiniBrowser")); # This call never returns.

Nit: Might be nice to think of a name for debugMacWebKitApp() that removes the need for the comment.  Like execMacWebKitAppForDebugging()?

Not necessary to land the patch, though.

> Tools/Scripts/webkitdirs.pm:2127
> +        debugMacWebKitApp(File::Spec->catfile(productDir(), "WebKitTestRunner")); # This call never returns.

Ditto.

[Daniel Bates]

(In reply to comment #6)
> (From update of attachment 110321 [details])
> 
> > Tools/Scripts/webkitdirs.pm:2102
> > +        debugMacWebKitApp(File::Spec->catfile(productDir(), "MiniBrowser.app", "Contents", "MacOS", "MiniBrowser")); # This call never returns.
> 
> Nit: Might be nice to think of a name for debugMacWebKitApp() that removes the need for the comment.  Like execMacWebKitAppForDebugging()?

Will rename debugMacWebKitApp() to execMacWebKitAppForDebugging() since execMacWebKitAppForDebugging() better describes the behavior of this function.

[Daniel Bates]

Committed r97562: <http://trac.webkit.org/changeset/97562>