[EFL][WebGL] Implement EGL support with GLX

[Kalyan]

Implement EGL support with GLX.

[Kalyan]

Created attachment 180545 [details]
patch

[EFL EWS Bot]

Comment on attachment 180545 [details]
patch

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

[Kalyan]

Comment on attachment 180545 [details]
patch

will upload a new patch with the build fix

[Kalyan]

Created attachment 180553 [details]
patch

[Kalyan]

Created attachment 180557 [details]
patch

fixed some white spaces.

[Noam Rosenthal]

I would like to hear Zeno's opinion on this one.

[Kenneth Rohde Christiansen]

Comment on attachment 180557 [details]
patch

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

> Source/WebCore/ChangeLog:8
> +        by passing -DENABLE_EGL=ON as cmake config parameter. This is disabled by default.
> +        Reviewed by NOBODY (OOPS!).

Reviewed by should be below the bugzilla link

> Source/WebCore/ChangeLog:29
> +        (WebCore):
> +        (WebCore::GLPlatformContext::createContext):
> +        (WebCore::GLPlatformContext::createOffScreenContext):
> +        (WebCore::GLPlatformContext::createCurrentContextWrapper):
> +        * platform/graphics/opengl/GLPlatformContext.h:
> +        * platform/graphics/opengl/GLPlatformSurface.cpp:
> +        (WebCore::GLPlatformSurface::createTransportSurface):
> +        * platform/graphics/surfaces/egl/EGLContext.cpp: Added.
> +        (WebCore):
> +        (WebCore::isRobustnessExtSupported):
> +        (WebCore::EGLCurrentContextWrapper::EGLCurrentContextWrapper):
> +        (WebCore::EGLCurrentContextWrapper::handle):
> +        (WebCore::EGLOffScreenContext::EGLOffScreenContext):
> +        (WebCore::EGLOffScreenContext::initialize):

Very little information here...

This patch needs a lot more information on what you are doing and why. Without this is it very hard to understand and review

> Source/WebCore/platform/graphics/opengl/GLDefs.h:62
> +static const EGLenum eglApi = EGL_OPENGL_ES_API;

eglAPIVersion ?

> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:82
>  #if USE(GLX)
> -    OwnPtr<GLPlatformContext> glxContext = adoptPtr(new GLXOffScreenContext());
> -    return glxContext.release();
> +    OwnPtr<GLPlatformContext> context = adoptPtr(new GLXOffScreenContext());
> +    return context.release();
> +#endif
> +
> +#if USE(EGL)

Why not #elif ?

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:65
> +            std::string foundExtensions(extensions);

why not use WebCore strings?

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:67
> +            std::string extName("EGL_EXT_create_context_robustness ");

extensionName
is the " " needed?

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:80
> +// FixMe: This is a temporary workaround till we are able to build evas with EGL support

We write FIXME: not FixMe:

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:97
> +    // set current rendering API

Set* API.* (Capital, punctuation mark)

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:98
> +    EGLBoolean eglResult = eglBindAPI(eglApi);

