Bug 76661 - [Qt] [WK2] Support threaded renderer in WK2
Summary: [Qt] [WK2] Support threaded renderer in WK2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Noam Rosenthal
URL:
Keywords: Qt
Depends on: 76674 80296 80714
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-19 13:39 PST by Viatcheslav Ostapenko
Modified: 2012-03-11 00:26 PST (History)
11 users (show)

See Also:


Attachments
Patch (28.00 KB, patch)
2012-01-19 13:58 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Updated patch. (32.14 KB, patch)
2012-01-19 14:39 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Fix windows build. (31.95 KB, patch)
2012-01-19 15:13 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
One more windows build fix. (31.98 KB, patch)
2012-01-19 15:29 PST, Viatcheslav Ostapenko
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Reworked patch. (28.57 KB, patch)
2012-01-19 18:20 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Patch (25.98 KB, patch)
2012-03-04 18:13 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (25.98 KB, patch)
2012-03-05 06:39 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (25.98 KB, patch)
2012-03-05 07:22 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Updated patch (59.78 KB, patch)
2012-03-08 21:19 PST, Viatcheslav Ostapenko
noam: review-
noam: commit-queue-
Details | Formatted Diff | Diff
Updated patch (59.79 KB, patch)
2012-03-09 09:37 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff
Updated patch (59.11 KB, patch)
2012-03-10 08:35 PST, Viatcheslav Ostapenko
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (57.90 KB, patch)
2012-03-10 11:59 PST, Viatcheslav Ostapenko
noam: review+
Details | Formatted Diff | Diff
Patch for commit. (57.90 KB, patch)
2012-03-10 22:00 PST, Viatcheslav Ostapenko
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Viatcheslav Ostapenko 2012-01-19 13:39:29 PST
Should remove "FIXME" in Tools/MiniBrowser/qt/main.cpp/main() .
Comment 1 Viatcheslav Ostapenko 2012-01-19 13:58:35 PST
Created attachment 123189 [details]
Patch
Comment 2 Viatcheslav Ostapenko 2012-01-19 14:28:23 PST
Comment on attachment 123189 [details]
Patch

Have to rebase after Jocelyns patch.
Comment 3 Viatcheslav Ostapenko 2012-01-19 14:39:12 PST
Created attachment 123197 [details]
Updated patch.
Comment 4 Viatcheslav Ostapenko 2012-01-19 15:13:19 PST
Created attachment 123206 [details]
Fix windows build.
Comment 5 Viatcheslav Ostapenko 2012-01-19 15:29:01 PST
Created attachment 123209 [details]
One more windows build fix.
Comment 6 Noam Rosenthal 2012-01-19 15:53:12 PST
Comment on attachment 123209 [details]
One more windows build fix.

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

OK, needs lots of work :)

> Source/WebKit2/Target.pri:258
> +    UIProcess/qt/LayerTreeHostProxyNodeQt.h \

Name is not by convention, since this is not a subclass of a WebCore/WebKit class.
How about QtLayerTreeSceneGraphNode

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:95
> +    return 0;

Why are we returning zero?

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:41
> +#include <QObject>

?

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:68
> +    void paintToCurrentGLContext(const WebCore::TransformationMatrix&, float, bool syncRemoteLayerData = true);

I tried to follow the logic of when syncRemoteLayerData is called and when not... I still don't get it.

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:187
> +template<typename T> struct MainThreadMessageSender {
> +    struct CallbackContext {
> +        CallbackContext(const T& message, LayerTreeHostProxy* layerTreeHostProxy) :
> +                m_messageID(T::messageID)
> +            ,   m_layerTreeHostProxy(layerTreeHostProxy)
> +        {
> +            m_encoder = CoreIPC::ArgumentEncoder::create(0);
> +            m_encoder->encode(message);
> +        }
> +        ~CallbackContext()
> +        {
> +        }
> +        OwnPtr<CoreIPC::ArgumentEncoder> m_encoder;
> +        CoreIPC::MessageID m_messageID;
> +        RefPtr<LayerTreeHostProxy> m_layerTreeHostProxy;
> +    };
> +    static void mainThreadCallback(void* data)
> +    {
> +        CallbackContext* context = static_cast<CallbackContext*>(data);
> +        DrawingAreaProxy* drawingAreaProxy = context->m_layerTreeHostProxy->drawingAreaProxy();
> +        if (drawingAreaProxy) {
> +            // Have to initialize destanation ID here because this data cannot be accessed from
> +            // painting thread.
> +            *reinterpret_cast<uint64_t*>(context->m_encoder->buffer()) = drawingAreaProxy->page()->pageID();
> +            CoreIPC::Connection* connection = drawingAreaProxy->page()->process()->connection();
> +            if (connection)
> +                connection->sendMessage(context->m_messageID, context->m_encoder.release(), 0);
> +        }
> +        delete context;
> +    }
> +
> +    static void sendMessageOnMainThread(const T& message, LayerTreeHostProxy* layerTreeHostProxy)
> +    {
> +        CallbackContext* context = new CallbackContext(message, layerTreeHostProxy);
> +        callOnMainThread(&mainThreadCallback, context);
> +    }
> +};

You need to do all that just to call updateViewport on the main thread?

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:220
> +static updateViewportOnMainThread(LayerTreeHostProxy* layerTreeHostProxy)
> +{
> +    layerTreeHostProxy->updateViewport();
> +}
> +#endif

the pointer to LayerTreeHostProxy is not thread safe, since that proxy might be deleted before the painting stopped.

> Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:178
> +    if (itemRect.isEmpty())
> +        return;
> +

