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

[bugzilla-daemon at webkit.org]

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





--- Comment #33 from Kalyan <kalyan.kondapally at intel.com>  2012-11-16 00:31:30 PST ---
(In reply to comment #24)
> (From update of attachment 174596 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174596&action=review
> 
> It seems to me there are still nits.

Fixed all the headers

> > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:46
> > +    if (initialized)
> 
> I don't understand this condition well. Could you explain why you need to add this check ?
It acts as a guard check to prevent us from querying the function pointer more than once. 


> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:53
> > +// GLXOffScreenContext
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:59
> > +
> 
> ditto.

These where meant to indicate start of a new class implementation. Removed them anyway.

> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:74
> > +    if (config) {
> 
> I think early return is better for this case.
> 
> e.g)
> 
> if (!config)
>     return false;

I would prefer as it is now, since we need to also handle the case when we have a valid configuration but context creation fails. 

> > Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:94
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:27
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:75
> > +// GLXTransportSurface
> 
> I don't know why we need to add this comment. There is no information.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:85
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:124
> > +
> 
> Generally, AFAIK, WebKit doesn't like to add a new line in front of checking for null.
k Fixed.

> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:129
> > +    XSetWindowAttributes a;
> 
> Can't use more meaningful name ?

fixed

> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:158
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:166
> > +// GLX PBUFFER SURFACE
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:176
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:183
> > +
> 
> ditto.
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:207
> > +
> 
> ditto.

fixed

> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:104
> > +        for (int i = 0; i < numReturned; ++i) {
> 
> Is unsigned better ?

No, as numReturned is of type int. glXChooseFBConfig accepts only int type.

> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:123
> > +        // Didnot find any visual supporting alpha, select the first available config.
> 
> Didnot -> Did not ?
> 
> > Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.h:151
> > +                m_pbufferfbConfig =  temp[0];
> 
> Two spaces between '=' and 'temp[0]'

fixed

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