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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=101291





--- Comment #4 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2012-11-06 00:20:30 PST ---
(From update of attachment 172498)
View in context: https://bugs.webkit.org/attachment.cgi?id=172498&action=review

> Source/WebCore/PlatformEfl.cmake:301
> +    platform/graphics/efl/GLXContextWrapper.cpp

What for platform that don't have X at all? Say wayland?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:23
> +#include "config.h"
> +#include <wtf/MainThread.h>
> +#include "GLNativeContext.h"
> +#include "GLXContextWrapper.h"

Thsi is not correct. Check style guide

#include "config.h"
#include "GLNativeContext.h"

#include the rest

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:33
> +PassOwnPtr<GLNativeContext> GLNativeContext::createContext(GraphicsContext3D::RenderStyle renderStyle,uint64_t windowHandle,bool useSharedContext)

space before bool, uint64_t etc. Someone should fix the style checker if it is not catching this

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:43
> +    if (!success) {
> +         return nullptr;
> +    }

no need for braces

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:47
> +        if (OwnPtr<GLNativeContext> glxContext  = GLNativeContext::createOffScreenContext(useSharedContext ? GLNativeContext::sharedOffScreenContext():0,windowHandle))

no double space before ==, space before windowHandle

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:51
> +        if (OwnPtr<GLNativeContext> glxContext  = GLNativeContext::createCurrentContextWrapper())

same

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:66
> +    return sharing.get();

sharing? m_sharedInstance ?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:120
> +PassOwnPtr<GLNativeContext> GLNativeContext::createOffScreenContext(GLNativeContext* sharingContext,uint64_t windowHandle)

space issue

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:122
> +    OwnPtr<GLNativeContext> glxContext = adoptPtr(new GLXOffScreenContextWrapper(sharingContext,windowHandle));

same

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:137
> +// FIXME: This should be a thread local eventually if we
> +// want to support using GLNativeContexts from multiple threads.

a local thread? what do you mean with that?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:159
> +bool GLNativeContext::pushContext()
> +{
> +    ASSERT(isMainThread());
> +    gCurrentContext = this;
> +    return true;
> +}

That doesnt look like a stack, so why it is called push? interface? comment?

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:177
> +void GLNativeContext::swapBuffers()
> +{
> +    ASSERT(isMainThread());
> +}

comment needed

> Source/WebCore/platform/graphics/efl/GLNativeContext.cpp:184
> +
> +
> +

too many newlines

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:40
> +    static  PassOwnPtr<GLNativeContext> createContext(GraphicsContext3D::RenderStyle renderStyle,uint64_t windowHandle = 0,bool useSharedContext = false);

You should really fix all this style :)

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:58
> +    // Make Context as current. It does nothing if it is already current.

These comments add no value, the name is already descriptive

> Source/WebCore/platform/graphics/efl/GLNativeContext.h:62
> +    //Taken from cairo port.

What does that mean? Don't we need them? :)

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:32
> +#include "GLXContextWrapper.h"
> +
> +
> +namespace WebCore {
> +
> +
> +GLXContextWrapper::GLXContextWrapper()

excessive newlines

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:33
> +    :GLNativeContext()

space after :

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:35
> +

remove newline

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:40
> +#if USE(EGL)

isn't this GLX? Why that name of the class then

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:41
> +    return eglGetCurrentSurface (EGL_DRAW);

no space before (

> Source/WebCore/platform/graphics/efl/GLXContextWrapper.cpp:107
> +    doPopContext();

not webkit style naming

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:-139
> -    Evas_GL_Config config = {
> -        EVAS_GL_RGBA_8888,
> -        EVAS_GL_DEPTH_BIT_8,
> -        EVAS_GL_STENCIL_NONE, // FIXME: set EVAS_GL_STENCIL_BIT_8 after fixing Evas_GL bug.
> -        EVAS_GL_OPTIONS_NONE,
> -        EVAS_GL_MULTISAMPLE_NONE
> -    };
> -

All this code is not needed anymore?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.