Bug 14745

Summary: WebKitTools/Scripts/run-launcher doesn't speak --gdk
Product: WebKit Reporter: Nigel Tao <nigel.tao.gnome>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
ddkilzer: review-
Patch
ddkilzer: review-
Patch
mrowe: review-
Patch ddkilzer: review+

Description Nigel Tao 2007-07-24 05:05:03 PDT
From WebKitTools/Scripts, I can currently do
./build-webkit --gdk

But this doesn't work
./run-launcher --gdk http://news.google.com/
Comment 1 Nigel Tao 2007-07-24 05:05:39 PDT
Created attachment 15660 [details]
Patch
Comment 2 David Kilzer (:ddkilzer) 2007-07-24 07:45:24 PDT
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!
Comment 3 Nigel Tao 2007-07-24 18:55:51 PDT
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.
Comment 4 David Kilzer (:ddkilzer) 2007-07-24 21:42:54 PDT
(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);
Comment 5 Nigel Tao 2007-07-26 06:26:06 PDT
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.
Comment 6 Darin Adler 2007-07-26 08:01:31 PDT
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.
Comment 7 David Kilzer (:ddkilzer) 2007-07-26 09:30:47 PDT
(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.

Comment 8 David Kilzer (:ddkilzer) 2007-07-26 09:40:34 PDT
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!
Comment 9 David Kilzer (:ddkilzer) 2007-07-27 09:15:28 PDT
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
Comment 10 Nigel Tao 2007-07-30 05:57:15 PDT
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.
Comment 11 Nigel Tao 2007-08-02 06:54:19 PDT
Created attachment 15807 [details]
Patch
Comment 12 Adam Treat 2007-08-02 08:44:28 PDT
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.
Comment 13 David Kilzer (:ddkilzer) 2007-08-02 09:52:54 PDT
(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.
Comment 14 Mark Rowe (bdash) 2007-08-06 02:37:04 PDT
Comment on attachment 15807 [details]
Patch

r- based on comments from Dave and Adam.
Comment 15 Nigel Tao 2007-08-06 06:38:26 PDT
Created attachment 15848 [details]
Patch
Comment 16 David Kilzer (:ddkilzer) 2007-08-06 09:51:40 PDT
Comment on attachment 15848 [details]
Patch

r=me
Comment 17 David Kilzer (:ddkilzer) 2007-08-06 11:13:38 PDT
$ svn commit WebKitTools/ChangeLog WebKitTools/Scripts/run-launcher 
Sending        WebKitTools/ChangeLog
Sending        WebKitTools/Scripts/run-launcher
Transmitting file data ..
Committed revision 24883.