Port Mozilla Qt to Qt5

RESOLVED FIXED in mozilla14

Status

Core Graveyard
Widget: Qt
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
mozilla14
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Qt 5 has some classes reorganization and don't have anymore Q_WS_X11 and QX11Info.

So in order to compile it on Qt5 we need to tweak existing non-X11 configuration.
(Assignee)

Comment 1

6 years ago
Created attachment 575359 [details] [diff] [review]
WIP1
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Depends on: 726461
(Assignee)

Comment 2

5 years ago
Created attachment 596924 [details] [diff] [review]
Qt5 building and basic working port

Ok, here is the patch which does:
1) Fix includes virtual path in order to keep it compiling with Qt4 and Qt5
2) Get rid of QX11Info as much as possible, and replace with pure X11 implementation (including cached X display in QtPlatform shared for Qt port instance)
3) Qt specific old input method implementation disabled because API has changed. need followup with new implementation
Attachment #575359 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Patch is changing Qt related functionality across different mozilla components.
Should I set multiple reviewers, or it is enough to have just one Qt reviewer here?
(Assignee)

Updated

5 years ago
Attachment #596924 - Flags: review?(mozilla)
Attachment #596924 - Flags: review?(mark.finkle)
Attachment #596924 - Flags: review?(karlt)
(Assignee)

Updated

5 years ago
Attachment #596924 - Flags: review?(mh+mozilla)
Comment on attachment 596924 [details] [diff] [review]
Qt5 building and basic working port

You can have my r+ on the dom/plugins changes.

I didn't really review gfxQtPlatform but it looks like there needs to be a better solution for getting the Display than the XOpenDisplay.  I assume Qt will often be using a different Display.  Both Qt and Gecko need to use the same connection in order avoid race conditions across the different connections.  There is also a limit (about 128 iirc) on the number of connections to a server, so these should not be consumed unnecessarily.
Attachment #596924 - Flags: review?(karlt)
(Assignee)

Comment 5

5 years ago
Created attachment 598763 [details] [diff] [review]
Qt5 port

Here is the version with more strict display/screen requests from qt5 xcb/xlib platform interface.
Attachment #596924 - Attachment is obsolete: true
Attachment #596924 - Flags: review?(mozilla)
Attachment #596924 - Flags: review?(mh+mozilla)
Attachment #596924 - Flags: review?(mark.finkle)
Attachment #598763 - Flags: review?(mozilla)
(Assignee)

Updated

5 years ago
Attachment #598763 - Flags: review?(mh+mozilla)
Attachment #598763 - Flags: review?(mark.finkle)
Comment on attachment 598763 [details] [diff] [review]
Qt5 port

Review of attachment 598763 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the build parts. I really can't review the Qt parts.
Attachment #598763 - Flags: review?(mh+mozilla) → review+
Comment on attachment 598763 [details] [diff] [review]
Qt5 port

The new screen accessors look fine to me. Looks like you have maintained support for older Qt versions too.

>diff --git a/gfx/cairo/cairo/src/cairo-qt-surface.cpp b/gfx/cairo/cairo/src/cairo-qt-surface.cpp
> #include <QtGui/QPaintDevice>
> #include <QtGui/QImage>
> #include <QtGui/QPixmap>
> #include <QtGui/QBrush>
> #include <QtGui/QPen>
>-#include <QtGui/QWidget>
>-#include <QtGui/QX11Info>
>+#include <QWidget>
> #include <QtCore/QVarLengthArray>

nit: Do you need to drop the "QtGui" folder? Should the others be changed too? Could you keep them all the same?

>diff --git a/widget/qt/nsWindow.cpp b/widget/qt/nsWindow.cpp

>-#include <QtGui/QApplication>
>-#include <QtGui/QDesktopWidget>
>+#include <QApplication>
>+#include <QDesktopWidget>
> #include <QtGui/QCursor>
>-#include <QtGui/QIcon>
>-#include <QtGui/QX11Info>
>-#include <QtGui/QGraphicsScene>
>-#include <QtGui/QGraphicsView>
>-#include <QtGui/QGraphicsSceneContextMenuEvent>
>-#include <QtGui/QGraphicsSceneDragDropEvent>
>-#include <QtGui/QGraphicsSceneMouseEvent>
>-#include <QtGui/QGraphicsSceneHoverEvent>
>-#include <QtGui/QGraphicsSceneWheelEvent>
>-#include <QtGui/QGraphicsSceneResizeEvent>
>-#include <QtGui/QStyleOptionGraphicsItem>
>+#include <QIcon>
>+#include <QGraphicsScene>
>+#include <QGraphicsView>
>+#include <QGraphicsSceneContextMenuEvent>
>+#include <QGraphicsSceneDragDropEvent>
>+#include <QGraphicsSceneMouseEvent>
>+#include <QGraphicsSceneHoverEvent>
>+#include <QGraphicsSceneWheelEvent>
>+#include <QGraphicsSceneResizeEvent>
>+#include <QStyleOptionGraphicsItem>
> #include <QPaintEngine>
>+#include <QMimeData>

You seem to be removing the folders in other files, especially here.

r+ since the code changes seem fine. Think about whether you want to drop the folder from other cairo-qt-surface.cpp includes
Attachment #598763 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 8

5 years ago
I'm dropping QtGui prefix, only for includes which has changed it's location after Qt5 components reorg.

Actually Cairo changes are not supposed to be here, it was pushed recently to upstream and handled in separate bug 726461... I'll remove it from this patch.
(Assignee)

Updated

5 years ago
Blocks: 735598
(Assignee)

Comment 9

5 years ago
Created attachment 608592 [details] [diff] [review]
Qt5 port

Refreshed to latest trunk, and tested on try, N9, beagle board, et.c ;)
Attachment #598763 - Attachment is obsolete: true
Attachment #598763 - Flags: review?(mozilla)
Attachment #608592 - Flags: review?(doug.turner)
Comment on attachment 608592 [details] [diff] [review]
Qt5 port

Review of attachment 608592 [details] [diff] [review]:
-----------------------------------------------------------------

make sure you run this on Try.  No need for bustages.  Thanks Oleg!
Attachment #608592 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/82fe9c59db85
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/82fe9c59db85
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.