[EFL] Refactor GraphicsContext3DEFL

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kalyan]

Refactor GraphicsContext3DEFL class

To help with the following things:
1) Easy Context Management for different render types.
2) To be able to easily add support for different gl backends.

[Gyuyoung Kim]

Please add [EFL] prefix for EFL bug.

[Kalyan]

Created attachment 172498 [details]
initial patch

First draft.

Support for Evas is not yet there. Idea is to get some initial feedback. This patch includes the interface(GLNativeContext) and an example implementation(GLXContextWrapper).

[WebKit Review Bot]

Attachment 172498 [details] did not pass style-queue:

Source/WebCore/platform/graphics/efl/GLNativeContext.h:21:  #ifndef header guard has wrong style, please use: GLNativeContext_h  [build/header_guard] [5]
Source/WebCore/platform/graphics/efl/GLNativeContext.h:24:  Header file should not contain WebCore config.h. Should be: alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/efl/GLNativeContext.h:26:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/efl/GLNativeContext.h:28:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/efl/GLNativeContext.h:36:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/efl/GLNativeContext.h:40:  The parameter name "renderStyle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/efl/GLNativeContext.h:40:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/graphics/efl/GLNativeContext.h:40:  Extra space between static and PassOwnPtr  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/efl/GLNativeContext.h:62:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/efl/GLNativeContext.h:69:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:21:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:22:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:33:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:42:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:43:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:47:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:120:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:122:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:181:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:81:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:83:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:84:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:86:  Declaration has space between type name and * in GLContextGLX *sharedContext  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:89:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:139:  Extra space between return and m_platformContext  [whitespace/declaration] [3]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:145:  Extra space before ) in if  [whitespace/parens] [5]
Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.h:49:  The parameter name "pageClient" adds no information,Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/PlatformEfl.cmake', u'Sourc..." exit_code: 1
 so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:55:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:42:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:57:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:60:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:218:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:26:  #ifndef header guard has wrong style, please use: GLXContextWrapper_h  [build/header_guard] [5]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:42:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:44:  "GL/glx.h" already included at Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:31  [build/include] [4]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:52:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:58:  Extra space before last semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:76:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:79:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 44 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 172498 [details]
initial patch

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

> Source/WebCore/PlatformEfl.cmake:301
> +    platform/graphics/efl/GLXContextWrapper.cpp

What for platform that don't have X at all? Say wayland?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:23
> +#include "config.h"
> +#include <wtf/MainThread.h>
> +#include "GLNativeContext.h"
> +#include "GLXContextWrapper.h"

Thsi is not correct. Check style guide

#include "config.h"
#include "GLNativeContext.h"

#include the rest

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:33
> +PassOwnPtr<GLNativeContext> GLNativeContext::createContext(GraphicsContext3D::RenderStyle renderStyle,uint64_t windowHandle,bool useSharedContext)

space before bool, uint64_t etc. Someone should fix the style checker if it is not catching this

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:43
> +    if (!success) {
> +         return nullptr;
> +    }

no need for braces

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:47
> +        if (OwnPtr<GLNativeContext> glxContext  = GLNativeContext::createOffScreenContext(useSharedContext ? GLNativeContext::sharedOffScreenContext():0,windowHandle))

no double space before ==, space before windowHandle

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:51
> +        if (OwnPtr<GLNativeContext> glxContext  = GLNativeContext::createCurrentContextWrapper())

same

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:66
> +    return sharing.get();

sharing? m_sharedInstance ?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:120
> +PassOwnPtr<GLNativeContext> GLNativeContext::createOffScreenContext(GLNativeContext* sharingContext,uint64_t windowHandle)

space issue

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:122
> +    OwnPtr<GLNativeContext> glxContext = adoptPtr(new GLXOffScreenContextWrapper(sharingContext,windowHandle));

same

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:137
> +// FIXME: This should be a thread local eventually if we
> +// want to support using GLNativeContexts from multiple threads.

a local thread? what do you mean with that?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:159
> +bool GLNativeContext::pushContext()
> +{
> +    ASSERT(isMainThread());
> +    gCurrentContext = this;
> +    return true;
> +}

That doesnt look like a stack, so why it is called push? interface? comment?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:177
> +void GLNativeContext::swapBuffers()
> +{
> +    ASSERT(isMainThread());
> +}

comment needed

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:184
> +
> +
> +

too many newlines

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:40
> +    static  PassOwnPtr<GLNativeContext> createContext(GraphicsContext3D::RenderStyle renderStyle,uint64_t windowHandle = 0,bool useSharedContext = false);

You should really fix all this style :)

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:58
> +    // Make Context as current. It does nothing if it is already current.

These comments add no value, the name is already descriptive

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:62
> +    //Taken from cairo port.

