155124 2016-03-07 11:32:27 -0800 [GTK] Add a run-epiphany helper script. 2016-04-03 14:00:02 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified NEW P2 Normal --- 1 clopez clopez andersca annulen ap bugs-noreply commit-queue dbates lforschler mcatanzaro mitz oldest_to_newest 1171685 0 clopez 2016-03-07 11:32:27 -0800 The idea is to be able to easily execute the system epiphany browser with the locally built WebKit, supporting both --release and --debug command line switches. 1171688 1 273198 clopez 2016-03-07 11:43:04 -0800 Created attachment 273198 Patch 1171741 2 273198 annulen 2016-03-07 14:07:21 -0800 Comment on attachment 273198 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273198&action=review > Tools/Scripts/run-epiphany:1 > +#!/usr/bin/perl -w You already have use warnings, no need for "-w" > Tools/Scripts/run-epiphany:38 > +my $binPath; Declare local variables where they are used > Tools/Scripts/run-epiphany:57 > + system(@jhbuildWrapper, "epiphany", @ARGV) or die; die $!; > Tools/Scripts/webkitdirs.pm:1110 > +sub setPortDefault($) I think it would be better to add just one function sub setPortName { $portName = shift; } And use it like here: setPortName(GTK); prohibitUnknownPort(); # ... 1171958 3 273198 cgarcia 2016-03-07 23:11:15 -0800 Comment on attachment 273198 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273198&action=review >> Tools/Scripts/run-epiphany:1 >> +#!/usr/bin/perl -w > > You already have use warnings, no need for "-w" I prefer to write new scripts in python instead of perl, unless we really need something from existing perl scripts. > Tools/Scripts/run-epiphany:5 > +# Copyright (C) 2005, 2007, 2013 Apple Inc. All rights reserved. > +# Copyright (C) 2007 Staikos Computing Services, Inc. <info@staikos.net> > +# Copyright (C) 2016 Igalia S.L. Where are these copyrights come from? > Tools/Scripts/run-epiphany:45 > +if (isGtk()) { If we are going to use perl, I would move this to webkitdirs.pm as a function, runEpiphany() like the existing runSafari or runMiniBrowser. > Tools/Scripts/run-epiphany:55 > + @jhbuildWrapper = wrapperPrefixIfNeeded(); I'm not sure about using the jhbuild for this. We don't have the required deps nor ephy in our jhbuild, so we will end up using the system installed ephy, for which we definitely don't want to use the (usually old) libs compiled in our jhbuild. > Tools/Scripts/run-epiphany:60 > +} else { > + die "Unsupported platform." > +} Can this really happen? you are setting GTK as port in setPortDefault(GTK); so isGtk will always be true here, we don't even need to check it. 1172011 4 clopez 2016-03-08 04:45:38 -0800 (In reply to comment #3) > Comment on attachment 273198 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=273198&action=review > > >> Tools/Scripts/run-epiphany:1 > >> +#!/usr/bin/perl -w > > > > You already have use warnings, no need for "-w" > > I prefer to write new scripts in python instead of perl, unless we really > need something from existing perl scripts. > I also prefer python, but I stick with was is already in place. Both run-minibrowser and run-safari are in Perl. > > Tools/Scripts/run-epiphany:5 > > +# Copyright (C) 2005, 2007, 2013 Apple Inc. All rights reserved. > > +# Copyright (C) 2007 Staikos Computing Services, Inc. <info@staikos.net> > > +# Copyright (C) 2016 Igalia S.L. > > Where are these copyrights come from? > from Tools/Scripts/run-minibrowser > > Tools/Scripts/run-epiphany:45 > > +if (isGtk()) { > > If we are going to use perl, I would move this to webkitdirs.pm as a > function, runEpiphany() like the existing runSafari or runMiniBrowser. > > > Tools/Scripts/run-epiphany:55 > > + @jhbuildWrapper = wrapperPrefixIfNeeded(); > > I'm not sure about using the jhbuild for this. We don't have the required > deps nor ephy in our jhbuild, so we will end up using the system installed > ephy, for which we definitely don't want to use the (usually old) libs > compiled in our jhbuild. > Good point. The problem is that the system libraries are usually not enough to run webkitgtk+ when it was built using the jhbuild. For example things like cairo-gl or openwebrtc are not available in my Debian system. Also since the main purpose is to test WebKitGTK+ rather than epiphany I prefer to use the libraries that were used to built WebKitGTK+ I guess that the best solution would be to run only the webkit process inside the jhbuild. Maybe something like WEB_PROCESS_CMD_PREFIX can help. > > Tools/Scripts/run-epiphany:60 > > +} else { > > + die "Unsupported platform." > > +} > > Can this really happen? you are setting GTK as port in setPortDefault(GTK); > so isGtk will always be true here, we don't even need to check it. Can happen if the user pass --portname other than --gtk (example: --efl) in the command line. 1172022 5 mcatanzaro 2016-03-08 07:02:56 -0800 I'm inclined to agree with Carlos, this script will probably be broken in general unless we start updating our jhbuild environment more regularly to keep up with stable Epiphany releases. For instance, your system should have Epiphany 3.18, but our jhbuild environment has GTK+ 3.16. If Epiphany 3.18 did not require GTK+ 3.18, that was just luck. (In reply to comment #4) > I guess that the best solution would be to run only the webkit process > inside the jhbuild. Maybe something like WEB_PROCESS_CMD_PREFIX can help. Interesting idea, that just might work. 1172023 6 mcatanzaro 2016-03-08 07:03:30 -0800 (In reply to comment #5) > I'm inclined to agree with Carlos Gosh darn it, this must be the third time in the past month.... I'm inclined to agree with Carlos Garcia. 1180210 7 273198 mcatanzaro 2016-04-03 13:51:36 -0700 Comment on attachment 273198 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273198&action=review I'm going to give this r- for the reasons discussed above. WebKit jhbuild is just too old to run system epiphany on recent systems. >> Tools/Scripts/run-epiphany:38 >> +my $binPath; > > Declare local variables where they are used Is it normal in Perl to not leave a blank line after imports? 1180211 8 273198 annulen 2016-04-03 14:00:02 -0700 Comment on attachment 273198 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273198&action=review >>> Tools/Scripts/run-epiphany:38 >>> +my $binPath; >> >> Declare local variables where they are used > > Is it normal in Perl to not leave a blank line after imports? People usually leave blank line, but perlcritic doesn't complain if it's missing 273198 2016-03-07 11:43:04 -0800 2016-04-03 13:51:36 -0700 Patch bug-155124-20160307204249.patch text/plain 4376 clopez U3VidmVyc2lvbiBSZXZpc2lvbjogMTk3Njk2CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggM2Q3ZTRjOGY2ZmMzNGE3YzM5ZGRmY2I4OTYzMmM5YTJm YjI2ZWY5MC4uYjg3NDJlYWIxMzdkZWZmMDg1MDljYmQyZjQ5YTk0OGY3N2U1NzU4OSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5 IEBACisyMDE2LTAzLTA3ICBDYXJsb3MgQWxiZXJ0byBMb3BleiBQZXJleiAgPGNsb3BlekBpZ2Fs aWEuY29tPgorCisgICAgICAgIFtHVEtdIEFkZCBhIHJ1bi1lcGlwaGFueSBoZWxwZXIgc2NyaXB0 LgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU1MTI0 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhlIHNj cmlwdCBydW5zIHRoZSBzeXN0ZW0gRXBpcGhhbnkgYnJvd3NlciB3aXRoIHRoZSBsb2NhbGx5IGJ1 aWx0IFdlYktpdEdUSysuCisgICAgICAgIEl0IHN1cHBvcnRzIGJvdGggLS1yZWxlYXNlIGFuZCAt LWRlYnVnIGNvbW1hbmQgbGluZSBzd2l0Y2hlcy4KKyAgICAgICAgSXQgdXNlcyBhdXRvbWF0aWNh bGx5IHRoZSBpbnRlcm5hbCBKSEJ1aWxkIGlmIGl0IGlzIGRldGVjdGVkLgorCisgICAgICAgICog U2NyaXB0cy9ydW4tZXBpcGhhbnk6IEFkZGVkLgorICAgICAgICAqIFNjcmlwdHMvd2Via2l0ZGly cy5wbToKKyAgICAgICAgKGlzVW5rbm93blBvcnQpOgorICAgICAgICAoc2V0UG9ydERlZmF1bHQp OgorCiAyMDE2LTAzLTA2ICBBbGV4ZXkgUHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29tPgogCiAg ICAgICAgIGJ1aWxkLXdlYmtpdCBwcmludHMgYW4gZXJyb3Igb24gaU9TIHNpbXVsYXRvciBib3Rz CmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3J1bi1lcGlwaGFueSBiL1Rvb2xzL1NjcmlwdHMv cnVuLWVwaXBoYW55Cm5ldyBmaWxlIG1vZGUgMTAwNzU1CmluZGV4IDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLjgwMjQ0ZGM1MmQ4NGJjZDI4OWUyNWZlY2U1MjJlNzQ2 Y2RjZTZmOWEKLS0tIC9kZXYvbnVsbAorKysgYi9Ub29scy9TY3JpcHRzL3J1bi1lcGlwaGFueQpA QCAtMCwwICsxLDYwIEBACisjIS91c3IvYmluL3BlcmwgLXcKKyMKKyMgQ29weXJpZ2h0IChDKSAy MDA1LCAyMDA3LCAyMDEzIEFwcGxlIEluYy4gQWxsIHJpZ2h0cyByZXNlcnZlZC4KKyMgQ29weXJp Z2h0IChDKSAyMDA3IFN0YWlrb3MgQ29tcHV0aW5nIFNlcnZpY2VzLCBJbmMuICA8aW5mb0BzdGFp a29zLm5ldD4KKyMgQ29weXJpZ2h0IChDKSAyMDE2IElnYWxpYSBTLkwuCisjCisjIFJlZGlzdHJp YnV0aW9uIGFuZCB1c2UgaW4gc291cmNlIGFuZCBiaW5hcnkgZm9ybXMsIHdpdGggb3Igd2l0aG91 dAorIyBtb2RpZmljYXRpb24sIGFyZSBwZXJtaXR0ZWQgcHJvdmlkZWQgdGhhdCB0aGUgZm9sbG93 aW5nIGNvbmRpdGlvbnMKKyMgYXJlIG1ldDoKKyMKKyMgMS4gIFJlZGlzdHJpYnV0aW9ucyBvZiBz b3VyY2UgY29kZSBtdXN0IHJldGFpbiB0aGUgYWJvdmUgY29weXJpZ2h0CisjICAgICBub3RpY2Us IHRoaXMgbGlzdCBvZiBjb25kaXRpb25zIGFuZCB0aGUgZm9sbG93aW5nIGRpc2NsYWltZXIuCisj IDIuICBSZWRpc3RyaWJ1dGlvbnMgaW4gYmluYXJ5IGZvcm0gbXVzdCByZXByb2R1Y2UgdGhlIGFi b3ZlIGNvcHlyaWdodAorIyAgICAgbm90aWNlLCB0aGlzIGxpc3Qgb2YgY29uZGl0aW9ucyBhbmQg dGhlIGZvbGxvd2luZyBkaXNjbGFpbWVyIGluIHRoZQorIyAgICAgZG9jdW1lbnRhdGlvbiBhbmQv b3Igb3RoZXIgbWF0ZXJpYWxzIHByb3ZpZGVkIHdpdGggdGhlIGRpc3RyaWJ1dGlvbi4KKyMgMy4g IE5laXRoZXIgdGhlIG5hbWUgb2YgdGhlIGNvcHlyaWdodCBob2xkZXIgbm9yIHRoZSBuYW1lcyBv ZiBpdHMKKyMgICAgIGNvbnRyaWJ1dG9ycyBtYXkgYmUgdXNlZCB0byBlbmRvcnNlIG9yIHByb21v dGUgcHJvZHVjdHMgZGVyaXZlZAorIyAgICAgZnJvbSB0aGlzIHNvZnR3YXJlIHdpdGhvdXQgc3Bl Y2lmaWMgcHJpb3Igd3JpdHRlbiBwZXJtaXNzaW9uLgorIworIyBUSElTIFNPRlRXQVJFIElTIFBS T1ZJREVEIEJZIFRIRSBDT1BZUklHSFQgSE9MREVSUyBBTkQgQ09OVFJJQlVUT1JTCisjICJBUyBJ UyIgQU5EIEFOWSBFWFBSRVNTIE9SIElNUExJRUQgV0FSUkFOVElFUywgSU5DTFVESU5HLCBCVVQg Tk9UCisjIExJTUlURUQgVE8sIFRIRSBJTVBMSUVEIFdBUlJBTlRJRVMgT0YgTUVSQ0hBTlRBQklM SVRZIEFORCBGSVRORVNTIEZPUgorIyBBIFBBUlRJQ1VMQVIgUFVSUE9TRSBBUkUgRElTQ0xBSU1F RC4gSU4gTk8gRVZFTlQgU0hBTEwgVEhFIENPUFlSSUdIVAorIyBPV05FUiBPUiBDT05UUklCVVRP UlMgQkUgTElBQkxFIEZPUiBBTlkgRElSRUNULCBJTkRJUkVDVCwgSU5DSURFTlRBTCwKKyMgU1BF Q0lBTCwgRVhFTVBMQVJZLCBPUiBDT05TRVFVRU5USUFMIERBTUFHRVMgKElOQ0xVRElORywgQlVU IE5PVAorIyBMSU1JVEVEIFRPLCBQUk9DVVJFTUVOVCBPRiBTVUJTVElUVVRFIEdPT0RTIE9SIFNF UlZJQ0VTOyBMT1NTIE9GIFVTRSwKKyMgREFUQSwgT1IgUFJPRklUUzsgT1IgQlVTSU5FU1MgSU5U RVJSVVBUSU9OKSBIT1dFVkVSIENBVVNFRCBBTkQgT04gQU5ZCisjIFRIRU9SWSBPRiBMSUFCSUxJ VFksIFdIRVRIRVIgSU4gQ09OVFJBQ1QsIFNUUklDVCBMSUFCSUxJVFksIE9SIFRPUlQKKyMgKElO Q0xVRElORyBORUdMSUdFTkNFIE9SIE9USEVSV0lTRSkgQVJJU0lORyBJTiBBTlkgV0FZIE9VVCBP RiBUSEUgVVNFCisjIE9GIFRISVMgU09GVFdBUkUsIEVWRU4gSUYgQURWSVNFRCBPRiBUSEUgUE9T U0lCSUxJVFkgT0YgU1VDSCBEQU1BR0UuCisKK3VzZSBzdHJpY3Q7Cit1c2Ugd2FybmluZ3M7Cit1 c2UgRmlsZTo6U3BlYzo6RnVuY3Rpb25zIHF3L2NhdGRpci87Cit1c2UgRmluZEJpbjsKK3VzZSBs aWIgJEZpbmRCaW46OkJpbjsKK3VzZSB3ZWJraXRkaXJzOworbXkgJGJpblBhdGg7CitteSAkbGli UGF0aDsKK215IEBqaGJ1aWxkV3JhcHBlcjsKK3NldFBvcnREZWZhdWx0KEdUSyk7Citwcm9oaWJp dFVua25vd25Qb3J0KCk7CitzZXRDb25maWd1cmF0aW9uKCk7CisKK2lmIChpc0d0aygpKSB7Cisg ICAgY2hlY2tGcmFtZXdvcmtzKCk7CisgICAgJGxpYlBhdGggPSBjYXRkaXIocHJvZHVjdERpcigp LCAibGliIik7CisgICAgJGJpblBhdGggPSBjYXRkaXIocHJvZHVjdERpcigpLCAiYmluIik7Cisg ICAgZGllICJDYW4ndCBmaW5kICRsaWJQYXRoIiB1bmxlc3MgLWQgJGxpYlBhdGg7CisgICAgZGll ICJDYW4ndCBmaW5kICRiaW5QYXRoIiB1bmxlc3MgLWQgJGJpblBhdGg7CisgICAgcHJlcGVuZFRv RW52aXJvbm1lbnRWYXJpYWJsZUxpc3QoIldFQktJVF9JTkpFQ1RFRF9CVU5ETEVfUEFUSCIsICRs aWJQYXRoKTsKKyAgICBwcmVwZW5kVG9FbnZpcm9ubWVudFZhcmlhYmxlTGlzdCgiV0VCS0lUX0VY RUNfUEFUSCIsICRiaW5QYXRoKTsKKyAgICBwcmVwZW5kVG9FbnZpcm9ubWVudFZhcmlhYmxlTGlz dCgiTERfTElCUkFSWV9QQVRIIiwgJGxpYlBhdGgpOworICAgIHByZXBlbmRUb0Vudmlyb25tZW50 VmFyaWFibGVMaXN0KCJQQVRIIiwgJGJpblBhdGgpOworICAgIEBqaGJ1aWxkV3JhcHBlciA9IHdy YXBwZXJQcmVmaXhJZk5lZWRlZCgpOworICAgIHByaW50ICJTdGFydGluZyBFcGlwaGFueS5cbiI7 CisgICAgc3lzdGVtKEBqaGJ1aWxkV3JhcHBlciwgImVwaXBoYW55IiwgQEFSR1YpIG9yIGRpZTsK K30gZWxzZSB7CisgICAgZGllICJVbnN1cHBvcnRlZCBwbGF0Zm9ybS4iCit9ClwgTm8gbmV3bGlu ZSBhdCBlbmQgb2YgZmlsZQpkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRkaXJzLnBt IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRkaXJzLnBtCmluZGV4IDk0Y2I4ZWM0OTk5NDU4ODBkYTIx Nzc2YmQwMzFkN2M1NTVmNjViMjkuLmJkOGVkNmFjZTU3MmE2Nzc0YTQyNTY2ZTUxZWRhODI0YzRj ZjJhYWQgMTAwNzU1Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0ZGlycy5wbQorKysgYi9Ub29s cy9TY3JpcHRzL3dlYmtpdGRpcnMucG0KQEAgLTExMDIsNiArMTEwMiwxNiBAQCBzdWIgaXNHdGso KQogICAgIHJldHVybiBwb3J0TmFtZSgpIGVxIEdUSzsKIH0KIAorc3ViIGlzVW5rbm93blBvcnQo KQoreworICAgIHJldHVybiBwb3J0TmFtZSgpIGVxIFVua25vd247Cit9CisKK3N1YiBzZXRQb3J0 RGVmYXVsdCgkKQoreworICAgICRwb3J0TmFtZSA9IHNoaWZ0IGlmIGlzVW5rbm93blBvcnQoKTsK K30KKwogIyBEZXRlcm1pbmUgaWYgdGhpcyBpcyBkZWJpYW4sIHVidW50dSwgbGluc3BpcmUsIG9y IHNvbWV0aGluZyBzaW1pbGFyLgogc3ViIGlzRGViaWFuQmFzZWQoKQogewo=