14745 2007-07-24 05:05:03 -0700 WebKitTools/Scripts/run-launcher doesn't speak --gdk 2007-08-06 11:13:38 -0700 1 1 1 Unclassified WebKit New Bugs 523.x (Safari 3) PC Linux RESOLVED FIXED P2 Normal --- 1 nigel.tao.gnome webkit-unassigned oldest_to_newest 3838 0 nigel.tao.gnome 2007-07-24 05:05:03 -0700 From WebKitTools/Scripts, I can currently do ./build-webkit --gdk But this doesn't work ./run-launcher --gdk http://news.google.com/ 3839 1 15660 nigel.tao.gnome 2007-07-24 05:05:39 -0700 Created attachment 15660 Patch 3820 2 15660 ddkilzer 2007-07-24 07:45:24 -0700 Comment on attachment 15660 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! 3764 3 nigel.tao.gnome 2007-07-24 18:55:51 -0700 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. 3768 4 ddkilzer 2007-07-24 21:42:54 -0700 (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); 3682 5 15690 nigel.tao.gnome 2007-07-26 06:26:06 -0700 Created attachment 15690 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. 3676 6 darin 2007-07-26 08:01:31 -0700 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. 3681 7 ddkilzer 2007-07-26 09:30:47 -0700 (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. 3674 8 15690 ddkilzer 2007-07-26 09:40:34 -0700 Comment on attachment 15690 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! 3603 9 ddkilzer 2007-07-27 09:15:28 -0700 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 3449 10 nigel.tao.gnome 2007-07-30 05:57:15 -0700 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. 3150 11 15807 nigel.tao.gnome 2007-08-02 06:54:19 -0700 Created attachment 15807 Patch 3145 12 manyoso 2007-08-02 08:44:28 -0700 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. 3140 13 ddkilzer 2007-08-02 09:52:54 -0700 (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. 2996 14 15807 mrowe 2007-08-06 02:37:04 -0700 Comment on attachment 15807 Patch r- based on comments from Dave and Adam. 2947 15 15848 nigel.tao.gnome 2007-08-06 06:38:26 -0700 Created attachment 15848 Patch 2943 16 15848 ddkilzer 2007-08-06 09:51:40 -0700 Comment on attachment 15848 Patch r=me 2939 17 ddkilzer 2007-08-06 11:13:38 -0700 $ svn commit WebKitTools/ChangeLog WebKitTools/Scripts/run-launcher Sending WebKitTools/ChangeLog Sending WebKitTools/Scripts/run-launcher Transmitting file data .. Committed revision 24883. 15660 2007-07-24 05:05:39 -0700 2007-07-26 06:26:06 -0700 Patch run-launcher-works-with-gdk.diff text/plain 1107 nigel.tao.gnome SW5kZXg6IFdlYktpdFRvb2xzL1NjcmlwdHMvcnVuLWxhdW5jaGVyCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdl YktpdFRvb2xzL1NjcmlwdHMvcnVuLWxhdW5jaGVyCShyZXZpc2lvbiAyNDUxMykKKysrIFdlYktp dFRvb2xzL1NjcmlwdHMvcnVuLWxhdW5jaGVyCSh3b3JraW5nIGNvcHkpCkBAIC00OSw3ICs0OSw3 IEBACiAkRU5We0xEX0xJQlJBUllfUEFUSH0gPSAkcHJvZHVjdERpcjsKIAogaWYgKGlzR2RrKCkp IHsKLSAgJGxhdW5jaGVyUGF0aCA9ICIkbGF1bmNoZXJQYXRoL1dlYktpdFRvb2xzL0dka0xhdW5j aGVyIjsKKyAgJGxhdW5jaGVyUGF0aCA9ICIkbGF1bmNoZXJQYXRoL1dlYktpdFRvb2xzL0dka0xh dW5jaGVyL0dka0xhdW5jaGVyIjsKIH0gZWxzaWYgKGlzUXQoKSkgewogICAkbGF1bmNoZXJQYXRo ID0gIiRsYXVuY2hlclBhdGgvV2ViS2l0UXQvUXRMYXVuY2hlci9RdExhdW5jaGVyIjsKIH0gZWxz ZSB7CkluZGV4OiBXZWJLaXRUb29scy9HZGtMYXVuY2hlci9tYWluLmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBXZWJLaXRUb29scy9HZGtMYXVuY2hlci9tYWluLmNwcAkocmV2aXNpb24gMjQ1MTMpCisrKyBX ZWJLaXRUb29scy9HZGtMYXVuY2hlci9tYWluLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTU0LDYg KzE1NCw4IEBACiAgICAgICAgICAgICBkdW1wUmVuZGVyVHJlZSA9IHRydWU7CiAgICAgICAgIGVs c2UgaWYgKHN0cmluZ0lzRXF1YWwoY3VyckFyZywgIi1kdW1wcmVuZGVydHJlZSIpKQogICAgICAg ICAgICAgZHVtcFJlbmRlclRyZWUgPSB0cnVlOworICAgICAgICBlbHNlIGlmIChjdXJyQXJnWzBd ID09ICctJykgIC8vIGN1cnJBcmcgbG9va3MgbGlrZSBhbiAodW5rbm93bikgLWZsYWcsIG5vdCBh IFVSTAorICAgICAgICAgICAgOyAgLy8gTm8tb3AuICBXZSdsbCBqdXN0IHNpbGVudGx5IGlnbm9y ZSB1bmtub3duIGZsYWdzCiAgICAgICAgIGVsc2UKICAgICAgICAgICAgIHVybCA9IGF1dG9jb3Jy ZWN0VVJMKGN1cnJBcmcpOwogICAgIH0K 15690 2007-07-26 06:26:06 -0700 2007-08-02 06:54:19 -0700 Patch run-launcher-works-with-gdk.diff text/plain 678 nigel.tao.gnome SW5kZXg6IFdlYktpdFRvb2xzL1NjcmlwdHMvcnVuLWxhdW5jaGVyCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdl YktpdFRvb2xzL1NjcmlwdHMvcnVuLWxhdW5jaGVyCShyZXZpc2lvbiAyNDY0MSkKKysrIFdlYktp dFRvb2xzL1NjcmlwdHMvcnVuLWxhdW5jaGVyCSh3b3JraW5nIGNvcHkpCkBAIC00OSw3ICs0OSwx MCBAQCBteSBAYXJncyA9IEBBUkdWOwogJEVOVntMRF9MSUJSQVJZX1BBVEh9ID0gJHByb2R1Y3RE aXI7CiAKIGlmIChpc0dkaygpKSB7Ci0gICRsYXVuY2hlclBhdGggPSAiJGxhdW5jaGVyUGF0aC9X ZWJLaXRUb29scy9HZGtMYXVuY2hlciI7CisgICRsYXVuY2hlclBhdGggPSAiJGxhdW5jaGVyUGF0 aC9XZWJLaXRUb29scy9HZGtMYXVuY2hlci9HZGtMYXVuY2hlciI7CisgICMgU3RyaXAgLS1nZGsg ZnJvbSB0aGUgYXJnLWxpc3QsIHNpbmNlIG90aGVyd2lzZSBHZGtMYXVuY2hlciB3aWxsIHRyeSB0 bworICAjIGludGVycHJldCBpdCBhcyBhIFVSTC4KKyAgQGFyZ3MgPSBncmVwKCEvXigtLWdkaykk LywgQEFSR1YpOwogfSBlbHNpZiAoaXNRdCgpKSB7CiAgICRsYXVuY2hlclBhdGggPSAiJGxhdW5j aGVyUGF0aC9XZWJLaXRRdC9RdExhdW5jaGVyL1F0TGF1bmNoZXIiOwogfSBlbHNlIHsK 15807 2007-08-02 06:54:19 -0700 2007-08-06 06:38:26 -0700 Patch 14745.diff text/plain 1261 nigel.tao.gnome SW5kZXg6IFdlYktpdFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJLaXRUb29scy9D aGFuZ2VMb2cJKHJldmlzaW9uIDI0ODIzKQorKysgV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCSh3b3Jr aW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMDctMDgtMDIgIE5pZ2VsIFRhbyAgPG5pZ2Vs dGFvQGdub21lLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICBGaXggYnVnIDE0NzQ1OiBXZWJLaXRUb29scy9TY3JpcHRzL3J1bi1sYXVuY2hlciBk b2Vzbid0IHNwZWFrIC0tZ2RrCisgICAgICAgIGh0dHA6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTE0NzQ1CisKKyAgICAgICAgKiBHZGtMYXVuY2hlci9tYWluLmNwcDoKKyAgICAg ICAgKG1haW4pOiBGaWx0ZXIgb3V0IGNvbW1hbmQgbGluZSBhcmd1bWVudHMgdGhhdCBsb29rIGxp a2UgZmxhZ3MgKGkuZS4KKyAgICAgICAgdGhleSBzdGFydCB3aXRoIGEgIi0iKSBmcm9tIGJlaW5n IGludGVycHJldGVkIGFzIGEgVVJMLCBpZiB0aGV5IGFyZW4ndAorICAgICAgICBvbmUgb2YgdGhl IHJlY29nbml6ZWQgZmxhZ3MuCisKIDIwMDctMDgtMDIgIEFkYW0gUm9iZW4gIDxhcm9iZW5AYXBw bGUuY29tPgogCiAgICAgICAgIERvbid0IGRlbGV0ZSB0aGUgc3RkZXJyIGZpbGUgcmlnaHQgYWZ0 ZXIgY3JlYXRpbmcgaXQKSW5kZXg6IFdlYktpdFRvb2xzL0dka0xhdW5jaGVyL21haW4uY3BwCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFdlYktpdFRvb2xzL0dka0xhdW5jaGVyL21haW4uY3BwCShyZXZpc2lvbiAy NDgyMykKKysrIFdlYktpdFRvb2xzL0dka0xhdW5jaGVyL21haW4uY3BwCSh3b3JraW5nIGNvcHkp CkBAIC0xMTksNyArMTE5LDcgQEAgaW50IG1haW4oaW50IGFyZ2MsIGNoYXIqIGFyZ3ZbXSkgCiAg ICAgICAgICAgICBkdW1wUmVuZGVyVHJlZSA9IHRydWU7CiAgICAgICAgIGVsc2UgaWYgKHN0cmlu Z0lzRXF1YWwoY3VyckFyZywgIi1kdW1wcmVuZGVydHJlZSIpKQogICAgICAgICAgICAgZHVtcFJl bmRlclRyZWUgPSB0cnVlOwotICAgICAgICBlbHNlCisgICAgICAgIGVsc2UgaWYgKGN1cnJBcmdb MF0gIT0gJy0nKQogICAgICAgICAgICAgdXJsID0gYXV0b2NvcnJlY3RVUkwoY3VyckFyZyk7CiAg ICAgfQogCg== 15848 2007-08-06 06:38:26 -0700 2007-08-06 09:51:40 -0700 Patch 14745.diff text/plain 1303 nigel.tao.gnome SW5kZXg6IFdlYktpdFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJLaXRUb29scy9D aGFuZ2VMb2cJKHJldmlzaW9uIDI0ODgxKQorKysgV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCSh3b3Jr aW5nIGNvcHkpCkBAIC0xLDMgKzEsMTQgQEAKKzIwMDctMDgtMDYgIE5pZ2VsIFRhbyAgPG5pZ2Vs dGFvQGdub21lLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICBGaXggYnVnIDE0NzQ1OiBXZWJLaXRUb29scy9TY3JpcHRzL3J1bi1sYXVuY2hlciBk b2Vzbid0IHNwZWFrIC0tZ2RrCisgICAgICAgIGh0dHA6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTE0NzQ1CisKKyAgICAgICAgKiBTY3JpcHRzL3J1bi1sYXVuY2hlcjoKKyAgICAg ICAgU2NydWIgdGhlICItLWdkayIgb3V0IG9mIHRoZSBjb21tYW5kIGxpbmUgYXJncywgaWYgZ2l2 ZW4sIHNvIHRoYXQKKyAgICAgICAgR2RrTGF1bmNoZXIgZG9lc24ndCB0cnkgdG8gaW50ZXJwcmV0 IGl0IGFzIGEgVVJMLgorCiAyMDA3LTA4LTAzICBBZGFtIFJvYmVuICA8YXJvYmVuQGFwcGxlLmNv bT4KIAogICAgICAgICBDYXRjaCBTSUdQSVBFIG9uIFdpbmRvd3Mgc28gdGhhdCBydW4td2Via2l0 LXRlc3RzIGRvZXNuJ3QgcXVpdCB3aGVuIERSVCBjcmFzaGVzCkluZGV4OiBXZWJLaXRUb29scy9T Y3JpcHRzL3J1bi1sYXVuY2hlcgo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJLaXRUb29scy9TY3JpcHRzL3J1 bi1sYXVuY2hlcgkocmV2aXNpb24gMjQ4ODEpCisrKyBXZWJLaXRUb29scy9TY3JpcHRzL3J1bi1s YXVuY2hlcgkod29ya2luZyBjb3B5KQpAQCAtNTAsNiArNTAsOSBAQCAkRU5We0xEX0xJQlJBUllf UEFUSH0gPSAkcHJvZHVjdERpcjsKIAogaWYgKGlzR2RrKCkpIHsKICAgJGxhdW5jaGVyUGF0aCA9 ICIkbGF1bmNoZXJQYXRoL1dlYktpdFRvb2xzL0dka0xhdW5jaGVyL0dka0xhdW5jaGVyIjsKKyAg IyBTdHJpcCAtLWdkayBmcm9tIHRoZSBhcmctbGlzdCwgc2luY2Ugb3RoZXJ3aXNlIEdka0xhdW5j aGVyIHdpbGwgdHJ5IHRvCisgICMgaW50ZXJwcmV0IGl0IGFzIGEgVVJMLgorICBAYXJncyA9IGdy ZXAoIS9eKC0tZ2RrKSQvLCBAYXJncyk7CiB9IGVsc2lmIChpc1F0KCkpIHsKICAgJGxhdW5jaGVy UGF0aCA9ICIkbGF1bmNoZXJQYXRoL2Jpbi9RdExhdW5jaGVyIjsKIH0gZWxzZSB7Cg==