Bug 100745 – [EFL][WK2][AC] Use smart pointers for Evas_GL types

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

We recently started using smart pointers in WK2 EFL port. However, the accelerated compositing code is still using raw pointers for Evas_GL* objects.
We should fix this.

[Chris Dumez]

Created attachment 171433 [details]
Patch

[Chris Dumez]

Any feedback on this patch please?

[Kenneth Rohde Christiansen]

Comment on attachment 171433 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171433&action=review

> Source/WebKit2/UIProcess/API/efl/EvasGLSurface.cpp:33
> +EvasGLSurface::EvasGLSurface(Evas_GL* evasGl, Evas_GL_Surface* surface)

Maybe call this passSurface as you pass ownership?

> Source/WebKit2/UIProcess/API/efl/EvasGLSurface.h:46
> +        return adoptPtr(new EvasGLSurface(evasGl, surface));

// Sets m_evasGL.

I suppose we should use GL and not Gl in accordance with style guide

> Source/WebKit2/UIProcess/API/efl/EvasGLSurface.h:55
> +    Evas_GL* m_evasGl;

What are you using this for? Ah it is being set by the ctor.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:254
> +        evas_gl_make_current(m_evasGl.get(), m_evasGlSurface->surface(), m_evasGlContext->context());

wouldn't ->get() not be more consistent? hmm I see that won't work as it already exists.

[Chris Dumez]

Created attachment 171633 [details]
Patch

Take Kenneth's feedback into consideration.

[Kenneth Rohde Christiansen]

Comment on attachment 171633 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171633&action=review

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:471
> +    evas_gl_native_surface_get(m_evasGL.get(), m_evasGLSurface->surface(), &nativeSurface);
>      evas_object_image_native_surface_set(sd->image, &nativeSurface);
>  
> -    evas_gl_make_current(m_evasGl, m_evasGlSurface, m_evasGlContext);
> +    evas_gl_make_current(m_evasGL.get(), m_evasGLSurface->surface(), m_evasGLContext->context());

why not call evasGL() instead of m_evasGL.get().

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:473
> +    Evas_GL_API* gl = evas_gl_api_get(evasGL());

like here we  are doing so

[Chris Dumez]

Created attachment 171645 [details]
Patch

Take Kenneth's feedback into consideration.

Good point, it looks nicer and more consistent.

[WebKit Review Bot]

Comment on attachment 171645 [details]
Patch

Clearing flags on attachment: 171645

Committed r133041: <http://trac.webkit.org/changeset/133041>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gyuyoung Kim]

Comment on attachment 171645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171645&action=review

Sorry for late comments. But, it seems to me there are some nits.

> Source/WebKit2/PlatformEfl.cmake:45
> +    UIProcess/API/efl/EvasGLContext.cpp

Wrong alphabetical order.

> Source/WebKit2/PlatformEfl.cmake:46
> +    UIProcess/API/efl/EvasGLSurface.cpp

ditto.

> Source/WebKit2/UIProcess/API/efl/EvasGLSurface.cpp:35
> +    , m_surface(passSurface)

passSurface ? I think surface is just enough.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:493
> +        EINA_LOG_DOM_WARN(_ewk_log_dom, "Failed to create GLContext.");

We already define WARN macro for this.

> Source/WebKit2/UIProcess/API/efl/EwkViewImpl.cpp:499
> +        EINA_LOG_DOM_WARN(_ewk_log_dom, "Failed to create GLSurface.");

ditto.