[webkit-reviews] review denied: [Bug 106763] [EFL][WK2][WebGL] Delay release of canvas shared objects until they are release on UI side. : [Attachment 183812] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam at webkit.org> has denied Viatcheslav Ostapenko
<ostap73 at gmail.com>'s request for review:
Bug 106763: [EFL][WK2][WebGL] Delay release of canvas shared objects until they
are release on UI side.
https://bugs.webkit.org/show_bug.cgi?id=106763

Attachment 183812: Patch
https://bugs.webkit.org/attachment.cgi?id=183812&action=review

------- Additional Comments from Noam Rosenthal <noam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183812&action=review


I'm sure we can find a more explicit solution;
Since a canvas can only be used in one layer, we should be able to free it
after we get the next RenderNextFrame, like we do with atlases.
In any case, this code is very hard to read and does things it's not supposed
to (see comments).

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:50
> +    if (m_window)
> +	   destroyWindow(m_window);
>      if (m_configVisualInfo) {
>	   XFree(m_configVisualInfo);
>	   m_configVisualInfo = 0;
>      }

Can you use the new OwnPtrX11 for this?

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.h:148
> +    void reSizeWindow(const IntRect&);

It should really be resize and not reSize

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:388
> +
> +    // If content layer was deleted it have to be reset immediately at the
> +    // texture mapper because webprocess shared window will be deleted.
> +    if (!m_contentsLayer)
> +	   m_layer->flushCompositingStateForThisLayerOnly(this);

No way.
m_contentsLayer is used for other things, not only for WebGL on X11.

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.
cpp:821
> +	   m_coordinator->destroyCanvas(id(), m_canvasSharedPlatformLayer,
false);

boolean trap

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.
cpp:825
> +	   m_coordinator->destroyCanvas(id(),
m_canvasSharedPlatformLayerToDestroy, false);

ditto