Bug 105825 – [EFL][WebGL] Refactor GLXImplementation.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kalyan]

Decouple native window management and logic to find surface configuration. This would align GLX implementation with EGL.

[Kalyan]

Created attachment 180862 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 180862 [details]
patch

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

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:81
> +    GLXFBConfig surfaceContextConfig(bool supportsXRenderExtension)

wouldn't an enum be better?

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:111
> +    {
> +        if (m_pbufferFBConfig)
> +            m_pbufferFBConfig = 0;
> +
> +        if (m_surfaceContextFBConfig)
> +            m_surfaceContextFBConfig = 0;
> +    }

why not just set them always to 0

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:51
> +    OwnPtr<GLXConfigHelper> m_ConfigHelper;

m_conf... not uppercase C

[Kalyan]

Created attachment 180865 [details]
patchv2

[Kalyan]

(In reply to comment #2)
> (From update of attachment 180862 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180862&action=review
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:81
> > +    GLXFBConfig surfaceContextConfig(bool supportsXRenderExtension)
> 
> wouldn't an enum be better?
I made changes so that the value is passed during construction of confighelper, as this value doesn't change during run time. We could probably go for enum in future if we  need to track more than one extension support. Any thoughts ??

> why not just set them always to 0
done

> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:51
> > +    OwnPtr<GLXConfigHelper> m_ConfigHelper;
> 
> m_conf... not uppercase C

done

[Kenneth Rohde Christiansen]

Comment on attachment 180865 [details]
patchv2

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

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:35
> +class GLXConfigHelper {

Helper is a bad name... it makes it difficult to understand what its responsibility is

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:42
> +        , m_VisualInfo(0)

no, not uppercase V! We should fix the style checker to catch this, can you file a bug for that?

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:51
> +    virtual ~GLXConfigHelper()
> +    {
> +        reset();
> +    }

reset() doesnt seem needed... it is the dtor!

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:72
> +            int numReturned;

numConfigurations would make more sense, or numAvailableConfigs

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:114
> +        int numReturned;

same

> Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:139
> +                }
> +            } else if (m_VisualInfo->depth == 32) {
> +#else
> +            if (m_VisualInfo->depth == 32) {
> +#endif

something is wrong with indentation here!

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:41
> +    m_sharedDisplay = m_nativeResource->nativeSharedDisplay();
> +    if (!m_sharedDisplay)

newline before if

[Kalyan]

Created attachment 180939 [details]
patchv3

[Kalyan]

(In reply to comment #5)
> (From update of attachment 180865 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180865&action=review
>  
> Helper is a bad name... it makes it difficult to understand what its responsibility is

changed it to GLXConfigSelector

> > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:42
> > +        , m_VisualInfo(0)
> 
> no, not uppercase V! We should fix the style checker to catch this, can you file a bug for that?

fixed, k I will file a bug for it.

> > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:51
> > +    virtual ~GLXConfigHelper()
> > +    {
> > +        reset();
> > +    }
> 
> reset() doesnt seem needed... it is the dtor!
ya, removed it.

> > Source/WebCore/platform/graphics/surfaces/glx/GLXConfigHelper.h:72
> > +            int numReturned; 
> same

done.

[WebKit Review Bot]

Comment on attachment 180939 [details]
patchv3

Clearing flags on attachment: 180939

Committed r138567: <http://trac.webkit.org/changeset/138567>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.