133780 2014-06-11 18:12:45 -0700 [GTK] Performance tests should be always ran with WKTR 2014-06-12 05:53:59 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 clopez webkit-unassigned benjamin berto cgarcia commit-queue glenn mrobinson pnormand rego rniwa svillar timothy zan zoltan oldest_to_newest 1015003 0 clopez 2014-06-11 18:12:45 -0700 The GTK port only supports WebKit2. Therefore the --webkit-test-runner/-2 switch for Tools/Scripts/run-perf-tests should be always enabled on this platform. 1015004 1 232926 clopez 2014-06-11 18:18:51 -0700 Created attachment 232926 Patch 1015019 2 232926 rniwa 2014-06-11 22:13:43 -0700 Comment on attachment 232926 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232926&action=review > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:71 > + # The GTK+ port only supports WebKit2, so it always uses WKTR. > + if self._port.name().startswith("gtk"): > + self._options.webkit_test_runner = True Should we split out some warning about this? 1015048 3 rego 2014-06-12 00:18:44 -0700 (In reply to comment #2) > (From update of attachment 232926 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232926&action=review > > > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:71 > > + # The GTK+ port only supports WebKit2, so it always uses WKTR. > > + if self._port.name().startswith("gtk"): > > + self._options.webkit_test_runner = True > > Should we split out some warning about this? For example right now "run-launcher --gtk -2" fails and returns: Starting webkit launcher. Cannot parse arguments: Unknown option -2 I don't know what's the best approach, maybe simply showing a warning and using always WebKit2 is the best option. But I think we should be consistent and do the same in: run-launcher, run-webkit-tests and run-perf-tests. Of course, this can be done in different patches. 1015082 4 clopez 2014-06-12 04:47:07 -0700 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 232926 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=232926&action=review > > > > > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:71 > > > + # The GTK+ port only supports WebKit2, so it always uses WKTR. > > > + if self._port.name().startswith("gtk"): > > > + self._options.webkit_test_runner = True > > > > Should we split out some warning about this? > > For example right now "run-launcher --gtk -2" fails and returns: > Starting webkit launcher. > Cannot parse arguments: Unknown option -2 > > I don't know what's the best approach, maybe simply showing a warning and using always WebKit2 is the best option. > But I think we should be consistent and do the same in: run-launcher, run-webkit-tests and run-perf-tests. Of course, this can be done in different patches. The bots are going to keep passing "--webkit-test-runner/-2" because that is how are implemented the steps on Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg Making it fail when "-2/--webkit-test-runner" is passed will break the bots. And I'm not sure about the usefulness of showing a warning. run-webkit-tests also uses WKTR always with GTK+ and no warning is shown: <http://trac.webkit.org/changeset/167426>. So I think the current patch is consistent with that. 1015087 5 232926 rego 2014-06-12 05:23:30 -0700 Comment on attachment 232926 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232926&action=review >>>> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:71 >>>> + self._options.webkit_test_runner = True >>> >>> Should we split out some warning about this? >> >> For example right now "run-launcher --gtk -2" fails and returns: >> Starting webkit launcher. >> Cannot parse arguments: Unknown option -2 >> >> I don't know what's the best approach, maybe simply showing a warning and using always WebKit2 is the best option. >> But I think we should be consistent and do the same in: run-launcher, run-webkit-tests and run-perf-tests. Of course, this can be done in different patches. > > The bots are going to keep passing "--webkit-test-runner/-2" because that is how are implemented the steps on Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg Making it fail when "-2/--webkit-test-runner" is passed will break the bots. > > And I'm not sure about the usefulness of showing a warning. run-webkit-tests also uses WKTR always with GTK+ and no warning is shown: <http://trac.webkit.org/changeset/167426>. So I think the current patch is consistent with that. Ok, fair enough. I didn't realize run-webkit-tests was fixed already. Maybe we could change run-launcher to use WebKit2 by default and not fail (maybe showing a warning in that particular case). 1015096 6 232926 commit-queue 2014-06-12 05:53:54 -0700 Comment on attachment 232926 Patch Clearing flags on attachment: 232926 Committed r169893: <http://trac.webkit.org/changeset/169893> 1015097 7 commit-queue 2014-06-12 05:53:59 -0700 All reviewed patches have been landed. Closing bug. 232926 2014-06-11 18:18:51 -0700 2014-06-12 05:53:54 -0700 Patch bug-133780-20140612031825.patch text/plain 1673 clopez U3VidmVyc2lvbiBSZXZpc2lvbjogMTY5ODY3CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggMjBmODY4M2EwZTg4MDIxODg3ZmFkN2Y5YmJlOTA2YWZm OTM5ZWE4NC4uYjdlOTBhZDU4YzI2MzJhY2U5MzIwMDliMTA0N2Y3NjA4NDQwOGNhMiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0 IEBACisyMDE0LTA2LTExICBDYXJsb3MgQWxiZXJ0byBMb3BleiBQZXJleiAgPGNsb3BlekBpZ2Fs aWEuY29tPgorCisgICAgICAgIFtHVEtdIFBlcmZvcm1hbmNlIHRlc3RzIHNob3VsZCBiZSBhbHdh eXMgcmFuIHdpdGggV0tUUi4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTEzMzc4MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgor CisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9wZXJmb3JtYW5jZV90ZXN0cy9wZXJmdGVzdHNy dW5uZXIucHk6CisgICAgICAgIChQZXJmVGVzdHNSdW5uZXIuX19pbml0X18pOiBFbmFibGUgd2Vi a2l0X3Rlc3RfcnVubmVyIHdoZW4gcG9ydAorICAgICAgICBpcyBHVEsuCisKIDIwMTQtMDYtMTEg IFNpbW9uIEZyYXNlciAgPHNpbW9uLmZyYXNlckBhcHBsZS5jb20+CiAKICAgICAgICAgV1RSIGNs ZWFudXA6IHB1c2ggcGVyLXRlc3Qgdmlld3BvcnQgY29uZmlndXJhdGlvbiBpbnRvIFRlc3RDb250 cm9sbGVyLCB3aGVyZSBwbGF0Zm9ybXMgY2FuIGN1c3RvbWl6ZSBpdApkaWZmIC0tZ2l0IGEvVG9v bHMvU2NyaXB0cy93ZWJraXRweS9wZXJmb3JtYW5jZV90ZXN0cy9wZXJmdGVzdHNydW5uZXIucHkg Yi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3BlcmZvcm1hbmNlX3Rlc3RzL3BlcmZ0ZXN0c3J1bm5l ci5weQppbmRleCBlZjI3OTAyYzE5NDcxYWNjZDA0ODE0MzlhNmQ1OWJmZTc1M2QwZGE2Li44YTA1 MjQ3NjVjYmYxZTAzZGRkNjU5YjJhN2RlYjQ1ZjY5YmEzODk2IDEwMDY0NAotLS0gYS9Ub29scy9T Y3JpcHRzL3dlYmtpdHB5L3BlcmZvcm1hbmNlX3Rlc3RzL3BlcmZ0ZXN0c3J1bm5lci5weQorKysg Yi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L3BlcmZvcm1hbmNlX3Rlc3RzL3BlcmZ0ZXN0c3J1bm5l ci5weQpAQCAtNjUsNiArNjUsMTEgQEAgY2xhc3MgUGVyZlRlc3RzUnVubmVyKG9iamVjdCk6CiAg ICAgICAgIGVsc2U6CiAgICAgICAgICAgICBzZWxmLl9ob3N0ID0gSG9zdCgpCiAgICAgICAgICAg ICBzZWxmLl9wb3J0ID0gc2VsZi5faG9zdC5wb3J0X2ZhY3RvcnkuZ2V0KHNlbGYuX29wdGlvbnMu cGxhdGZvcm0sIHNlbGYuX29wdGlvbnMpCisKKyAgICAgICAgIyBUaGUgR1RLKyBwb3J0IG9ubHkg c3VwcG9ydHMgV2ViS2l0Miwgc28gaXQgYWx3YXlzIHVzZXMgV0tUUi4KKyAgICAgICAgaWYgc2Vs Zi5fcG9ydC5uYW1lKCkuc3RhcnRzd2l0aCgiZ3RrIik6CisgICAgICAgICAgICBzZWxmLl9vcHRp b25zLndlYmtpdF90ZXN0X3J1bm5lciA9IFRydWUKKwogICAgICAgICBzZWxmLl9ob3N0LmluaXRp YWxpemVfc2NtKCkKICAgICAgICAgc2VsZi5fd2Via2l0X2Jhc2VfZGlyX2xlbiA9IGxlbihzZWxm Ll9wb3J0LndlYmtpdF9iYXNlKCkpCiAgICAgICAgIHNlbGYuX2Jhc2VfcGF0aCA9IHNlbGYuX3Bv cnQucGVyZl90ZXN0c19kaXIoKQo=