Bug 144685

Created attachment 252469 [details] Patch

Created attachment 252470 [details] Try to fix EFL build

Comment on attachment 252470 [details] Try to fix EFL build View in context: https://bugs.webkit.org/attachment.cgi?id=252470&action=review > Source/WebCore/platform/graphics/PlatformDisplay.cpp:139 > +#if USE(OPENGL_ES_2) > + static const EGLenum eglAPIVersion = EGL_OPENGL_ES_API; > +#else > + static const EGLenum eglAPIVersion = EGL_OPENGL_API; > +#endif > + if (eglBindAPI(eglAPIVersion) == EGL_FALSE) { > + LOG_ERROR("Failed to set EGL API(%d).", eglGetError()); > + terminateEGLDisplay(); > + return; > + } This needs to be called once per program, not once per display. Perhaps you could surround this with std::once. > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:153 > - m_glxPixmap = glXCreateGLXPixmap(m_sharedDisplay, visualInfo.get(), m_pixmap); > + m_glxPixmap = glXCreateGLXPixmap(X11Helper::nativeDisplay(), visualInfo.get(), m_pixmap); This is GLX-related, so perhaps you could update the bug title and ChangeLog slightly?

(In reply to comment #3) > Comment on attachment 252470 [details] > Try to fix EFL build > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252470&action=review > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:139 > > +#if USE(OPENGL_ES_2) > > + static const EGLenum eglAPIVersion = EGL_OPENGL_ES_API; > > +#else > > + static const EGLenum eglAPIVersion = EGL_OPENGL_API; > > +#endif > > + if (eglBindAPI(eglAPIVersion) == EGL_FALSE) { > > + LOG_ERROR("Failed to set EGL API(%d).", eglGetError()); > > + terminateEGLDisplay(); > > + return; > > + } > > This needs to be called once per program, not once per display. Perhaps you > could surround this with std::once. Display is already a single instance, so this will be called only once per display, per process. > > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:153 > > - m_glxPixmap = glXCreateGLXPixmap(m_sharedDisplay, visualInfo.get(), m_pixmap); > > + m_glxPixmap = glXCreateGLXPixmap(X11Helper::nativeDisplay(), visualInfo.get(), m_pixmap); > > This is GLX-related, so perhaps you could update the bug title and ChangeLog > slightly? But nothing has actually changed here, I can add and remove PlatformDisplay from GLDefs.h

(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 252470 [details] > > Try to fix EFL build > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=252470&action=review > > > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:139 > > > +#if USE(OPENGL_ES_2) > > > + static const EGLenum eglAPIVersion = EGL_OPENGL_ES_API; > > > +#else > > > + static const EGLenum eglAPIVersion = EGL_OPENGL_API; > > > +#endif > > > + if (eglBindAPI(eglAPIVersion) == EGL_FALSE) { > > > + LOG_ERROR("Failed to set EGL API(%d).", eglGetError()); > > > + terminateEGLDisplay(); > > > + return; > > > + } > > > > This needs to be called once per program, not once per display. Perhaps you > > could surround this with std::once. > > Display is already a single instance, so this will be called only once per > display, per process. eglBindAPI isn't supposed to be called once per-display. I supposed to be called once. > > > > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:153 > > > - m_glxPixmap = glXCreateGLXPixmap(m_sharedDisplay, visualInfo.get(), m_pixmap); > > > + m_glxPixmap = glXCreateGLXPixmap(X11Helper::nativeDisplay(), visualInfo.get(), m_pixmap); > > > > This is GLX-related, so perhaps you could update the bug title and ChangeLog > > slightly? > > But nothing has actually changed here, I can add and remove PlatformDisplay > from GLDefs.h I'm not sure I understand what you mean, but the change is so small that you can probably skip mentioning GLX in the ChangeLog if you don't think it matters.

Created attachment 252475 [details] Patch for landing

Created attachment 252476 [details] Patch for landing

Created attachment 252478 [details] Patch for landing

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > Comment on attachment 252470 [details] > > > Try to fix EFL build > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=252470&action=review > > > > > > > Source/WebCore/platform/graphics/PlatformDisplay.cpp:139 > > > > +#if USE(OPENGL_ES_2) > > > > + static const EGLenum eglAPIVersion = EGL_OPENGL_ES_API; > > > > +#else > > > > + static const EGLenum eglAPIVersion = EGL_OPENGL_API; > > > > +#endif > > > > + if (eglBindAPI(eglAPIVersion) == EGL_FALSE) { > > > > + LOG_ERROR("Failed to set EGL API(%d).", eglGetError()); > > > > + terminateEGLDisplay(); > > > > + return; > > > > + } > > > > > > This needs to be called once per program, not once per display. Perhaps you > > > could surround this with std::once. > > > > Display is already a single instance, so this will be called only once per > > display, per process. > > eglBindAPI isn't supposed to be called once per-display. I supposed to be > called once. There's only one display, so it will be called once. Anyway, I've moved the egl initializations repeated in several places here, so if it was wrong it can be fixed in a follow up patch, this patch shouldn't change any behaviour. But I insist, this is called only once per process.

(In reply to comment #9) > There's only one display, so it will be called once. Anyway, I've moved the > egl initializations repeated in several places here, so if it was wrong it > can be fixed in a follow up patch, this patch shouldn't change any > behaviour. But I insist, this is called only once per process. Okay. I misunderstood what you wrote earlier. Sorry about that. This sounds fine.

Committed r183868: <http://trac.webkit.org/changeset/183868>