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

[bugzilla-daemon at webkit.org]

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





--- Comment #7 from Kalyan <kalyan.kondapally at intel.com>  2012-11-06 06:00:29 PST ---
(In reply to comment #4)
> (From update of attachment 172498 [details])
> 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
> 

Please ignore the style checks for now. This was more to see if the approach would be good enough for us. All the things will be fixed in the next patch. 

> > 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?

This would come back with evas implementation, otherwise no it is not needed

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