RESOLVED FIXED Bug 105825
[EFL][WebGL] Refactor GLXImplementation.
https://bugs.webkit.org/show_bug.cgi?id=105825
Summary [EFL][WebGL] Refactor GLXImplementation.
Kalyan
Reported 2012-12-28 03:34:18 PST
Decouple native window management and logic to find surface configuration. This would align GLX implementation with EGL.
Attachments
patch (27.85 KB, patch)
2012-12-28 04:41 PST, Kalyan
no flags
patchv2 (27.82 KB, patch)
2012-12-28 05:27 PST, Kalyan
no flags
patchv3 (28.96 KB, patch)
2012-12-29 07:50 PST, Kalyan
no flags
Kalyan
Comment 1 2012-12-28 04:41:52 PST
Kenneth Rohde Christiansen
Comment 2 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
Kalyan
Comment 3 2012-12-28 05:27:42 PST
Kalyan
Comment 4 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
Kenneth Rohde Christiansen
Comment 5 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
Kalyan
Comment 6 2012-12-29 07:50:37 PST
Kalyan
Comment 7 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.
WebKit Review Bot
Comment 8 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>
WebKit Review Bot
Comment 9 2012-12-29 10:13:16 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.