Summary: | WebKitTools/Scripts/run-launcher doesn't speak --gdk | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nigel Tao <nigel.tao.gnome> | ||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | ||||||||||||
Priority: | P2 | ||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Nigel Tao
2007-07-24 05:05:03 PDT
Created attachment 15660 [details]
Patch
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! 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. (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); 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.
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. (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 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! 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 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. Created attachment 15807 [details]
Patch
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. (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 on attachment 15807 [details]
Patch
r- based on comments from Dave and Adam.
Created attachment 15848 [details]
Patch
Comment on attachment 15848 [details]
Patch
r=me
$ svn commit WebKitTools/ChangeLog WebKitTools/Scripts/run-launcher Sending WebKitTools/ChangeLog Sending WebKitTools/Scripts/run-launcher Transmitting file data .. Committed revision 24883. |