[Webkit-unassigned] [Bug 101291] [EFL] Refactor GraphicsContext3DEFL
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 15 21:31:21 PST 2012
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.
More information about the webkit-unassigned
mailing list