[Webkit-unassigned] [Bug 103704] [EFL][WK2] Implement Accelerated2DCanvas on WK2 Efl port

[bugzilla-daemon at webkit.org]

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


Noam Rosenthal <noam at webkit.org> changed:

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




--- Comment #7 from Noam Rosenthal <noam at webkit.org>  2012-11-29 22:52:15 PST ---
(From update of attachment 176902)
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 :)

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