[Webkit-unassigned] [Bug 101291] [EFL] Refactor GraphicsContext3DEFL

[bugzilla-daemon at webkit.org]

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.