[webkit-reviews] review denied: [Bug 76661] [Qt] [WK2] Support threaded renderer in WK2 : [Attachment 123234] Reworked patch.

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Viatcheslav Ostapenko
<ostapenko.viatcheslav at nokia.com>'s request for review:
Bug 76661: [Qt] [WK2] Support threaded renderer in WK2
https://bugs.webkit.org/show_bug.cgi?id=76661

Attachment 123234: Reworked patch.
https://bugs.webkit.org/attachment.cgi?id=123234&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
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.