Bug 105825 - [EFL][WebGL] Refactor GLXImplementation.
Summary: [EFL][WebGL] Refactor GLXImplementation.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Kalyan
URL:
Keywords:
Depends on:
Blocks: 105286
  Show dependency treegraph
 
Reported: 2012-12-28 03:34 PST by Kalyan
Modified: 2012-12-29 10:13 PST (History)
8 users (show)

See Also:


Attachments
patch (27.85 KB, patch)
2012-12-28 04:41 PST, Kalyan
no flags Details | Formatted Diff | Diff
patchv2 (27.82 KB, patch)
2012-12-28 05:27 PST, Kalyan
no flags Details | Formatted Diff | Diff
patchv3 (28.96 KB, patch)
2012-12-29 07:50 PST, Kalyan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kalyan 2012-12-28 03:34:18 PST
Decouple native window management and logic to find surface configuration. This would align GLX implementation with EGL.
Comment 1 Kalyan 2012-12-28 04:41:52 PST
Created attachment 180862 [details]
patch
Comment 2 Kenneth Rohde Christiansen 2012-12-28 04:56:48 PST
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
Comment 3 Kalyan 2012-12-28 05:27:42 PST
Created attachment 180865 [details]
patchv2
Comment 4 Kalyan 2012-12-28 05:30:47 PST
(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
Comment 5 Kenneth Rohde Christiansen 2012-12-29 05:16:40 PST
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
Comment 6 Kalyan 2012-12-29 07:50:37 PST
Created attachment 180939 [details]
patchv3
Comment 7 Kalyan 2012-12-29 07:55:13 PST
(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.
Comment 8 WebKit Review Bot 2012-12-29 10:13:12 PST
Comment on attachment 180939 [details]
patchv3

Clearing flags on attachment: 180939

Committed r138567: <http://trac.webkit.org/changeset/138567>
Comment 9 WebKit Review Bot 2012-12-29 10:13:16 PST
All reviewed patches have been landed.  Closing bug.