Bug 101291

Please add [EFL] prefix for EFL bug.

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

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.

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?

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)

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?

(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

(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

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.

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

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?

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

(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??

Created attachment 174209 [details] patch Review changes

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

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 *

> > > 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.

(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

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.

Created attachment 174596 [details] rebasedpatch

(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

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

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

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]'

CC'ing Hyowon,

Created attachment 174605 [details] Added missing file

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.

Comment on attachment 174605 [details] Added missing file Attachment 174605 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14859220

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.

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

Created attachment 174621 [details] review fixes

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

(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

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

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

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

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

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.

(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

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

Comment on attachment 174840 [details] review changes Clearing flags on attachment: 174840 Committed r135074: <http://trac.webkit.org/changeset/135074>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 102619

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

(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

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

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.

(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(); }

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.

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

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.

(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; }

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

(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; }

Created attachment 175321 [details] review changes

Comment on attachment 175321 [details] review changes Clearing flags on attachment: 175321 Committed r135468: <http://trac.webkit.org/changeset/135468>

All reviewed patches have been landed. Closing bug.

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

(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