why not just result or success

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:117
> +    if (config) {
> +
> +        if (isRobustnessExtSupported(m_display))

Unneeded, weird newline

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:143
> +    // set current rendering API

Comment style

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:144
> +    EGLBoolean eglResult = eglBindAPI(eglApi);

success ?

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:31
> +#include "NotImplemented.h"

You dont seem to use this

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:39
> +        LOG_ERROR("No valid hardware display found. \n");

unneeded space in error message

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:51
> +        LOG_ERROR("Failed to create XWindow. \n");

same

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:59
> +        LOG_ERROR("Failed to create EGL surface. \n");

again

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:85
> +    // post EGL surface to window

commetn style

> Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:53
> +            int maj, min;

major, minor please

> Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:56
> +            m_eglDisplay = eglGetDisplay((EGLNativeDisplayType) SharedX11Resources::x11Display());

C++ cast?

> Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:59
> +                LOG_ERROR("EGLDisplay Initialization failed");

You added \n before... does this LOG_ERROR need \n or not? Consistency please

> Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:74
> +                LOG_ERROR("failed to set EGL API(%d).\n", eglGetError());

is \n needed?

> Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:174
> +        visualId = (VisualID)eglValue;

c++ cast

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.h:158
> +// Dummy Implementation to avoid ifdefs

Could be a better comment. Like when is it used? also missing punctation mark

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.h:159
> +class DummySharedResources : public SharedX11Resources {

I tihnk the style is calling it EmptySharedResources... like we have the EmptyChromeClient or similar in WebCore

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.h:170
> +        return 0;

return nullptr?

[Kalyan]

Created attachment 180586 [details]
patch

review changes

[Kalyan]

(In reply to comment #7)
> (From update of attachment 180557 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180557&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        by passing -DENABLE_EGL=ON as cmake config parameter. This is disabled by default.
> 

> > +        (WebCore::EGLOffScreenContext::initialize):
> 
> Very little information here...
> 
> This patch needs a lot more information on what you are doing and why. Without this is it very hard to understand and review

Added relevant info to the changelog.
> 
> > Source/WebCore/platform/graphics/opengl/GLDefs.h:62
> > +static const EGLenum eglApi = EGL_OPENGL_ES_API;
> 
> eglAPIVersion ?

Fixed.

> > +#endif
> > +
> > +#if USE(EGL)
> 
> Why not #elif ?
Fixed.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:65
> > +            std::string foundExtensions(extensions);
> 
> why not use WebCore strings?

Changed to use WebCore strings.
> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:67
> > +            std::string extName("EGL_EXT_create_context_robustness ");
> 
> extensionName
> is the " " needed?
Yes.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:80
> > +// FixMe: This is a temporary workaround till we are able to build evas with EGL support
> 
> We write FIXME: not FixMe:
> 
Fixed
> 
> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:144
> > +    EGLBoolean eglResult = eglBindAPI(eglApi);
> 
> success ?

Changed.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.cpp:31
> > +#include "NotImplemented.h"
> 
> You dont seem to use this

Removed

> 
> > Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:56
> > +            m_eglDisplay = eglGetDisplay((EGLNativeDisplayType) SharedX11Resources::x11Display());
> 
> C++ cast?
Fixed

> 
> You added \n before... does this LOG_ERROR need \n or not? Consistency please
> 
> > Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:74
> > +                LOG_ERROR("failed to set EGL API(%d).\n", eglGetError());
> 
> is \n needed?

Fixed.

> > Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:174
> > +        visualId = (VisualID)eglValue;
> 
> c++ cast

Done

> > Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.h:158
> > +// Dummy Implementation to avoid ifdefs
> 
> Could be a better comment. Like when is it used? also missing punctation mark
> 
> > Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.h:159

Removed DummySharedResources as it wasn't needed.

[Kalyan]

Created attachment 180602 [details]
rebasedpatch

rebased patch

[Noam Rosenthal]

Comment on attachment 180602 [details]
rebasedpatch

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

The dependency between EGL & X11 needs to be more explicit and encapsulated.

> Source/WebCore/ChangeLog:24
> +        EGL implementation for Context Management.

Put comment after filenames

> Source/WebCore/ChangeLog:44
> +        EGL implementation for Offscreen Surface.

Ditto

> Source/WebCore/ChangeLog:57
> +        X integration.

Ditto

> Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.h:37
> +class EGLTransportSurface : public X11OffScreenWindow {

This should be an EGLX11TransportSurface.
EGL doesn't always support X11.

> Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:37
> +class SharedEGLResources : public SharedX11Resources {

What is the benefit of tying EGL resources to X11 resources? Can we somehow separate them a bit better?

[Kalyan]

(In reply to comment #11)
> (From update of attachment 180602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180602&action=review
> 
> The dependency between EGL & X11 needs to be more explicit and encapsulated.
> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.h:37
> > +class EGLTransportSurface : public X11OffScreenWindow {
> 
> This should be an EGLX11TransportSurface.
> EGL doesn't always support X11.

We use a EGLSurface backed by a native pixmap. All the stuff related to native resource management(XWindow in this case) has been moved to X11OffscreenWindow. We could get rid of the inheritance and make EGLTransportSurface construct the respective class (NativeOffscreenWindow i.e X11OffScreenWindow) as needed. Idea is not to have any native window dependencies in this class. Does that sound ok ??
 
> > Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:37
> > +class SharedEGLResources : public SharedX11Resources {
> 
> What is the benefit of tying EGL resources to X11 resources? Can we somehow separate them a bit better?

Purpose was for X integration. Here we use X11 resources to find the right configs and XVisuals. Ofcourse, I could try moving this to SharedX11Resources class. Get rid of the inheritance and let this class construct SharedX11Resources as needed.

[Kalyan]

Created attachment 180633 [details]
patch

[Kalyan]

(In reply to comment #11)
> (From update of attachment 180602 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180602&action=review

> > Source/WebCore/ChangeLog:57
> > +        X integration.
> 
> Ditto
Fixed

> > Source/WebCore/platform/graphics/surfaces/egl/EGLSurface.h:37
> > +class EGLTransportSurface : public X11OffScreenWindow {
> 
> This should be an EGLX11TransportSurface.
> EGL doesn't always support X11.


> > Source/WebCore/platform/graphics/surfaces/glx/EGLXWindowResources.h:37
> > +class SharedEGLResources : public SharedX11Resources {
> 
> What is the benefit of tying EGL resources to X11 resources? Can we somehow separate them a bit better?

I have separated the X parts now. This is removed completely and moved the logic to find EGLConfig to EGLConfigHelper class. This doesn't have any dependencies on X. Native Window resource Management and EGL related things are separated now.

[Kalyan]

Comment on attachment 180633 [details]
patch

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

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:75
>      XVisualInfo* visInfo = m_sharedResources->visualInfo();

I have some ifdefs like this in place as this is common code shared between GLX and EGL implementation. Once we finalize this, I will make the changes for GLX implementation and remove them in a separate changeset. I was trying to avoid any major GLX changes in this changeset.

[Noam Rosenthal]

Comment on attachment 180633 [details]
patch

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

Have to sort out the m_contextHandle stuff...

> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:89
> +    OwnPtr<GLPlatformContext> context = adoptPtr(new GLXCurrentContextWrapper());
> +    return context.release();

You don't need two lines here, adoptPtr already returns a PassOwnPtr.

return adoptPtr(new GLXCurrentContextWrapper());

> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:92
> +    OwnPtr<GLPlatformContext> context = adoptPtr(new EGLCurrentContextWrapper());
> +    return context.release();

ditto

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigHelper.cpp:116
> +#if USE(OPENGL_ES_2)
> +            EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> +#else
> +            EGL_RENDERABLE_TYPE, EGL_OPENGL_BIT,
> +#endif

Instead of repeating this twice, you can have a function EGLint getRenderableType() that returns a different value based on the define.

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigHelper.cpp:140
> +#if USE(OPENGL_ES_2)
> +            EGL_RENDERABLE_TYPE, EGL_OPENGL_ES2_BIT,
> +#else
> +            EGL_RENDERABLE_TYPE, EGL_OPENGL_BIT,
> +#endif

Ditto

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:29
> +#if USE(ACCELERATED_COMPOSITING) && USE(EGL)

I don't understand why all these classes are wrapped with ACCELERATED_COMPOSITING.

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:57
> +    static bool queryDone = false;

queryDone => didQueryForRobustnessExtension

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:58
> +    static bool isEGLEXTsupported = false;

isRobustnessExtensionSupported

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:77
> +// FIXME: This is a temporary workaround till we are able to build evas with EGL support

till => until
Period at end of sentence(.)

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:111
> +        if (!m_contextHandle && m_contextHandle != EGL_NO_CONTEXT)

EGL_NO_CONTEXT is 0
So this line means if (!handle && handle)

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:116
> +        if (m_contextHandle && m_contextHandle != EGL_NO_CONTEXT)

This is superfluously redundant :)

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.h:40
> +    PlatformContext handle() const;

add OVERRIDE keyword for overrides

[Kalyan]

Created attachment 180664 [details]
patch

[Kalyan]

(In reply to comment #16)
> (From update of attachment 180633 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180633&action=review
> 
> Have to sort out the m_contextHandle stuff...
> 
> You don't need two lines here, adoptPtr already returns a PassOwnPtr.
> 
> return adoptPtr(new GLXCurrentContextWrapper());

done

> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:29
> > +#if USE(ACCELERATED_COMPOSITING) && USE(EGL)
> 
> I don't understand why all these classes are wrapped with ACCELERATED_COMPOSITING.

Removed the checks.
 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:57
> > +    static bool queryDone = false;
> 
> queryDone => didQueryForRobustnessExtension
> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:58
> > +    static bool isEGLEXTsupported = false;
> 
> isRobustnessExtensionSupported
> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:77
> > +// FIXME: This is a temporary workaround till we are able to build evas with EGL support
> 
> till => until
> Period at end of sentence(.)

done

> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:111
> > +        if (!m_contextHandle && m_contextHandle != EGL_NO_CONTEXT)
> 
> EGL_NO_CONTEXT is 0
> So this line means if (!handle && handle)

oops missed that. Fixed now

> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:116
> > +        if (m_contextHandle && m_contextHandle != EGL_NO_CONTEXT)
> 
> This is superfluously redundant :)

:)

> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.h:40
> > +    PlatformContext handle() const;
> 
> add OVERRIDE keyword for overrides

done

[Kenneth Rohde Christiansen]

Comment on attachment 180664 [details]
patch

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

> Source/WebCore/ChangeLog:75
> +        Helper class to initialize EGL resources and choose right EGL configiration.

spelling configuration*

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigHelper.cpp:130
> +    return m_pbufferfbConfig;

m_pBufferFBConfig ?

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigHelper.cpp:156
> +    m_surfaceContextfbConfig = 0;

...FBConfig ?

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:79
> +// FIXME: This is a temporary workaround until we are able to build evas with EGL support.
> +PlatformContext EGLCurrentContextWrapper::handle() const

What about othre non-evas platforms

[Dongseong Hwang]

Comment on attachment 180664 [details]
patch

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

Great work. I don't fully understand yet. some nits after looking over.

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:35
> +#endif

Is it needed?

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:135
> +    if (!eglBindAPI(eglAPIVersion)) {

calling eglBindAPI every makeCurrent seems to be a bit heavy.. I'm not sure :)

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.h:40
> +    PlatformContext handle() const OVERRIDE;

nits: virtual PlatformContext handle() const OVERRIDE;

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.h:45
> +    WTF_MAKE_NONCOPYABLE(EGLOffScreenContext);

Not necessary because GLPlatformContext is already non-copyable.

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.h:54
> +    bool isCurrentContext() const OVERRIDE;

nits: I think all above methods should have 'virtual' keyword.

> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:105
> +    m_bufferHandle = tempHandleId;

m_bufferHandle is still needed?

[Kalyan]

(In reply to comment #19)
> (From update of attachment 180664 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180664&action=review
> 
> > Source/WebCore/ChangeLog:75
> > +        Helper class to initialize EGL resources and choose right EGL  
> What about othre non-evas platforms

This should work if there is support for EGL. We need to use some kind of if defs for our port and replace glx call with EGL. This is a temporary workaround till we have our view get the surface and context from PlatformSurface. Quick solution would be to get evas built with EGL support though

[Kalyan]

Comment on attachment 180664 [details]
patch

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

>> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:35
>> +#endif
> 
> Is it needed?

Yes, we use glx call to query for current context. i.e see line 81 below

>> Source/WebCore/platform/graphics/surfaces/glx/X11WindowResources.cpp:105
>> +    m_bufferHandle = tempHandleId;
> 
> m_bufferHandle is still needed?

Yes, till this changes are taken into account in GLX side. I plan to do it in another changeset, so we could avoid any major GLX changes in this one.

[Kalyan]

Created attachment 180772 [details]
patch

[Kalyan]

(In reply to comment #19)
> (From update of attachment 180664 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180664&action=review

> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLConfigHelper.cpp:130
> > +    return m_pbufferfbConfig;
> 
> m_pBufferFBConfig ?
> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLConfigHelper.cpp:156
> > +    m_surfaceContextfbConfig = 0;
> 
> ...FBConfig ?
> 

fixed

[Kalyan]

(In reply to comment #20)
> (From update of attachment 180664 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180664&action=review
> 
> Great work. I don't fully understand yet. some nits after looking over.
> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:35
> > +#endif
> 
> Is it needed?
> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:135
> > +    if (!eglBindAPI(eglAPIVersion)) {
> 
> calling eglBindAPI every makeCurrent seems to be a bit heavy.. I'm not sure :)
> 
 Removed. We either bind to opengles_2 or opengl api, thus it should be sufficient to do it during context creation.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.h:45
> > +    WTF_MAKE_NONCOPYABLE(EGLOffScreenContext);
> 
> Not necessary because GLPlatformContext is already non-copyable.

done

> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.h:54
> > +    bool isCurrentContext() const OVERRIDE;
> 
> nits: I think all above methods should have 'virtual' keyword.
> 
done

[Kenneth Rohde Christiansen]

Comment on attachment 180772 [details]
patch

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

> Source/WebCore/platform/graphics/surfaces/egl/EGLConfigHelper.cpp:166
> +    EGLConfig rightConfig = 0;

what is a right config? a correct one? why not just config?

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:96
> +        LOG_ERROR("Failed to set EGL API(%d).\n", eglGetError());

Are you sure \n is needed here?

> Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:108
> +    EGLConfig config = surface->configuration();
> +    if (!config)
> +        return false;
> +
> +    if (config) {

That config check seems useless, as you check above

[Kalyan]

Created attachment 180806 [details]
patch

[Kalyan]

(In reply to comment #26)
> (From update of attachment 180772 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180772&action=review
> 
> > Source/WebCore/platform/graphics/surfaces/egl/EGLConfigHelper.cpp:166
> > +    EGLConfig rightConfig = 0;
> 
> what is a right config? a correct one? why not just config?

changed to config.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:96
> > +        LOG_ERROR("Failed to set EGL API(%d).\n", eglGetError());
> 
> Are you sure \n is needed here?
I think it is good to have it.

> > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:108
> > +    EGLConfig config = surface->configuration();
> > +    if (!config)
> > +        return false;
> > +
> > +    if (config) {
> 
> That config check seems useless, as you check above
 removed.

[Laszlo Gombos]

(In reply to comment #28)
> (In reply to comment #26)
> > (From update of attachment 180772 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=180772&action=review

> > > Source/WebCore/platform/graphics/surfaces/egl/EGLContext.cpp:96
> > > +        LOG_ERROR("Failed to set EGL API(%d).\n", eglGetError());
> > 
> > Are you sure \n is needed here?
> I think it is good to have it.

This will add an extra empty line, I think Kenneth has a point here.

[Kalyan]

Comment on attachment 180806 [details]
patch

Will update a new patch after removing \n from the LOG_ERROR calls.

[Kalyan]

Created attachment 180814 [details]
patch

[WebKit Review Bot]

Comment on attachment 180814 [details]
patch

Clearing flags on attachment: 180814

Committed r138513: <http://trac.webkit.org/changeset/138513>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.