[Webkit-unassigned] [Bug 101291] [EFL] Refactor GraphicsContext3DEFL
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Nov 12 07:02:36 PST 2012
https://bugs.webkit.org/show_bug.cgi?id=101291
--- Comment #10 from Kenneth Rohde Christiansen <kenneth at webkit.org> 2012-11-12 07:04:17 PST ---
(From update of attachment 173639)
View in context: https://bugs.webkit.org/attachment.cgi?id=173639&action=review
Jsut a quick look though, many style issues, should be easy to fix. The changelog needs so info on what you changed, what the different classes represents etc
> Source/WebCore/ChangeLog:37
> + (WebCore::GLNativeContext::destroy):
> + * platform/graphics/efl/GLNativeContext.h: Added.
> + (WebCore):
> + (GLNativeContext):
> + * platform/graphics/efl/GLNativeSurface.cpp: Added.
> + (WebCore):
> + (WebCore::GLNativeSurface::createOffscreenSurface):
> + (WebCore::GLNativeSurface::createTransportSurface):
> + (WebCore:::m_sharedDisplay):
For such big changes we normally add comments here to say what changed and why
> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:26
> +#include "config.h"
After the config include the next include must be the header file for the cpp
> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:30
> +#include "GLNativeContext.h"
ie this one
> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:38
> +PassOwnPtr<GLNativeContext> GLNativeContext::createContext(GraphicsContext3D::RenderStyle renderStyle)
I don't like Style so much, rather Mode. Style means CSS normally
> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:41
> +{
> +
> + static bool initializedShims = false;
unneeded newline
> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:44
> + if (!initializedShims) {
didInitializeShims
> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:47
> + glGetGraphicsResetStatusARB = (PFNGLGETGRAPHICSRESETSTATUSARBPROC)glXGetProcAddressARB((const GLubyte*)"glGetGraphicsResetStatusARB");
We should probably use C++ casts here
> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:68
> + }
> + return nullptr;
> +}
i would add newline before retunr
> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:106
> + if (surface && !surface->handle().isValid())
> + return false;
In this case you dont need to relase anything? or set m_currentContext to 0?
> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:117
> + } else if (platformMakeCurrent(surface)) {
no double space before if
> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:202
> +#endif
> +
> +
> +
> +
> +
wow unneeded newlines
> Source/WebCore/platform/graphics/efl/GLNativeContext.h:41
> +class GLNativeContext {
> +
> + WTF_MAKE_NONCOPYABLE(GLNativeContext);
> +public:
I would add WTF... to the top
> Source/WebCore/platform/graphics/efl/GLNativeContext.h:48
> + enum NativeContextReset {
> + NativeCONTEXT_NO_ERROR = 0,
> + NativeCONTEXT_GUILTY_CONTEXT_RESET = 0x8253,
> + NativeCONTEXT_INNOCENT_CONTEXT_RESET = 0x8254,
> + NativeCONTEXT_UNKNOWN_CONTEXT_RESET = 0x8255,
> + };
any comment to where these numbers come from?
> Source/WebCore/platform/graphics/efl/GLNativeContext.h:60
> + // Makes this and surface as current context and drawable.
> + // Calling this function with no surface is same as calling releaseCurrent.
> + // Does nothing if this is already current Context.
> + bool makeCurrent(GLNativeSurface* = 0);
Why isn't an ASSERT better? ie. forcing people to actually use releaseCurrent
> Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:38
> +namespace WebCore {
> +
> +
> +PassRefPtr<GLNativeSurface> GLNativeSurface::createOffscreenSurface()
only one newline
> Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:43
> + return surface;
.release() no?
> Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:98
> +void GLNativeSurface::copyTexture(uint32_t texture, const IntRect& sourceRect)
> +{
> +
> + if (!m_fboId)
no newline here!
> Source/WebCore/platform/graphics/efl/GLNativeSurface.cpp:128
> +void GLNativeSurface::setGeometry(const IntRect& newRect)
no double newline before newRect
> Source/WebCore/platform/graphics/efl/GLNativeSurface.h:71
> + // Returns Geometry of surface.
> + const IntRect& geometry() const;
> +
> + // Get the underlying platform specific surface handle.
> + GraphicsSurfaceToken handle() const;
> +
> + platformDisplay sharedDisplay() const;
> +
> + // Swaps front and back buffers. This has no effect for single buffered surfaces.
> + virtual void swapBuffers();
> +
> + // If Supported, Copies contents of texture to back buffer.
> + virtual void copyTexture(uint32_t texture, const IntRect& sourceRect);
> +
> + // Resizes the viewPort on supported implementations.
> + virtual void setGeometry(const IntRect& newRect);
These comments are mostly useless or self explanatory
> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:102
> + if (config) {
> +
> + initializeARBExtensions();
No if/for/while etc content can start with newline
> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:109
> + m_supportsRobustContextCreation = true;
I wonder if we can find a better name
> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:163
> +#endif
> +
> +
> +
newlines...
> Source/WebCore/platform/graphics/efl/GLXContextWrapper.h:33
> +class GLXCurrentContextWrapper :public GLNativeContext {
space after :
> Source/WebCore/platform/graphics/efl/GLXSurface.cpp:48
> + :GLNativeSurface()
space after :
> Source/WebCore/platform/graphics/efl/GLXSurface.h:39
> +class SharedResources {
why not share from RefCounted?
> Source/WebCore/platform/graphics/efl/GLXSurface.h:45
> + SharedResources()
> + {
> + ++m_refCount;
> + }
> +
instead of this!
> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:105
> + m_contextLostCallback->onContextLost();
we dont use on, what about contextDidReset() or so
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list