WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Kalyan
Comment 1
2012-12-28 04:41:52 PST
Created
attachment 180862
[details]
patch
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
Created
attachment 180865
[details]
patchv2
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
Created
attachment 180939
[details]
patchv3
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.
Top of Page
Format For Printing
XML
Clone This Bug