Is this related?
Comment 7 Viatcheslav Ostapenko 2012-01-19 16:10:07 PST
(In reply to comment #6)
> (From update of attachment 123209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123209&action=review
> 
> OK, needs lots of work :)
> 
> > Source/WebKit2/Target.pri:258
> > +    UIProcess/qt/LayerTreeHostProxyNodeQt.h \
> 
> Name is not by convention, since this is not a subclass of a WebCore/WebKit class.
> How about QtLayerTreeSceneGraphNode

Ok. I'm very bad in naming ;)

> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:95
> > +    return 0;
> 
> Why are we returning zero?

But what should it return if LayerTreeHostProxy is not allocated yet?
Allocation is moved to LayerTreeHostProxy::updatePaintNode

> > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:41
> > +#include <QObject>
> 
> ?

Oops!

> > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:68
> > +    void paintToCurrentGLContext(const WebCore::TransformationMatrix&, float, bool syncRemoteLayerData = true);
> 
> I tried to follow the logic of when syncRemoteLayerData is called and when not... I still don't get it.

It's simple.
In Qt case it will be never call from paintToCurrentGLContext, only from updatePaintNode . It left there with default parameter if others use this function.

> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:187
> > +template<typename T> struct MainThreadMessageSender {
> > +    struct CallbackContext {
> > +        CallbackContext(const T& message, LayerTreeHostProxy* layerTreeHostProxy) :
> > +                m_messageID(T::messageID)
> > +            ,   m_layerTreeHostProxy(layerTreeHostProxy)
> > +        {
> > +            m_encoder = CoreIPC::ArgumentEncoder::create(0);
> > +            m_encoder->encode(message);
> > +        }
> > +        ~CallbackContext()
> > +        {
> > +        }
> > +        OwnPtr<CoreIPC::ArgumentEncoder> m_encoder;
> > +        CoreIPC::MessageID m_messageID;
> > +        RefPtr<LayerTreeHostProxy> m_layerTreeHostProxy;
> > +    };
> > +    static void mainThreadCallback(void* data)
> > +    {
> > +        CallbackContext* context = static_cast<CallbackContext*>(data);
> > +        DrawingAreaProxy* drawingAreaProxy = context->m_layerTreeHostProxy->drawingAreaProxy();
> > +        if (drawingAreaProxy) {
> > +            // Have to initialize destanation ID here because this data cannot be accessed from
> > +            // painting thread.
> > +            *reinterpret_cast<uint64_t*>(context->m_encoder->buffer()) = drawingAreaProxy->page()->pageID();
> > +            CoreIPC::Connection* connection = drawingAreaProxy->page()->process()->connection();
> > +            if (connection)
> > +                connection->sendMessage(context->m_messageID, context->m_encoder.release(), 0);
> > +        }
> > +        delete context;
> > +    }
> > +
> > +    static void sendMessageOnMainThread(const T& message, LayerTreeHostProxy* layerTreeHostProxy)
> > +    {
> > +        CallbackContext* context = new CallbackContext(message, layerTreeHostProxy);
> > +        callOnMainThread(&mainThreadCallback, context);
> > +    }
> > +};
> 
> You need to do all that just to call updateViewport on the main thread?
> 
> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:220
> > +static updateViewportOnMainThread(LayerTreeHostProxy* layerTreeHostProxy)
> > +{
> > +    layerTreeHostProxy->updateViewport();
> > +}
> > +#endif
> 
> the pointer to LayerTreeHostProxy is not thread safe, since that proxy might be deleted before the painting stopped.

No, it can't. Paint node holds the reference to LayerTreeHostProxy. I've mentioned this in ChangeLog.

> > Source/WebKit2/UIProcess/qt/QtViewportInteractionEngine.cpp:178
> > +    if (itemRect.isEmpty())
> > +        return;
> > +
> 
> Is this related?

I don't know, but I was getting division errors in this functions when I was running API tests. The item indeed was 0 size and was resized later.
Comment 8 Noam Rosenthal 2012-01-19 16:16:56 PST
> But what should it return if LayerTreeHostProxy is not allocated yet?
> Allocation is moved to LayerTreeHostProxy::updatePaintNode
OK.

> It's simple.
> In Qt case it will be never call from paintToCurrentGLContext, only from updatePaintNode . It left there with default parameter if others use this function.
> 

How about just removing that parameter, and allowing people to call the sync function directly if they so choose. Otherwise it's a boolean trap.

> > You need to do all that just to call updateViewport on the main thread?
I'm sure we can do the callOnMainThread stuff with less code. This is really elaborate.

> No, it can't. Paint node holds the reference to LayerTreeHostProxy. I've mentioned this in ChangeLog.
OK.

> I don't know, but I was getting division errors in this functions when I was running API tests. The item indeed was 0 size and was resized later.
OK, should be a different patch.
Comment 9 Viatcheslav Ostapenko 2012-01-19 16:18:54 PST
(In reply to comment #6)
> (From update of attachment 123209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123209&action=review
> > +    }
> > +};
> 
> You need to do all that just to call updateViewport on the main thread?

No. Sending messages to webprocess is bigger part and also has to be done from main thread.
Comment 10 Viatcheslav Ostapenko 2012-01-19 16:22:11 PST
(In reply to comment #8)
> > But what should it return if LayerTreeHostProxy is not allocated yet?
> > Allocation is moved to LayerTreeHostProxy::updatePaintNode
> OK.
> 
> > It's simple.
> > In Qt case it will be never call from paintToCurrentGLContext, only from updatePaintNode . It left there with default parameter if others use this function.
> > 
> 
> How about just removing that parameter, and allowing people to call the sync function directly if they so choose. Otherwise it's a boolean trap.

Ok

> > > You need to do all that just to call updateViewport on the main thread?
> I'm sure we can do the callOnMainThread stuff with less code. This is really elaborate.
> 
> > No, it can't. Paint node holds the reference to LayerTreeHostProxy. I've mentioned this in ChangeLog.
> OK.
> 
> > I don't know, but I was getting division errors in this functions when I was running API tests. The item indeed was 0 size and was resized later.
> OK, should be a different patch.

Ok. I'll create patch.
Comment 11 Viatcheslav Ostapenko 2012-01-19 18:20:26 PST
Created attachment 123234 [details]
Reworked patch.
Comment 12 Noam Rosenthal 2012-01-20 07:50:09 PST
Comment on attachment 123234 [details]
Reworked patch.

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

Some more comments. Getting better!

> Source/WebKit2/ChangeLog:52
> +        (WebKit::QtLayerTreeSceneGraphNode::QtLayerTreeSceneGraphNode):

Sorry to ask you to rename something twice, let's do QtLayerTreeSGNode

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:93
> +    DrawingAreaProxy* drawingArea = d->webPageProxy->drawingArea();
> +    if (drawingArea && drawingArea->layerTreeHostProxy())
> +        return drawingArea->layerTreeHostProxy()->updatePaintNode(oldNode);

Add a layerTreeHostProxy() function to QQuickWebPagePrivate instead of doing this twice

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:138
> +    DrawingAreaProxy* drawingArea = d->webPageProxy->drawingArea();
> +    if (drawingArea && drawingArea->layerTreeHostProxy())
> +        return drawingArea->layerTreeHostProxy()->setPaintContentsScale(scale);

Ditto

> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp:69
>  #if USE(ACCELERATED_COMPOSITING)
> +    // LayerTreeHostProxy can be still referenced by paint node,
> +    // so drawing area should be cleared to avoid derefencing pointer to deallocated object.
> +#if USE(TEXTURE_MAPPER)
> +    if (m_layerTreeHostProxy)
> +        m_layerTreeHostProxy->setDrawingAreaProxy(0);
> +#endif

Instead, we should have a detachFromDrawingArea() function in layerTreeHostProxy; In that function you can call purgeBackingStores. That way, you don't need to send a requestPurge from the render thread, which would make the bridge even simpler (all you really need is updateViewport).

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:54
> +#if PLATFORM(QT)
> +class LayerTreeHostProxyBase : public ThreadSafeRefCounted<LayerTreeHostProxyBase>, public WebCore::GraphicsLayerClient {
> +};
> +#else
> +class LayerTreeHostProxyBase : public RefCounted<LayerTreeHostProxyBase>, public WebCore::GraphicsLayerClient {
> +};
> +#endif

Not sure we need to make this separation. For now let's just make it ThreadSafeRefCounted, and other ports that want to use it simply won't use the ThreadSafe aspect.

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:56
> +class LayerTreeHostProxy : public LayerTreeHostProxyBase {

If you remove the former #ifdef, this becomes unnecessary and you can simply subclass ThreadSafeRC<> and GraphicsLayerClient

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:85
> +    void setPaintContentsScale(float scale) { m_paintContentsScale = scale; }

setContentsScale

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:149
> +    float m_paintContentsScale;

m_contentsScale

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:157
> +#ifndef NDEBUG
> +    bool m_inPainting;
> +#endif

Not needed, see other comments.

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:158
> +    RefPtr<LayerTreeHostProxy> m_layerTreeHostProxy;

Shouldn't this be a ThreadSafeRefPtr?

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:247
>  void LayerTreeHostProxy::didFireViewportUpdateTimer(Timer<LayerTreeHostProxy>*)
>  {
> -    updateViewport();
> +    invokeMethodOnMainThread(&WebKit::LayerTreeHostProxy::updateViewport);
>  }

Is this timer still necessary? can't we just invoke the function from paintToCurrentGLContext ?

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:251
> +    ASSERT(!m_inPainting);

ASSERT(isMainThread());

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:634
> +    ASSERT(!m_inPainting);

ASSERT(isMainThread());

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:642
> +    ASSERT(!m_inPainting);

ASSERT(isMainThread());

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:657
> +    invokeMethodOnMainThread(&WebKit::LayerTreeHostProxy::requestPurgeBackingStores);

See previous comment, suggesting we request the PurgeBackingStores when detaching from the drawing-area, which saves us this cross-thread call.

> Source/WebKit2/UIProcess/qt/QtLayerTreeSceneGraphNode.cpp:50
> +#ifndef NDEBUG
> +    m_layerTreeHostProxy->m_inPainting = true;
> +#endif

Not needed. See previous comments.

> Source/WebKit2/UIProcess/qt/QtLayerTreeSceneGraphNode.cpp:59
> +#ifndef NDEBUG
> +    m_layerTreeHostProxy->m_inPainting = false;
> +#endif

Not needed.
Comment 13 Viatcheslav Ostapenko 2012-01-20 08:42:29 PST
(In reply to comment #12)
> (From update of attachment 123234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123234&action=review
> 
> Some more comments. Getting better!
> 
> > Source/WebKit2/ChangeLog:52
> > +        (WebKit::QtLayerTreeSceneGraphNode::QtLayerTreeSceneGraphNode):
> 
> Sorry to ask you to rename something twice, let's do QtLayerTreeSGNode
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:93
> > +    DrawingAreaProxy* drawingArea = d->webPageProxy->drawingArea();
> > +    if (drawingArea && drawingArea->layerTreeHostProxy())
> > +        return drawingArea->layerTreeHostProxy()->updatePaintNode(oldNode);
> 
> Add a layerTreeHostProxy() function to QQuickWebPagePrivate instead of doing this twice
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:138
> > +    DrawingAreaProxy* drawingArea = d->webPageProxy->drawingArea();
> > +    if (drawingArea && drawingArea->layerTreeHostProxy())
> > +        return drawingArea->layerTreeHostProxy()->setPaintContentsScale(scale);
> 
> Ditto
> 
> > Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp:69
> >  #if USE(ACCELERATED_COMPOSITING)
> > +    // LayerTreeHostProxy can be still referenced by paint node,
> > +    // so drawing area should be cleared to avoid derefencing pointer to deallocated object.
> > +#if USE(TEXTURE_MAPPER)
> > +    if (m_layerTreeHostProxy)
> > +        m_layerTreeHostProxy->setDrawingAreaProxy(0);
> > +#endif
> 
> Instead, we should have a detachFromDrawingArea() function in layerTreeHostProxy; In that function you can call purgeBackingStores. That way, you don't need to send a requestPurge from the render thread, which would make the bridge even simpler (all you really need is updateViewport).
> 
> > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:54
> > +#if PLATFORM(QT)
> > +class LayerTreeHostProxyBase : public ThreadSafeRefCounted<LayerTreeHostProxyBase>, public WebCore::GraphicsLayerClient {
> > +};
> > +#else
> > +class LayerTreeHostProxyBase : public RefCounted<LayerTreeHostProxyBase>, public WebCore::GraphicsLayerClient {
> > +};
> > +#endif
> 
> Not sure we need to make this separation. For now let's just make it ThreadSafeRefCounted, and other ports that want to use it simply won't use the ThreadSafe aspect.
> 
> > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:56
> > +class LayerTreeHostProxy : public LayerTreeHostProxyBase {
> 
> If you remove the former #ifdef, this becomes unnecessary and you can simply subclass ThreadSafeRC<> and GraphicsLayerClient
> 
> > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:85
> > +    void setPaintContentsScale(float scale) { m_paintContentsScale = scale; }
> 
> setContentsScale
> 
> > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:149
> > +    float m_paintContentsScale;
> 
> m_contentsScale
> 
> > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:157
> > +#ifndef NDEBUG
> > +    bool m_inPainting;
> > +#endif
> 
> Not needed, see other comments.
> 
> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:158
> > +    RefPtr<LayerTreeHostProxy> m_layerTreeHostProxy;
> 
> Shouldn't this be a ThreadSafeRefPtr?
> 
> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:247
> >  void LayerTreeHostProxy::didFireViewportUpdateTimer(Timer<LayerTreeHostProxy>*)
> >  {
> > -    updateViewport();
> > +    invokeMethodOnMainThread(&WebKit::LayerTreeHostProxy::updateViewport);
> >  }
> 
> Is this timer still necessary? can't we just invoke the function from paintToCurrentGLContext ?

I think this timer forces Qt to do repaint.
If nothing other happens Qt stops repainting.
IMHO, something could be marked dirty and this would cause repaint, but now I don't know what can be marked and this could be fixed later.

> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:251
> > +    ASSERT(!m_inPainting);
> 
> ASSERT(isMainThread());

For some reason I decided, that isMainThread is not defined on Mac. Is it?

> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:657
> > +    invokeMethodOnMainThread(&WebKit::LayerTreeHostProxy::requestPurgeBackingStores);
> 
> See previous comment, suggesting we request the PurgeBackingStores when detaching from the drawing-area, which saves us this cross-thread call.

Destruction of LayerTreeHostProxy is not the only case.
Paint node is deallocated when item removed from scene. This how we force invisible pages to drop resources for memory saving.
Comment 14 Jocelyn Turcotte 2012-01-20 08:55:10 PST
Comment on attachment 123234 [details]
Reworked patch.

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

The clean up of QQuickWebPage looks very nice, some other comments:

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:36
> +    setClip(true);

Why is this needed, isn't the texture mapper overriding the clip anyway?

>> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:93
>> +        return drawingArea->layerTreeHostProxy()->updatePaintNode(oldNode);
> 
> Add a layerTreeHostProxy() function to QQuickWebPagePrivate instead of doing this twice

I don't like the idea of the LayerTreeHost being referenced both on the scene graph and in WebKit.
The problem is when the WebView gets destroyed on the UI thread, the rendering thread must be able to continue. And delaying the destruction of the LayerTreeHostProxy alone until the rendering thread is done destroying the scene graph nodes seems wrong to me.

Isn't it possible to instead move all the texture mapper related objects/nodes ownership to the scene graph node that we attach on the scene graph, and leave the LayerTreeHostProxy referenced by the UI thread only? To put it in other words, all the texture mapper node tree would be owned only by the object we return from updatePaintNode.

> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp:68
> +        m_layerTreeHostProxy->setDrawingAreaProxy(0);

This is the kind of hacks that results on having a dual ownership across threads. It strongly smells future maintenance problems.
Comment 15 Viatcheslav Ostapenko 2012-01-20 09:19:27 PST
(In reply to comment #14)
> (From update of attachment 123234 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123234&action=review
> 
> The clean up of QQuickWebPage looks very nice, some other comments:
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:36
> > +    setClip(true);
> 
> Why is this needed, isn't the texture mapper overriding the clip anyway?

1. It doesn't work if display doesn't have stencil buffer.
2. I think we shouldn't have clipping it texturemapper, because Qt does it more effectively choosing between stencil/scissor clipping.
3. It seems there is problems with clipping when we have overshoot panning. At least I've seen painting artefacts and switching to item clipping solves it.

> >> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:93
> >> +        return drawingArea->layerTreeHostProxy()->updatePaintNode(oldNode);
> > 
> > Add a layerTreeHostProxy() function to QQuickWebPagePrivate instead of doing this twice
> 
> I don't like the idea of the LayerTreeHost being referenced both on the scene graph and in WebKit.
> The problem is when the WebView gets destroyed on the UI thread, the rendering thread must be able to continue. And delaying the destruction of the LayerTreeHostProxy alone until the rendering thread is done destroying the scene graph nodes seems wrong to me.
> 
> Isn't it possible to instead move all the texture mapper related objects/nodes ownership to the scene graph node that we attach on the scene graph, and leave the LayerTreeHostProxy referenced by the UI thread only? To put it in other words, all the texture mapper node tree would be owned only by the object we return from updatePaintNode.

Just tried to keep patch minimal. Agreed with Noam on irc that we should do real move.
Comment 16 Kenneth Rohde Christiansen 2012-01-22 15:00:37 PST
Comment on attachment 123234 [details]
Reworked patch.

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

> Source/WebKit2/ChangeLog:10
> +        Added threaded painting support to Qt Webkit2.
> +        All TextureMapper and GraphicsLayer objects are created on rendering
> +        thread and can be accessed through rendering thread only.

Maybe we should make it clear that we are talking about the ui side (which I guess we are without looking at the actual code)

> Source/WebKit2/ChangeLog:18
> +        Layer updates are received through m_messagesToRenderer queue and
> +        applied to QtLayerTreeSceneGraphNode tree in updatePaintNode, when UI thread
> +        is synchronously locked by render thread, so no extra synchronization 
> +        is necessary.
> +        Messages from TextureMapper are delivered back through "callOnMainThread"
> +        interface.
> +        LayerTreeHostProxy is thread safe ref-counted and held alive until paint
> +        node or drawing area destruction.

I would like some newlines to make reading the changelog easier. Or add these details below (in the file listing) like others do

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:88
> +    typedef void (LayerTreeHostProxy::*MethodPtr)();

Wouldnt it be nicer to have this closer to where it is used?

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:116
> +    void requestPurgeBackingStores();

sounds weird to me. what about requestBackingStorePurge? or requestGlobalBackingStorePurge? noam?

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:153
> +struct LayerTreeHostProxyMethodRef {

Ref is a bit overloaded.. it is mostly used for the C classes. Maybe we should just call it Reference, or Handle ?

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:154
> +    LayerTreeHostProxyMethodRef(LayerTreeHostProxy* layerTreeHostProxy, LayerTreeHostProxy::MethodPtr method) :

coding style. Add the : to the next line

> Source/WebKit2/UIProcess/qt/QtLayerTreeSceneGraphNode.cpp:63
> +// FIXME: temporary until Qt Scenegraph will support custom painting.
> +char const* const* QtLayerTreeSceneGraphNode::ProxyMaterialShader::attributeNames() const

any bug number?
Comment 17 Noam Rosenthal 2012-03-04 18:13:06 PST
Created attachment 130040 [details]
Patch
Comment 18 WebKit Review Bot 2012-03-04 18:16:05 PST
Attachment 130040 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:101:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:108:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:115:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Noam Rosenthal 2012-03-04 19:02:51 PST
(In reply to comment #18)
> Attachment 130040 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:101:  Extra space before ( in function call  [whitespace/parens] [4]
> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:108:  Extra space before ( in function call  [whitespace/parens] [4]
> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:115:  Extra space before ( in function call  [whitespace/parens] [4]
> Total errors found: 3 in 11 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

False positives. Filed https://bugs.webkit.org/show_bug.cgi?id=80231
Comment 20 Kenneth Rohde Christiansen 2012-03-05 06:16:45 PST
Comment on attachment 130040 [details]
Patch

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

> Source/WebKit2/ChangeLog:20
> +        Based on a previous patch by Viatcheslav Ostapenko.

I would leave out "previous"

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:86
> +    QtSGWebPageNode* node = static_cast<QtSGWebPageNode*>(oldNode);

Maybe QtWebPageSGNode* sceneNode would be more clear?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:149
> +    MutexLocker lock(m_paintNodeAccessMutex);

isn't access included in mutex terminology?

> Source/WebKit2/UIProcess/API/qt/qquickwebpage_p_p.h:39
> +    virtual void didDeleteSGWebPageNode();

didDeleteSceneGraphNode() ?

>> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:101
>> +    static void call(PassRefPtr<T> objectToGuard, const Function<void ()>& function)
> 
> Extra space before ( in function call  [whitespace/parens] [4]

why void () ? and not void() ?

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:335
> +    // The pending tiles state is on its way for the screen, tell the web process to render the next one.

to* the screen?

> Source/WebKit2/UIProcess/qt/QtSGWebPageNode.cpp:29
> +    , m_contentsScale(1.)

I think WebKit style says to use just 1 here

> Source/WebKit2/UIProcess/qt/QtSGWebPageNode.cpp:45
> +void QtSGWebPageNode::setContentsScale(float s)

scale
Comment 21 Noam Rosenthal 2012-03-05 06:39:23 PST
Created attachment 130115 [details]
Patch
Comment 22 Kenneth Rohde Christiansen 2012-03-05 06:44:26 PST
Comment on attachment 130115 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:86
> +    QtSGWebPageNode* sceneNode = static_cast<QtSGWebPageNode*>(oldNode);

You didn't like the QtWebPageSGNode suggestion? tronical?
Comment 23 Noam Rosenthal 2012-03-05 07:03:17 PST
(In reply to comment #22)
> (From update of attachment 130115 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130115&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:86
> > +    QtSGWebPageNode* sceneNode = static_cast<QtSGWebPageNode*>(oldNode);
> 
> You didn't like the QtWebPageSGNode suggestion? tronical?

Ah, completely missed it. I don't really mind either way :)
Comment 24 Kenneth Rohde Christiansen 2012-03-05 07:07:32 PST
Comment on attachment 130115 [details]
Patch

r=me with comments given on irc
Comment 25 Noam Rosenthal 2012-03-05 07:22:09 PST
Created attachment 130126 [details]
Patch for landing
Comment 26 WebKit Review Bot 2012-03-05 07:52:32 PST
Comment on attachment 130126 [details]
Patch for landing

Clearing flags on attachment: 130126

Committed r109748: <http://trac.webkit.org/changeset/109748>
Comment 27 WebKit Review Bot 2012-03-05 07:52:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Csaba Osztrogonác 2012-03-05 08:47:21 PST
(In reply to comment #26)
> (From update of attachment 130126 [details])
> Clearing flags on attachment: 130126
> 
> Committed r109748: <http://trac.webkit.org/changeset/109748>

Great ... You made all tests crash. :( Could you guys fix it?
Comment 29 Noam Rosenthal 2012-03-05 09:13:07 PST
> Great ... You made all tests crash. :( Could you guys fix it?

No worries, it's part of the trade when making large changes like this :)
btw a better way to say this is "This patch made all the tests crash" rather than "you made all the tests crash".
Comment 30 Noam Rosenthal 2012-03-05 10:48:34 PST
(In reply to comment #28)
> (In reply to comment #26)
> > (From update of attachment 130126 [details] [details])
> > Clearing flags on attachment: 130126
> > 
> > Committed r109748: <http://trac.webkit.org/changeset/109748>
> 
> Great ... You made all tests crash. :( Could you guys fix it?

Can you please point me to the exact place in waterfall where this happens? I spent a while and still couldn't figure out a place where all tests crash.
Comment 31 Noam Rosenthal 2012-03-05 14:34:10 PST
Apparently this code is still very unstable. I'll research and post a new patch later on.
Comment 32 Csaba Osztrogonác 2012-03-05 23:16:17 PST
(In reply to comment #29)
> > Great ... You made all tests crash. :( Could you guys fix it?
> 
> No worries, it's part of the trade when making large changes like this :)
> btw a better way to say this is "This patch made all the tests crash" rather than "you made all the tests crash".

I'm so sorry for being too offensive. Of course not you made all tests crash, but your patch which is absolutely independent from you. :) Next time I try to be more polite and less aggressive if "a patch contributed by you" break anything.
Comment 33 Csaba Osztrogonác 2012-03-05 23:17:43 PST
(In reply to comment #30)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > (From update of attachment 130126 [details] [details] [details])
> > > Clearing flags on attachment: 130126
> > > 
> > > Committed r109748: <http://trac.webkit.org/changeset/109748>
> > 
> > Great ... You made all tests crash. :( Could you guys fix it?
> 
> Can you please point me to the exact place in waterfall where this happens? I spent a while and still couldn't figure out a place where all tests crash.

Sure, http://lmgtfy.com/?q=qtwebkit+buildbots (first hit)
Comment 34 Csaba Osztrogonác 2012-03-05 23:18:46 PST
(In reply to comment #31)
> Apparently this code is still very unstable. I'll research and post a new patch later on.

Otherwise thanks for this rollout - http://trac.webkit.org/changeset/109748 .
Comment 35 Viatcheslav Ostapenko 2012-03-08 21:19:28 PST
Created attachment 130973 [details]
Updated patch
Comment 36 Noam Rosenthal 2012-03-08 22:02:21 PST
Comment on attachment 130973 [details]
Updated patch

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

Something about this doesn't feel right. I understand why the sequence of things and why it's needed for thread-safety, but the class abstraction seems a bit over-complicated. I have only minor suggestions right but let me look at it more thoroughly tomorrow to see if something can be simplified.
I'm afraid that right now complex thread-safe code like this would be very prone to bugs that are hard to detect or fix.

> Source/WebKit2/ChangeLog:101
> +        (WebKit::QtLayerTreeSGNode::QtLayerTreeSGNode):

QtWebPageSGNode

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:69
> +    QSGNode* updatePaintNode(QSGNode* oldNode);

Put in PLATFORM(QT)

> Source/WebKit2/UIProcess/LayerTreePaintData.h:49
> +    void paintToCurrentGLContext(const WebCore::TransformationMatrix&, float, const WebCore::FloatRect&);

Spell out:
float opacity
FloatRect clip

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:163
> +void LayerTreeHostProxy::setPaintScale(float scale)

setScale

> Source/WebKit2/UIProcess/qt/LayerTreePaintDataQt.cpp:22
> +#include "LayerTreePaintData.h"

Let's call it WebLayerTreeRenderer

> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:22
> +#if USE(ACCELERATED_COMPOSITING)

This is not needed

> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:43
> +    QMatrix4x4 m;

m -> matrix

> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:46
> +    m.scale(m_layerTreePaintData->scale());

Please explain

> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:56
> +static QPointF toViewPortCoordinates(const QPointF& point, const QRectF& viewport)

ViewPort -> Viewport

> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:59
> +    return QPointF((point.x() + 1) * viewport.width() * 0.5,
> +                   viewport.height() - (point.y() + 1) * viewport.height() * 0.5);

Comment

> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:85
> +        QMatrix4x4 m = *(state.projectionMatrix);

Hmmm... we're multiplying by the projectionMatrix and then applying toViewPortCoordinates which cancels it out. Seems like this is not 100% thought out.

> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:95
> +            currentClip.setTopLeft(toViewPortCoordinates(m.map(clipRect.topLeft()), viewportRect));
> +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.topRight()), viewportRect));
> +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomLeft()), viewportRect));
> +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomRight()), viewportRect));

This looks very much like a rect.unite... why not just do that?
Comment 37 Noam Rosenthal 2012-03-08 22:04:04 PST
Comment on attachment 130973 [details]
Updated patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:115
> +        d->webPageProxy->drawingArea()->layerTreeHostProxy()->setPaintScale(scale);

Maybe we should just expose setTransform instead of setPaintScale
Comment 38 Viatcheslav Ostapenko 2012-03-08 22:25:47 PST
(In reply to comment #36)
> (From update of attachment 130973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review
> 
> Something about this doesn't feel right. I understand why the sequence of things and why it's needed for thread-safety, but the class abstraction seems a bit over-complicated. I have only minor suggestions right but let me look at it more thoroughly tomorrow to see if something can be simplified.
> I'm afraid that right now complex thread-safe code like this would be very prone to bugs that are hard to detect or fix.

It is, actually, not more complex, then the previous implementation. 
It looks big change because of things moved, but, IMHO, it simplifies things a bit.
It was Jocelyns suggestion from irc talk to move paint objects to the separate class. Now it divides clearly, what can be used from paint thread and what from main thread with few exceptions.

> > Source/WebKit2/ChangeLog:101
> > +        (WebKit::QtLayerTreeSGNode::QtLayerTreeSGNode):
> 
> QtWebPageSGNode
> 
> > Source/WebKit2/UIProcess/LayerTreeHostProxy.h:69
> > +    QSGNode* updatePaintNode(QSGNode* oldNode);
> 
> Put in PLATFORM(QT)
> 
> > Source/WebKit2/UIProcess/LayerTreePaintData.h:49
> > +    void paintToCurrentGLContext(const WebCore::TransformationMatrix&, float, const WebCore::FloatRect&);
> 
> Spell out:
> float opacity
> FloatRect clip

Style check will complain. I hate myself declaration without parameter names.

> > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:163
> > +void LayerTreeHostProxy::setPaintScale(float scale)
> 
> setScale

There is another scale which called contentScale. 

> > Source/WebKit2/UIProcess/qt/LayerTreePaintDataQt.cpp:22
> > +#include "LayerTreePaintData.h"
> 
> Let's call it WebLayerTreeRenderer
> 
> > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:22
> > +#if USE(ACCELERATED_COMPOSITING)
> 
> This is not needed
> 
> > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:43
> > +    QMatrix4x4 m;
> 
> m -> matrix

Will conflict with matrix method.

> > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:46
> > +    m.scale(m_layerTreePaintData->scale());
> 
> Please explain

I would be happy to remove this matrix scaling. I don't understand, why we don't use item scaling and apply scale manually. Commit that introduced this manual scaling doesn't explain it clearly. 

> > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:56
> > +static QPointF toViewPortCoordinates(const QPointF& point, const QRectF& viewport)
> 
> ViewPort -> Viewport
> 
> > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:59
> > +    return QPointF((point.x() + 1) * viewport.width() * 0.5,
> > +                   viewport.height() - (point.y() + 1) * viewport.height() * 0.5);
> 
> Comment
> 
> > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:85
> > +        QMatrix4x4 m = *(state.projectionMatrix);
> 
> Hmmm... we're multiplying by the projectionMatrix and then applying toViewPortCoordinates which cancels it out. Seems like this is not 100% thought out.

I just looked through scissor clipping code in SG and used it as a sample.
So far this works fine for rectangular clippings, but has to be fixed in any case for non-rectangular clippings.

> > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:95
> > +            currentClip.setTopLeft(toViewPortCoordinates(m.map(clipRect.topLeft()), viewportRect));
> > +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.topRight()), viewportRect));
> > +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomLeft()), viewportRect));
> > +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomRight()), viewportRect));
> 
> This looks very much like a rect.unite... why not just do that?

After mapping of coordinates through matrix the clipRect could be rotated and unite will not work.
Comment 39 Viatcheslav Ostapenko 2012-03-08 22:27:15 PST
(In reply to comment #37)
> (From update of attachment 130973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:115
> > +        d->webPageProxy->drawingArea()->layerTreeHostProxy()->setPaintScale(scale);
> 
> Maybe we should just expose setTransform instead of setPaintScale

Oh no!
This will require mutexes which I'd like to avoid.
I'd better revert the patch which makes manual scaling.
Comment 40 Noam Rosenthal 2012-03-09 06:23:27 PST
> Oh no!
> This will require mutexes which I'd like to avoid.
That's the kind of stuff you need to put in comments :)
Comment 41 Noam Rosenthal 2012-03-09 06:29:29 PST
(In reply to comment #38)
> (In reply to comment #36)
> > (From update of attachment 130973 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review
> > 
> > Something about this doesn't feel right. I understand why the sequence of things and why it's needed for thread-safety, but the class abstraction seems a bit over-complicated. I have only minor suggestions right but let me look at it more thoroughly tomorrow to see if something can be simplified.
> > I'm afraid that right now complex thread-safe code like this would be very prone to bugs that are hard to detect or fix.
> 
> It is, actually, not more complex, then the previous implementation. 
> It looks big change because of things moved, but, IMHO, it simplifies things a bit.
> It was Jocelyns suggestion from irc talk to move paint objects to the separate class. Now it divides clearly, what can be used from paint thread and what from main thread with few exceptions.

> > 
> > Spell out:
> > float opacity
> > FloatRect clip
> 
> Style check will complain. I hate myself declaration without parameter names.
Style check will only complain if you do something like FloatRect rect or IntSize size.

> 
> > > Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:163
> > > +void LayerTreeHostProxy::setPaintScale(float scale)
> > 
> > setScale
> 
> There is another scale which called contentScale. 
setRenderScale

> > m -> matrix
> 
> Will conflict with matrix method.
Then use renderMatrix.

> 
> > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:46
> > > +    m.scale(m_layerTreePaintData->scale());
> > 
> > Please explain
> 
> I would be happy to remove this matrix scaling. I don't understand, why we don't use item scaling and apply scale manually. Commit that introduced this manual scaling doesn't explain it clearly. 
> 
> > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:56
> > > +static QPointF toViewPortCoordinates(const QPointF& point, const QRectF& viewport)
> > 
> > ViewPort -> Viewport
> > 
> > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:59
> > > +    return QPointF((point.x() + 1) * viewport.width() * 0.5,
> > > +                   viewport.height() - (point.y() + 1) * viewport.height() * 0.5);
> > 
> > Comment
> > 
> > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:85
> > > +        QMatrix4x4 m = *(state.projectionMatrix);
> > 
> > Hmmm... we're multiplying by the projectionMatrix and then applying toViewPortCoordinates which cancels it out. Seems like this is not 100% thought out.
> 
> I just looked through scissor clipping code in SG and used it as a sample.
Put that in a comment.

> So far this works fine for rectangular clippings, but has to be fixed in any case for non-rectangular clippings.
Add a FIXME

> > > Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:95
> > > +            currentClip.setTopLeft(toViewPortCoordinates(m.map(clipRect.topLeft()), viewportRect));
> > > +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.topRight()), viewportRect));
> > > +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomLeft()), viewportRect));
> > > +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomRight()), viewportRect));
> > 
> > This looks very much like a rect.unite... why not just do that?
> 
> After mapping of coordinates through matrix the clipRect could be rotated and unite will not work.
unite(m.mapRect(...))

I need to investigate this code; It seems too much like a black box.
Comment 42 Noam Rosenthal 2012-03-09 06:43:46 PST
Comment on attachment 130973 [details]
Updated patch

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

>>> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:115
>>> +        d->webPageProxy->drawingArea()->layerTreeHostProxy()->setPaintScale(scale);
>> 
>> Maybe we should just expose setTransform instead of setPaintScale
> 
> Oh no!
> This will require mutexes which I'd like to avoid.
> I'd better revert the patch which makes manual scaling.

I don't see how having one mutex to protect the matrix when committing the contentScale is an issue.

>>>> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:95
>>>> +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomRight()), viewportRect));
>>> 
>>> This looks very much like a rect.unite... why not just do that?
>> 
>> After mapping of coordinates through matrix the clipRect could be rotated and unite will not work.
> 
> unite(m.mapRect(...))
> 
> I need to investigate this code; It seems too much like a black box.

Also, couldn't we use the existing scissor-box instead of regenerating it?
Comment 43 Viatcheslav Ostapenko 2012-03-09 09:04:35 PST
(In reply to comment #40)
> > Oh no!
> > This will require mutexes which I'd like to avoid.
> That's the kind of stuff you need to put in comments :)

Actually, there is comment about this near setScale in LayerTreePaintData.cpp .
Comment 44 Viatcheslav Ostapenko 2012-03-09 09:17:53 PST
(In reply to comment #42)
> (From update of attachment 130973 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130973&action=review
> 
> >>> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:115
> >>> +        d->webPageProxy->drawingArea()->layerTreeHostProxy()->setPaintScale(scale);
> >> 
> >> Maybe we should just expose setTransform instead of setPaintScale
> > 
> > Oh no!
> > This will require mutexes which I'd like to avoid.
> > I'd better revert the patch which makes manual scaling.
> 
> I don't see how having one mutex to protect the matrix when committing the contentScale is an issue.

From my experience it is better to avoid mutexes for high performance and mobile systems. In this case atomic operation works just fine.

> >>>> Source/WebKit2/UIProcess/qt/QtLayerTreeSGNode.cpp:95
> >>>> +            adjustBoundingRect(currentClip, toViewPortCoordinates(m.map(clipRect.bottomRight()), viewportRect));
> >>> 
> >>> This looks very much like a rect.unite... why not just do that?
> >> 
> >> After mapping of coordinates through matrix the clipRect could be rotated and unite will not work.
> > 
> > unite(m.mapRect(...))
> > 
> > I need to investigate this code; It seems too much like a black box.
> 
> Also, couldn't we use the existing scissor-box instead of regenerating it?

On internal branch I just use clipping already created by scene graph renderer and it works fine (except for some bugs in clipping update that are already fixed on Qt trunk), but in general case we do not know what renderer is used and cannot rely on renderer scissor/stencil, so we have to generate clipping ourselves until further clarification.
BTW, I cleaned up clipping rect calculation based on your comments.
Comment 45 Viatcheslav Ostapenko 2012-03-09 09:37:51 PST
Created attachment 131047 [details]
Updated patch
Comment 46 Viatcheslav Ostapenko 2012-03-10 08:35:19 PST
Created attachment 131174 [details]
Updated patch
Comment 47 Noam Rosenthal 2012-03-10 09:07:15 PST
Comment on attachment 131174 [details]
Updated patch

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

> Source/WebCore/ChangeLog:8
> +        Add conversions from/to Qt QMatrix4x4. Used by Qt WebKit2

Add TransformationMatrix conversions

> Source/WebKit2/ChangeLog:10
> +        painting objects are moved to separate class WebLayerTreeRenderer. 

moved to a separate class called WebLayerTreeRenderer

> Source/WebKit2/ChangeLog:14
> +        from paint thread only.

from QtScenegraph's paint thread only.

> Source/WebKit2/ChangeLog:18
> +        Layer tree updates from render queue are fetched in
> +        updatePaintNode call stack when main thread is locked.
> +        Messages from paint thread to web process are passed through
> +        MainThreadGuardedInvoker call gate (implemented by Noam Rosenthal).

Weird line endings...
(implemented by Noam Rosenthal and previously reviewed by Kenneth Rohde Christiansen)

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:96
> +        QMatrix4x4 renderMatirx = matrix() ? *matrix() : QMatrix4x4();

Matirx -> Matrix

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:171
> +        // This means that LayerTreeHostProxy was deleted and recreated
> +        // while old paint node survived. This could happen if web process
> +        // have crashed. In this case we have to recreate paint node.

Can be a few less lines... Try wrapping at 80 or 100

> Source/WebKit2/UIProcess/LayerTreeHostProxy.cpp:90
> +

Extra space

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:70
> +    WebLayerTreeRenderer* webLayerTreeRenderer() const { return m_webLayerTreeRenderer.get(); }

webLayerTreeRenderer() -> layerTreeRenderer()

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:76
> +    RefPtr<WebLayerTreeRenderer> m_webLayerTreeRenderer;

m_webLayerTreeRender -> m_renderer

> Source/WebKit2/UIProcess/WebLayerTreeRenderer.cpp:379
> +

extra line
Comment 48 WebKit Review Bot 2012-03-10 09:47:21 PST
Comment on attachment 131174 [details]
Updated patch

Attachment 131174 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11935140

New failing tests:
perf/array-reverse.html
Comment 49 Noam Rosenthal 2012-03-10 09:53:23 PST
(In reply to comment #48)
> (From update of attachment 131174 [details])
> Attachment 131174 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11935140
> 
> New failing tests:
> perf/array-reverse.html

Flaky, unrelated.
Comment 50 Viatcheslav Ostapenko 2012-03-10 11:59:17 PST
Created attachment 131181 [details]
Updated patch
Comment 51 Noam Rosenthal 2012-03-10 19:36:33 PST
Comment on attachment 131181 [details]
Updated patch

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

> Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:114
> +
> +    void setScale(float scale) { m_scale = scale; }

Empty line after the function instead of before

> Source/WebKit2/UIProcess/LayerTreeHostProxy.cpp:50
> +    m_renderer->syncRemoteContent();

I think this line needs to be removed; We should only call syncRemoteContent from updateNode, and not from render; otherwise we might have a thread issue with m_renderQueue.
Comment 52 Viatcheslav Ostapenko 2012-03-10 21:30:25 PST
(In reply to comment #51)
> (From update of attachment 131181 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131181&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebpage.cpp:114
> > +
> > +    void setScale(float scale) { m_scale = scale; }
> 
> Empty line after the function instead of before
> 
> > Source/WebKit2/UIProcess/LayerTreeHostProxy.cpp:50
> > +    m_renderer->syncRemoteContent();
> 
> I think this line needs to be removed; We should only call syncRemoteContent from updateNode, and not from render; otherwise we might have a thread issue with m_renderQueue.

We don't call LayerTreeHostProxy::paintToCurrentGLContext at all. Our painting calls rendering directly.
I just left it there if somebody else (EFL?) uses it. This way it works exactly as before.
Comment 53 Noam Rosenthal 2012-03-10 21:36:33 PST
> > I think this line needs to be removed; We should only call syncRemoteContent from updateNode, and not from render; otherwise we might have a thread issue with m_renderQueue.
> 
> We don't call LayerTreeHostProxy::paintToCurrentGLContext at all. Our painting calls rendering directly.
> I just left it there if somebody else (EFL?) uses it. This way it works exactly as before.

Ah, ok.
Comment 54 Viatcheslav Ostapenko 2012-03-10 22:00:52 PST
Created attachment 131204 [details]
Patch for commit.
Comment 55 WebKit Review Bot 2012-03-11 00:26:24 PST
Comment on attachment 131204 [details]
Patch for commit.

Clearing flags on attachment: 131204

Committed r110388: <http://trac.webkit.org/changeset/110388>
Comment 56 WebKit Review Bot 2012-03-11 00:26:32 PST
All reviewed patches have been landed.  Closing bug.