Last Comment Bug 703155 - Port Mozilla Qt to Qt5
: Port Mozilla Qt to Qt5
Status: RESOLVED FIXED
:
Product: Core Graveyard
Classification: Graveyard
Component: Widget: Qt (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla14
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
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: ---


Attachments
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 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 Oleg Romashin (:romaxa) 2011-11-17 18:40:44 PST
Created attachment 575359 [details] [diff] [review]
WIP1
Comment 2 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 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 Karl Tomlinson (: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 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 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 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 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 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 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 Ed Morley [:emorley] 2012-03-27 05:22:32 PDT
https://hg.mozilla.org/mozilla-central/rev/82fe9c59db85

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