Last Comment Bug 703155 - Port Mozilla Qt to Qt5
: Port Mozilla Qt to Qt5
Product: Core Graveyard
Classification: Graveyard
Component: Widget: Qt (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla14
Assigned To: Oleg Romashin (:romaxa)
Depends on: 726461
Blocks: 735598
  Show dependency treegraph
Reported: 2011-11-16 17:19 PST by Oleg Romashin (:romaxa)
Modified: 2016-07-11 21:54 PDT (History)
7 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

WIP1 (45.56 KB, patch)
2011-11-17 18:40 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Qt5 building and basic working port (29.48 KB, patch)
2012-02-13 21:57 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Qt5 port (31.26 KB, patch)
2012-02-19 21:09 PST, Oleg Romashin (:romaxa)
mark.finkle: review+
mh+mozilla: review+
Details | Diff | Splinter Review
Qt5 port (40.52 KB, patch)
2012-03-22 20:59 PDT, Oleg Romashin (:romaxa)
doug.turner: review+
Details | Diff | Splinter Review

Description User image Oleg Romashin (:romaxa) 2011-11-16 17:19:36 PST
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.
Comment 1 User image Oleg Romashin (:romaxa) 2011-11-17 18:40:44 PST
Created attachment 575359 [details] [diff] [review]
Comment 2 User image Oleg Romashin (:romaxa) 2012-02-13 21:57:44 PST
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
Comment 3 User image Oleg Romashin (:romaxa) 2012-02-14 10:57:43 PST
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?
Comment 4 User image Karl Tomlinson (back Feb 1 :karlt) 2012-02-14 14:24:32 PST
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.
Comment 5 User image Oleg Romashin (:romaxa) 2012-02-19 21:09:59 PST
Created attachment 598763 [details] [diff] [review]
Qt5 port

Here is the version with more strict display/screen requests from qt5 xcb/xlib platform interface.
Comment 6 User image Mike Hommey [:glandium] 2012-02-19 23:12:07 PST
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.
Comment 7 User image Mark Finkle (:mfinkle) (use needinfo?) 2012-02-21 22:20:48 PST
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
Comment 8 User image Oleg Romashin (:romaxa) 2012-02-21 22:52:04 PST
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.
Comment 9 User image Oleg Romashin (:romaxa) 2012-03-22 20:59:58 PDT
Created attachment 608592 [details] [diff] [review]
Qt5 port

Refreshed to latest trunk, and tested on try, N9, beagle board, et.c ;)
Comment 10 User image Doug Turner (:dougt) 2012-03-26 12:09:31 PDT
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!
Comment 12 User image Ed Morley [:emorley] 2012-03-27 05:22:32 PDT

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