[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





--- Comment #17 from Kyungjin Kim <gen.kim at samsung.com>  2012-12-02 21:34:53 PST ---
(In reply to comment #7)
> (From update of attachment 176902 [details])
> 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.

I updated ChangeLog for 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.

done.
> 
> > 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() ...

Thanks for your suggestion. I made hasGraphicsSurface() as a virtual to be handled in Canvas2DLayerEfl because I'm worried if it might be able to be executed for unwanted layers, feed me back please.
> 
> > 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 :)

We are planning to implement Accelerated2DCanvas on WK2 Efl port based on COORDINATED_GRAPHICS for both cases using GRAPHICS_SURFACE and no GRAPHICS_SURFACE because it is not enabled by default in EFL currently. 
Graphics surface version of Accelerated2DCanvas will be implemented soon.
I updated ChangeLog.

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