Bug 76661 – [Qt] [WK2] Support threaded renderer in WK2

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

Should remove "FIXME" in Tools/MiniBrowser/qt/main.cpp/main() .

[Viatcheslav Ostapenko]

Created attachment 123189 [details]
Patch

[Viatcheslav Ostapenko]

Comment on attachment 123189 [details]
Patch

Have to rebase after Jocelyns patch.

[Viatcheslav Ostapenko]

Created attachment 123197 [details]
Updated patch.

[Viatcheslav Ostapenko]

Created attachment 123206 [details]
Fix windows build.

[Viatcheslav Ostapenko]

Created attachment 123209 [details]
One more windows build fix.

[Noam Rosenthal]

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?

[Viatcheslav Ostapenko]

(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.

[Noam Rosenthal]

> 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.

[Viatcheslav Ostapenko]

(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.

[Viatcheslav Ostapenko]

(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.

[Viatcheslav Ostapenko]

Created attachment 123234 [details]
Reworked patch.

[Noam Rosenthal]

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.

[Viatcheslav Ostapenko]

(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.

[Jocelyn Turcotte]

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.

[Viatcheslav Ostapenko]

(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.

[Kenneth Rohde Christiansen]

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?

[Noam Rosenthal]

Created attachment 130040 [details]
Patch

[WebKit Review Bot]

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.

[Noam Rosenthal]

(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

[Kenneth Rohde Christiansen]

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

[Noam Rosenthal]

Created attachment 130115 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Noam Rosenthal]

(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 :)

[Kenneth Rohde Christiansen]

Comment on attachment 130115 [details]
Patch

r=me with comments given on irc

[Noam Rosenthal]

Created attachment 130126 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 130126 [details]
Patch for landing

Clearing flags on attachment: 130126

Committed r109748: <http://trac.webkit.org/changeset/109748>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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?

[Noam Rosenthal]

> 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".

[Noam Rosenthal]

(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.

[Noam Rosenthal]

Apparently this code is still very unstable. I'll research and post a new patch later on.

[Csaba Osztrogonác]

(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.

[Csaba Osztrogonác]

(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)

[Csaba Osztrogonác]

(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 .

[Viatcheslav Ostapenko]

Created attachment 130973 [details]
Updated patch

[Noam Rosenthal]

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?

[Noam Rosenthal]

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

[Viatcheslav Ostapenko]

(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.

[Viatcheslav Ostapenko]

(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.

[Noam Rosenthal]

> Oh no!
> This will require mutexes which I'd like to avoid.
That's the kind of stuff you need to put in comments :)

[Noam Rosenthal]

(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.

[Noam Rosenthal]

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?

[Viatcheslav Ostapenko]

(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 .

[Viatcheslav Ostapenko]

(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.

[Viatcheslav Ostapenko]

Created attachment 131047 [details]
Updated patch

[Viatcheslav Ostapenko]

Created attachment 131174 [details]
Updated patch

[Noam Rosenthal]

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

[WebKit Review Bot]

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

[Noam Rosenthal]

(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.

[Viatcheslav Ostapenko]

Created attachment 131181 [details]
Updated patch

[Noam Rosenthal]

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.

[Viatcheslav Ostapenko]

(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.

[Noam Rosenthal]

> > 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.

[Viatcheslav Ostapenko]

Created attachment 131204 [details]
Patch for commit.

[WebKit Review Bot]

Comment on attachment 131204 [details]
Patch for commit.

Clearing flags on attachment: 131204

Committed r110388: <http://trac.webkit.org/changeset/110388>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.