What does that mean? Don't we need them? :)

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:32
> +#include "GLXContextWrapper.h"
> +
> +
> +namespace WebCore {
> +
> +
> +GLXContextWrapper::GLXContextWrapper()

excessive newlines

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:33
> +    :GLNativeContext()

space after :

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:35
> +

remove newline

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:40
> +#if USE(EGL)

isn't this GLX? Why that name of the class then

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:41
> +    return eglGetCurrentSurface (EGL_DRAW);

no space before (

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:107
> +    doPopContext();

not webkit style naming

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:-139
> -    Evas_GL_Config config = {
> -        EVAS_GL_RGBA_8888,
> -        EVAS_GL_DEPTH_BIT_8,
> -        EVAS_GL_STENCIL_NONE, // FIXME: set EVAS_GL_STENCIL_BIT_8 after fixing Evas_GL bug.
> -        EVAS_GL_OPTIONS_NONE,
> -        EVAS_GL_MULTISAMPLE_NONE
> -    };
> -

All this code is not needed anymore?

[Simon Hausmann]

Comment on attachment 172498 [details]
initial patch

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

This patch is also missing a ChangeLog.

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:73
> +// Because of driver bugs, exiting the program when there are active pbuffers
> +// can crash the X server (this has been observed with the official Nvidia drivers).
> +// We need to ensure that we clean everything up on exit. There are several reasons
> +// that GraphicsContext3Ds will still be alive at exit, including user error (memory
> +// leaks) and the page cache. In any case, we don't want the X server to crash.

This part doesn't make much sense to me. atexit() in libraries is a very dangerous thing to use, especially given that you don't know in which order your handlers are called. (what if the nvidia driver also installs atexit() handlers?)

Besides, the process can crash at any time and then it would bring down the x server, too -> I don't think we should try to work around bugs in the X server.

(JMHO, feel free to ignore)

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:54
> +PlatformNativeContext GLXContextWrapper::currentNativeContext()
> +{
> +#if USE(EGL)
> +    return eglGetCurrentContext();
> +#else
> +    return glXGetCurrentContext();
> +#endif
> +}

It seems wrong to me to mix EGL and GLX code in a file/class that has GLX in its name. They are different APIs (serving similar purposes, but they are not the same)

[Mikhail Pozdnyakov]

Comment on attachment 172498 [details]
initial patch

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

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:65
> +    DEFINE_STATIC_LOCAL(OwnPtr<GLNativeContext>, sharing, (createOffScreenContext()));

why OwnPtr here?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:111
> +    for (size_t i = 0; i < contextList.size(); ++i)

re-calculating the size each time?

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:46
> +    virtual PlatformGraphicsContext3D handle() = 0;

const method?

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:49
> +    virtual bool isCurrentContext() = 0;

const method?

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:87
> +    void doPopContext();

do we really need this method? isn't popContex enough?

[Kalyan]

(In reply to comment #4)
> (From update of attachment 172498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172498&action=review
> 
> > Source/WebCore/PlatformEfl.cmake:301
> > +    platform/graphics/efl/GLXContextWrapper.cpp
> 
> What for platform that don't have X at all? Say wayland?
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:23
> > +#include "config.h"
> > +#include <wtf/MainThread.h>
> > +#include "GLNativeContext.h"
> > +#include "GLXContextWrapper.h"
> 
> Thsi is not correct. Check style guide
> 
> #include "config.h"
> #include "GLNativeContext.h"
> 
> #include the rest
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:33
> > +PassOwnPtr<GLNativeContext> GLNativeContext::createContext(GraphicsContext3D::RenderStyle renderStyle,uint64_t windowHandle,bool useSharedContext)
> 
> space before bool, uint64_t etc. Someone should fix the style checker if it is not catching this
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:43
> > +    if (!success) {
> > +         return nullptr;
> > +    }
> 
> no need for braces
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:47
> > +        if (OwnPtr<GLNativeContext> glxContext  = GLNativeContext::createOffScreenContext(useSharedContext ? GLNativeContext::sharedOffScreenContext():0,windowHandle))
> 
> no double space before ==, space before windowHandle
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:51
> > +        if (OwnPtr<GLNativeContext> glxContext  = GLNativeContext::createCurrentContextWrapper())
> 
> same
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:66
> > +    return sharing.get();
> 
> sharing? m_sharedInstance ?
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:120
> > +PassOwnPtr<GLNativeContext> GLNativeContext::createOffScreenContext(GLNativeContext* sharingContext,uint64_t windowHandle)
> 
> space issue
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:122
> > +    OwnPtr<GLNativeContext> glxContext = adoptPtr(new GLXOffScreenContextWrapper(sharingContext,windowHandle));
> 
> same
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:137
> > +// FIXME: This should be a thread local eventually if we
> > +// want to support using GLNativeContexts from multiple threads.
> 
> a local thread? what do you mean with that?
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:159
> > +bool GLNativeContext::pushContext()
> > +{
> > +    ASSERT(isMainThread());
> > +    gCurrentContext = this;
> > +    return true;
> > +}
> 
> That doesnt look like a stack, so why it is called push? interface? comment?
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:177
> > +void GLNativeContext::swapBuffers()
> > +{
> > +    ASSERT(isMainThread());
> > +}
> 
> comment needed
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:184
> > +
> > +
> > +
> 
> too many newlines
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.h:40
> > +    static  PassOwnPtr<GLNativeContext> createContext(GraphicsContext3D::RenderStyle renderStyle,uint64_t windowHandle = 0,bool useSharedContext = false);
> 
> You should really fix all this style :)
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.h:58
> > +    // Make Context as current. It does nothing if it is already current.
> 
> These comments add no value, the name is already descriptive
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.h:62
> > +    //Taken from cairo port.
> 
> What does that mean? Don't we need them? :)
> 
> > Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:32
> > +#include "GLXContextWrapper.h"
> > +
> > +
> > +namespace WebCore {
> > +
> > +
> > +GLXContextWrapper::GLXContextWrapper()
> 
> excessive newlines
> 
> > Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:33
> > +    :GLNativeContext()
> 
> space after :
> 
> > Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:35
> > +
> 
> remove newline
> 
> > Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:40
> > +#if USE(EGL)
> 
> isn't this GLX? Why that name of the class then
> 
> > Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:41
> > +    return eglGetCurrentSurface (EGL_DRAW);
> 
> no space before (
> 
> > Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:107
> > +    doPopContext();
> 
> not webkit style naming
> 

Please ignore the style checks for now. This was more to see if the approach would be good enough for us. All the things will be fixed in the next patch. 

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:-139
> > -    Evas_GL_Config config = {
> > -        EVAS_GL_RGBA_8888,
> > -        EVAS_GL_DEPTH_BIT_8,
> > -        EVAS_GL_STENCIL_NONE, // FIXME: set EVAS_GL_STENCIL_BIT_8 after fixing Evas_GL bug.
> > -        EVAS_GL_OPTIONS_NONE,
> > -        EVAS_GL_MULTISAMPLE_NONE
> > -    };
> > -
> 
> All this code is not needed anymore?

This would come back with evas implementation, otherwise no it is not needed

[Kalyan]

(In reply to comment #5)
> (From update of attachment 172498 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172498&action=review
> 
> This patch is also missing a ChangeLog.
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:73
> > +// Because of driver bugs, exiting the program when there are active pbuffers
> > +// can crash the X server (this has been observed with the official Nvidia drivers).
> > +// We need to ensure that we clean everything up on exit. There are several reasons
> > +// that GraphicsContext3Ds will still be alive at exit, including user error (memory
> > +// leaks) and the page cache. In any case, we don't want the X server to crash.
> 
> This part doesn't make much sense to me. atexit() in libraries is a very dangerous thing to use, especially given that you don't know in which order your handlers are called. (what if the nvidia driver also installs atexit() handlers?)
> 
> Besides, the process can crash at any time and then it would bring down the x server, too -> I don't think we should try to work around bugs in the X server.
> 
> (JMHO, feel free to ignore)
> 
> > Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:54
> > +PlatformNativeContext GLXContextWrapper::currentNativeContext()
> > +{
> > +#if USE(EGL)
> > +    return eglGetCurrentContext();
> > +#else
> > +    return glXGetCurrentContext();
> > +#endif
> > +}
> 
> It seems wrong to me to mix EGL and GLX code in a file/class that has GLX in its name. They are different APIs (serving similar purposes, but they are not the same)

k, will seperate them to different classes

[Kalyan]

Created attachment 173639 [details]
patch

GraphicsContext3DEFL would create the surface and context as needed. We don't recreate the surface when resized. Contexts are created with  glXCreateContextAttribsARB(if supported) else fallback to glxCreateNewContext. Implemented partial support for EXT_robustness.

[Kenneth Rohde Christiansen]

Comment on attachment 173639 [details]
patch

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

Jsut a quick look though, many style issues, should be easy to fix. The changelog needs so info on what you changed, what the different classes represents etc

> Source/WebCore/ChangeLog:37
> +        (WebCore::GLNativeContext::destroy):
> +        * platform/graphics/efl/GLNativeContext.h: Added.
> +        (WebCore):
> +        (GLNativeContext):
> +        * platform/graphics/efl/GLNativeSurface.cpp: Added.
> +        (WebCore):
> +        (WebCore::GLNativeSurface::createOffscreenSurface):
> +        (WebCore::GLNativeSurface::createTransportSurface):
> +        (WebCore:::m_sharedDisplay):

For such big changes we normally add comments here to say what changed and why

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:26
> +#include "config.h"

After the config include the next include must be the header file for the cpp

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:30
> +#include "GLNativeContext.h"

ie this one

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:38
> +PassOwnPtr<GLNativeContext> GLNativeContext::createContext(GraphicsContext3D::RenderStyle renderStyle)

I don't like Style so much, rather Mode. Style means CSS normally

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:41
> +{
> +
> +    static bool initializedShims = false;

unneeded newline

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:44
> +    if (!initializedShims) {

didInitializeShims

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:47
> +        glGetGraphicsResetStatusARB = (PFNGLGETGRAPHICSRESETSTATUSARBPROC)glXGetProcAddressARB((const GLubyte*)"glGetGraphicsResetStatusARB");

We should probably use C++ casts here

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:68
> +    }
> +    return nullptr;
> +}

i would add newline before retunr

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:106
> +    if (surface && !surface->handle().isValid())
> +        return false;

In this case you dont need to relase anything? or set m_currentContext to 0?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:117
> +    } else  if (platformMakeCurrent(surface)) {

no double space before if

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:202
> +#endif
> +
> +
> +
> +
> +

wow unneeded newlines

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:41
> +class GLNativeContext {
> +
> +    WTF_MAKE_NONCOPYABLE(GLNativeContext);
> +public:

I would add WTF... to the top

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:48
> +    enum NativeContextReset {
> +        NativeCONTEXT_NO_ERROR = 0,
> +        NativeCONTEXT_GUILTY_CONTEXT_RESET = 0x8253,
> +        NativeCONTEXT_INNOCENT_CONTEXT_RESET = 0x8254,
> +        NativeCONTEXT_UNKNOWN_CONTEXT_RESET = 0x8255,
> +    };

any comment to where these numbers come from?

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:60
> +    // Makes this and surface as current context and drawable.
> +    // Calling this function with no surface is same as calling releaseCurrent.
> +    // Does nothing if this is already current Context.
> +    bool makeCurrent(GLNativeSurface* = 0);

Why isn't an ASSERT better? ie. forcing people to actually use releaseCurrent

> Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:38
> +namespace WebCore {
> +
> +
> +PassRefPtr<GLNativeSurface> GLNativeSurface::createOffscreenSurface()

only one newline

> Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:43
> +        return surface;

.release() no?

> Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:98
> +void GLNativeSurface::copyTexture(uint32_t texture, const IntRect& sourceRect)
> +{
> +
> +    if (!m_fboId)

no newline here!

> Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:128
> +void GLNativeSurface::setGeometry(const IntRect&  newRect)

no double newline before newRect

> Source/WebCore/platform/graphics/efl/GLNativeSurface.h:71
> +    // Returns Geometry of surface.
> +    const IntRect& geometry() const;
> +
> +    // Get the underlying platform specific surface handle.
> +    GraphicsSurfaceToken handle() const;
> +
> +    platformDisplay sharedDisplay() const;
> +
> +    // Swaps front and back buffers. This has no effect for single buffered surfaces.
> +    virtual void swapBuffers();
> +
> +    // If Supported, Copies contents of texture to back buffer.
> +    virtual void copyTexture(uint32_t texture, const IntRect& sourceRect);
> +
> +    // Resizes the viewPort on supported implementations.
> +    virtual void setGeometry(const IntRect& newRect);

These comments are mostly useless or self explanatory

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:102
> +    if (config) {
> +
> +        initializeARBExtensions();

No if/for/while etc content can start with newline

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:109
> +            m_supportsRobustContextCreation = true;

I wonder if we can find a better name

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:163
> +#endif
> +
> +
> +

newlines...

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:33
> +class GLXCurrentContextWrapper :public GLNativeContext {

space after :

> Source/WebCore/platform/graphics/efl/GLXSurface.cpp:48
> +    :GLNativeSurface()

space after :

> Source/WebCore/platform/graphics/efl/GLXSurface.h:39
> +class SharedResources {

why not share from RefCounted?

> Source/WebCore/platform/graphics/efl/GLXSurface.h:45
> +    SharedResources()
> +    {
> +        ++m_refCount;
> +    }
> +

instead of this!

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:105
> +        m_contextLostCallback->onContextLost();

we dont use on, what about contextDidReset() or so

[Noam Rosenthal]

Comment on attachment 173639 [details]
patch

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

> Source/WebCore/PlatformEfl.cmake:288
> +    platform/graphics/efl/GLXSurface.cpp
> +    platform/graphics/efl/GLXContextWrapper.cpp

Why not put these in platform/graphics/glx?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:28
> +#if USE(3D_GRAPHICS) || USE(ACCELERATED_COMPOSITING)

Are both really needed?

[Kalyan]

(In reply to comment #11)
> (From update of attachment 173639 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173639&action=review
> 
> > Source/WebCore/PlatformEfl.cmake:288
> > +    platform/graphics/efl/GLXSurface.cpp
> > +    platform/graphics/efl/GLXContextWrapper.cpp
> Why not put these in platform/graphics/glx?

will move it under glx

> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:28
> > +#if USE(3D_GRAPHICS) || USE(ACCELERATED_COMPOSITING)
> 
> Are both really needed?

USE(ACCELERATED_COMPOSITING) should be enough I guess.

[Kalyan]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 173639 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=173639&action=review
> > 
> > > Source/WebCore/PlatformEfl.cmake:288
> > > +    platform/graphics/efl/GLXSurface.cpp
> > > +    platform/graphics/efl/GLXContextWrapper.cpp
> > Why not put these in platform/graphics/glx?
> 
> will move it under glx

I started to think if platform/graphics/surfaces/glx is a better place??

[Kalyan]

Created attachment 174209 [details]
patch

Review changes

[Kalyan]

Review flags not added yet, there seems to be issue of applying the patch(from cwt analysis). Working on it.

[Kenneth Rohde Christiansen]

Comment on attachment 174209 [details]
patch

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

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:38
> +static PFNGLGETGRAPHICSRESETSTATUSARBPROC  glGetGraphicsResetStatusARB = 0;

remove double space before gl

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:44
> +    static bool initializedShims = false;
> +    static bool didInitializeShims = true;

why both?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:155
> +bool GLNativeContext::isContextLost() const
> +{
> +    return m_contextLost;
> +}

isValid ?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:166
> +PlatformNativeContext GLNativeContext::handle() const

It is confusing that the class is called GL*NativeContext* but this handle() returns the PlatformNativeContext.

Should the class just be called GLContext? Better name?

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:72
> +    // Destroys any gl resources associated with this context.

GL

> Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:47
> +#if HAVE(GLX)
> +    OwnPtr<GLNativeSurface> surface =  adoptPtr(new GLXPBuffer());
> +
> +    if (surface->handle().isValid())
> +        return surface.release();
> +#endif

Later this will handle EGL?

> Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:92
> +PlatformSurfaceConfig GLNativeSurface::surfaceConfig()

::configuration()

> Source/WebCore/platform/graphics/efl/GLNativeSurface.h:43
> +public:
> +

I would move public: down one line and remove the next newline

> Source/WebCore/platform/graphics/efl/GLNativeSurface.h:47
> +    // Create a GL surface used for offsreen rendering. The results can be transported

*screen* (spelling)

> Source/WebCore/platform/graphics/efl/GLNativeSurface.h:79
> +    bool m_RestoreNeeded;

non capital r!

> Source/WebCore/platform/graphics/efl/GLNativeSurface.h:85
> +    PlatformNativeSurface m_drawable;
> +
> +};

remove newline

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:170
> +    m_private->freeResources();

releaseResources() ?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:46
> +    , m_PendingSurfaceResize(false)

non capital P!

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:71
> +            freeResources();

releaseResources()

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:119
> +        m_contextLostCallback->onContextLost();

We dont use on_ something... didLooseContext

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:39
> +static long X11Redirect = 1L << 9;

Where does this come from? comment?

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:41
> +class SharedResources : public WTF::RefCountedBase {

SharedX11Resources?

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:48
> +            m_staticSharedResource =  new SharedResources();

no double space before new

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:125
> +        // Didnt find any visual supporting alpha, select the first available config.

Did not *

[Kalyan]

> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:119
> > +        m_contextLostCallback->onContextLost();
> 
> We dont use on_ something... didLooseContext
> 

This is a call back interface provided by graphicscontext3d.

[Kalyan]

(In reply to comment #16)
> (From update of attachment 174209 [details])



> > Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:47
> > +#if HAVE(GLX)
> > +    OwnPtr<GLNativeSurface> surface =  adoptPtr(new GLXPBuffer());
> > +
> > +    if (surface->handle().isValid())
> > +        return surface.release();
> > +#endif
> 
> Later this will handle EGL?
> 

Yes

[Igor Trindade Oliveira]

Comment on attachment 174209 [details]
patch

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

> Source/WebCore/ChangeLog:12
> +

Please, explain in the changelog why you are removing the Evas code and adding a bunch of code to handle the context.

[Kalyan]

Created attachment 174596 [details]
rebasedpatch

[Kalyan]

(In reply to comment #16)
> (From update of attachment 174209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174209&action=review
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:38
> > +static PFNGLGETGRAPHICSRESETSTATUSARBPROC  glGetGraphicsResetStatusARB = 0;
> 
> remove double space before gl
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:44
> > +    static bool initializedShims = false;
> > +    static bool didInitializeShims = true;
> 
> why both?
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:155
> > +bool GLNativeContext::isContextLost() const
> > +{
> > +    return m_contextLost;
> > +}
> 
> isValid ?
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:166
> > +PlatformNativeContext GLNativeContext::handle() const
> 
> It is confusing that the class is called GL*NativeContext* but this handle() returns the PlatformNativeContext.
> 
> Should the class just be called GLContext? Better name?
> 
> > Source/WebCore/platform/graphics/efl/GLNativeContext.h:72
> > +    // Destroys any gl resources associated with this context.
> 
> GL
> 
> > Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:47
> > +#if HAVE(GLX)
> > +    OwnPtr<GLNativeSurface> surface =  adoptPtr(new GLXPBuffer());
> > +
> > +    if (surface->handle().isValid())
> > +        return surface.release();
> > +#endif
> 
> Later this will handle EGL?
> 
> > Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:92
> > +PlatformSurfaceConfig GLNativeSurface::surfaceConfig()
> 
> ::configuration()
> 
> > Source/WebCore/platform/graphics/efl/GLNativeSurface.h:43
> > +public:
> > +
> 
> I would move public: down one line and remove the next newline
> 
> > Source/WebCore/platform/graphics/efl/GLNativeSurface.h:47
> > +    // Create a GL surface used for offsreen rendering. The results can be transported
> 
> *screen* (spelling)
> 
> > Source/WebCore/platform/graphics/efl/GLNativeSurface.h:79
> > +    bool m_RestoreNeeded;
> 
> non capital r!
> 
> > Source/WebCore/platform/graphics/efl/GLNativeSurface.h:85
> > +    PlatformNativeSurface m_drawable;
> > +
> > +};
> 
> remove newline
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:170
> > +    m_private->freeResources();
> 
> releaseResources() ?
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:46
> > +    , m_PendingSurfaceResize(false)
> 
> non capital P!
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:71
> > +            freeResources();
> 
> releaseResources()

changed 
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:119

> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:41
> > +class SharedResources : public WTF::RefCountedBase {
> 
> SharedX11Resources?

changed 
 
 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:125
> > +        // Didnt find any visual supporting alpha, select the first available config.
> 
> Did not *
fixed

[Kalyan]

(In reply to comment #19)
> (From update of attachment 174209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174209&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +
> 
> Please, explain in the changelog why you are removing the Evas code and adding a bunch of code to handle the context.

Updated the ChangeLog as to why this change would help us.

[EFL EWS Bot]

Comment on attachment 174596 [details]
rebasedpatch

Attachment 174596 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14857477

[Gyuyoung Kim]

Comment on attachment 174596 [details]
rebasedpatch

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

It seems to me there are still nits.

> Source/WebCore/platform/graphics/efl/GLSurface.cpp:27
> +

Don't need to add a new line.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:118
> +        // FIXME: restore context

restore -> Restore

> Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:27
> +

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:46
> +    if (initialized)

I don't understand this condition well. Could you explain why you need to add this check ?

> Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:53
> +// GLXOffScreenContext

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:59
> +

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:74
> +    if (config) {

I think early return is better for this case.

e.g)

if (!config)
    return false;

> Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:94
> +

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:27
> +

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:75
> +// GLXTransportSurface

I don't know why we need to add this comment. There is no information.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:85
> +

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:124
> +

Generally, AFAIK, WebKit doesn't like to add a new line in front of checking for null.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:129
> +    XSetWindowAttributes a;

Can't use more meaningful name ?

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:158
> +

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:166
> +// GLX PBUFFER SURFACE

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:176
> +

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:183
> +

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:207
> +

ditto.

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:104
> +        for (int i = 0; i < numReturned; ++i) {

Is unsigned better ?

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:123
> +        // Didnot find any visual supporting alpha, select the first available config.

Didnot -> Did not ?

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:151
> +                m_pbufferfbConfig =  temp[0];

Two spaces between '=' and 'temp[0]'

[Gyuyoung Kim]

CC'ing Hyowon,

[Kalyan]

Created attachment 174605 [details]
Added missing file

[Igor Trindade Oliveira]

The idea about optimization is really interesting, however my concern here is about maintainability of the code. Image if each vendor implemented different extensions inside WebKit, it would be a big mess.
However If the reviewers decided that this patch is a good idea  I believe this code should live inside graphics/opengl. It is also interesting for GTK guys.

[EFL EWS Bot]

Comment on attachment 174605 [details]
Added missing file

Attachment 174605 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14859220

[Noam Rosenthal]

Comment on attachment 174605 [details]
Added missing file

Seems to me like this patch contains a lot of valuable things, but that it should be broken up to a few smaller patch. e.g. the use of glGetGraphicsResetStatusARB can be added in a different patch.

[Kenneth Rohde Christiansen]

(In reply to comment #27)
> The idea about optimization is really interesting, however my concern here is about maintainability of the code. Image if each vendor implemented different extensions inside WebKit, it would be a big mess.
> However If the reviewers decided that this patch is a good idea  I believe this code should live inside graphics/opengl. It is also interesting for GTK guys.

I believe it is a good idea. I don't believe the extension use will run out of hand, they will be used where it makes a performance difference and the best place to make use of them is in WebKit.

I agree that graphics/opengl is a better place for this.

[Kalyan]

Created attachment 174621 [details]
review fixes

[Kenneth Rohde Christiansen]

Comment on attachment 174621 [details]
review fixes

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

> Source/WebCore/platform/graphics/efl/GLContext.h:1
> +/*

Will you move these to say Source/WebCore/platform/graphics/opengl/GLContext.h ?

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:100
> +{
> +    if (m_drawable) {
> +        if (m_restoreNeeded) {
> +            GLint oldFBO;
> +            glGetIntegerv(GL_FRAMEBUFFER_BINDING, &oldFBO);
> +            glBindFramebuffer(GL_FRAMEBUFFER, 0);
> +            glXSwapBuffers(sharedDisplay(), m_drawable);
> +            glBindFramebuffer(GL_FRAMEBUFFER, oldFBO);
> +        } else
> +            glXSwapBuffers(sharedDisplay(), m_drawable);
> +    }
> +}

This we write as 

if (!m_drawable)
    return;

...

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:179
> +    return pBufferConfig();

pBufferConfiguration then, or just rename this to config() then. We need to be consistent

[Kalyan]

(In reply to comment #24)
> (From update of attachment 174596 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174596&action=review
> 
> It seems to me there are still nits.

Fixed all the headers

> > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:46
> > +    if (initialized)
> 
> I don't understand this condition well. Could you explain why you need to add this check ?
It acts as a guard check to prevent us from querying the function pointer more than once. 


> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:53
> > +// GLXOffScreenContext
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:59
> > +
> 
> ditto.

These where meant to indicate start of a new class implementation. Removed them anyway.

> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:74
> > +    if (config) {
> 
> I think early return is better for this case.
> 
> e.g)
> 
> if (!config)
>     return false;

I would prefer as it is now, since we need to also handle the case when we have a valid configuration but context creation fails. 

> > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:94
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:27
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:75
> > +// GLXTransportSurface
> 
> I don't know why we need to add this comment. There is no information.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:85
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:124
> > +
> 
> Generally, AFAIK, WebKit doesn't like to add a new line in front of checking for null.
k Fixed.

> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:129
> > +    XSetWindowAttributes a;
> 
> Can't use more meaningful name ?

fixed

> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:158
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:166
> > +// GLX PBUFFER SURFACE
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:176
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:183
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:207
> > +
> 
> ditto.

fixed

> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:104
> > +        for (int i = 0; i < numReturned; ++i) {
> 
> Is unsigned better ?

No, as numReturned is of type int. glXChooseFBConfig accepts only int type.

> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:123
> > +        // Didnot find any visual supporting alpha, select the first available config.
> 
> Didnot -> Did not ?
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:151
> > +                m_pbufferfbConfig =  temp[0];
> 
> Two spaces between '=' and 'temp[0]'

fixed

[Kalyan]

(In reply to comment #30)
> (In reply to comment #27)
> > The idea about optimization is really interesting, however my concern here is about maintainability of the code. Image if each vendor implemented different extensions inside WebKit, it would be a big mess.
> > However If the reviewers decided that this patch is a good idea  I believe this code should live inside graphics/opengl. It is also interesting for GTK guys.
> 
> I believe it is a good idea. I don't believe the extension use will run out of hand, they will be used where it makes a performance difference and the best place to make use of them is in WebKit.
> 
> I agree that graphics/opengl is a better place for this.

Agreed. I will make the needed change.

[EFL EWS Bot]

Comment on attachment 174621 [details]
review fixes

Attachment 174621 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14873048

[Hyowon Kim]

I think this refactoring is a good approach.
But all Evas_GL related codes would be removed from GraphicsContext3D.
Evas_GL is necessary In WebKit1 and for the UI process in WebKit2.
To be able to use it, another bug is needed, isn't it? 
(This patch seems to be too huge to also cover it.)

[WebKit Review Bot]

Comment on attachment 174621 [details]
review fixes

Attachment 174621 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14843942

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Kalyan]

Created attachment 174840 [details]
review changes

Following Changes are included:
1)Moved GLContext and GLSurface from platform/graphics/efl to platform/graphics/opengl
2)Renamed GLContext and GLSurface as GLPlatformContext and GLPlatformSurface respectively.

[Kalyan]

(In reply to comment #32)
> (From update of attachment 174621 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174621&action=review
> 
> > Source/WebCore/platform/graphics/efl/GLContext.h:1
> > +/*
> 
> Will you move these to say Source/WebCore/platform/graphics/opengl/GLContext.h ?

done 

> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:100
> > +{
> > +    if (m_drawable) {
> > +        if (m_restoreNeeded) {
> > +            GLint oldFBO;
> > +            glGetIntegerv(GL_FRAMEBUFFER_BINDING, &oldFBO);
> > +            glBindFramebuffer(GL_FRAMEBUFFER, 0);
> > +            glXSwapBuffers(sharedDisplay(), m_drawable);
> > +            glBindFramebuffer(GL_FRAMEBUFFER, oldFBO);
> > +        } else
> > +            glXSwapBuffers(sharedDisplay(), m_drawable);
> > +    }
> > +}
> 
> This we write as 
> 
> if (!m_drawable)
>     return;
> 
 fixed 

> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:179
> > +    return pBufferConfig();
> 
> pBufferConfiguration then, or just rename this to config() then. We need to be consistent

fixed

[Kalyan]

(In reply to comment #36)
> I think this refactoring is a good approach.
> But all Evas_GL related codes would be removed from GraphicsContext3D.
> Evas_GL is necessary In WebKit1 and for the UI process in WebKit2.
> To be able to use it, another bug is needed, isn't it? 
> (This patch seems to be too huge to also cover it.)

Yes, I will open another bug for this.

[WebKit Review Bot]

Comment on attachment 174840 [details]
review changes

Clearing flags on attachment: 174840

Committed r135074: <http://trac.webkit.org/changeset/135074>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 102619

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #43)
> Re-opened since this is blocked by bug 102619

For the backtrace, see, for example, <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6041/steps/layout-test/logs/stdio>.

[Kalyan]

(In reply to comment #44)
> (In reply to comment #43)
> > Re-opened since this is blocked by bug 102619
> 
> For the backtrace, see, for example, <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6041/steps/layout-test/logs/stdio>.

K, working on it

[Kalyan]

(In reply to comment #45)
> (In reply to comment #44)
> > (In reply to comment #43)
> > > Re-opened since this is blocked by bug 102619
> > 
> > For the backtrace, see, for example, <http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6041/steps/layout-test/logs/stdio>.
> 
> K, working on it


Investigation so far has revealed that the issue is with how we detect GLX support.

It seems HAVE_GLX is enabled automatically if WebGL is enabled (in OptionEfl.cmake which is wrong too). If WebGL is not enabled than we never enable it. Currently WebGL is not enabled by default. Working on a patch for correctly detecting GLX support. I will create a separate issue for this.

[Gyuyoung Kim]

Comment on attachment 174840 [details]
review changes

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

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:51
> +    if  (!m_platformContext)

Two spaces in "if  (!m_"

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:113
> +    bool returnValue = m_platformContext->makeCurrent(m_platformSurface.get());

Why do you use local variable here? Is below example more clear? I wonder if there is any reason.

 if (m_contextLostCallback && !m_platformContext->isValid()) {
     m_contextLostCallback->onContextLost();
     return false;
 }

 return m_platformContext->makeCurrent(m_platformSurface.get());

> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:107
> +    m_contextLost = false;

I think 127 line is more proper place for this.

> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:117
> +    bool returnValue = false;

If possible, I think we should not use local variable for return.

> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:122
> +        returnValue = true;

Can't we return here ? Because, it looks there is no process related to this logic below.

> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:125
> +        returnValue = true;

ditto.

[Kalyan]

(In reply to comment #47)
> (From update of attachment 174840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174840&action=review
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:51
> > +    if  (!m_platformContext)
> 
> Two spaces in "if  (!m_"

k will fix this

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:113
> > +    bool returnValue = m_platformContext->makeCurrent(m_platformSurface.get());
> 
> Why do you use local variable here? Is below example more clear? I wonder if there is any reason.
> 
>  if (m_contextLostCallback && !m_platformContext->isValid()) {
>      m_contextLostCallback->onContextLost();
>      return false;
>  }

This would be fine if we always had a valid call back function. This might not be true when GraphicsContext3D is used by a class other than WebGLGraphicsContext3D. We might end up in a situation where a callback is not set up. I.e in Webkit1 AcceleratedCompositingContextEfl creates GraphicsContex3D, GraphicsContex3D is also used by Texture Mapper to create a temporary wrapper for current Context.

>  return m_platformContext->makeCurrent(m_platformSurface.get());
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:107
> > +    m_contextLost = false;
> 
> I think 127 line is more proper place for this.

No, we use this value to define if a context is valid or not (isValid() function). Thus, it would be a safe bet to reset it always.
 
> > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:117
> > +    bool returnValue = false;
> 
> If possible, I think we should not use local variable for return.
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:122
> > +        returnValue = true;
> 
> Can't we return here ? Because, it looks there is no process related to this logic below.
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:125
> > +        returnValue = true;
> 
> ditto.

We cannot return here, since we query for the current status of the context after making it current. We can get rid of  the local variable though. We can return the value of isValid() directly in makeCurrent.
bool GLPlatformContext::makeCurrent(GLPlatformSurface* surface)
{
...
...
return isValid();
}

[Gyuyoung Kim]

Comment on attachment 174840 [details]
review changes

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

>>> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:113
>>> +    bool returnValue = m_platformContext->makeCurrent(m_platformSurface.get());
>> 
>> Why do you use local variable here? Is below example more clear? I wonder if there is any reason.
>> 
>>  if (m_contextLostCallback && !m_platformContext->isValid()) {
>>      m_contextLostCallback->onContextLost();
>>      return false;
>>  }
>> 
>>  return m_platformContext->makeCurrent(m_platformSurface.get());
> 
> This would be fine if we always had a valid call back function. This might not be true when GraphicsContext3D is used by a class other than WebGLGraphicsContext3D. We might end up in a situation where a callback is not set up. I.e in Webkit1 AcceleratedCompositingContextEfl creates GraphicsContex3D, GraphicsContex3D is also used by Texture Mapper to create a temporary wrapper for current Context.

Can other classes use this function now ? If not, I'd like to recommend to use early return with "FIXME:". Then, you can fix this again when other classes start to use this function. How do you think ?

>>> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:117
>>> +    bool returnValue = false;
>> 
>> If possible, I think we should not use local variable for return.
> 
> We cannot return here, since we query for the current status of the context after making it current. We can get rid of  the local variable though. We can return the value of isValid() directly in makeCurrent.
> bool GLPlatformContext::makeCurrent(GLPlatformSurface* surface)
> {
> ...
> ...
> return isValid();
> }

isValid() looks fine.

[Kalyan]

(In reply to comment #49)
> (From update of attachment 174840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174840&action=review
> 
> >>> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:113
> >>> +    bool returnValue = m_platformContext->makeCurrent(m_platformSurface.get());
> >> 
> >> Why do you use local variable here? Is below example more clear? I wonder if there is any reason.
> >> 
> >>  if (m_contextLostCallback && !m_platformContext->isValid()) {
> >>      m_contextLostCallback->onContextLost();
> >>      return false;
> >>  }
> >> 
> >>  return m_platformContext->makeCurrent(m_platformSurface.get());
> > 
> > This would be fine if we always had a valid call back function. This might not be true when GraphicsContext3D is used by a class other than WebGLGraphicsContext3D. We might end up in a situation where a callback is not set up. I.e in Webkit1 AcceleratedCompositingContextEfl creates GraphicsContex3D, GraphicsContex3D is also used by Texture Mapper to create a temporary wrapper for current Context.
> 
> Can other classes use this function now ? If not, I'd like to recommend to use early return with "FIXME:". Then, you can fix this again when other classes start to use this function. How do you think ?
> 

What this function basically does is as follows:
It makes the context and surface associated with the GraphicsContext3D as the current GL Context and Drawable. Thus, it would be used frequently.

[Gyuyoung Kim]

Comment on attachment 174840 [details]
review changes

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

>>>>> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:113
>>>>> +    bool returnValue = m_platformContext->makeCurrent(m_platformSurface.get());
>>>> 
>>>> Why do you use local variable here? Is below example more clear? I wonder if there is any reason.
>>>> 
>>>>  if (m_contextLostCallback && !m_platformContext->isValid()) {
>>>>      m_contextLostCallback->onContextLost();
>>>>      return false;
>>>>  }
>>>> 
>>>>  return m_platformContext->makeCurrent(m_platformSurface.get());
>>> 
>>> This would be fine if we always had a valid call back function. This might not be true when GraphicsContext3D is used by a class other than WebGLGraphicsContext3D. We might end up in a situation where a callback is not set up. I.e in Webkit1 AcceleratedCompositingContextEfl creates GraphicsContex3D, GraphicsContex3D is also used by Texture Mapper to create a temporary wrapper for current Context.
>> 
>> Can other classes use this function now ? If not, I'd like to recommend to use early return with "FIXME:". Then, you can fix this again when other classes start to use this function. How do you think ?
> 
> What this function basically does is as follows:
> It makes the context and surface associated with the GraphicsContext3D as the current GL Context and Drawable. Thus, it would be used frequently.

If so, I don't object to use as it is. But, I still don't like to use bool returnValue.

[Kalyan]

(In reply to comment #51)
> (From update of attachment 174840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174840&action=review
> 
> >>>>> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:113
> >>>>> +    bool returnValue = m_platformContext->makeCurrent(m_platformSurface.get());
> >>>> 
> >>>> Why do you use local variable here? Is below example more clear? I wonder if there is any reason.
> >>>> 
> >>>>  if (m_contextLostCallback && !m_platformContext->isValid()) {
> >>>>      m_contextLostCallback->onContextLost();
> >>>>      return false;
> >>>>  }
> >>>> 
> >>>>  return m_platformContext->makeCurrent(m_platformSurface.get());
> >>> 
> >>> This would be fine if we always had a valid call back function. This might not be true when GraphicsContext3D is used by a class other than WebGLGraphicsContext3D. We might end up in a situation where a callback is not set up. I.e in Webkit1 AcceleratedCompositingContextEfl creates GraphicsContex3D, GraphicsContex3D is also used by Texture Mapper to create a temporary wrapper for current Context.
> >> 
> >> Can other classes use this function now ? If not, I'd like to recommend to use early return with "FIXME:". Then, you can fix this again when other classes start to use this function. How do you think ?
> > 
> > What this function basically does is as follows:
> > It makes the context and surface associated with the GraphicsContext3D as the current GL Context and Drawable. Thus, it would be used frequently.
> 
> If so, I don't object to use as it is. But, I still don't like to use bool returnValue.

> >>>>  if (m_contextLostCallback && !m_platformContext->isValid()) {
> >>>>      m_contextLostCallback->onContextLost();
> >>>>      return false;
> >>>>  }
> >>>> 
> >>>>  return m_platformContext->makeCurrent(m_platformSurface.get());

Sorry, I didnt check this properly before. This is completely wrong. We need to first make the context current before checking the status(if we have a valid context or not). 
so basically it goes as follows:
1)Make the context current with the given surface.    i.e. m_platformContext->makeCurrent(m_platformSurface.get())
2)Check if the first step succeeded and we still have a valid context.
3)return true if the above two conditions are met else false

If the changes are done in PlatformContext(as discussed above to return isValid()), then steps 1 and 2 are merged into one.

Than we have something like:
bool GraphicsContext3DPrivate::makeContextCurrent()
{
    bool success = m_platformContext->makeCurrent(m_platformSurface.get());

    if (!success && m_contextLostCallback) {
        m_contextLostCallback->onContextLost();
        // FIXME: Restore context
    }

    return success;
}

[Gyuyoung Kim]

(In reply to comment #52)
t());
 
> Sorry, I didnt check this properly before. This is completely wrong. We need to first make the context current before checking the status(if we have a valid context or not). 
> so basically it goes as follows:
> 1)Make the context current with the given surface.    i.e. m_platformContext->makeCurrent(m_platformSurface.get())
> 2)Check if the first step succeeded and we still have a valid context.
> 3)return true if the above two conditions are met else false
> 
> If the changes are done in PlatformContext(as discussed above to return isValid()), then steps 1 and 2 are merged into one.
> 
> Than we have something like:
> bool GraphicsContext3DPrivate::makeContextCurrent()
> {
>     bool success = m_platformContext->makeCurrent(m_platformSurface.get());
> 
>     if (!success && m_contextLostCallback) {
>         m_contextLostCallback->onContextLost();
>         // FIXME: Restore context
>     }
> 
>     return success;
> }

Yes, this code make sense now. Thanks.

[Kalyan]

(In reply to comment #53)
> (In reply to comment #52)
> t());
> 
> > Sorry, I didnt check this properly before. This is completely wrong. We need to first make the context current before checking the status(if we have a valid context or not). 
> > so basically it goes as follows:
> > 1)Make the context current with the given surface.    i.e. m_platformContext->makeCurrent(m_platformSurface.get())
> > 2)Check if the first step succeeded and we still have a valid context.
> > 3)return true if the above two conditions are met else false
> > 
> > If the changes are done in PlatformContext(as discussed above to return isValid()), then steps 1 and 2 are merged into one.
> > 
> > Than we have something like:
> > bool GraphicsContext3DPrivate::makeContextCurrent()
> > {
> >     bool success = m_platformContext->makeCurrent(m_platformSurface.get());
> > 
> >     if (!success && m_contextLostCallback) {
> >         m_contextLostCallback->onContextLost();
> >         // FIXME: Restore context
> >     }
> > 
> >     return success;
> > }
> 
> Yes, this code make sense now. Thanks.

Unable to make a context current and a context being reset are two different things. We need to explicitly check for both and I don't feel right to merge them into one

Something like this would be more fool proof:

bool GraphicsContext3DPrivate::makeContextCurrent()
{
    bool success = m_platformContext->makeCurrent(m_platformSurface.get());

    if (!success && !m_platformContext->isValid() && m_contextLostCallback) {
        m_contextLostCallback->onContextLost();
        // FIXME: Restore context
    }

    return success;
}

[Kalyan]

Created attachment 175321 [details]
review changes

[WebKit Review Bot]

Comment on attachment 175321 [details]
review changes

Clearing flags on attachment: 175321

Committed r135468: <http://trac.webkit.org/changeset/135468>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jussi Kukkonen (jku)]

wk2 build bots broke after this. I assume it's related.

[ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/InspectorBackendDispatcher.cpp.o
In file included from /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:32:0:
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:33:39: fatal error: X11/extensions/Xcomposite.h: No such file or directory
compilation terminated.
[ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/InspectorFrontend.cpp.o
[ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/InspectorTypeBuilder.cpp.o
[ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/ColorData.cpp.o
In file included from /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:27:0:
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:33:39: fatal error: X11/extensions/Xcomposite.h: No such file or directory
compilation terminated.
[ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/HTMLEntityTable.cpp.o
[ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/CSSPropertyNames.cpp.o
[ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/CSSValueKeywords.cpp.o
[ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/UserAgentStyleSheetsData.cpp.o
[ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/CSSGrammar.cpp.o
[ 81%] [ 81%] make[2]: *** [Source/WebCore/CMakeFiles/webcore_efl.dir/platform/graphics/opengl/GLPlatformSurface.cpp.o] Error 1
make[2]: *** Waiting for unfinished jobs....
Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/XPathGrammar.cpp.o
Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/HTMLNames.cpp.o
make[2]: *** [Source/WebCore/CMakeFiles/webcore_efl.dir/platform/graphics/surfaces/glx/GLXSurface.cpp.o] Error 1
make[1]: *** [Source/WebCore/CMakeFiles/webcore_efl.dir/all] Error 2
make: *** [all] Error 2

[Gyuyoung Kim]

(In reply to comment #58)
> wk2 build bots broke after this. I assume it's related.
> 
> [ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/InspectorBackendDispatcher.cpp.o
> In file included from /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/platform/graphics/opengl/GLPlatformSurface.cpp:32:0:
> /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:33:39: fatal error: X11/extensions/Xcomposite.h: No such file or directory
> compilation terminated.
> [ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/InspectorFrontend.cpp.o
> [ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/InspectorTypeBuilder.cpp.o
> [ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/ColorData.cpp.o
> In file included from /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:27:0:
> /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:33:39: fatal error: X11/extensions/Xcomposite.h: No such file or directory
> compilation terminated.
> [ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/HTMLEntityTable.cpp.o
> [ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/CSSPropertyNames.cpp.o
> [ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/CSSValueKeywords.cpp.o
> [ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/UserAgentStyleSheetsData.cpp.o
> [ 81%] Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/CSSGrammar.cpp.o
> [ 81%] [ 81%] make[2]: *** [Source/WebCore/CMakeFiles/webcore_efl.dir/platform/graphics/opengl/GLPlatformSurface.cpp.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/XPathGrammar.cpp.o
> Building CXX object Source/WebCore/CMakeFiles/webcore_efl.dir/__/__/DerivedSources/WebCore/HTMLNames.cpp.o
> make[2]: *** [Source/WebCore/CMakeFiles/webcore_efl.dir/platform/graphics/surfaces/glx/GLXSurface.cpp.o] Error 1
> make[1]: *** [Source/WebCore/CMakeFiles/webcore_efl.dir/all] Error 2
> make: *** [all] Error 2

This build breaks were disappeared after installing libxcomposite-dev