Bug 72947 - [Qt] Add support for using OWRT with Qt5
Summary: [Qt] Add support for using OWRT with Qt5
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-11-22 06:06 PST by Csaba Osztrogonác
Modified: 2011-11-25 06:23 PST (History)
3 users (show)

See Also:


Attachments
proposed fix (1.05 KB, patch)
2011-11-24 06:23 PST, Ádám Kallai
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
proposed fix (1.55 KB, patch)
2011-11-25 04:01 PST, Ádám Kallai
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
proposed fix (1.61 KB, patch)
2011-11-25 05:57 PST, Ádám Kallai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2011-11-22 06:06:45 PST
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.
Comment 1 Ádám Kallai 2011-11-24 06:23:46 PST
Created attachment 116503 [details]
proposed fix
Comment 2 Simon Hausmann 2011-11-24 06:52:25 PST
Comment on attachment 116503 [details]
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)
Comment 3 Csaba Osztrogonác 2011-11-24 07:16:35 PST
Comment on attachment 116503 [details]
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).
Comment 4 Ádám Kallai 2011-11-25 04:01:33 PST
Created attachment 116592 [details]
proposed fix
Comment 5 Simon Hausmann 2011-11-25 04:12:42 PST
Comment on attachment 116592 [details]
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?
Comment 6 Simon Hausmann 2011-11-25 04:13:36 PST
Ooops, I didn't see Ossy's comment. Let me see again :)
Comment 7 Simon Hausmann 2011-11-25 04:15:15 PST
(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.
Comment 8 Csaba Osztrogonác 2011-11-25 04:48:32 PST
Comment on attachment 116592 [details]
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.
Comment 9 Csaba Osztrogonác 2011-11-25 04:52:10 PST
(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.
Comment 10 Ádám Kallai 2011-11-25 05:57:28 PST
Created attachment 116608 [details]
proposed fix
Comment 11 Csaba Osztrogonác 2011-11-25 06:05:00 PST
Comment on attachment 116608 [details]
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.)
Comment 12 Csaba Osztrogonác 2011-11-25 06:23:07 PST
Landed in http://trac.webkit.org/changeset/101169