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

[bugzilla-daemon at webkit.org]

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





--- Comment #24 from Gyuyoung Kim <gyuyoung.kim at samsung.com>  2012-11-15 21:33:08 PST ---
(From update of attachment 174596)
View in context: https://bugs.webkit.org/attachment.cgi?id=174596&action=review

It seems to me there are still nits.

> Source/WebCore/platform/graphics/efl/GLSurface.cpp:27
> +

Don't need to add a new line.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:118
> +        // FIXME: restore context

restore -> Restore

> Source/WebCore/platform/graphics/surfaces/glx/GLXContext.cpp:27
> +

ditto.

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

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

ditto.

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

ditto.

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

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

> Source/WebCore/platform/graphics/surfaces/glx/GLXSurface.cpp:129
> +    XSetWindowAttributes a;

Can't use more meaningful name ?

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

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

Is unsigned better ?

> 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]'

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