Bug 72947 - [Qt] Add support for using OWRT with Qt5
: [Qt] Add support for using OWRT with Qt5
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: Qt, QtTriaged
:
:
  Show dependency treegraph
 
Reported: 2011-11-22 06:06 PST by
Modified: 2011-11-25 06:23 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-11-24 06:23:46 PST -------
Created an attachment (id=116503) [details]
proposed fix
------- Comment #2 From 2011-11-24 06:52:25 PST -------
(From update of attachment 116503 [details])
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 From 2011-11-24 07:16:35 PST -------
(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.

>> 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 From 2011-11-25 04:01:33 PST -------
Created an attachment (id=116592) [details]
proposed fix
------- Comment #5 From 2011-11-25 04:12:42 PST -------
(From update of attachment 116592 [details])
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 From 2011-11-25 04:13:36 PST -------
Ooops, I didn't see Ossy's comment. Let me see again :)
------- Comment #7 From 2011-11-25 04:15:15 PST -------
(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?

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 From 2011-11-25 04:48:32 PST -------
(From update of attachment 116592 [details])
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 From 2011-11-25 04:52:10 PST -------
(In reply to comment #7)
> (In reply to comment #3)
> > (From update of attachment 116503 [details] [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 From 2011-11-25 05:57:28 PST -------
Created an attachment (id=116608) [details]
proposed fix
------- Comment #11 From 2011-11-25 06:05:00 PST -------
(From update of attachment 116608 [details])
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 From 2011-11-25 06:23:07 PST -------
Landed in http://trac.webkit.org/changeset/101169