72947 2011-11-22 06:06:45 -0800 [Qt] Add support for using OWRT with Qt5 2011-11-25 06:23:19 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) All All RESOLVED FIXED Qt, QtTriaged P2 Normal --- 1 ossy webkit-unassigned hausmann kadam ossy oldest_to_newest 507551 0 ossy 2011-11-22 06:06:45 -0800 It seems -graphicssystem command line argument isn't supported by Qt5: // ### Qt4 compatibility, remove? static inline void setGraphicsSystem(const QString &) {} But ORWT still pass this argument (-graphicssystem raster) to QtTestBrowser, and QtTestBrowser open an empty window with URL-bar: http://raster and "Host not found" text. We shouldn't pass "-graphicssystem raster" argument if Qt version >= 5. 508746 1 116503 kadam 2011-11-24 06:23:46 -0800 Created attachment 116503 proposed fix 508754 2 116503 hausmann 2011-11-24 06:52:25 -0800 Comment on attachment 116503 proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=116503&action=review I think the idea is good. The ChangeLog entry is missing and I have a few nitpicks :) > Tools/Scripts/old-run-webkit-tests:242 > + } elsif (getQtVersion() eq "5.0") { > + $platform = "qt-5.0"; This seems unrelated? > Tools/Scripts/old-run-webkit-tests:1222 > + if(!(getQtVersion() ge "5.0")) { Shouldn't that properly check the major version of Qt only? (if major-version < 4) 508765 3 116503 ossy 2011-11-24 07:16:35 -0800 Comment on attachment 116503 proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=116503&action=review >> Tools/Scripts/old-run-webkit-tests:242 >> + $platform = "qt-5.0"; > > This seems unrelated? No, we need this change too, because qt-5.0 platform wasn't supported by ORWT before. >> Tools/Scripts/old-run-webkit-tests:1222 >> + if(!(getQtVersion() ge "5.0")) { > > Shouldn't that properly check the major version of Qt only? (if major-version < 4) Of course we can check only the major version, but getQtVersion() (webkitdirs.pm) parses qmake's output and return major&minor version in this format as string. I think, that if ((getQtVersion() lt "5.0")) would be good, because it is equivalent to (non-existent-yet) (getQtMajorVersion() < 5). 509071 4 116592 kadam 2011-11-25 04:01:33 -0800 Created attachment 116592 proposed fix 509082 5 116592 hausmann 2011-11-25 04:12:42 -0800 Comment on attachment 116592 proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=116592&action=review > Tools/ChangeLog:1 > +2011-11-25 Ádám Kallai <Kallai.Adam@stud.u-szeged.hu> It looks like your first name isn't properly utf-8 encoded here. Is that possible? > Tools/Scripts/old-run-webkit-tests:242 > + } elsif (getQtVersion() eq "5.0") { > + $platform = "qt-5.0"; This looks like an unrelated change. Is that intentional? > Tools/Scripts/old-run-webkit-tests:1224 > - unshift @configurationArgs, qw(-graphicssystem raster -style windows); > + unshift @configurationArgs, qw(-style windows); > + if(getQtVersion() lt "5.0") { > + unshift @configurationArgs, qw(-graphicssystem raster); > + } This looks much better :). Out of curiosity: Does 'lt' work with strings like "5.0" like that? 509083 6 hausmann 2011-11-25 04:13:36 -0800 Ooops, I didn't see Ossy's comment. Let me see again :) 509084 7 hausmann 2011-11-25 04:15:15 -0800 (In reply to comment #3) > (From update of attachment 116503 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116503&action=review > > >> Tools/Scripts/old-run-webkit-tests:242 > >> + $platform = "qt-5.0"; > > > > This seems unrelated? > > No, we need this change too, because qt-5.0 platform wasn't supported by ORWT before. Shouldn't this be mentioned in the ChangeLog then? Right now the title of the bug is about not passing -graphicssystem raster when running OWRT with Qt 5. I do however have the impression that the title of this bug should be "Add support for using OWRT with Qt 5" or similar, and that the ChangeLog should mention this line. 509096 8 116592 ossy 2011-11-25 04:48:32 -0800 Comment on attachment 116592 proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=116592&action=review >> Tools/ChangeLog:1 >> +2011-11-25 Ádám Kallai <Kallai.Adam@stud.u-szeged.hu> > > It looks like your first name isn't properly utf-8 encoded here. Is that possible? I think pretty diff can't handle correct the utf8 names. Check one of my patches: https://bugs.webkit.org/attachment.cgi?id=114235&action=review or one of Tor Arne's patches: https://bugs.webkit.org/attachment.cgi?id=104833&action=review >> Tools/Scripts/old-run-webkit-tests:1224 >> + } > > This looks much better :). Out of curiosity: Does 'lt' work with strings like "5.0" like that? Unfortunately perl differs from bash. lt, eq, gt, ... are for string comparision in perl. 509098 9 ossy 2011-11-25 04:52:10 -0800 (In reply to comment #7) > (In reply to comment #3) > > (From update of attachment 116503 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=116503&action=review > > > > >> Tools/Scripts/old-run-webkit-tests:242 > > >> + $platform = "qt-5.0"; > > > > > > This seems unrelated? > > > > No, we need this change too, because qt-5.0 platform wasn't supported by ORWT before. > > Shouldn't this be mentioned in the ChangeLog then? You're right, we should. > Right now the title of the bug is about not passing -graphicssystem raster when running OWRT with Qt 5. I do however have the impression that the title of this bug should be "Add support for using OWRT with Qt 5" or similar, and that the ChangeLog should mention this line. It's reasonable, I fixed the title of the bug. 509124 10 116608 kadam 2011-11-25 05:57:28 -0800 Created attachment 116608 proposed fix 509128 11 116608 ossy 2011-11-25 06:05:00 -0800 Comment on attachment 116608 proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=116608&action=review LGTM, r=me. > Tools/Scripts/old-run-webkit-tests:1222 > + if(getQtVersion() lt "5.0") { only a little nit: missing space after the if.(I'll fix it locally before landing.) 509137 12 ossy 2011-11-25 06:23:07 -0800 Landed in http://trac.webkit.org/changeset/101169 116503 2011-11-24 06:23:46 -0800 2011-11-24 07:16:35 -0800 proposed fix ORWT.patch text/plain 1074 kadam ZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMvb2xkLXJ1bi13ZWJraXQtdGVzdHMgYi9Ub29scy9T Y3JpcHRzL29sZC1ydW4td2Via2l0LXRlc3RzCmluZGV4IGVkYTg2YWYuLjFlNWM2NjEgMTAwNzU1 Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvb2xkLXJ1bi13ZWJraXQtdGVzdHMKKysrIGIvVG9vbHMvU2Ny aXB0cy9vbGQtcnVuLXdlYmtpdC10ZXN0cwpAQCAtMjM4LDYgKzIzOCw4IEBAIGlmIChpc0FwcGxl TWFjV2ViS2l0KCkpIHsKICAgICAgICAgJHBsYXRmb3JtID0gInF0LWFybSI7CiAgICAgfSBlbHNp ZiAoZ2V0UXRWZXJzaW9uKCkgZXEgIjQuOCIpIHsKICAgICAgICAgJHBsYXRmb3JtID0gInF0LTQu OCI7CisgICAgfSBlbHNpZiAoZ2V0UXRWZXJzaW9uKCkgZXEgIjUuMCIpIHsKKyAgICAgICAgJHBs YXRmb3JtID0gInF0LTUuMCI7CiAgICAgfSBlbHNpZiAoaXNMaW51eCgpKSB7CiAgICAgICAgICRw bGF0Zm9ybSA9ICJxdC1saW51eCI7CiAgICAgfSBlbHNpZiAoaXNXaW5kb3dzKCkgfHwgaXNDeWd3 aW4oKSkgewpAQCAtMTIxNiw3ICsxMjE4LDEwIEBAIGlmIChpc0d0aygpKSB7CiAgIHB1c2goQGNv bmZpZ3VyYXRpb25BcmdzLCAnLTInKSBpZiAkdXNlV2ViS2l0VGVzdFJ1bm5lcjsKICAgc3lzdGVt ICJUb29scy9TY3JpcHRzL3J1bi1sYXVuY2hlciIsIEBjb25maWd1cmF0aW9uQXJncywgImZpbGU6 Ly8iLiR0ZXN0UmVzdWx0cyBpZiAkbGF1bmNoU2FmYXJpOwogfSBlbHNpZiAoaXNRdCgpKSB7Ci0g IHVuc2hpZnQgQGNvbmZpZ3VyYXRpb25BcmdzLCBxdygtZ3JhcGhpY3NzeXN0ZW0gcmFzdGVyIC1z dHlsZSB3aW5kb3dzKTsKKyAgdW5zaGlmdCBAY29uZmlndXJhdGlvbkFyZ3MsIHF3KC1zdHlsZSB3 aW5kb3dzKTsKKyAgaWYoIShnZXRRdFZlcnNpb24oKSBnZSAiNS4wIikpIHsKKyAgICB1bnNoaWZ0 IEBjb25maWd1cmF0aW9uQXJncywgcXcoLWdyYXBoaWNzc3lzdGVtIHJhc3Rlcik7CisgIH0KICAg aWYgKGlzQ3lnd2luKCkpIHsKICAgICAkdGVzdFJlc3VsdHMgPSAiLyIgLiB0b1dpbmRvd3NQYXRo KCR0ZXN0UmVzdWx0cyk7CiAgICAgJHRlc3RSZXN1bHRzID1+IHMvXFwvXC8vZzsK 116592 2011-11-25 04:01:33 -0800 2011-11-25 04:48:32 -0800 proposed fix ORWT.patch text/plain 1584 kadam ZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBiL1Rvb2xzL0NoYW5nZUxvZwppbmRleCBhNmU4 NjMwLi5jNzBkNDgyIDEwMDY0NAotLS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTEtMTEtMjUgIMOBZMOhbSBLYWxsYWkgIDxLYWxs YWkuQWRhbUBzdHVkLnUtc3plZ2VkLmh1PgorCisgICAgICAgIFtRdF0gRG9uJ3QgcGFzcyAtZ3Jh cGhpY3NzeXN0ZW0gdG8gUXRUZXN0QnJvd3NlciBpZiB1c2luZyBRdDUKKyAgICAgICAgCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03Mjk0NworCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogU2NyaXB0cy9vbGQt cnVuLXdlYmtpdC10ZXN0czoKKwogMjAxMS0xMS0yMSAgWXV0YSBLaXRhbXVyYSAgPHl1dGFrQGNo cm9taXVtLm9yZz4KIAogICAgICAgICBbR1RLXSBFbmFibGUgV2ViU29ja2V0IGh5YmkgdGVzdHMK ZGlmZiAtLWdpdCBhL1Rvb2xzL1NjcmlwdHMvb2xkLXJ1bi13ZWJraXQtdGVzdHMgYi9Ub29scy9T Y3JpcHRzL29sZC1ydW4td2Via2l0LXRlc3RzCmluZGV4IGVkYTg2YWYuLjYyNzI1ZTAgMTAwNzU1 Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvb2xkLXJ1bi13ZWJraXQtdGVzdHMKKysrIGIvVG9vbHMvU2Ny aXB0cy9vbGQtcnVuLXdlYmtpdC10ZXN0cwpAQCAtMjM4LDYgKzIzOCw4IEBAIGlmIChpc0FwcGxl TWFjV2ViS2l0KCkpIHsKICAgICAgICAgJHBsYXRmb3JtID0gInF0LWFybSI7CiAgICAgfSBlbHNp ZiAoZ2V0UXRWZXJzaW9uKCkgZXEgIjQuOCIpIHsKICAgICAgICAgJHBsYXRmb3JtID0gInF0LTQu OCI7CisgICAgfSBlbHNpZiAoZ2V0UXRWZXJzaW9uKCkgZXEgIjUuMCIpIHsKKyAgICAgICAgJHBs YXRmb3JtID0gInF0LTUuMCI7CiAgICAgfSBlbHNpZiAoaXNMaW51eCgpKSB7CiAgICAgICAgICRw bGF0Zm9ybSA9ICJxdC1saW51eCI7CiAgICAgfSBlbHNpZiAoaXNXaW5kb3dzKCkgfHwgaXNDeWd3 aW4oKSkgewpAQCAtMTIxNiw3ICsxMjE4LDEwIEBAIGlmIChpc0d0aygpKSB7CiAgIHB1c2goQGNv bmZpZ3VyYXRpb25BcmdzLCAnLTInKSBpZiAkdXNlV2ViS2l0VGVzdFJ1bm5lcjsKICAgc3lzdGVt ICJUb29scy9TY3JpcHRzL3J1bi1sYXVuY2hlciIsIEBjb25maWd1cmF0aW9uQXJncywgImZpbGU6 Ly8iLiR0ZXN0UmVzdWx0cyBpZiAkbGF1bmNoU2FmYXJpOwogfSBlbHNpZiAoaXNRdCgpKSB7Ci0g IHVuc2hpZnQgQGNvbmZpZ3VyYXRpb25BcmdzLCBxdygtZ3JhcGhpY3NzeXN0ZW0gcmFzdGVyIC1z dHlsZSB3aW5kb3dzKTsKKyAgdW5zaGlmdCBAY29uZmlndXJhdGlvbkFyZ3MsIHF3KC1zdHlsZSB3 aW5kb3dzKTsKKyAgaWYoZ2V0UXRWZXJzaW9uKCkgbHQgIjUuMCIpIHsKKyAgICB1bnNoaWZ0IEBj b25maWd1cmF0aW9uQXJncywgcXcoLWdyYXBoaWNzc3lzdGVtIHJhc3Rlcik7CisgIH0KICAgaWYg KGlzQ3lnd2luKCkpIHsKICAgICAkdGVzdFJlc3VsdHMgPSAiLyIgLiB0b1dpbmRvd3NQYXRoKCR0 ZXN0UmVzdWx0cyk7CiAgICAgJHRlc3RSZXN1bHRzID1+IHMvXFwvXC8vZzsK 116608 2011-11-25 05:57:28 -0800 2011-11-25 06:23:19 -0800 proposed fix ORWT.patch text/plain 1646 kadam ZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBiL1Rvb2xzL0NoYW5nZUxvZwppbmRleCBhNmU4 NjMwLi5hMTIwMjM0IDEwMDY0NAotLS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTEtMTEtMjUgIMOBZMOhbSBLYWxsYWkgIDxLYWxs YWkuQWRhbUBzdHVkLnUtc3plZ2VkLmh1PgorCisgICAgICAgIFtRdF0gQWRkIHN1cHBvcnQgZm9y IHVzaW5nIE9XUlQgd2l0aCBRdDUKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTcyOTQ3CisKKyAgICAgICAgQWRkIHF0LTUuMCBwbGF0Zm9ybSBhbmQgZG9u J3QgcGFzcyAtZ3JhcGhpY3NzeXN0ZW0gdG8gUXRUZXN0QnJvd3NlciBpZiB1c2luZyBRdDUuCisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBTY3JpcHRz L29sZC1ydW4td2Via2l0LXRlc3RzOgorCiAyMDExLTExLTIxICBZdXRhIEtpdGFtdXJhICA8eXV0 YWtAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFtHVEtdIEVuYWJsZSBXZWJTb2NrZXQgaHliaSB0 ZXN0cwpkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy9vbGQtcnVuLXdlYmtpdC10ZXN0cyBiL1Rv b2xzL1NjcmlwdHMvb2xkLXJ1bi13ZWJraXQtdGVzdHMKaW5kZXggZWRhODZhZi4uNjI3MjVlMCAx MDA3NTUKLS0tIGEvVG9vbHMvU2NyaXB0cy9vbGQtcnVuLXdlYmtpdC10ZXN0cworKysgYi9Ub29s cy9TY3JpcHRzL29sZC1ydW4td2Via2l0LXRlc3RzCkBAIC0yMzgsNiArMjM4LDggQEAgaWYgKGlz QXBwbGVNYWNXZWJLaXQoKSkgewogICAgICAgICAkcGxhdGZvcm0gPSAicXQtYXJtIjsKICAgICB9 IGVsc2lmIChnZXRRdFZlcnNpb24oKSBlcSAiNC44IikgewogICAgICAgICAkcGxhdGZvcm0gPSAi cXQtNC44IjsKKyAgICB9IGVsc2lmIChnZXRRdFZlcnNpb24oKSBlcSAiNS4wIikgeworICAgICAg ICAkcGxhdGZvcm0gPSAicXQtNS4wIjsKICAgICB9IGVsc2lmIChpc0xpbnV4KCkpIHsKICAgICAg ICAgJHBsYXRmb3JtID0gInF0LWxpbnV4IjsKICAgICB9IGVsc2lmIChpc1dpbmRvd3MoKSB8fCBp c0N5Z3dpbigpKSB7CkBAIC0xMjE2LDcgKzEyMTgsMTAgQEAgaWYgKGlzR3RrKCkpIHsKICAgcHVz aChAY29uZmlndXJhdGlvbkFyZ3MsICctMicpIGlmICR1c2VXZWJLaXRUZXN0UnVubmVyOwogICBz eXN0ZW0gIlRvb2xzL1NjcmlwdHMvcnVuLWxhdW5jaGVyIiwgQGNvbmZpZ3VyYXRpb25BcmdzLCAi ZmlsZTovLyIuJHRlc3RSZXN1bHRzIGlmICRsYXVuY2hTYWZhcmk7CiB9IGVsc2lmIChpc1F0KCkp IHsKLSAgdW5zaGlmdCBAY29uZmlndXJhdGlvbkFyZ3MsIHF3KC1ncmFwaGljc3N5c3RlbSByYXN0 ZXIgLXN0eWxlIHdpbmRvd3MpOworICB1bnNoaWZ0IEBjb25maWd1cmF0aW9uQXJncywgcXcoLXN0 eWxlIHdpbmRvd3MpOworICBpZihnZXRRdFZlcnNpb24oKSBsdCAiNS4wIikgeworICAgIHVuc2hp ZnQgQGNvbmZpZ3VyYXRpb25BcmdzLCBxdygtZ3JhcGhpY3NzeXN0ZW0gcmFzdGVyKTsKKyAgfQog ICBpZiAoaXNDeWd3aW4oKSkgewogICAgICR0ZXN0UmVzdWx0cyA9ICIvIiAuIHRvV2luZG93c1Bh dGgoJHRlc3RSZXN1bHRzKTsKICAgICAkdGVzdFJlc3VsdHMgPX4gcy9cXC9cLy9nOwo=