Bug 72947 – [Qt] Add support for using OWRT with Qt5

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

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.

[Ádám Kallai]

Created attachment 116503 [details]
proposed fix

[Simon Hausmann]

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)

[Csaba Osztrogonác]

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).

[Ádám Kallai]

Created attachment 116592 [details]
proposed fix

[Simon Hausmann]

Comment on attachment 116592 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=116592&action=review

> Tools/ChangeLog:1
> [phone]  Ãdám Kallai  <[e-mail]>

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?

[Simon Hausmann]

Ooops, I didn't see Ossy's comment. Let me see again :)

[Simon Hausmann]

(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.

[Csaba Osztrogonác]

Comment on attachment 116592 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=116592&action=review

>> Tools/ChangeLog:1
>> [phone]  Ãdám Kallai  <[e-mail]>
> 
> 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.

[Csaba Osztrogonác]

(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.

[Ádám Kallai]

Created attachment 116608 [details]
proposed fix

[Csaba Osztrogonác]

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.)

[Csaba Osztrogonác]

Landed in http://trac.webkit.org/changeset/101169