Bug 62969 – [WK2][Qt] Move from QGraphicsView to Qt Scene Graph

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Simon Hausmann]

This bug tracks the move of the WebKit2-Qt build from QGraphicsView to the Qt Scene Graph.

The goals are:

1) Building WebKit2-Qt requires Qt 5
2) The rendering is done exclusively with the Qt Scene Graph - we don't need to support any other rendering paths (like QGraphicsView or QWidget)
3) No patches that remove Qt 4 support should be landed until the build bots are set up accordingly

[Simon Hausmann]

Proposed steps:

1) Move QGrahicsWKView over to a QSGPainted item first and make the MiniBrowser use QSGCanvas. (simple patch work in progress)

2) Then we turn BrowserView.cpp/h into the class that controls the viewport, i.e. scroll position, etc.

3) Make QGrahpicsWKView a plain QObject, make the tiles scene graph nodes, make BrowserView.cpp/h tell QGraphicsWKView the scroll positions, etc. and let QGraphicsWKView position the nodes accordingly (maybe let TiledDrawingAreaProxyQt actually do that :)

4) Move BrowserView.cpp/h into the library, give it a better name, merge QGraphicsWKView/QWKPage (or improve the split simply)

5) Move MiniBrowser over to real QML

6) ...

7) Profit :)

[Simon Hausmann]

Created attachment 97771 [details]
Simple patch to illustrate scene graph port

This is a simple patch (500 loc) that is work in progress that illustrates the porting path to the scene graph, initially based on the FBO based painted item.

It requires QML_NO_THREADED_RENDERER=1: paint() is otherwise called from the rendering thread, which will start a timer that was constructed in the gui thread. That's not possible. I think the NO_THREADED_RENDERER workaround is fine since we anyway want to move away from PaintedItem and position the tiles in the scene
graph ourselves. If the node updates can only be done from the rendering thread, then we need to find another solution for the timer problem...

[Gopal Raghavan]

Simon,
Similar work already happening in http://gitorious.net/+mob-team/webkit/mob-team-qtwebkit2/commits/qml2-inprogress-sl

We can probably collaborate.
Thanks
--
Gopal

[Kenneth Rohde Christiansen]

Comment on attachment 97771 [details]
Simple patch to illustrate scene graph port

Looks pretty nice and sounds like a good plan

[Benjamin Poulain]

Created attachment 100807 [details]
Patch

[Benjamin Poulain]

Created attachment 100808 [details]
Patch

[Benjamin Poulain]

Sorry about the previous patch, I forgot to squash one local change.

This is very the basics to get the existing code running on Qt 5. More changes are needed to get the webview to be more stable and to have good performance.

[Early Warning System Bot]

Comment on attachment 100808 [details]
Patch

Attachment 100808 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9064231

[Jocelyn Turcotte]

Comment on attachment 100808 [details]
Patch

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

This patch works actually pretty nicely :)
Some comments:

> Source/WebKit2/UIProcess/qt/TouchViewInterface.cpp:76
> -void TouchViewInterface::pinchGestureRequestUpdate(const QPointF& pinchCenterInPageViewCoordinate, qreal totalScaleFactor)
> +void TouchViewInterface::pinchGestureRequestUpdate(const QPointF&, qreal totalScaleFactor)
>  {
> -    // FIXME: it is a bit more complicated than that, changes of the center position should move the page even
> -    // if the zoom factor does not change. Both the zoom and the panning should be handled through the physics
> -    // engine.
> +    // FIXME: it is a more complicated than that:
> +    // -the scale should be done centered on the pinch center.
> +    // -changes of the center position should move the page even if the zoom factor
> +    //  does not change. Both the zoom and the panning should be handled through the physics engine.
>      const qreal scale = m_pinchStartScale * totalScaleFactor;
> -    m_pageView->setTransformOriginPoint(pinchCenterInPageViewCoordinate);
>      m_pageView->setScale(scale);

Even though you added the FIXME, this makes pinch zooming pretty difficult to use meanwhile.
This should work OK I think:

    QPointF oldPinchCenterOnParent = m_pageView->mapToItem(m_pageView->parentItem(), pinchCenterInPageViewCoordinate);
    m_pageView->setScale(scale);
    QPointF newPinchCenterOnParent = m_pageView->mapToItem(m_pageView->parentItem(), pinchCenterInPageViewCoordinate);
    m_pageView->setPos(m_pageView->pos() - (newPinchCenterOnParent - oldPinchCenterOnParent));

> Tools/MiniBrowser/qt/main.cpp:39
> +    qputenv("QML_NO_THREADED_RENDERER", QByteArray("1"));

It would be nice to have a comment on why this is needed here, so that we can know when we can remove it.

[Andreas Kling]

Comment on attachment 100808 [details]
Patch

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

r=me.

I must say the event delivery looks pretty disgusting, but there's not much we can do about that here. :(
All in all, looks like a good place to start. Please consider Jocelyn's comments before landing.

> Source/WebKit2/ChangeLog:14
> +        QTouchWebView become a QSGItem with the only role of clipping
> +        children items.

become -> becomes
children items -> child items

> Source/WebKit2/ChangeLog:17
> +        QTouchWebPage become a QSGPaintedItem in order to render the content
> +        of the flattened page. This is a temporary work around to get
> +        something running withtout deep change on the drawing area proxy.

become -> becomes
withtout -> without
deep change -> deep changes

[Andreas Kling]

Comment on attachment 100808 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.h:31
> +#include <stdio.h>

Derp.

[Benjamin Poulain]

Created attachment 102160 [details]
Patch

[Benjamin Poulain]

Comment on attachment 102160 [details]
Patch

Clearing flags on attachment: 102160

Committed r91863: <http://trac.webkit.org/changeset/91863>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #13)
> (From update of attachment 102160 [details])
> Clearing flags on attachment: 102160
> 
> Committed r91863: <http://trac.webkit.org/changeset/91863>

It broke the WK2 build. :-/

http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Release%20WebKit2/builds/9717/steps/compile-webkit/logs/stdio

Reopen to fix it.

[Csaba Osztrogonác]

I really don't understand why did you land code which doesn't build ...

[Balazs Kelemen]

(In reply to comment #16)
> I really don't understand why did you land code which doesn't build ...

Calm down Ossy! Our environments are different especially with Qt5. I'm sure it built in the environment where it was developed.

[Csaba Osztrogonác]

(In reply to comment #17)
> (In reply to comment #16)
> > I really don't understand why did you land code which doesn't build ...
> 
> Calm down Ossy! Our environments are different especially with Qt5. I'm sure it built in the environment where it was developed.

But we should have _same_ environment as all developers ... 
A bot with different environment is absolutely useless ... 

And I think the author of the patch should check the bot after his commit.

[Balazs Kelemen]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > I really don't understand why did you land code which doesn't build ...
> > 
> > Calm down Ossy! Our environments are different especially with Qt5. I'm sure it built in the environment where it was developed.
> 
> But we should have _same_ environment as all developers ... 
> A bot with different environment is absolutely useless ... 
> 

I need to quote the Black Adder: "have you ever take a journey on earth"? :)

[Andras Becsi]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > I really don't understand why did you land code which doesn't build ...
> > 
> > Calm down Ossy! Our environments are different especially with Qt5. I'm sure it built in the environment where it was developed.
> 
> But we should have _same_ environment as all developers ... 
> A bot with different environment is absolutely useless ... 
> 
> And I think the author of the patch should check the bot after his commit.

The Qt5 environment is not yet stable, so you should really calm down a bit. I updated the Qt5 env on the bot, and now the patch builds.