Bug 62969 - [WK2][Qt] Move from QGraphicsView to Qt Scene Graph
Summary: [WK2][Qt] Move from QGraphicsView to Qt Scene Graph
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on: 65189 65310
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-20 02:08 PDT by Simon Hausmann
Modified: 2011-07-28 05:14 PDT (History)
17 users (show)

See Also:


Attachments
Simple patch to illustrate scene graph port (17.97 KB, patch)
2011-06-20 02:51 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff
Patch (39.99 KB, patch)
2011-07-14 07:56 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (41.09 KB, patch)
2011-07-14 08:00 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (42.27 KB, patch)
2011-07-27 11:59 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2011-06-20 02:08:47 PDT
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
Comment 1 Simon Hausmann 2011-06-20 02:21:10 PDT
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 :)
Comment 2 Simon Hausmann 2011-06-20 02:51:59 PDT
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...
Comment 3 Gopal Raghavan 2011-06-20 07:16:46 PDT
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
Comment 4 Kenneth Rohde Christiansen 2011-06-21 10:06:50 PDT
Comment on attachment 97771 [details]
Simple patch to illustrate scene graph port

Looks pretty nice and sounds like a good plan
Comment 5 Benjamin Poulain 2011-07-14 07:56:25 PDT
Created attachment 100807 [details]
Patch
Comment 6 Benjamin Poulain 2011-07-14 08:00:39 PDT
Created attachment 100808 [details]
Patch
Comment 7 Benjamin Poulain 2011-07-14 08:03:36 PDT
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.
Comment 8 Early Warning System Bot 2011-07-14 09:44:44 PDT
Comment on attachment 100808 [details]
Patch

Attachment 100808 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9064231
Comment 9 Jocelyn Turcotte 2011-07-20 12:32:36 PDT
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.
Comment 10 Andreas Kling 2011-07-26 10:11:47 PDT
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
Comment 11 Andreas Kling 2011-07-27 03:58:17 PDT
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.
Comment 12 Benjamin Poulain 2011-07-27 11:59:01 PDT
Created attachment 102160 [details]
Patch
Comment 13 Benjamin Poulain 2011-07-27 12:21:53 PDT
Comment on attachment 102160 [details]
Patch

Clearing flags on attachment: 102160

Committed r91863: <http://trac.webkit.org/changeset/91863>
Comment 14 Benjamin Poulain 2011-07-27 12:22:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Csaba Osztrogonác 2011-07-28 01:39:58 PDT
(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.
Comment 16 Csaba Osztrogonác 2011-07-28 01:46:39 PDT
I really don't understand why did you land code which doesn't build ...
Comment 17 Balazs Kelemen 2011-07-28 01:53:38 PDT
(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.
Comment 18 Csaba Osztrogonác 2011-07-28 02:12:58 PDT
(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.
Comment 19 Balazs Kelemen 2011-07-28 02:16:59 PDT
(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"? :)
Comment 20 Andras Becsi 2011-07-28 03:06:27 PDT
(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.