[Webkit-unassigned] [Bug 76661] [Qt] [WK2] Support threaded renderer in WK2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76661


Noam Rosenthal <noam.rosenthal at nokia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #123234|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #12 from Noam Rosenthal <noam.rosenthal at nokia.com>  2012-01-20 07:50:09 PST ---
(From update of attachment 123234)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.