[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





--- Comment #13 from Viatcheslav Ostapenko <ostapenko.viatcheslav at nokia.com>  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.

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