[webkit-reviews] review denied: [Bug 56935] [Qt] Implement accelerated compositing on WK2 Qt port : [Attachment 97231] Patch 6: Glue LayerTreeHost with DrawingAreaProxy

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Noam Rosenthal
<noam.rosenthal at nokia.com>'s request for review:
Bug 56935: [Qt] Implement accelerated compositing on WK2 Qt port
https://bugs.webkit.org/show_bug.cgi?id=56935

Attachment 97231: Patch 6: Glue LayerTreeHost with DrawingAreaProxy
https://bugs.webkit.org/attachment.cgi?id=97231&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=97231&action=review

Quick review. This patch needs some cleaning.

> Source/WebKit2/UIProcess/DrawingAreaProxy.h:97
> +    virtual void updateWebView() { }
> +    virtual WebCore::IntRect viewportVisibleRect() const { return
WebCore::IntRect(); }
> +    virtual WebCore::IntRect contentsRect() const = 0;
> +    virtual float contentsScale() const = 0;
> +    virtual bool backingStoreReady() const { return false; }

Let's refactor that to share as much as possible between the two proxies.
What can be implemented in DrawingAreaProxy should be implemented here,
including the LayerTreeHostProxy. The methods that are specific to a view
should be pure virtual.


> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:83
> +    DrawingAreaProxy* drawingAreaProxy() { return this; }

What the fuck???

> Source/WebKit2/UIProcess/DrawingAreaProxyImpl.h:84
> +    WebPageProxy* webPageProxy() const { return m_webPageProxy; }

Wait, what?

> Source/WebKit2/UIProcess/LayerTreeHostProxy.h:62
> +    GraphicsLayer* layerByID(WebLayerID id) { return id ?
m_layers.get(id).layer : 0; }

It is a bit mysterious why the id 0 cannot be associated with a layer. Usually
the IDs of WK2 can wrap around when it overflows and the 0 is not a special
value.
Out of curiousity, where are those IDs defined?

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:41
> +GraphicsLayer* LayerTreeHostProxy::createLayer()
> +{
> +    return new GraphicsLayerTextureMapper(this);
> +}

This is an open invitation for leaks, the return type should be PassOwnPtr. :)

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:52
> +	   if (!m_layers.contains(id))
> +	       continue;
> +	   delete m_layers.get(id).layer;
> +	   m_layers.remove(id);

You are doing 3 lookup of the exact same instance. It is O(1) each time but
that is not free.
Use HashMap::find() to keep a reference on the value through the iterator.

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:58
> +    for (LayerMap::iterator it = m_layers.begin(); it != m_layers.end();
++it) {

We usually use a local variable for the end iterator since iterators are not
necessarily free to instanciate.

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:60
> +	   if (it->second.layer != layer)
> +	       continue;

This pattern is weird and will get very ineffective if there can be many layer.

What is the call site for this method? It seems it should take an ID as a
parameter instead of the layer. Alternatively, a second hashmap could be
maintained for the painting case.

> Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp:87
> +LayerTreeHostProxy::~LayerTreeHostProxy()
> +{
> +}

I think you can skip writing this :)