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

[bugzilla-daemon at webkit.org]

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





--- Comment #49 from Gyuyoung Kim <gyuyoung.kim at samsung.com>  2012-11-19 22:52:18 PST ---
(From update of attachment 174840)
View in context: https://bugs.webkit.org/attachment.cgi?id=174840&action=review

>>> 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;
>>  }
>> 
>>  return m_platformContext->makeCurrent(m_platformSurface.get());
> 
> 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.

Can other classes use this function now ? If not, I'd like to recommend to use early return with "FIXME:". Then, you can fix this again when other classes start to use this function. How do you think ?

>>> Source/WebCore/platform/graphics/opengl/GLPlatformContext.cpp:117
>>> +    bool returnValue = false;
>> 
>> If possible, I think we should not use local variable for return.
> 
> 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();
> }

isValid() looks fine.

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