Bug 16315 – FindSafari Needs Path-Only Option

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

I wrote a little script (see Bug #16314) to launch Drosera.  Unfortunately, the "FindSafari.exe /printSafariLauncher" command outputs both the environment needed to run as well as the launch command for Safari.

FindSafari would be more useful to something like the Drosera script if it could just output the environment commands (perhaps with a "/printSafariEnvironment" option)

What I currently get from FindSafari is this:
====================================================
$ ./FindSafari.exe  /printSafariLauncher
@echo off
mkdir 2>NUL "%TMP%\WebKitNightly\Safari.resources"
xcopy /y /i /d "C:\Program Files\Safari\Safari.exe" "%TMP%\WebKitNightly"
xcopy /y /i /d /e "C:\Program Files\Safari\Safari.resources" "%TMP%\WebKitNightl
y\Safari.resources"
set PATH="%CD%;C:\Program Files\Safari\;%PATH%"
"%TMP%\WebKitNightly\Safari.exe" /customWebKit
====================================================

What I would *LIKE* is this:
====================================================
$ ./FindSafari.exe  /printSafariLauncher
@echo off
mkdir 2>NUL "%TMP%\WebKitNightly\Safari.resources"
xcopy /y /i /d "C:\Program Files\Safari\Safari.exe" "%TMP%\WebKitNightly"
xcopy /y /i /d /e "C:\Program Files\Safari\Safari.resources" "%TMP%\WebKitNightl
y\Safari.resources"
set PATH="%CD%;C:\Program Files\Safari\;%PATH%"
====================================================

[Brent Fulgham]

Created attachment 17759 [details]
Modification to FindSafari.exe to emit environment information only.

The attached patch modifies FindSafari to avoid issuing the command to launch Safari.  This is useful to allow use of Drosera on the Windows platform (see Bug #16314).

[Adam Roben (:aroben)]

Comment on attachment 17759 [details]
Modification to FindSafari.exe to emit environment information only.

+    if (!printEnvironment)
+    {

Our code style guidelines say that the brace should be on the same line as the if. See <http://webkit.org/coding/coding-style.html>.

It seems a little strange that if you pass both /printSafariLauncher and /printSafariEnvironment, you only get the environment and don't get a launcher at all. Perhaps /printSafariLauncher should override /printSafariEnvironment?

r- so that the above can be considered.

[Brent Fulgham]

Created attachment 17877 [details]
Updated patch based on initial review.

Updated patch based on aroben's review comments.

[Adam Roben (:aroben)]

Comment on attachment 17877 [details]
Updated patch based on initial review.

Looks like you uploaded the same patch again by accident.

[Brent Fulgham]

Created attachment 17883 [details]
Updated based on review.

[Brent Fulgham]

Comment on attachment 17883 [details]
Updated based on review.

This should be the correct patch.

[Adam Roben (:aroben)]

Comment on attachment 17883 [details]
Updated based on review.

+        * ChangeLog:

We normally don't list the ChangeLog in the list of changed files.

r=me. Thanks for the patch!

[Mark Rowe (bdash)]

Landed in r28788.