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

[bugzilla-daemon at webkit.org]

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





--- Comment #48 from Kalyan <kalyan.kondapally at intel.com>  2012-11-19 22:25:28 PST ---
(In reply to comment #47)
> (From update of attachment 174840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174840&action=review
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:51
> > +    if  (!m_platformContext)
> 
> Two spaces in "if  (!m_"

k will fix this

> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:113
> > +    bool returnValue = m_platformContext->makeCurrent(m_platformSurface.get());
> 
> Why do you use local variable here? Is below example more clear? I wonder if there is any reason.
> 
>  if (m_contextLostCallback && !m_platformContext->isValid()) {
>      m_contextLostCallback->onContextLost();
>      return false;
>  }

This would be fine if we always had a valid call back function. This might not be true when GraphicsContext3D is used by a class other than WebGLGraphicsContext3D. We might end up in a situation where a callback is not set up. I.e in Webkit1 AcceleratedCompositingContextEfl creates GraphicsContex3D, GraphicsContex3D is also used by Texture Mapper to create a temporary wrapper for current Context.

>  return m_platformContext->makeCurrent(m_platformSurface.get());
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:107
> > +    m_contextLost = false;
> 
> I think 127 line is more proper place for this.

No, we use this value to define if a context is valid or not (isValid() function). Thus, it would be a safe bet to reset it always.

> > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:117
> > +    bool returnValue = false;
> 
> If possible, I think we should not use local variable for return.
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:122
> > +        returnValue = true;
> 
> Can't we return here ? Because, it looks there is no process related to this logic below.
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:125
> > +        returnValue = true;
> 
> ditto.

We cannot return here, since we query for the current status of the context after making it current. We can get rid of  the local variable though. We can return the value of isValid() directly in makeCurrent.
bool GLPlatformContext::makeCurrent(GLPlatformSurface* surface)
{
...
...
return isValid();
}

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