Bug 14745 – WebKitTools/Scripts/run-launcher doesn't speak --gdk

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nigel Tao]

From WebKitTools/Scripts, I can currently do
./build-webkit --gdk

But this doesn't work
./run-launcher --gdk http://news.google.com/

[Nigel Tao]

Created attachment 15660 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 15660 [details]
Patch

Thanks for the patch, Nigel!  In the future, please set the "review?" flag on patches to make sure they're reviewed in a timely manner.

>-  $launcherPath = "$launcherPath/WebKitTools/GdkLauncher";
>+  $launcherPath = "$launcherPath/WebKitTools/GdkLauncher/GdkLauncher";

This looks correct.

>+        else if (currArg[0] == '-')  // currArg looks like an (unknown) -flag, not a URL
>+            ;  // No-op.  We'll just silently ignore unknown flags

This looks like a different bug?  Why is this change needed?

However, this patch is missing a ChangeLog entry.  See this page for details on how to create one using the prepare-ChangeLog script:

http://webkit.org/coding/contributing.html

Setting review- flag for no ChangeLog and unknown change to main.cpp.  Please make these fixes and repost the patch.  Thanks!

[Nigel Tao]

The if (currArg[0] == '-') check is because, otherwise, if I run
./run-launcher --gdk
the --gdk gets passed to GdkLauncher, which then tries to interpret "--gdk" as a URL, which means a blank page comes up instead of the default page (www.google.com).

But, yeah, I'll re-cook the patch with a ChangeLog entry.  Thanks for educating a webkit noob.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> The if (currArg[0] == '-') check is because, otherwise, if I run
> ./run-launcher --gdk
> the --gdk gets passed to GdkLauncher, which then tries to interpret "--gdk" as
> a URL, which means a blank page comes up instead of the default page
> (www.google.com).

Please modify the Perl script (run-launcher) to consume the "--gdk" switch so that it's not passed to GdkLauncher.  That way you don't have to change main.cpp.  Thanks!

Something like this would work:

my @args = grep(!/^(--gdk)$/, @ARGV);

[Nigel Tao]

Created attachment 15690 [details]
Patch

This patch created by the svn-create-patch script.

There is no corresponding ChangeLog entry because:
$ ./prepare-ChangeLog 
  Running status to find changed, added, or removed files.
  Reviewing diff to determine which lines changed.
  Extracting affected function names from source files.
No ChangeLog found for run-launcher.

I've set the review? flag.  The bugzilla attachment form also has a Requestee field, but I don't know who to put in there, so I've left it blank.

[Darin Adler]

Please note: The prepare-ChangeLog script is provided as a convenience to help you write the ChangeLog message. If it doesn't work, you can just use any text editor to write one by hand.

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> There is no corresponding ChangeLog entry because:
> $ ./prepare-ChangeLog 
>   Running status to find changed, added, or removed files.
>   Reviewing diff to determine which lines changed.
>   Extracting affected function names from source files.
> No ChangeLog found for run-launcher.

Please file a new bug!  I assume you were in the WebKitTools/Scripts directory when you ran this?  Usually I run that script like this (from the "top-level" directory with WebKitTools, WebCore, JavaScriptCore, etc. directories in it):

$ ./WebKitTools/Scripts/prepare-ChangeLog WebKitTools/Scripts

> I've set the review? flag.  The bugzilla attachment form also has a Requestee
> field, but I don't know who to put in there, so I've left it blank.

That's fine--it's more common to leave that blank.

[David Kilzer (:ddkilzer)]

Comment on attachment 15690 [details]
Patch

Looks great!  I'm going to give it an r- since I'd like to see a ChangeLog entry, and I have one minor quibble below.

>+  # Strip --gdk from the arg-list, since otherwise GdkLauncher will try to
>+  # interpret it as a URL.
>+  @args = grep(!/^(--gdk)$/, @ARGV);

Please use @args instead of @ARGV here.  If someone changes the script later to process @args differently above, then this code won't pick up those changes.  Thanks!

[David Kilzer (:ddkilzer)]

Note that part of this fix was committed in r24721, but please rework the patch to allow the --gdk argument as well!

http://trac.webkit.org/projects/webkit/changeset/24721

[Nigel Tao]

On second thoughts, I think that the better place to fix this (the --gdk flag being passed to GdkLauncher and subsequently interpreted as a URL) is in WebKitTools/GdkLauncher/main.cpp rather than in WebKitTools/Scripts/run-launcher.  You could patch run-launcher to explicitly scrub the --gdk argument, but you would miss any other flags that run-launcher takes (or could take, in the future), and GdkLauncher would (incorrectly) try to interpret them as a URL.

The patch would look something like:
------------------------------------------------------
--- WebKitTools/GdkLauncher/main.cpp    (revision 24771)
+++ WebKitTools/GdkLauncher/main.cpp    (working copy)
@@ -119,7 +119,7 @@
             dumpRenderTree = true;
         else if (stringIsEqual(currArg, "-dumprendertree"))
             dumpRenderTree = true;
-        else
+        else if (currArg[0] != '-')  // currArg looks like an (unknown) -flag, not a URL
             url = autocorrectURL(currArg);
     }
------------------------------------------------------

If you agree that this is the right approach, then I'll prepare a proper ChangeLog entry etc.

[Nigel Tao]

Created attachment 15807 [details]
Patch

[Adam Treat]

This isn't the right approach IMO.

You should patch the perl to scrub this argument.

run-launcher really shouldn't be taking that many arguments.  And all the rest you want to pass to the app.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> On second thoughts, I think that the better place to fix this (the --gdk flag
> being passed to GdkLauncher and subsequently interpreted as a URL) is in
> WebKitTools/GdkLauncher/main.cpp rather than in
> WebKitTools/Scripts/run-launcher.  You could patch run-launcher to explicitly
> scrub the --gdk argument, but you would miss any other flags that run-launcher
> takes (or could take, in the future), and GdkLauncher would (incorrectly) try
> to interpret them as a URL.

Sorry for the late reply, Nigel.

I agree with Adam's comments in Comment #12 that Perl should remove the argument before calling gdklauncher.

In my opinion, gdklauncher should be using getopt() (or similar) routine for parsing command-line switches instead of using custom code.  Then you have much more robust command-line switch parsing, and you can decide whether to ignore errors or report them.

[Mark Rowe (bdash)]

Comment on attachment 15807 [details]
Patch

r- based on comments from Dave and Adam.

[Nigel Tao]

Created attachment 15848 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 15848 [details]
Patch

r=me

[David Kilzer (:ddkilzer)]

$ svn commit WebKitTools/ChangeLog WebKitTools/Scripts/run-launcher 
Sending        WebKitTools/ChangeLog
Sending        WebKitTools/Scripts/run-launcher
Transmitting file data ..
Committed revision 24883.