[webkit-reviews] review denied: [Bug 103704] [EFL][WK2] Implement Accelerated2DCanvas on WK2 Efl port : [Attachment 176902] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam at webkit.org> has denied Kyungjin Kim
<gen.kim at samsung.com>'s request for review:
Bug 103704: [EFL][WK2] Implement Accelerated2DCanvas on WK2 Efl port
https://bugs.webkit.org/show_bug.cgi?id=103704

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

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


This is a good first attempt :)
However a lot of the choices in this patch don't explain themselves, and there
are no comments in the changelog or in the code to explain them...

> Source/WebCore/ChangeLog:10
> +	   Implement accelerated 2D canvas using Coordinated Graphics on WK2
Efl port.
> +	   This implementation is based on COORDINATED_GRAPHICS.
> +

Needs more information.

> Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:51
> +#else
> +    void paintContents(GraphicsContext&, const IntRect&);
> +#endif

This shouldn't really be inside the #else clause.
There could be a situation where graphics surfaces are compiled in, but
allocating a graphisc surface didn't succeed, in which case this layer should
still paint with software.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:50
> +#if ENABLE(ACCELERATED_2D_CANVAS)
> +    virtual void paintContents(GraphicsContext&, const IntRect&) { }
> +    virtual bool is2D() { return false; }
> +#endif

This is a bit confusing.
Perhaps hasGraphicsSurface() would be a better choice, that always returns
false if ENABLE(GRAPHICS_SURFACE) is false.

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.
cpp:682
> +#if ENABLE(ACCELERATED_2D_CANVAS) && !USE(GRAPHICS_SURFACE)
> +    if (m_canvasPlatformLayer && m_canvasPlatformLayer->is2D()) {
> +	   m_canvasPlatformLayer->paintContents(*context, rect);
> +	   return;
> +    }
> +#endif

How about
if (m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface())
   ... paintContents() ...

>
Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.
cpp:766
> +#if ENABLE(ACCELERATED_2D_CANVAS) && !USE(GRAPHICS_SURFACE)
> +    if (m_canvasPlatformLayer && m_canvasPlatformLayer->is2D())
> +	   setDrawsContent(true);
> +#endif

This is quite strange.
Hard for me to understand why you would want an accelerated 2d canvas layer
with no graphics surface; which would essentially mean it's not accelerated.
I'm willing to listen to reason but there is no explanation in the Changelog :)